[6.0][region-isolation] Do not squelch use-after-sending error even if the value is isolated to the same actor #74495
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
|
@swift-ci test |
…require. TLDR: The reason why I am doing this is it ensures that temporary store_borrow that we create when materializing a value before were treated as uses. So we would error on this: ```swift @mainactor func transferToMain<T>(_ t: T) async {} func test() async { let x = NonSendableKlass() await transferToMain(x) await transferToMain(x) } ``` ---- store_borrow is an instruction intended to be used to initialize temporary alloc_stack with borrows. Since it is a temporary, we do not want to error on the temporaries initialization... instead, we want to error on the use of the temporary parameter. This is achieved by making it so that store_borrow still performs an assign/merge, but does not require that src/dest be alive. So the regions still merge (yielding diagnostics for later uses). It also required me to make it so that PartitionOp::{Assign,Merge} do not require by default. Instead, we want the individual operations to always emit a PartitionOp::Require explicitly (which they already did). One thing to be aware of is that when it comes to diagnostics, we already know how to find a temporaries original value and how to handle that. So this is the last part of making store_borrow behave nicely. rdar://129237675 (cherry-picked from commit bd472b1)
…s dead. (cherry picked from commit 3aee7da)
gottesmm
force-pushed
the
release/6.0-129237675
branch
from
d74f464 to
88c953e
Compare
|
@swift-ci test |
…e value is isolated to the same actor. rdar://132074953 (cherry picked from commit ebedf63)
gottesmm
force-pushed
the
release/6.0-129237675
branch
from
88c953e to
2026f39
Compare
gottesmm
changed the title
|
@swift-ci test |
|
@swift-ci test windows platform |
hborla
approved these changes
Jul 20, 2024
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: This PR contains two different fixes that were decided to go in together.
The first fix ensures we error when we send a non-Sendable value twice to the same isolation domain:
this is a more conservative pattern that is simpler to understand vs the old behavior.
The second fix ensures that temporary store_borrow that we create when materializing a value before is not treated as uses. This causes us to emit bad diagnostics by erroring on temporaries rather than the real value causing us to potentially identify diagnostics as being due to nonisolated uses (the marshaling code) rather than the thing we actually want to error on... the call we are making.
Radars:
Original PRs:
Risk: Low.
The first commit just turns off a small block of code that caused us to squelch an error if it has the same isolation as the transfer instruction. So we are only allowing through additional diagnostics that we did not emit before.
The second one even though it looks larger is really just refactoring that makes it so that merge/assign region isolation operations no longer implicitly require the value to be live. Instead, when we build those region isolation operations, we just insert explicitly require live operations. So from a correctness perspective, we are making a purely algebraic transformation of the code. From a functionality perspective, instead of having the checker know that it needs to check liveness when performing assignments/merges, we just use different operations. Using this I then changed store_borrow to no longer be a standard Store like instruction and gave it a custom implementation which is basically a Store like instruction implementation except we do not insert the extra require instructions.
Testing: Added tests to the test suite and updated tests.
Reviewer: N/A