Dyno: adjust compilerError to interrupt resolution of functions (#27004)

Resolves Cray/chapel-private#7229.

Depends on #26999.

Quoting from that issue: a lot of Chapel production code (including in
library modules) looks like this:

```Chapel
proc foo(param cond) {
    if cond then compilerError("This condition should not be met");
    codeThatOnlyCompilesIfCondIsFalse();
}
```

These rely on the fact that `codeThatOnlyCompilesIfCondIsFalse()` will
not be resolved if an error is already emitted. This means terminating
resolution of `foo()`, much like we do with `return` in
#26955.
This PR implements that.

One conceptual curiosity is that this conflicts with Dyno's existing
approach of attempting to continue resolving programs to get partial
information. Specifically, expressions that emitted errors are marked
with `ErroneousType`, and if the rest of the types can be inferred
despite the error, we infer them. This is desirable (partial information
is useful in editors, for example), but counter to stopping resolution
in its tracks. This PR goes for a two-phase handling of `compilerError`,
which distinguishes functions that call compiler error directly and
functions that simply observe errors emitted by a call.

* __For functions that invoke `compilerError` directly__, this PR makes
use of the changes in #26955
to stop resolution in its tracks. This requires some communication
between the resolver and the other branch-sensitive passes to inform
them of the fatal error, which I achieve using a new flag on
`ResolvedExpression` called `causedFatalError_`[^1].
* Functions that are meant as "helpers" for invoking `compilerError`
(i.e., those that are ignored using the `depth` argument) are considered
"direct" callers to `compilerError` for the purposes of this phase.
That's because they mostly just serve to augment the `compilerError` in
some way.
* __For invocations of functions that error__, this PR simply marks the
call as `ErroneousType`, without interrupting the resolution of the
current function etc. Thus, if we call `foo()`, and `foo()` issues a
`compilerError`, we print the error and mark `foo()` as `ErroneousType`,
but (as described above) continue resolving with partial information.

This way, I believe we can reconcile the expectation of `compilerError`
as terminating resolution, while still allowing us to compute partial
information in other contexts. See the program tested by this PR:

```Chapel
      proc foo() {
        bar();
        return 42;
      }
      proc bar() {
        compilerError("Hello");
        compilerWarnning("inside bar, after call to compilerError");
      }
      var x = foo();
```

Here, the error "Hello" immediately terminates resolution of `bar()` (so
the "inside bar" warning is never printed). This error is issued on the
`bar();` line, but we proceed past this line, and, since the `return 42`
doesn't depend on the call to `bar()`, correctly determine that the `var
x` is an `int`.

## Testing
- [x] dyno tests
- [x] paratest
- [x] spectests still work as before

Reviewed by @benharsh -- thanks!

[^1]: I have checked and on my machine, this flag does not cause the
`ResolvedExpression` type to increaase in size, presumably due to the
way that struct is padded. So this does not cause size / memory
penalties .

Author: DanilaFe

frontend/include/chpl/resolution/resolution-types.h

@@ -2408,6 +2408,8 @@ class ResolvedExpression {
   ID toId_;
   // Is this a reference to a compiler-created primitive?
   bool isBuiltin_ = false;
+  // Did this expression cause the function to stop being resolved?
+  bool causedFatalError_ = false;
 
   // For a function call, what is the most specific candidate,
   // or when using return intent overloading, what are the most specific
@@ -2440,6 +2442,9 @@ class ResolvedExpression {
   /** check whether this resolution result refers to a compiler builtin like `bool`. */
   bool isBuiltin() const { return isBuiltin_; }
 
+  /** check whether this resolution result caused a fatal error. */
+  bool causedFatalError() const { return causedFatalError_; }
+
   /** For a function call, what is the most specific candidate, or when using
    * return intent overloading, what are the most specific candidates? The
    * choice between these needs to happen later than the main function
@@ -2464,6 +2469,9 @@ class ResolvedExpression {
   /** set the isPrimitive flag */
   void setIsBuiltin(bool isBuiltin) { isBuiltin_ = isBuiltin; }
 
+  /** set the causedFatalError flag */
+  void setCausedFatalError(bool causedFatalError) { causedFatalError_ = causedFatalError; }
+
   /** set the toId */
   void setToId(ID toId) { toId_ = toId; }
 
@@ -2491,6 +2499,7 @@ class ResolvedExpression {
     return type_ == other.type_ &&
            toId_ == other.toId_ &&
            isBuiltin_ == other.isBuiltin_ &&
+           causedFatalError_ == other.causedFatalError_ &&
            mostSpecific_ == other.mostSpecific_ &&
            poiScope_ == other.poiScope_ &&
            associatedActions_ == other.associatedActions_ &&
@@ -2503,6 +2512,7 @@ class ResolvedExpression {
     type_.swap(other.type_);
     toId_.swap(other.toId_);
     std::swap(isBuiltin_, other.isBuiltin_);
+    std::swap(causedFatalError_, other.causedFatalError_);
     mostSpecific_.swap(other.mostSpecific_);
     std::swap(poiScope_, other.poiScope_);
     std::swap(associatedActions_, other.associatedActions_);

frontend/lib/resolution/BranchSensitiveVisitor.h

@@ -46,14 +46,18 @@ struct ControlFlowInfo {
   // if this frame hit a 'continue', the loop it continued
   const uast::Loop* loopContinues_ = nullptr;
 
+  // did we encounter a 'compilerError'? These terminate resolution.
+  bool fatalError_ = false;
+
  public:
   ControlFlowInfo() = default;
 
   /* have we hit a statement that would prevent further execution of a block? */
-  bool isDoneExecuting() const { return returnsOrThrows_ || loopBreaks_ || loopContinues_; }
+  bool isDoneExecuting() const { return returnsOrThrows_ || loopBreaks_ || loopContinues_ || fatalError_; }
   bool returnsOrThrows() const { return returnsOrThrows_; }
   bool breaks() const { return loopBreaks_ != nullptr; }
   bool continues() const { return loopContinues_ != nullptr; }
+  bool fatalError() const { return fatalError_; }
 
   // The below mark functions check if the block is already done executing.
   // This is useful because in cases like 'return; continue', we don't want
@@ -63,6 +67,7 @@ struct ControlFlowInfo {
   void markReturnOrThrow() { if (!isDoneExecuting()) returnsOrThrows_ = true; }
   void markBreak(const uast::Loop* markWith) { if (!isDoneExecuting()) loopBreaks_ = markWith; }
   void markContinue(const uast::Loop* markWith) { if (!isDoneExecuting()) loopContinues_ = markWith; }
+  void markFatalError() { if (!isDoneExecuting()) fatalError_ = true; }
 
   // resetting break and continue is useful for when we are handling 'param' for loops.
   // If we hit a continue, we might want to move on to the next iteration of
@@ -89,6 +94,9 @@ struct ControlFlowInfo {
       loopContinues_ = nullptr;
     }
 
+    // errors in one branch are sufficient to infect all branches
+    fatalError_ |= other.fatalError_;
+
     return *this;
   }
 
@@ -107,6 +115,7 @@ struct ControlFlowInfo {
       returnsOrThrows_ |= other.returnsOrThrows_;
       loopBreaks_ = other.loopBreaks_;
       loopContinues_ = other.loopContinues_;
+      fatalError_ |= other.fatalError_;
     }
   }
 
@@ -119,6 +128,7 @@ struct ControlFlowInfo {
       returnsOrThrows_ |= other.returnsOrThrows_;
       if (other.loopBreaks_ != loop) loopBreaks_ = other.loopBreaks_;
       if (other.loopContinues_ != loop) loopContinues_ = other.loopContinues_;
+      fatalError_ |= other.fatalError_;
     }
   }
 };
@@ -492,6 +502,12 @@ struct BranchSensitiveVisitor {
     currentFrame()->controlFlowInfo.markContinue(loop);
   }
 
+  /** note that the current frame, if any, has encountered a fatal error */
+  void markFatalError() {
+    if (scopeStack.empty()) return;
+    currentFrame()->controlFlowInfo.markFatalError();
+  }
+
   /** have we hit a statement that would prevent further execution of the current block / frame? */
   bool isDoneExecuting() {
     if (scopeStack.empty()) return false;

frontend/lib/resolution/Resolver.cpp

@@ -2300,6 +2300,7 @@ bool Resolver::CallResultWrapper::noteResultWithoutError(
     const CallResolutionResult& result,
     optional<ActionAndId> actionAndId) {
   bool needsErrors = false;
+  bool markErroneous = false;
 
   for (auto& diagnostic : gatherUserDiagnostics(resolver.rc, result)) {
     // The diagnostic's depth means it's aimed for further up the call stack.
@@ -2308,14 +2309,26 @@ bool Resolver::CallResultWrapper::noteResultWithoutError(
       resolver.userDiagnostics.emplace_back(diagnostic.message(),
                                             diagnostic.kind(),
                                             diagnostic.depth() - 1);
+
+      if (diagnostic.kind() == CompilerDiagnostic::ERROR) {
+        if (!astForContext->isFnCall()) {
+          resolver.context->warning(astForContext, "propagating compiler errors past non-call frames is not supported");
+        }
+
+        // functions that invoke compilerError(), and their helper code skipped
+        // via the depth, should not be resolved past the point of the error.
+        resolver.markFatalError();
+        if (r) r->setCausedFatalError(true);
+      }
     } else if (diagnostic.depth() - 1 == 0) {
       // we're asked not to emit errors, and only return if errors are
       // needed.
       needsErrors = true;
+      markErroneous = diagnostic.kind() == CompilerDiagnostic::ERROR;
     }
   }
 
-  if (!result.exprType().hasTypePtr()) {
+  if (!result.exprType().hasTypePtr() || markErroneous) {
     if (!actionAndId && r) {
       // Only set the type to erroneous if we're handling an actual user call,
       // and not an associated action.

frontend/lib/resolution/VarScopeVisitor.cpp

@@ -340,97 +340,102 @@ void VarScopeVisitor::exit(const OpCall* ast, RV& rv) {
   exitAst(ast);
 }
 
-
 bool VarScopeVisitor::enter(const FnCall* callAst, RV& rv) {
   enterAst(callAst);
 
-  if (rv.hasAst(callAst)) {
-    // Does any return-intent-overload use 'in', 'out', or 'inout'?
-    // This filter is intended as an optimization.
-    const MostSpecificCandidates& candidates = rv.byAst(callAst).mostSpecific();
-    bool anyInOutInout = false;
-    bool isMethod = false;
-    for (const MostSpecificCandidate& candidate : candidates) {
-      if (candidate) {
-        auto fn = candidate.fn();
-        if (fn->untyped()->isMethod()) isMethod = true;
-
-        int n = fn->numFormals();
-        for (int i = 0; i < n; i++) {
-          const QualifiedType& formalQt = fn->formalType(i);
-          auto kind = formalQt.kind();
-          if (kind == QualifiedType::OUT ||
-              kind == QualifiedType::IN || kind == QualifiedType::CONST_IN ||
-              kind == QualifiedType::INOUT) {
-            anyInOutInout = true;
-            break;
-          }
-        }
-        if (anyInOutInout) {
+  auto rr = rv.byPostorder().byAstOrNull(callAst);
+  if (!rr) return false;
+
+  if (rr->causedFatalError()) {
+    markFatalError();
+    return false;
+  }
+
+  // Does any return-intent-overload use 'in', 'out', or 'inout'?
+  // This filter is intended as an optimization.
+  const MostSpecificCandidates& candidates = rr->mostSpecific();
+  bool anyInOutInout = false;
+  bool isMethod = false;
+  for (const MostSpecificCandidate& candidate : candidates) {
+    if (candidate) {
+      auto fn = candidate.fn();
+      if (fn->untyped()->isMethod()) isMethod = true;
+
+      int n = fn->numFormals();
+      for (int i = 0; i < n; i++) {
+        const QualifiedType& formalQt = fn->formalType(i);
+        auto kind = formalQt.kind();
+        if (kind == QualifiedType::OUT ||
+            kind == QualifiedType::IN || kind == QualifiedType::CONST_IN ||
+            kind == QualifiedType::INOUT) {
+          anyInOutInout = true;
           break;
         }
       }
+      if (anyInOutInout) {
+        break;
+      }
+    }
+  }
+
+  if (!anyInOutInout) {
+    // visit the actuals to gather mentions
+    for (auto actualAst : callAst->actuals()) {
+      actualAst->traverse(rv);
+    }
+  } else {
+    // Use FormalActualMap to figure out which variable ID
+    // is passed to a formal with out/in/inout intent.
+    // Issue an error if it does not match among return intent overloads.
+    //
+    // TODO: Should we store the resolved CallInfo so we don't need to build
+    // it back up here?
+    std::vector<const AstNode*> actualAsts;
+    auto ci = CallInfo::create(context, callAst, rv.byPostorder(),
+                               /* raiseErrors */ false,
+                               &actualAsts);
+
+    if (isMethod && ci.isMethodCall() == false) {
+      // Create a dummy 'this' actual
+      ci = ci.createWithReceiver(ci, QualifiedType());
+      actualAsts.insert(actualAsts.begin(), nullptr);
     }
 
-    if (!anyInOutInout) {
-      // visit the actuals to gather mentions
-      for (auto actualAst : callAst->actuals()) {
+    // compute a vector indicating which actuals are passed to
+    // an 'out' formal in all return intent overloads
+    std::vector<QualifiedType> actualFormalTypes;
+    std::vector<Qualifier> actualFormalIntents;
+    computeActualFormalIntents(context, candidates, ci, actualAsts,
+                               actualFormalIntents, actualFormalTypes);
+
+    int actualIdx = 0;
+    for (auto actual : ci.actuals()) {
+      (void) actual; // avoid compilation error about unused variable
+
+      const AstNode* actualAst = actualAsts[actualIdx];
+      Qualifier kind = actualFormalIntents[actualIdx];
+
+      // handle an actual that is passed to an 'out'/'in'/'inout' formal
+      if (actualAst == nullptr) {
+        CHPL_ASSERT(ci.isMethodCall() && actualIdx == 0);
+      } else if (kind == Qualifier::OUT) {
+        handleOutFormal(callAst, actualAst,
+                        actualFormalTypes[actualIdx], rv);
+      } else if ((kind == Qualifier::IN || kind == Qualifier::CONST_IN) &&
+                 !(ci.name() == "init" && actualIdx == 0)) {
+        // don't do this for the 'this' argument to 'init', because it
+        // is not getting copied.
+        handleInFormal(callAst, actualAst,
+                       actualFormalTypes[actualIdx], rv);
+      } else if (kind == Qualifier::INOUT) {
+        handleInoutFormal(callAst, actualAst,
+                          actualFormalTypes[actualIdx], rv);
+      } else {
+        // otherwise, visit the actuals to gather mentions
         actualAst->traverse(rv);
       }
-    } else {
-      // Use FormalActualMap to figure out which variable ID
-      // is passed to a formal with out/in/inout intent.
-      // Issue an error if it does not match among return intent overloads.
-      //
-      // TODO: Should we store the resolved CallInfo so we don't need to build
-      // it back up here?
-      std::vector<const AstNode*> actualAsts;
-      auto ci = CallInfo::create(context, callAst, rv.byPostorder(),
-                                 /* raiseErrors */ false,
-                                 &actualAsts);
-
-      if (isMethod && ci.isMethodCall() == false) {
-        // Create a dummy 'this' actual
-        ci = ci.createWithReceiver(ci, QualifiedType());
-        actualAsts.insert(actualAsts.begin(), nullptr);
-      }
 
-      // compute a vector indicating which actuals are passed to
-      // an 'out' formal in all return intent overloads
-      std::vector<QualifiedType> actualFormalTypes;
-      std::vector<Qualifier> actualFormalIntents;
-      computeActualFormalIntents(context, candidates, ci, actualAsts,
-                                 actualFormalIntents, actualFormalTypes);
-
-      int actualIdx = 0;
-      for (auto actual : ci.actuals()) {
-        (void) actual; // avoid compilation error about unused variable
-
-        const AstNode* actualAst = actualAsts[actualIdx];
-        Qualifier kind = actualFormalIntents[actualIdx];
-
-        // handle an actual that is passed to an 'out'/'in'/'inout' formal
-        if (actualAst == nullptr) {
-          CHPL_ASSERT(ci.isMethodCall() && actualIdx == 0);
-        } else if (kind == Qualifier::OUT) {
-          handleOutFormal(callAst, actualAst,
-                          actualFormalTypes[actualIdx], rv);
-        } else if ((kind == Qualifier::IN || kind == Qualifier::CONST_IN) &&
-                   !(ci.name() == "init" && actualIdx == 0)) {
-          // don't do this for the 'this' argument to 'init', because it
-          // is not getting copied.
-          handleInFormal(callAst, actualAst,
-                         actualFormalTypes[actualIdx], rv);
-        } else if (kind == Qualifier::INOUT) {
-          handleInoutFormal(callAst, actualAst,
-                            actualFormalTypes[actualIdx], rv);
-        } else {
-          // otherwise, visit the actuals to gather mentions
-          actualAst->traverse(rv);
-        }
-
-        actualIdx++;
-      }
+      actualIdx++;
     }
   }
 

frontend/lib/resolution/return-type-inference.cpp

@@ -337,6 +337,9 @@ struct ReturnTypeInferrer : BranchSensitiveVisitor<DefaultFrame, ResolvedVisitor
   const types::Param* determineIfValue(const uast::AstNode* ast, RV& rv) override;
   void traverseNode(const uast::AstNode* ast, RV& rv) override;
 
+  bool enter(const FnCall* ast, RV& rv);
+  void exit(const FnCall* ast, RV& rv);
+
   bool enter(const Function* fn, RV& rv);
   void exit(const Function* fn, RV& rv);
 
@@ -505,6 +508,22 @@ void ReturnTypeInferrer::traverseNode(const uast::AstNode* ast, RV& rv) {
   ast->traverse(rv);
 }
 
+bool ReturnTypeInferrer::enter(const FnCall* ast, RV& rv) {
+  enterScope(ast, rv);
+
+  if (auto rr = rv.byPostorder().byAstOrNull(ast)) {
+    if (rr->causedFatalError()) {
+      markFatalError();
+    }
+  }
+
+  return true;
+}
+
+void ReturnTypeInferrer::exit(const FnCall* ast, RV& rv) {
+  exitScope(ast, rv);
+}
+
 bool ReturnTypeInferrer::enter(const Function* fn, RV& rv) {
   return false;
 }