Skip to content

[move-only] Fix two small errors around handling capturing of noncopyable self by local functions #67414

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jul 20, 2023

Conversation

gottesmm
Copy link
Contributor

@gottesmm gottesmm commented Jul 19, 2023

Specifically:


[move-only] Do not try to capture self as an immutable box.

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

[move-only] Make sure we mark local function inout parameters with mark_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

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
@gottesmm
Copy link
Contributor Author

@swift-ci test

@gottesmm
Copy link
Contributor Author

The reason why we still don't emit the mark_must_check errors is that the inout usage by closure pass puts a semantic function on the violating closure that says do not emit move only diagnostics in this function.

@gottesmm gottesmm force-pushed the rdar112555589-112547982 branch from 45bdcac to 52eca57 Compare July 19, 2023 21:39
@gottesmm
Copy link
Contributor Author

@swift-ci test

…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
@gottesmm gottesmm force-pushed the rdar112555589-112547982 branch from 52eca57 to b060e5b Compare July 19, 2023 22:02
@gottesmm
Copy link
Contributor Author

@swift-ci test

@gottesmm gottesmm merged commit 7aea9d4 into swiftlang:main Jul 20, 2023
@gottesmm gottesmm deleted the rdar112555589-112547982 branch July 20, 2023 04:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants