Skip to content

Dyno: adjust compilerError to interrupt resolution of functions #27004

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Mar 28, 2025

Conversation

DanilaFe
Copy link
Contributor

@DanilaFe DanilaFe commented Mar 27, 2025

Resolves https://github.com/Cray/chapel-private/issues/7229.

Depends on #26999.

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

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 Dyno: unify return, break, throw etc. handling across Resolver, type inference, and split init etc. #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:

      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

  • dyno tests
  • paratest
  • spectests still work as before

Reviewed by @benharsh -- thanks!

Footnotes

  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 .

@DanilaFe DanilaFe force-pushed the compiler-error-flow branch from e84f95c to 76d416b Compare March 27, 2025 20:45
@DanilaFe DanilaFe requested a review from benharsh March 27, 2025 23:52
@DanilaFe DanilaFe merged commit 0553761 into chapel-lang:main Mar 28, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants