Skip to content

Add AST representation for coroutines #78508

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

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open

Add AST representation for coroutines #78508

wants to merge 19 commits into from

Conversation

asl
Copy link
Contributor

@asl asl commented Jan 9, 2025

Make use of it to differentiate array subscript modify accessor.

@asl
Copy link
Contributor Author

asl commented Jan 9, 2025

@swift-ci please test

@asl
Copy link
Contributor Author

asl commented Jan 9, 2025

This is a proof-of-concept PR adding AST representation for coroutines and making use of it for array subscript modify accessor differentiation.

Further background at https://forums.swift.org/t/pitch-yield-once-functions-first-class-coroutines/77081

@asl
Copy link
Contributor Author

asl commented Jan 9, 2025

swiftlang/swift-syntax#2932

@swift-ci please test

@ahoppen ahoppen removed their request for review January 9, 2025 13:06
@asl
Copy link
Contributor Author

asl commented Jan 9, 2025

swiftlang/swift-syntax#2932

@swift-ci please test

@CodaFi
Copy link
Contributor

CodaFi commented Jan 9, 2025

I'm a bit nervous about modeling yield types this way, it seems to share a lot of the downsides of inout being a formal part of the type AST. Do we actually need to unify yield types in the type checker? If they're just signal carrying a function type's yield information, and can only appear in a particular position (returns), maybe we should model it as a separate data type and compute it as you've done so here. Naively, it seems like the yield return data is a possibly-empty vector of Type ASTs with bits marking if an element at a particular position is a yield or a return.

@CodaFi
Copy link
Contributor

CodaFi commented Jan 9, 2025

These are merely questions, they should not block this PR, which is otherwise fine to me.

@asl
Copy link
Contributor Author

asl commented Jan 9, 2025

These are merely questions, they should not block this PR, which is otherwise fine to me.

I tend to agree with you as I was also having the exactly same questions. At SIL level we separate yields from returns, but it is the open question how we'd proceed on AST level.

Adding a new wrapper type looked much less invasive to me as we rely on type system to distinguish between yields and results and in the majority of cases we'd just need to separate them and that's it, so it worked nicely. Moving them into a dedicated place would probably require explicit handling in more places... But yes, in this sense they are pretty much similar to inouts in their handling.

One important point is that yields are a bit special and sometimes should be considered as parameters as coroutines are not substituted in the same way as regular functions (see the patch 07f03bd "Use pattern substitutions to consistently abstract yields" for some more details).

@asl
Copy link
Contributor Author

asl commented Jan 10, 2025

Looks like ABI checker has some order stability issues? We are having e.g.:

+Subscript MutableCollection.subscript(_:) has generic signature change from <Self where Self : Swift.MutableCollection> to <Self, R where Self : Swift.MutableCollection, R : Swift.RangeExpression, Self.Index == R.Bound>
+Subscript MutableCollection.subscript(_:) has generic signature change from <Self, R where Self : Swift.MutableCollection, R : Swift.RangeExpression, Self.Index == R.Bound> to <Self where Self : Swift.MutableCollection>

@@ -7213,6 +7213,24 @@ ConstraintSystem::matchTypes(Type type1, Type type2, ConstraintKind kind,
case TypeKind::Unresolved:
return getTypeMatchFailure(locator);

case TypeKind::YieldResult: {
if (simplifyType(desugar1)->isEqual(simplifyType(desugar2)))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't use simplifyType here, the idea is to be able to match types incrementally by resolving more and more of inner types as matcher goes.

if (simplifyType(desugar1)->isEqual(simplifyType(desugar2)))
return getTypeMatchSuccess();

if (kind != ConstraintKind::Bind)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How are these mismatches going to be diagnosed? do we need a special fix for this?

auto *yield2 = cast<YieldResultType>(desugar2);

if (yield1->isInOut() != yield2->isInOut())
return getTypeMatchFailure(locator);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like this might warrant a special fix otherwise mismatches here would result in "failed to produce a diagnostic" because the comparison is going to fail in diagnostic mode as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants