-
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 |
include/swift/AST/Types.h
Outdated
| static bool classof(const TypeBase *T) { | ||
| return T->getKind() == TypeKind::YieldResult; | ||
| } | ||
| }; |
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.
Alright, this not how I would suggest designing this; I think you need function types to have a list of yields, basically like its parameter list.
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.
Right. But this is what I wanted to avoid ... as this would require the changes over all the codebase wrt types and typechecking. Is there a way to overcome 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.
Well, if we were really doing this properly, we'd need basically those exact same changes anyway. Everywhere that's calling the "without yields" function is arguably wrong and is just skating by because we aren't really supporting this as a feature in most of the compiler. That's why this is a hugely invasive change that I'm really concerned about.
@slavapestov, what do you 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.
I'm not a fan of introducing a YieldResultType and modeling a function's results as a tuple where some elements are yields; please let's not do that.
I think adding a list of yield results to AnyFunctionType is the way to go. There would be a fair amount of churn since we take function types apart into parameters/results/extInfo and then re-assemble them in various places as you've noted, but I think the changes should be mechanical for the most part, and it's important for the long-term health of the code base to model these fundamental concepts properly.
As a side benefit, the SIL lowering logic for constructing SILFunctionTypes for coroutine accessors could be simplified as a result of this change, because now you will have all of the relevant information in one place instead of having to "assemble" the SIL function type from two distinct pieces of information.
(There is also some precedent for this change. We already did a similar refactoring when we introduced AnyFunctionType's parameter list in the first place -- prior to Swift 5, a function type's "input type" used to be just a single type, which could then be a tuple type to simulate multiple parameters (!). Untangling this was somewhat painful, but not horribly painful.)
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.
As a side benefit, the SIL lowering logic for constructing SILFunctionTypes for coroutine accessors could be simplified as a result of this change, because now you will have all of the relevant information in one place instead of having to "assemble" the SIL function type from two distinct pieces of information.
Yes, I already have this code in #78508 and it is generic for accessors and non-accessor coroutines.
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.
@slavapestov @rjmccall I think I'm having the question wrt syntax. In SIL we parse yields as a part of result type:
$@yield_once @convention(method) (@inout S) -> @yields @inout Float // Single yield$@yield_once @convention(method) (@inout S) -> (@yields @inout Float, Float) // Yield and result
And then we're having a special rule in type resolver:
- If result type is a tuple type with labels it is resolved as tuple type
- Otherwise it is resolved component-wise
What do we want to have at Swift level? Same syntax or something else (e.g. make yields a context-sensititve keyword and parse source after -> in a special way allowing, e.g. -> yields Foo, Float? I think multiple yields should not be tuple, but rather separate ones as potentially we might have a mixture of indirect and direct yields and something like func foo() -> yields (inout T, U) looks worse than func foo() -> yields inout T, yields U.
What are your preferences here?
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 a great question. Ultimately, we're not actually committing to a permanent syntax here — if we added this to the language for real, it would need an evolution proposal, but this is just for testing and the representational convenience of the frontend, right? So I think we should prioritize not interfering too much with normal parsing and type-checking, and if that leads us to some very hacky syntax (maybe even an attribute — @yields(inout T, U)?), that's okay.
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.
@rjmccall I already checked how the things to be organized both in C++ and Swift parser. And it seems the cleanest solution that would not interfere much is actually to do similar to thrown error type. The reason is a bit "fun": C++ and Swift parsers do parsing of result type / clause a bit different, essentially for new Swift parser arrow is a part of syntax and I really do not want to interfere with the existing ResultClause thing and do "parsing in the middle" as this would require also changes in roundtrip text emission that I am trying to avoid. Second thing is that current parsers expect inout in front of any attributed type (so inout @yields T would parse as-is, but @yields inout T would require parser change in common place). But for inout @yields T we will later have issues around type resolution as we'd need to special case non-type @yields attribute.
That said, I will plugin-in into existing effects specifier parsing. It is already generic enough as it supports all kinds of async and rethrows and throws combination, so adding another yield there seems most straightforward in terms of amount of required parser and type resolution changes.
So, we'll have for now (with yields being another context-specific keyword):
func foo() yields T -> ()I'm already having a prototype that could be used as a start.
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.
Okay, that sounds fine. And this will only be accepted under an experimental flag, right?
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.
Still needs to add this, but eventually yes :)
|
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.
?
| return appendOperator("XE"); | ||
| } | ||
| if (fn->isCoroutine()) | ||
| return appendOperator("Xy"); |
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.
Hmm. You should think about this together with the yields — probably this should be something like a list of yields followed by a "this is a coroutine" operator, rather than being a one-off operator up here. Also, do we need to mangle the different yield_once kinds, or are we just assuming that we can use the new ABI for all such functions? (That would be best, but I'm not sure if it's ready for that.)
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, right. It is something left from the old approach... I need to resolve TODO in ASTMangler::appendFunctionSignature below. Ideally I'd use new ABI for non-accessors coroutines starting from the beginning. Although I do not know if some AST-level things would be required... So far it is indeed "old" one. But I guess it would be ok to break the ABI here as it is not something stable here.
| 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
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