[6.2][sil-isolation-info] When determining isolation of a function arg, use its VarDecl. #80953
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
…e its VarDecl. Otherwise, we can be inconsistent with isolations returned by other parts of the code. Previously we were just treating it always as self + nom decl, which is clearly wrong if a type is not self (e.x.: if it is an isolated parameter). rdar://135459885 (cherry picked from commit 0ece31e)
|
@swift-ci test |
gottesmm
changed the title
gottesmm
commented
Apr 21, 2025
| @@ -13,9 +13,6 @@ actor Test { | |||
| func withTaskLocal(isolation: isolated (any Actor)? = #isolation, | |||
| _ body: (consuming NonSendableValue, isolated (any Actor)?) -> Void) async { | |||
| Self.$local.withValue(12) { | |||
| // Unexpected errors here: | |||
There was a problem hiding this comment.
NOTE: We already did not emit this error. But this PR exercised the code in this area and when I was working on it the error came back, so I looked at it. The comment here is from when we still emitted the error here (and I put in my previous fix), so I removed it to prevent any confusion.
gottesmm
commented
Apr 21, 2025
| if (getKind() != Kind::Actor) | ||
| return false; | ||
| return getActorIsolation() == actorIsolation; | ||
| return getActorIsolation() == other; |
There was a problem hiding this comment.
I did this renaming since SILIsolationInfo has a field called actorIsolation. It can get confusing what one is referring to int he debugger.
|
@swift-ci test macOS platform |
ktoso
approved these changes
Apr 22, 2025
|
@swift-ci test macOS platform |
|
@swift-ci clean test macOS platform |
tbkka
approved these changes
Apr 24, 2025
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.
Explanation: What this patch does is it changes the way that we determine the SILIsolationInfo that we derive from function arguments that are isolated parameters. Previously, we would incorrectly just use self + nom decl to create a SILIsolationInfo... which is obviously wrong. Now instead, we use the correct formulation which is to construct the isolation info frmo the VarDecl of the SILFunctionArgument. This ensures that in cases like the following we correctly realize that the closure's isolation (which is the VarDecl) is the same as self whose isolation is derived from the isolated parameter's VarDecl preventing an error from being emitted:
Scope: Changes how we compute SILIsolationInfo from SILFunctionArguments that are isolated parameters.
Resolves: rdar://135459885
Main PR: #80905
Risk: Low. This just tweaks how we compute isolation. It shouldn't affect anything beyond situations that use isolated parameters.
Testing: Added tests that show that we emit the correct diagnostic and added a SIL level test that directly shows that we infer the isolation correctly.
Reviewer: @ktoso