-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feature: Add basic support for become
expr/tail calls
#15003
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
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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
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
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
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
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
31 changes: 31 additions & 0 deletions
31
crates/parser/test_data/parser/inline/ok/0209_become_expr.rast
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
SOURCE_FILE | ||
FN | ||
FN_KW "fn" | ||
WHITESPACE " " | ||
NAME | ||
IDENT "foo" | ||
PARAM_LIST | ||
L_PAREN "(" | ||
R_PAREN ")" | ||
WHITESPACE " " | ||
BLOCK_EXPR | ||
STMT_LIST | ||
L_CURLY "{" | ||
WHITESPACE "\n " | ||
EXPR_STMT | ||
BECOME_EXPR | ||
BECOME_KW "become" | ||
WHITESPACE " " | ||
CALL_EXPR | ||
PATH_EXPR | ||
PATH | ||
PATH_SEGMENT | ||
NAME_REF | ||
IDENT "foo" | ||
ARG_LIST | ||
L_PAREN "(" | ||
R_PAREN ")" | ||
SEMICOLON ";" | ||
WHITESPACE "\n" | ||
R_CURLY "}" | ||
WHITESPACE "\n" |
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
fn foo() { | ||
become foo(); | ||
} |
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
Oops, something went wrong.
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.
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.
MIR support is actually not hard (I already have a MIR impl in the compiler after all), but I'm not sure how to properly lower it from hir.
become x
is only valid ifx
is a call (f(x)
,y.m()
) and MIR really would like to see this as simple as possible.In the compiler I lower HIR->THIR as-is, then run a check on THIR that tail calls are well-formed and then do something akin to this:
Not sure what is the best way to do this in r-a
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.
What is the problem of doing the same in r-a? Just instead of
bug!
usenot_supported!
or add a variant toMirLowerError
.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.
The problem is that THIR has much simpler representation of functions and I'm not sure how to correctly handle both function and method calls.
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.
Aha. How it will be a lowered into mir? It would be a new terminator, like
BecomeCall
?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.
Yes, it will be a new
TailCall
terminator. There is an experimental branch, it looks like this: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.
So I see three ways to implement it:
become
doesn't support overloaded operators, it is just call and method call.InferenceResult
for recording the simple form of function calls (or changing the currentmethod_resolution
field). This is similar to THIR in rustc.MirLowerCtx
, which if true it means the nextTerminator::Call
should beTerminator::TailCall
.If you want to go with 2, I would suggest doing it in a separate PR, to minimize git conflicts (Assuming this PR is going to be not merged in a long time)
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.
I think 3 is error prone with code like
2 I'm not exactly sure I understand.
Yes, this is true.
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.
I think 1 would be easiest to go with for now even if it's not the cleanest. 2 seems kind of invasive with the current structure I think
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.
This is also my understanding, yes. I'll try option 1.