[no-implicit-copy] Make sure to error on copies on borrowed structural sub-values. #41095
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
…l sub-values. Example: consume(x.field). This turned out to be a pretty simple extension of the underlying model. The cases we are interested in are caused by a non-reference nominal type having an extracted field being passed to a consuming use. This always requires a copy. The reason I missed this was I originally wrote the test cases around this for classes which do not have this problem since the class is move only, not the field due to class being a reference type. I then cargo culted this test case for struct/other types and did not notice that we should have started to error on these tests. On an interesting note, I caught this on my branch where I am preparing the optimizer to allow for values to have a move only bit. One of the constraints is that once we are in guaranteed SIL, copy_value can not be used on any moveOnly type (e.x.: $@moveOnly T). To ensure this doesn't happen, the move only checker: 1. Uses copy propagation to rewrite the copies of the base owned value. 2. Emit a diagnostic error upon any copies we found were needed due to the owned value being consumed. 3. If a diagnostic was emitted, rewrite all copies of move only typed values to be explicit_copy_value to ensure that in canonical SIL we do not violate the invariant that copy_value can not be applied to move only type values. This is ok to do since we are going to error and just want to avoid breaking the invariant. The end effect of this is that if we do not emit any diagnostic, any copy_value on a move only typed value that is not eliminated by the checker hits an assert in the verifier... allowing us to know that if a program successfully compiles that all "move only" proofs successfully ran, guaranteeing safety!
Looks like switch handling has some issue with it right now.
gottesmm
commented
Jan 30, 2022
| SmallVector<SILInstruction *, 4> foundConsumesOfBorrowedSubValues; | ||
| SmallVector<SILInstruction *, 4> foundCopiesOfBorrowedSubValues; | ||
| SmallVector<SILInstruction *, 4> foundDestroysOfBorrowedSubValues; | ||
| SmallVector<SILInstruction *, 4> foundRecursiveBorrows; |
There was a problem hiding this comment.
@atrick NOTE: I am not actually using all of these right now. With time though I am going to need to eliminate any temporary unneeded copies that I find during my def->use walks.
|
@swift-ci smoke test |
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.
Example: consume(x.field). This turned out to be a pretty simple extension of
the underlying model. The cases we are interested in are caused by a
non-reference nominal type having an extracted field being passed to a consuming
use. This always requires a copy.
The reason I missed this was I originally wrote the test cases around this for
classes which do not have this problem since the class is move only, not the
field due to class being a reference type. I then cargo culted this test case
for struct/other types and did not notice that we should have started to error
on these tests.
On an interesting note, I caught this on my branch where I am preparing the
optimizer to allow for values to have a move only bit. One of the constraints is
that once we are in guaranteed SIL, copy_value can not be used on any moveOnly
type (e.x.: $@moveOnly T). To ensure this doesn't happen, the move only checker:
Uses copy propagation to rewrite the copies of the base owned value.
Emit a diagnostic error upon any copies we found were needed due to the owned
value being consumed.
If a diagnostic was emitted, rewrite all copies of move only typed values to
be explicit_copy_value to ensure that in canonical SIL we do not violate the
invariant that copy_value can not be applied to move only type values. This
is ok to do since we are going to error and just want to avoid breaking the
invariant.
The end effect of this is that if we do not emit any diagnostic, any copy_value
on a move only typed value that is not eliminated by the checker hits an assert
in the verifier... allowing us to know that if a program successfully compiles
that all "move only" proofs successfully ran, guaranteeing safety!
NOTE: I also added on top of this a commit with some enum tests. I am just expanding code coverage.