-
Notifications
You must be signed in to change notification settings - Fork 10.6k
Add AST representation for coroutines #85372
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
base: main
Are you sure you want to change the base?
Conversation
|
@tkremenek @rjmccall Following discussions on LLVM Dev Meeting, this is a stripped-down version of #78508:
Essentially the PR contains AST-related bits for coroutine declarations and coroutine function types as well as required boilerplate for AST mangling, dumping, serialization, etc. @xedin comments from the original PR were addressed as well. |
|
Please test with following pull request: swiftlang/llvm-project#11396 |
8805213 to
29ab221
Compare
|
Please test with following pull request: swiftlang/llvm-project#11396 |
29ab221 to
28dadc4
Compare
|
Please test with following pull request: swiftlang/llvm-project#11396 |
|
Please test with following pull request: swiftlang/llvm-project#11396 |
|
swiftlang/swift-syntax#3225 |
|
The updated PR contains the support for yields w/o dedicated Things to note:
Please let me know if there is something that should be changed / done differently in this approach. |
|
swiftlang/swift-syntax#3225 |
|
swiftlang/swift-syntax#3225 swiftlang/llvm-project#12064 |
|
Looks like |
|
swiftlang/swift-syntax#3225 swiftlang/llvm-project#12064 |
rjmccall
left a comment
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 just worrying about the AST part of this is fine for now, yeah. This seems like good progress.
| HasSendingResult = 0x00000010U, | ||
|
|
||
| // Values if we have any yields | ||
| IsCoroutine = 0x00000020U, |
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.
Do you really need runtime metadata support? I don't think there's anything wrong with the code you're adding here, but I don't know if I'm happy about baking ABI support into the runtime for a feature we haven't actually added to the language.
Here (and many other places), I would like to avoid baking in that "coroutine" means this specific kind of coroutine. We do have other coroutine-like features, and I think we will probably add more. "YieldOnceCoroutine" is unambiguous.
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, just to clarify – in other places you'd also want to have, say FunctionType::isYieldOnceCoroutine()? This looks different from e.g. SIL level when SILFunctionType::isCoroutine() means any coroutine (being it yield_once or yield_many).
|
|
||
| public: | ||
| /// 'Constructor' Factory Function | ||
| #if 0 |
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.
Refactor residue?
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.
Oh, no. As I mentioned, I have not decided whether to keep or not yield-less ctors. As one one hand they are less verbose, but on other hand make it easy to forget about yields leading to subtle unintended differences
| } | ||
|
|
||
| CanFunctionType substGenericArgs(SubstitutionMap subs) const; | ||
| #if 0 |
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.
?
| Out << "\n"; | ||
| abort(); | ||
| } | ||
| if (FT->hasExtInfo() && FT->isCoroutine()) { |
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.
| if (FT->hasExtInfo() && FT->isCoroutine()) { | |
| if (FT->isCoroutine()) { |
| _label("operation", | ||
| _sending(_function(_async(_throws(_thick)), _typeparam(0), | ||
| _parameters())))), | ||
| _parameters(), _yields())))), |
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.
We should be able to figure out a way to not need a builder for the yields when we don't have any.
| FunctionType::ExtInfo info; | ||
| auto innerFunction = | ||
| FunctionType::get(params, linearFnResultGen.build(builder), info); | ||
| FunctionType::get(params, {}, linearFnResultGen.build(builder), info); |
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.
It's unfortunate that we need this change in a million places. It was probably really nice for writing this patch that you had to fix a bunch of generic places to propagate and handle yields, but now that that's done, I wonder if we shouldn't just have a factory that defaults to not having any yields.
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.
That's exactly what I mentioned above. I do not know :) I'm worrying that the caller might forget about yields and it might lead to dropping yields should they present during various things here and there
…unction types and declarations
|
@swift-ci please test |
This is a proof-of-concept PR adding AST representation for coroutines
Further background at https://forums.swift.org/t/pitch-yield-once-functions-first-class-coroutines/77081