Fix an infinite recursion bug in query system (chapel-lang#26061)

It's another query system bug episode!

Closes chapel-lang#24855.

## The Bug
This time, the issue is so tricky that I have not been able to construct
a query-system-only reproducer; it seems that the precise combination of
elements from `resolution-queries.cpp` and `scope-queries.cpp` is
necessary to trigger the bug.

In general, though, the trouble is with the `isQueryRunning` function.
Queries in Dyno use `isQueryRunning` to avoid creating infinite
recursion. The culprit here in particular is `resolveVisibilityStmts`
and `resolveVisibilityStmtsQuery`. The non-query version of the function
runs a check with `isQueryRunning`, and returns early if recursion would
be encountered.

This is perfectly sound -- when you're not re-running queries. However,
when the query system is checking if old results can be re-used, it will
perform a bottom-up traversal of the dependency graph (see [this query
system
documentation](https://chapel-lang.org/docs/developer/compiler-internals/queries-impl.html#recomputation-model)
to understand what I mean in more detail). At this time, the "parent"
query is not technically running.

What happens, then, is that during recomputation,
`resolveVisibilityStmts` is called bottom up, meaning that the parent
instance of `resolveVisibilityStmtsQuery` is not running (even though it
_is above the second invocation in the graph_).
`resolveVisibilityStmtsQuery` is then invoked when it wasn't invoked the
previous time (since `isQueryRunning` returns false). This adds the
query to the dependency graph, creating an (undetected) cycle. The cycle
causes infinite recursion in the Dyno logic where the computational
graph is assumed to be acyclic.

## The Fix
The solution is to ensure that we know whether or not a query is
_notionally_ running at any given point; this could mean that it's
_actually_ running (and we're going top-down), or that we're traversing
this query's dependency subgraph (in which case it should _look_ like
the query is running to maintain the veil over the query system). To do
this, I added a new flag to query result entries, `beingTestedForReuse`.

I suspect I could've gotten away with some clever uses of `lastChecked`
instead of adding a new flag. However, `lastChecked` has a lot of uses
in the query system, and adding another use was nontrivial. Since I
found it hard to think through the consequence of changes to
`lastChecked` -- which, I hope, would mean anyone else reading this code
for the first time would find it hard too -- I opted for the simpler
solution and a documentation comment.

David's original reproducer is part of this PR to lock down this
behavior (though it depends on the implementation of scope resolution,
as opposed to being a "pure" query system test). I could not come up
with a simple reproducer, and think it's not worth the time to keep
trying.

Reviewed by @mppf -- thanks!

## Testing
- [x] dyno tests

frontend/include/chpl/framework/Context-detail.h

@@ -260,8 +260,20 @@ class QueryMapResultBase {
   // lastChanged indicates the last revision in which the query result
   // has changed
   mutable RevisionNumber lastChanged = -1;
-
-  mutable QueryDependencyVec dependencies;
+  // This field exists to support isQueryRunning. When traversingg the dependencies
+  // of a query, we may re-run dependency queries to see if their results change.
+  // Sometimes, these queries will check isQueryRunning. At this time, the
+  // actual query (this) is not running, but we want to return 'true' to hide
+  // the details of the query system from the query author. This field is set to
+  // 'true' when the query is being tested for re-computation, so that
+  // we can return 'true' when isQueryRunning is called in that case.
+  //
+  // Note (Daniel 10/08/2024): there may be a way to combine this field with
+  // lastChanged somehow, since that's what we use for isQueryRunning while
+  // the query really is running. However, the semantics of that are nontrivial,
+  // and that field is used for a lot of things, so for the time being, the
+  // extra boolean is fine.
+  mutable bool beingTestedForReuse = false;
 
   // Whether or not errors from this query result have been shown to the
   // user (they may not have been if some query checked for errors).
@@ -273,13 +285,16 @@ class QueryMapResultBase {
   // This is not too strongly connected to emittedErrors (which tracks whether
   // errors --- if any --- were shown to the user for this query result only)
   mutable bool errorsPresentInSelfOrDependencies = false;
+
+  mutable QueryDependencyVec dependencies;
   mutable std::set<const QueryMapResultBase*> recursionErrors;
   mutable QueryErrorVec errors;
 
   QueryMapBase* parentQueryMap;
 
   QueryMapResultBase(RevisionNumber lastChecked,
                      RevisionNumber lastChanged,
+                     bool beingTestedForReuse,
                      bool emittedErrors,
                      bool errorsPresentInSelfOrDependencies,
                      std::set<const QueryMapResultBase*> recursionErrors,
@@ -302,20 +317,21 @@ class QueryMapResult final : public QueryMapResultBase {
   //  * a default-constructed result
   QueryMapResult(QueryMap<ResultType, ArgTs...>* parentQueryMap,
                  std::tuple<ArgTs...> tupleOfArgs)
-    : QueryMapResultBase(-1, -1, false, false, {}, parentQueryMap),
+    : QueryMapResultBase(-1, -1, false, false, false, {}, parentQueryMap),
       tupleOfArgs(std::move(tupleOfArgs)),
       result() {
   }
   QueryMapResult(RevisionNumber lastChecked,
                  RevisionNumber lastChanged,
+                 bool beingTestedForReuse,
                  bool emittedErrors,
                  bool errorsPresentInSelfOrDependencies,
                  std::set<const QueryMapResultBase*> recursionErrors,
                  QueryMap<ResultType, ArgTs...>* parentQueryMap,
                  std::tuple<ArgTs...> tupleOfArgs,
                  ResultType result)
-    : QueryMapResultBase(lastChecked, lastChanged, emittedErrors,
-                         errorsPresentInSelfOrDependencies,
+    : QueryMapResultBase(lastChecked, lastChanged, beingTestedForReuse,
+                         emittedErrors, errorsPresentInSelfOrDependencies,
                          std::move(recursionErrors), parentQueryMap),
       tupleOfArgs(std::move(tupleOfArgs)),
       result(std::move(result)) {

frontend/include/chpl/framework/Context.h

@@ -227,6 +227,10 @@ class Context {
       context_ = nullptr;
     }
 
+    bool isCleared() {
+      return context_ == nullptr;
+    }
+
     ~RecomputeMarker() {
       restore();
     }

frontend/include/chpl/framework/query-impl.h

@@ -233,7 +233,9 @@ Context::getResult(QueryMap<ResultType, ArgTs...>* queryMap,
     //  printf("Found result %p %s\n", savedElement, queryMap->queryName);
   }
 
-  if (newElementWasAdded == false && savedElement->lastChecked == -1) {
+  if (newElementWasAdded == false &&
+      (savedElement->lastChecked == -1 ||
+       savedElement->beingTestedForReuse)) {
     // A recursion error was encountered. We will try to gracefully handle
     // this error by adding it to the set of recursion errors on this
     // result.
@@ -560,7 +562,7 @@ Context::isQueryRunning(
     return false;
   }
 
-  return search2->lastChecked == -1;
+  return search2->lastChecked == -1 || search2->beingTestedForReuse;
 }
 
 template<typename ResultType,

frontend/include/chpl/resolution/ResolutionContext.h

@@ -454,8 +454,14 @@ class ResolutionContext::GlobalQuery {
     }
 
     // Otherwise we are computing, so set the recompute marker.
+    // We don't want it to go out of scope now (since that undoes the mark),
+    // but this is just the 'begin' function; we do want it to go out of scope
+    // when the query is done (when 'end' is called). So, marker it in a field.
     const bool isRecomputing = false;
     auto activeRecomputeMarker = context_->markRecomputing(isRecomputing);
+
+    // We better not be saving another marker, since we only have room to save one.
+    CHPL_ASSERT(recomputeMarker_.isCleared());
     std::swap(recomputeMarker_, activeRecomputeMarker);
 
     // Set the stopwatch if it is compile-time enabled.

frontend/lib/framework/Context.cpp

@@ -1018,6 +1018,7 @@ void Context::recomputeIfNeeded(const QueryMapResultBase* resultEntry) {
   // changed since the last revision in which we computed this?
   // If so, compute it again.
   bool useSaved = true;
+  resultEntry->beingTestedForReuse = true;
   for (auto& dependency : resultEntry->dependencies) {
     const QueryMapResultBase* dependencyQuery = dependency.query;
     if (dependencyQuery->lastChanged > resultEntry->lastChanged) {
@@ -1042,6 +1043,7 @@ void Context::recomputeIfNeeded(const QueryMapResultBase* resultEntry) {
       }
     }
   }
+  resultEntry->beingTestedForReuse = false;
 
   if (useSaved == false) {
     auto marker = markRecomputing(true);
@@ -1124,6 +1126,7 @@ bool Context::queryCanUseSavedResult(
     useSaved = false;
   } else {
     useSaved = true;
+    resultEntry->beingTestedForReuse = true;
     for (auto& dependency: resultEntry->dependencies) {
       const QueryMapResultBase* dependencyQuery = dependency.query;
 
@@ -1142,6 +1145,7 @@ bool Context::queryCanUseSavedResult(
         break;
       }
     }
+    resultEntry->beingTestedForReuse = false;
     if (useSaved == true) {
       updateForReuse(resultEntry);
     }
@@ -1339,15 +1343,17 @@ void queryArgsPrintSep(std::ostream& s) {
 
 QueryMapResultBase::QueryMapResultBase(RevisionNumber lastChecked,
                    RevisionNumber lastChanged,
+                   bool beingTestedForReuse,
                    bool emittedErrors,
                    bool errorsPresentInSelfOrDependencies,
                    std::set<const QueryMapResultBase*> recursionErrors,
                    QueryMapBase* parentQueryMap)
   : lastChecked(lastChecked),
     lastChanged(lastChanged),
-    dependencies(),
+    beingTestedForReuse(beingTestedForReuse),
     emittedErrors(emittedErrors),
     errorsPresentInSelfOrDependencies(errorsPresentInSelfOrDependencies),
+    dependencies(),
     recursionErrors(std::move(recursionErrors)),
     errors(),
     parentQueryMap(parentQueryMap) {

frontend/test/resolution/testResolve.cpp

@@ -1761,6 +1761,48 @@ static void test29(Context* context) {
   }
 }
 
+// This bug is hard to replicate with queries alone, but does seem to show
+// up in some cases of the query system.
+static void testInfiniteCycleBug() {
+  auto context = buildStdContext();
+  auto ctx = context.get();
+
+  ctx->advanceToNextRevision(false);
+  setupModuleSearchPaths(ctx, false, false, {}, {});
+
+  CompilerFlags flags;
+  flags.set(CompilerFlags::WARN_UNSTABLE, true);
+  setCompilerFlags(ctx, std::move(flags));
+
+  std::string program0 =
+    R""""(
+    proc foo() {
+      var x = 0;
+      proc bar() { return x; }
+      return bar();
+    }
+    var x = foo();
+    )"""";
+
+  std::ignore = resolveQualifiedTypeOfX(ctx, program0);
+
+  ctx->advanceToNextRevision(false);
+  setupModuleSearchPaths(ctx, false, false, {}, {});
+
+  std::string program1 =
+    R""""(
+    proc baz() {
+      var x = 0;
+      proc ding() { return x; }
+      return bar();
+    }
+    var x = baz();
+    )"""";
+
+  std::ignore = resolveQualifiedTypeOfX(ctx, program1);
+}
+
+
 int main() {
   test1();
   test2();
@@ -1794,5 +1836,7 @@ int main() {
   test28(ctx.get());
   test29(ctx.get());
 
+  testInfiniteCycleBug();
+
   return 0;
 }