Skip to content

🍒 Diagnose additional @objcImpl member mismatch errors #66149

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 4 commits into from
Jun 13, 2023

Conversation

beccadax
Copy link
Contributor

@beccadax beccadax commented May 25, 2023

Cherry-pick of #66073 to release/5.9:

This PR fills in several gaps in @objcImplementation checking. Sema now diagnoses @objcImpl implementations with:

  • The wrong settability (i.e. a let used for a readwrite property)
  • The wrong kind (i.e. a method used for a property)
  • The wrong type (i.e. an Int parameter used for a Float parameter)

I have carved out a special exception to the type checking which allows a nonnull T argument or result in the ObjC header match a T! type in the Swift implementation. This allows implementations to handle nils that are not formally allowed by the ObjC header but are nevertheless passed by some ObjC clients, and to even return nil when passed a nil in these cases. This is a little different from the standard covariance rules for a member, which would allow this kind of difference in an argument type but would only permit the opposite in a result type.

This PR also reimplements the way async methods are matched so that it’s more compatible with member type checking.

Explanation: @_objcImplementation checking currently does not diagnose some mismatches between the header and extension, such as using a method to implement a property or declaring the wrong types. This PR adds these checks, hopefully improving adopter code quality.

Scope: Projects adopting @_objcImplementation in Swift 5.9, before the feature is official or fully stable.

Issue: Fixes rdar://102063730

Risk: Very low. Thanks to #66113, the new diagnostics will be emitted as warnings, so newly-caught mistakes (or new regressions in the diagnostics!) will not prevent adopters from building their projects.

Testing: Includes unit tests; these verify that the new diagnostics are emitted as warnings in release/5.9.

Reviewer: Reviewed by @tshortli on main (except for the last commit, which only adjusts the tests for release/5.9).

@beccadax beccadax added the 🍒 release cherry pick Flag: Release branch cherry picks label May 25, 2023
@beccadax beccadax requested a review from tshortli May 25, 2023 21:03
@beccadax beccadax requested a review from a team as a code owner May 25, 2023 21:03
@beccadax
Copy link
Contributor Author

@swift-ci please test

beccadax added 4 commits June 1, 2023 14:02
Sema now diagnoses @objcImpl implementations with:

• The wrong settability (i.e. a `let` used for a `readwrite` property)
• The wrong kind (i.e. a method used for a property)
swiftlang#65253 (79e2697) made it so that the async and completion-handler variants of a member would both be matched by the same implementation, but the technique used composes poorly with typechecking work later in this series of commits. Reimplement it so that async variants are filtered out of `ObjCImplementationChecker::unmatchedRequirements` and then async candidates are compared against async alternatives later, during matching.
Check the types of @objcImpl candidates against their requirements and diagnose *most* mismatches.

Unlike typical type matching rules, @objcImpl allows an implementation’s parameter *and* result types to be an IUO when the requirement isn’t. This runs against the normal covariance rules in the case of the result type. It’s meant to allow an implementation to handle nils passed to it and return nils even if the declaration formally claims nils are not permitted; this is occasionally necessary to reimplement Objective-C APIs without breaking ABI compatibility.

Fixes rdar://102063730.
@beccadax beccadax force-pushed the implementation-errors-5.9 branch from c456629 to 16853f8 Compare June 1, 2023 23:48
@beccadax
Copy link
Contributor Author

beccadax commented Jun 1, 2023

@swift-ci please test

@beccadax
Copy link
Contributor Author

@swift-ci please smoke test and merge

@swift-ci swift-ci merged commit 92a8db2 into swiftlang:release/5.9 Jun 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🍒 release cherry pick Flag: Release branch cherry picks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants