Dyno: fix query recursion bug

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

@@ -228,17 +228,47 @@ auto queryArgsToStrings(const std::tuple<Ts...>& tuple) {
   // return ss.str();
 }
 
-// Performance: this struct only contains a pointer and an additional bit
-// field. We could probably get away with storing `errorCollectionRoot`
-// in the last bit of the result pointer, and and thus reduce the overhead
+// Performance: this struct only contains a pointer and a few additional small
+// fields. We could possibly get away with storing these bit fields
+// in the last bits of the result pointer, and and thus reduce the overhead
 // of this struct.
 struct QueryDependency {
+  enum DependencyKind {
+    /* Dependency introduced via a call to another query. */
+    DEPENDENCY_DIRECT,
+    /* Dependency introduced via 'isQueryRunning', and 'isQueryRunning' returned
+       true. */
+    DEPENDENCY_CHECK_TRUE,
+    /* Dependency introduced via 'isQueryRunning', and 'isQueryRunning' returned
+       false. */
+    DEPENDENCY_CHECK_FALSE,
+  };
+
   const QueryMapResultBase* query;
   bool errorCollectionRoot;
+  DependencyKind kind;
 
   QueryDependency(const QueryMapResultBase* query,
-                  bool errorCollectionRoot) :
-    query(query), errorCollectionRoot(errorCollectionRoot) {}
+                  bool errorCollectionRoot,
+                  DependencyKind kind) :
+    query(query), errorCollectionRoot(errorCollectionRoot), kind(kind) {}
+
+  bool isCheck() const {
+    return kind == DEPENDENCY_CHECK_TRUE || kind == DEPENDENCY_CHECK_FALSE;
+  }
+
+  bool isDirect() const {
+    return kind == DEPENDENCY_DIRECT;
+  }
+
+  // returns true when all of the following conditions are met:
+  //
+  //   * this is a check dependency (introduced via isQueryRunning)
+  //   * the result of isQueryRunning has changed since this dependency was added
+  //
+  // This means that regardless of the result of the query or its dependencies,
+  // the dependent query should be re-run.
+  bool checkHasChanged() const;
 };
 
 using QueryDependencyVec = std::vector<QueryDependency>;

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

@@ -555,24 +555,58 @@ bool
 Context::isQueryRunning(
      const ResultType& (*queryFunction)(Context* context, ArgTs...),
      const std::tuple<ArgTs...>& tupleOfArgs) {
+  // The parent query has effectively created a dependecy on the query being
+  // checked (since its behavior will be different depending on if it's running),
+  // so we need to note it as a check dependency. However, the query map,
+  // or query result entry, may not exist yet, so create empty ones if they
+  // don't. If the query being checked has never been run, we'll insert
+  // a never-executed result, which will get executed when needed.
+
   // Look up the map entry for this query name
   const void* queryFuncV = (const void*) queryFunction;
   // Look up the map entry for this query
   auto search = this->queryDB.find(queryFuncV);
   if (search == this->queryDB.end()) {
+    // need to create a new entry for the query but that's hard because we need
+    // to know its string name and whether it's an input query. Can't assert
+    // here since the first call to (isQueryRunning() else query()) will
+    // hit the assertion, always. So, just return false.
+    //
+    // TODO: can we do something clever here?
     return false;
   }
 
   // found an entry for this query
   QueryMapBase* base = search->second.get();
   auto queryMap = (QueryMap<ResultType, ArgTs...>*)base;
+
+  // find the query, or create a new uninitialized result
   QueryMapResult<ResultType, ArgTs...> key(queryMap, tupleOfArgs);
+  const QueryMapResult<ResultType, ArgTs...>* dependencyQuery = nullptr;
   auto search2 = queryMap->map.find(key);
   if (search2 == queryMap->map.end()) {
-    return false;
+    // no query like this has ever been executed before. Insert a blank result,
+    // without running the query, so that we have something to list as a dependency.
+    dependencyQuery = getResult(queryMap, tupleOfArgs);
+    dependencyQuery->lastChecked = -2; // special value: never checked, but also not running.
+  } else {
+    dependencyQuery = &(*search2);
+  }
+
+  bool isRunning = dependencyQuery->lastChecked == -1 || dependencyQuery->beingTestedForReuse;
+
+  if (!queryStack.empty()) {
+    // The parent query has effectively created a dependecy on search2
+    // by checking if it's running, so note that. Note that it's safe to
+    // always add an edge here instead of checking for recursion errors
+    // since these dependency edges are not traversed.
+    auto kind = isRunning ? QueryDependency::DEPENDENCY_CHECK_TRUE
+                          : QueryDependency::DEPENDENCY_CHECK_FALSE;
+    auto newDependency = QueryDependency(dependencyQuery, false, kind);
+    queryStack.back()->dependencies.push_back(newDependency);
   }
 
-  return search2->lastChecked == -1 || search2->beingTestedForReuse;
+  return isRunning;
 }
 
 template<typename ResultType,

frontend/lib/framework/Context.cpp

@@ -567,6 +567,8 @@ void Context::gatherRecursionTrace(const querydetail::QueryMapResultBase* root,
 
   CHPL_ASSERT(result->recursionErrors.count(root) > 0);
   for (auto& dep : result->dependencies) {
+    if (!dep.isDirect()) continue;
+
     if (dep.query->recursionErrors.count(root) > 0) {
       if (auto te = dep.query->tryTrace()) {
         trace.emplace_back(*te);
@@ -1020,6 +1022,14 @@ void Context::recomputeIfNeeded(const QueryMapResultBase* resultEntry) {
   bool useSaved = true;
   resultEntry->beingTestedForReuse = true;
   for (auto& dependency : resultEntry->dependencies) {
+    if (dependency.isCheck()) {
+      if (dependency.checkHasChanged()) {
+        useSaved = false;
+        break;
+      }
+      continue;
+    }
+
     const QueryMapResultBase* dependencyQuery = dependency.query;
     if (dependencyQuery->lastChanged > resultEntry->lastChanged) {
       useSaved = false;
@@ -1128,6 +1138,14 @@ bool Context::queryCanUseSavedResult(
     useSaved = true;
     resultEntry->beingTestedForReuse = true;
     for (auto& dependency: resultEntry->dependencies) {
+      if (dependency.isCheck()) {
+        if (dependency.checkHasChanged()) {
+          useSaved = false;
+          break;
+        }
+        continue;
+      }
+
       const QueryMapResultBase* dependencyQuery = dependency.query;
 
       if (dependency.errorCollectionRoot) {
@@ -1187,6 +1205,8 @@ void Context::emitHiddenErrorsFor(const querydetail::QueryMapResultBase* result)
   }
   result->emittedErrors = true;
   for (auto& dependency : result->dependencies) {
+    if (!dependency.isDirect()) continue;
+
     if (!dependency.query->emittedErrors && !dependency.errorCollectionRoot) {
       emitHiddenErrorsFor(dependency.query);
     }
@@ -1202,6 +1222,8 @@ static void storeErrorsForHelp(const querydetail::QueryMapResultBase* result,
     into.storeError(error.first->clone());
   }
   for (auto& dependency : result->dependencies) {
+    if (!dependency.isDirect()) continue;
+
     if (!dependency.errorCollectionRoot &&
         dependency.query->errorsPresentInSelfOrDependencies) {
       storeErrorsForHelp(dependency.query, visited, into);
@@ -1236,7 +1258,9 @@ void Context::saveDependencyInParent(const QueryMapResultBase* resultEntry) {
     } else {
       bool errorCollectionRoot = !errorCollectionStack.empty() &&
                                  errorCollectionStack.back().collectingQuery() == parentQuery;
-      parentQuery->dependencies.push_back(QueryDependency(resultEntry, errorCollectionRoot));
+      auto newDependency = QueryDependency(resultEntry, errorCollectionRoot,
+                                           QueryDependency::DEPENDENCY_DIRECT);
+      parentQuery->dependencies.push_back(std::move(newDependency));
       parentQuery->errorsPresentInSelfOrDependencies |=
         resultEntry->errorsPresentInSelfOrDependencies;
     }
@@ -1342,6 +1366,14 @@ void queryArgsPrintSep(std::ostream& s) {
   s << ", ";
 }
 
+bool QueryDependency::checkHasChanged() const {
+  CHPL_ASSERT(isCheck());
+  // note: the condition on 'query' should match isQueryRunning
+  bool isRunning = query->lastChecked == -1 || query->beingTestedForReuse;
+  return ((isRunning && kind == DEPENDENCY_CHECK_FALSE) ||
+          (!isRunning && kind == DEPENDENCY_CHECK_TRUE));
+}
+
 QueryMapResultBase::QueryMapResultBase(RevisionNumber lastChecked,
                    RevisionNumber lastChanged,
                    bool beingTestedForReuse,