Dyno: support labeled break continue #26999
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Danila Fedorin <[email protected]>
Signed-off-by: Danila Fedorin <[email protected]>
Signed-off-by: Danila Fedorin <[email protected]>
Signed-off-by: Danila Fedorin <[email protected]>
Signed-off-by: Danila Fedorin <[email protected]>
Signed-off-by: Danila Fedorin <[email protected]>
Signed-off-by: Danila Fedorin <[email protected]>
Signed-off-by: Danila Fedorin <[email protected]>
3 tasks
benharsh
approved these changes
Mar 27, 2025
frontend/lib/resolution/Resolver.cpp
Outdated
| @@ -4147,6 +4180,10 @@ static QualifiedType computeDefaultsIfNecessary(Resolver& rv, | |||
| void Resolver::resolveIdentifier(const Identifier* ident) { | |||
| ResolvedExpression& result = byPostorder.byAst(ident); | |||
|
|
|||
| if (ident->id().str() == "looplabel@4") { | |||
| debuggerBreakHere(); | |||
| } | |||
Signed-off-by: Danila Fedorin <[email protected]>
Signed-off-by: Danila Fedorin <[email protected]>
DanilaFe
added a commit
that referenced
this pull request
Mar 28, 2025
…7004) 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 .
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes https://github.com/Cray/chapel-private/issues/7245.
As per #21712, this PR treats labels as being part of the same scoping structure as every other Chapel variable etc. Thus, this PR modifies scope populating code to store labeled loop into scopes for use in
continueandbreakstatements.This PR adds a new intent (in the spirit of
MODULE,FUNCTION, etc.), calledLOOP. This intent marks loop variables, and is intended to keep them separate from non-loop things. The PR also adds some checks to ensure that loop references can only occur inbreakandcontinue, and thatbreakandcontinuewith non-loop things are disallowed.For control-flow tracking purposes, this PR generalizes the previous "continues" flag to a "pointer to loop continued by this statement". This way, we can differentiate branches that continue different loops. This naturally enables resolving nested
paramloops with complicated control flow, as well.Since we don't try reasoning about the dynamic behavior of non-
paramloops (their body might not run at all, etc.), this has limited impact overall. However, I still believe that labeledparamloops and the nicer error messages are important.Reviewed by @benharsh -- thanks!
Testing