prevent nonisolated mutation of isolated properties through an existential #59655
Conversation
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
…ntial
The underlying problem was that the ActorIsolationChecker was not properly
recording the mutation environment in which a lookup through an existential was
happening. That's because the AST looks like this:
```
(inout_expr ...
(open_existential_expr ...
(opaque_value_expr ...
(declref_expr ...
(member_ref_expr ...
(opaque_value_expr ...
```
and the walker creates a link from the `open_existential_expr` to the `inout_expr`,
but that kind of expression is not checked for its usage environment. Instead, we
need to look through that OpenExistentialExpr recursively so that a link from
its sub-expression, the `member_ref_expr`, to the `inout_expr` is formed instead.
The side-effect of that missing link causing the bug is that the `usageEnv` of
that `member_ref_expr` appears to be empty when we don't have the link. A missing
link is assumed to mean that it's not in a mutating environment, and thus the
usage is treated as a read.
This is why in swiftlang#59573 / rdar://95509917 we are seeing the compiler report that an
`await` is needed around the assignment expr. The checker thinks it's a read, but
it's actually a mutation, because the member ref is happening in an `inout` expr.
Resolves rdar://95509917
Resolves swiftlang#59573
|
@swift-ci please test |
ktoso
approved these changes
Jun 23, 2022
|
hm, seems the macOS failure is unrelated. @swift-ci please smoke test and merge |
|
This looks great! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The underlying problem was that the ActorIsolationChecker was not properly
recording the mutation environment in which a lookup through an existential was
happening. That's because the AST looks like this:
and the walker creates a link from the
open_existential_exprto theinout_expr,but that kind of expression is not checked for its usage environment. Instead, we
need to look through that OpenExistentialExpr recursively so that a link from
its sub-expression, the
member_ref_expr, to theinout_expris formed instead.The side-effect of that missing link causing the bug is that the
usageEnvofthat
member_ref_exprappears to be empty when we don't have the link. A missinglink is assumed to mean that it's not in a mutating environment, and thus the
usage is treated as a read.
This is why in #59573 / rdar://95509917 we are seeing the compiler report that an
awaitis needed around the assignment expr. The checker thinks it's a read, butit's actually a mutation, because the member ref is happening in an
inoutexpr.Resolves rdar://95509917
Resolves #59573
This PR supersedes #59580