[5.9][move-only] Fix our handling of capturing self by local functions #67416
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
The problem here is that the logic was conditionalized on all noncopyable parameters that are borrowed as having the ValueOwnership::Shared flag set. This is only true for user written parameters. Implicit noncopyable parameters like self do not have ValueOwnership::Shared set upon them. We could potentially do that in Sema, but Sema does not know what the proper convention of self is since that information is in TypeLowering today. With that in mind, conditionalize the logic here so we do the right thing. rdar://112547982 (cherry picked from commit 6675084)
…rk_must_check. Originally, we were relying on capture info to determine if we needed to insert this mark_must_check. This ignored that the way that we are handling escaping/non-escaping is something that is approximated in the AST but actually determined at SIL level. With that in mind, rather than relying on the capture info here, just rely on us having an inout argument. The later SIL level checking for inout escapes is able to handle mark_must_check and knows how to turn off noncopyable errors in the closure where we detect the error to prevent us from emitting further errors due to the mark_must_check here. I discovered this while playing with the previous commit. rdar://112555589 (cherry picked from commit b060e5b)
|
@swift-ci test |
jckarter
approved these changes
Jul 19, 2023
tbkka
approved these changes
Jul 19, 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.
• Description: This patch fixes two different issues that cause us to crash when processing self being captured by a local function.
The first issue is that we were attempting to access self (which is an object) as an address when self was captured by a local function parameter. The reason for this is that when the capture code was brought up, we said that we would treat captured params that were marked with borrow as not being boxed. This is true for user written params, but compiler generated params like self need to have this property and are not marked as being boxed. I put in a special check for self here. Importantly, this does not apply to other parameters like setters since we immediately put the newValue into a box so we use different codegen.
The second issue is that we were not performing move checking the noncopyable captured arguments of such local functions causing us to not reject invalid code. The reason why we were doing this is that some time ago were emitting noncopyable errors even after we emitted an inout use by escaping capture error. As a first effort to stop that, we stopped inserting mark_must_check if the AST did not think that a captured argument was a non-escaping argument with the idea that we were for sure going to emit an escaping inout error. In this case, the AST information around what is no-escape is too conservative and we have a case where the capture info states that a parameter is escaping, but it is actually no-escaping. Rather than rely on imprecise AST information, I removed the no escape check here since subsequently to our initial effort to stop such extra diagnostics from being emitted we came up with a better method for not emitting such errors: marking the closure where the error occurs with a semantic tag that turns off the checker. So I was able to just insert the mark_must_check ensuring that we /do/ perform the checking, the escaping inout diagnostic works correctly and doesn't care about the mark_must_check being there/inserts the semantic tag, and everything is good in the world.
• Risk: Very low. Both of these fixes are small and only affect noncopyable types. Both only perturb code that only has to do with these specific capture code paths.
• Original PR: #67414
• Reviewed By: @jckarter
• Testing: Added Swift tests
• Resolves: rdar://112547982 & rdar://112555589