[6.0][region-isolation] Emit an error when we assign a non-disconnected value into a sending result. #75377

gottesmm · 2024-07-19T23:37:23Z

NOTE: Other PR: #74495. All commits before caf944e are part of that other PR.

Explanation: This PR ensures that we can only assign disconnected values into sending results.

All commits except for 6649fac are basically refactoring commits that I split off to make them easy to eye ball by a reviewer that they are algebraic. Specifically:

caf944e just adds an overload of <<(raw_ostream &os) for a few types that I used in the logging.
a54ddae just moves a definition from one header file to another. I found that I needed this in PartitionUtils.h but it was defined in RegionAnalysis.h.
753f06e. This is a pure refactor that just changes how assign and merge work so that they track not just the representative value of the source value, but also the operand of the actual use. This ensures that we can get back to the original source value and use that for emitting better diagnostics.
6649fac : This is the main commit. In this commit I add support to the partition op evaluator to detect a non-disconnected value being assigned into a sending result. This required me to add a diagnostic to assign and merge isolation operations that detects this case and emits a diagnostic. In order to get a good diagnostic, I wanted to use the operand (hence my need for 753f06e). I did it as a small separate diagnostic to make it easier to know that it is not affecting any other code paths for reviewers.
cf23b4a. In this commit, I implemented stubbing support for the function has sending result check since I cannot use that in the unit tests. The stubbing allows me to always just return false for that check in the unit tests rather than blowing up by attempting to access a function through a mocked operand pointer.

Radars:

rdar://127318392

Original PRs:

[region-isolation] Emit an error when we assign a non-disconnected value into a sending result. #75351

Risk: Low. This is only adding an additional diagnostic. Additionally, the diagnostic can only be triggered if the value has a sending meaning that functions without sending results can never be affected by these changes. So we do get the additional diagnostics only in a very narrow case. Also most of the changes here are actually algebraic... the only one that really does anything interesting is 6649fac making it easier to review, lowering the risk even further
Reviewer: @hborla @ktoso

@mainactor

…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)

…e value is isolated to the same actor. rdar://132074953 (cherry picked from commit ebedf63)

…edIsolationInfo. Just slicing off a nice addition. These already had appropriate print methods so this commit just exposes the print API in the raw_ostream format. (cherry picked from commit 0b42f35)

…PartitionUtils.h and add APIs for mapping an ElementID -> Representative. This is just moving up the declaration in the chain of dependencies so that I can write logic in PartitionUtils.h using it. I also added entrypoints to lookup the ReprensetativeValue for our various emitters. (cherry picked from commit ace94b0)

… of just the representative of the source value when constructing assign and merge. This will let me know the exact source operand used instead of the source value representative. This will ensure that the name associated with the diagnostic is not of the representative value, but the actual value that was the source of the assign. This is an NFCI commit that is an algebraic refactor. (cherry picked from commit ae797d4)

…lue into a sending result. rdar://127318392 (cherry picked from commit bcbf5c5)

…dingResult() so that the unittests can override it. The unittests for PartitionUtils pass in mocked operands and instructions that cannot be dereferenced. Adding this static CRTP helper allows for the unittest PartitionOpEvaluator subclass to just return false for it instead of dereferencing operands or instructions. The rest of the evaluators just get to use the default "normal" implementation that actually accesses program state. (cherry picked from commit 1086264)

gottesmm · 2024-07-19T23:38:06Z

NOTE: This builds on 198685c since their tests slightly overlap.

Any commits before caf944e are not part of this cherry-pick.

gottesmm · 2024-07-19T23:38:56Z

Other PR: #74495

gottesmm · 2024-07-20T01:25:56Z

@swift-ci test

gottesmm added 8 commits July 19, 2024 16:11

[region-isolation] Remove expected-complete-warning from a test. It i…

452333f

…s dead. (cherry picked from commit 3aee7da)

[region-isolation] Do not squelch use-after-transfer error even if th…

2026f39

…e value is isolated to the same actor. rdar://132074953 (cherry picked from commit ebedf63)

Add operator<<(llvm::raw_ostream &os, SILIsolationInfo|SILDynamicMerg…

caf944e

…edIsolationInfo. Just slicing off a nice addition. These already had appropriate print methods so this commit just exposes the print API in the raw_ostream format. (cherry picked from commit 0b42f35)

[region-isolation] Emit an error when we assign a non-disconnected va…

6649fac

…lue into a sending result. rdar://127318392 (cherry picked from commit bcbf5c5)

gottesmm requested a review from a team as a code owner July 19, 2024 23:37

gottesmm enabled auto-merge July 19, 2024 23:38

hborla approved these changes Jul 20, 2024

View reviewed changes

gottesmm merged commit e6aae02 into swiftlang:release/6.0 Jul 20, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[6.0][region-isolation] Emit an error when we assign a non-disconnected value into a sending result. #75377

[6.0][region-isolation] Emit an error when we assign a non-disconnected value into a sending result. #75377

Uh oh!

gottesmm commented Jul 19, 2024 •

edited

Loading

Uh oh!

gottesmm commented Jul 19, 2024

Uh oh!

gottesmm commented Jul 19, 2024

Uh oh!

gottesmm commented Jul 20, 2024

Uh oh!

Uh oh!

Uh oh!

[6.0][region-isolation] Emit an error when we assign a non-disconnected value into a sending result. #75377

[6.0][region-isolation] Emit an error when we assign a non-disconnected value into a sending result. #75377

Uh oh!

Conversation

gottesmm commented Jul 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gottesmm commented Jul 19, 2024

Uh oh!

gottesmm commented Jul 19, 2024

Uh oh!

gottesmm commented Jul 20, 2024

Uh oh!

Uh oh!

Uh oh!

gottesmm commented Jul 19, 2024 •

edited

Loading