[5.9][move-only] More batched closure changes #65712
Merged
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
…as such even if the closure doesn't actually escape Specifically, we already have the appropriate semantics for arguments captured by escaping closures but in certain cases allocbox to stack is able to prove that the closure doesn’t actually escape. This results in the capture being converted into a non-escaping SIL form. This then causes the move checker to emit the wrong kind of error. The solution is to create an early allocbox to stack that doesn’t promote move only types in boxes from heap -> stack if it is captured by an escaping closure but does everything else normally. Then once the move checking is completed, we run alloc box to stack an additional time to ensure that we keep the guarantee that heap -> stack is performed in those cases. rdar://108905586 (cherry picked from commit 224674c)
…ng non-consuming uses appropriately. We are doing so now. (cherry picked from commit 8682b17)
…ble twice. We are no longer doing that in the specific test. (cherry picked from commit 444701c)
Specifically: 1. I added consuming test cases alongside __owned test cases. 2. I inserted the actual error messages so that we can see if we are emitting the correct error messages. 3. I enabled the NONTRIVIAL and NONTRIVIAL/REABSTRACT variants of the tests. There are some differences in the address only version of the test that I am still looking into. (cherry picked from commit 4cd7a3b)
…ther local, emit the correct error message. With some of the changes that I have made, we began to emit a mark_must_check [no_copy] on a copy_addr here. This change teaches the move address checker how to recognize that in this case if we have a project_box it is actually b/c we have something captured by an escaping closure. (cherry picked from commit 7f0ef3d)
|
@swift-ci test |
gottesmm
changed the title
kavon
approved these changes
May 5, 2023
|
@swift-ci test macOS platform |
|
MacOS tests failed in a flaky way in an unrelated test that doesn't have any noncopyable stuff in it. |
tbkka
approved these changes
May 5, 2023
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.
CCC. This PR contains a few commits that do some small cleanups in the tests and
add more tests/test coverage , so I did not include them in the CCC. The CCC for
the major changes are as follows:
Commit 2f7c7e9:
• Description: [move-only] Ensure that we treat captured escaping closure
arguments as such even if the closure doesn't actually escape.
Specifically, we already have the appropriate semantics for arguments captured
by escaping closures but in certain cases allocbox to stack is able to prove
that the closure doesn’t actually escape. This results in the capture being
converted into a non-escaping SIL form. This then causes the move checker to
emit the wrong kind of error.
The solution is to create an early allocbox to stack that doesn’t promote move
only types in boxes from heap -> stack if it is captured by an escaping closure
but does everything else normally. Then once the move checking is completed, we
run alloc box to stack an additional time to ensure that we keep the guarantee
that heap -> stack is performed in those cases.
• Risk: Low risk. Only affects noncopyable types.
• Original PR: #65694
• Reviewed By: @kavon
• Testing: I added diagnostic tests that checked this behavior.
• Resolves: rdar://108905586
Commit 018f56f:
• Description: [move-only] When assigning a var that escapes into a closure into
another local, emit the correct error message.
With some of the changes that I have made, we began to emit a mark_must_check
[no_copy] on a copy_addr here. This change teaches the move address checker how
to recognize that in this case if we have a project_box it is actually b/c we
have something captured by an escaping closure.
• Risk: Low risk. Only affects noncopyable types.
• Original PR: #65694
• Reviewed By: @kavon
• Testing: I updated the diagnostic tests that hit this behavior to have the new
correct diagnostic checks.
• Resolves: rdar://108972170