Warn about async-imported selector conflicts #59743
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
The change in swiftlang#59479 inadvertently fixed another bug in selector conflict checking: Swift did not notice when an ObjC header declared an async-imported method, but a Swift extension to the same class added another method with the same selector. Unfortunately, fixing this bug was source-breaking, and it also caused Swift to diagnose spurious conflicts between the various names that a single ObjC method might be imported with. Soften the error to a warning in Swift 5 mode and suppress the spurious diagnostics. Fixes rdar://95887113.
|
@swift-ci please smoke test |
|
@swift-ci please test source compatibility |
hamishknight
approved these changes
Jun 28, 2022
kavon
reviewed
Jun 28, 2022
kavon
reviewed
Jun 28, 2022
kavon
approved these changes
Jun 28, 2022
Refactor ObjC conflict diagnosis code to sort conflict data more thoroughly, filter out unwanted declarations earlier, and just generally behave in ways that are more likely to work correctly. This change increases the determinism of the ordering of diagnostics and the selection of the “correct” declaration that the others are considered to conflict with, increasing my confidence that the diagnostics will work correctly in untested corner cases or if the compiler is refactored so that declarations are recorded in a different order. It also adds a new selection rule—@objc without vs. with explicit selector—that I believe will slightly improve the diagnostics we produce. And it replaces a lot of really dodgy-looking logic that may have only worked reliably when a conflict involved exactly two methods.
beccadax
force-pushed
the
hyrums-law-strikes-again
branch
from
a8a614d to
8c9a6ce
Compare
|
@swift-ci please smoke test |
|
@swift-ci please test source compatibility |
|
When I looked more closely at the existing code, I became less and less convinced that it would reliably do the right thing when there were more than two methods involved in a conflict, so I’ve rewritten the function to work in a way that I think will be more reliable. This perturbed a bunch of tests (!), so I also added a new rule to the set used to choose which method “should” have the selector. It’s a bigger diff than I want, but I’m much more confident that it will work correctly in cases we don’t directly test. |
hamishknight
approved these changes
Jun 29, 2022
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.
The change in #59479 inadvertently fixed another bug in selector conflict checking: Swift did not notice when an ObjC header declared an async-imported method, but a Swift extension to the same class added another method with the same selector. Unfortunately, fixing this bug was source-breaking, and it also caused Swift to diagnose spurious conflicts between the various names that a single ObjC method might be imported with. Soften the error to a warning in Swift 5 mode and suppress the spurious diagnostics.
Fixes rdar://95887113.