Skip to content

[CSApply/Distributed] Identify implicitly throwing calls while applying solution #40130

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 8 commits into from
Nov 15, 2021

Conversation

xedin
Copy link
Contributor

@xedin xedin commented Nov 10, 2021

Calls to distributed actor methods could be pointing to a thunk for remote access,
such thunks are implicitly throwing. Their existence has to be identified early in order
for later steps of solution application to mark context as throwing e.g. implicitly
generated autoclosures for async let initializers rely on that to make closure as
throwing when necessary.

Resolves: rdar://83610106

@xedin xedin requested review from ktoso and DougGregor November 10, 2021 21:56
@xedin xedin added the distributed Feature → concurrency: distributed actor label Nov 10, 2021
Copy link
Contributor

@ktoso ktoso left a comment

Choose a reason for hiding this comment

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

LGTM, thanks a lot @xedin! 💯

@xedin xedin force-pushed the rdar-83610106-with-locals branch from 0e71a98 to 90b0079 Compare November 11, 2021 00:07
@xedin
Copy link
Contributor Author

xedin commented Nov 11, 2021

@swift-ci please clean test

@xedin xedin changed the title [CSApply] Identify implicitly throwing calls while applying solution [CSApply/Distributed] Identify implicitly throwing calls while applying solution Nov 11, 2021
Copy link
Member

@DougGregor DougGregor left a comment

Choose a reason for hiding this comment

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

This is a great direction. I think we're going to need to pull more information into the constraint system to get actor-isolation checking working properly for closures when isolation is queried from inside the constraint solver like this; see my comments inline.

It's useful not only inside of isolation checking but in the
type-checker as well.
…stributed thunk

Make it possible for type-checker to determine whether the given
reference in the given context represents a call to a remote distributed
actor.
Use newly added `isDistributedThunk` check to determine whether
the given call is to a remote distributed actor and if so mark
it as implicitly throwing.

Resolves: rdar://83610106
…itter

This logic cannot live in `ActorIsolationChecker` because
all of the relevant information is only accessible through
constraint system while applying solutions.
…nger useful

`ConstraintSystem` has its own method to determine this.
@xedin xedin force-pushed the rdar-83610106-with-locals branch from 90b0079 to 70360eb Compare November 13, 2021 06:10
Copy link
Member

@DougGregor DougGregor left a comment

Choose a reason for hiding this comment

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

This looks like it'll work, thank you!

@xedin
Copy link
Contributor Author

xedin commented Nov 13, 2021

@swift-ci please clean test

@xedin
Copy link
Contributor Author

xedin commented Nov 13, 2021

@swift-ci please test Windows platform

1 similar comment
@ktoso
Copy link
Contributor

ktoso commented Nov 15, 2021

@swift-ci please test Windows platform

@ktoso
Copy link
Contributor

ktoso commented Nov 15, 2021

Awesome! And CI green everywhere as well now 🥳

@xedin xedin merged commit 21350a9 into swiftlang:main Nov 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
distributed Feature → concurrency: distributed actor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants