Only warn about self capture in nested closures until Swift 6 #38947
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
Swift has diagnosed implicit uses of class-reference `self` in escaping closures for a long time as potentially leading to reference cycles. PR swiftlang#35898 fixed a bug where we failed to diagnose this condition in nested closures. Unfortunately, treating this as an error has proven problematic because there's a lot of code doing it. Requiring that code to be thoroughly stamped out before we ship a compiler is just too much to ask. Stage the fix in by treating it as a warning in Swift versions prior to 6. As with the non-nested case, this warning can be suppressed by explicitly either capturing `self` or spelling out `self.` in the access. Fixes rdar://80847025.
|
@swift-ci Please test |
|
Looks like this broke non-asserts builds, fixing in #38957 |
rjmccall
added a commit
to rjmccall/swift
that referenced
this pull request
Sep 1, 2021
We've always emitted an error if we saw an implicit use of a self parameter of class type from an escaping closure. In PR swiftlang#35898, I fixed this to also emit an error if the reference was to an explicit capture of self that wasn't made in the current closure. That was causing some source incompatibilities that we decided were too severe, so in PR swiftlang#38947 I weakened that to a warning when the diagnostic walk was within multiple levels of closures, because I have always thought of this as a fix to nested closures. However, this was the wrong condition in two ways. First, the diagnostic walk does not always start from the outermost function declaration; it can also start from a multi-statement closure. In that case, we'll still end up emitting an error when we see uses of explicit captures from the closure when we walk it, and so we still have a source incompatibility. That is rdar://82545600. Second, the old diagnostic did actually fire correctly in nested closures as long as the code was directly referring to the original self parameter and not any intervening captures. Therefore, swiftlang#38947 actually turned some things into warnings that had always been errors. The fix is to produce a warning exactly when the referenced declaration was an explicit capture.
WowbaggersLiquidLunch
pushed a commit
to WowbaggersLiquidLunch/swift
that referenced
this pull request
Mar 31, 2022
We've always emitted an error if we saw an implicit use of a self parameter of class type from an escaping closure. In PR swiftlang#35898, I fixed this to also emit an error if the reference was to an explicit capture of self that wasn't made in the current closure. That was causing some source incompatibilities that we decided were too severe, so in PR swiftlang#38947 I weakened that to a warning when the diagnostic walk was within multiple levels of closures, because I have always thought of this as a fix to nested closures. However, this was the wrong condition in two ways. First, the diagnostic walk does not always start from the outermost function declaration; it can also start from a multi-statement closure. In that case, we'll still end up emitting an error when we see uses of explicit captures from the closure when we walk it, and so we still have a source incompatibility. That is rdar://82545600. Second, the old diagnostic did actually fire correctly in nested closures as long as the code was directly referring to the original self parameter and not any intervening captures. Therefore, swiftlang#38947 actually turned some things into warnings that had always been errors. The fix is to produce a warning exactly when the referenced declaration was an explicit capture.
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.
Swift has diagnosed implicit uses of class-reference
selfin escaping closures for a long time as potentially leading to reference cycles. PR #35898 fixed a bug where we failed to diagnose this condition in nested closures. Unfortunately, treating this as an error has proven problematic because there's a lot of code doing it. Requiring that code to be thoroughly stamped out before we ship a compiler is just too much to ask. Stage the fix in by treating it as a warning in Swift versions prior to 6.As with the non-nested case, this warning can be suppressed by explicitly either capturing
selfor spelling outself.in the access.Fixes rdar://80847025.