-
Notifications
You must be signed in to change notification settings - Fork 384
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
Support Control Flow type Refinements for "break" and "continue" statements #1004
Merged
alexmccord
merged 7 commits into
luau-lang:master
from
AmberGraceSoftware:loop-controlflow-refinement-amber
Sep 21, 2023
Merged
Support Control Flow type Refinements for "break" and "continue" statements #1004
alexmccord
merged 7 commits into
luau-lang:master
from
AmberGraceSoftware:loop-controlflow-refinement-amber
Sep 21, 2023
Conversation
This file contains 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
alexmccord
reviewed
Sep 21, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the extremely delayed review! I have some changes that I want to see first before I can merge this in.
Ended up pushing the changes I wanted to see because it's been sitting in limbo for too long and I didn't want it to wait longer. |
alexmccord
approved these changes
Sep 21, 2023
Thanks! |
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.
Fixes: #913
This PR adds support for type refinements around guard clauses that use
break
andcontinue
statements inside a loop, similar to how guard clauses withreturn
is supported.I had some free time today, so I figure I'd give a shot at a naïve fix for this at the very least.
Resulting Change:
Luau now supports type refinements within loops where a
continue
orbreak
guard clause was used.For example:
ControlFlow enum
I added some bit flags to the ControlFlow enum, as it looks like was defined in an incomplete/imprecise way. There are inline comments in the
ControlFlow.h
file that you are free to remove; I added them for the sake of documenting my reasoning.The changes to the ControlFlow enum make it more explicit that edge cases of mixed exits need to be considered in future additions to control flow analysis.
A "MixedExit" control flow looks something like this, where a conditional always has an exit, but this exit might be to the nearest loop scope, or to the nearest function scope:
A "MixedFunctionExit" is specifically a mixed exit that always exits the nearest function scope:
A "MixedLoopExit" is specifically a mixed exit that always exits the nearest loop scope:
It looks like this proliferation was what the previous author of this type refinement code was getting at, but didn't completely represent. For example, calling
TypeInfer::check()
on an if statement would always returnControlFlow::Return
for any mixed function return, even if that mixed return might actually include an exception rather than a return.Probably makes no difference either way, but I figured specificity is better than ambiguity in this instance.