Skip to content

Allow @objc protocols to declare async alternatives #59479

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 5 commits into from
Jun 17, 2022

Conversation

beccadax
Copy link
Contributor

@beccadax beccadax commented Jun 16, 2022

In #41828, I corrected a longstanding oversight where Swift did not check for conflicting selectors in protocols. Unfortunately, we’ve discovered a situation where we want to permit this. You should be able to declare an @objc protocol in Swift which mimics the behavior of an imported ObjC protocol with an __attribute__((swift_async)) requirement, allowing either a completion handler implementation or an async implementation to satisfy both requirements:

@objc protocol HandleThing {
    @objc(handleThing:completion:)
    func handle(thing: Thing) async

    @available(*, renamed: “handle(thing:)”)    // specifies an async alternative
    @objc(handleThing:completion:)
    func handle(thing: Thing, completion: @escaping () -> Void)
}

This PR suppresses the diagnostic in this situation, and also makes sure that only the completion-handler-based requirement gets printed into the generated header, rather than printing both.

Fixes rdar://94175167.

beccadax added 4 commits June 16, 2022 14:06
This allows us to stop using `vec.size() == 1` to prevent duplicate conflicts from being diagnosed, which will become untenable in the next commit.
This will be used to break cycles in a future commit.
An @objc protocol can now explicitly declare both the `async` and completion handler signatures of a method as long as the completion handler one is marked with `@available(*, renamed:)`.

This is not yet handled correctly in PrintAsClang.
The ObjCMethodLookupTable for protocols was not being serialized and rebuilt on load, so NominalTypeDecl::lookupDirect() on selectors was not working correctly for deserialized types. Correct this oversight.
@beccadax beccadax force-pushed the you-completion-me branch from f28fbfb to 0731d2d Compare June 16, 2022 21:08
@beccadax
Copy link
Contributor Author

@swift-ci please smoke test

If Swift sees this pattern of methods in an @objc protocol:

```
    func hello() async -> Int

    @available(*, renamed: “hello()”)
    func hello(completion: @escaping (Int) -> Void)
```

Then PrintAsClang will print only the completion-handler-based method, not the async one, into a generated header, on the assumption that the completion-handler method may have greater availability.

Fixes rdar://94175167.
@beccadax beccadax force-pushed the you-completion-me branch from 0731d2d to 5459737 Compare June 17, 2022 01:27
@beccadax
Copy link
Contributor Author

@swift-ci please smoke test

@beccadax beccadax requested a review from xymus June 17, 2022 07:16
Copy link
Contributor

@xymus xymus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good!

@beccadax beccadax merged commit a4a6d09 into swiftlang:main Jun 17, 2022
beccadax added a commit to beccadax/swift that referenced this pull request Jun 28, 2022
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.
beccadax added a commit to beccadax/swift that referenced this pull request Jun 28, 2022
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.
beccadax added a commit to beccadax/swift that referenced this pull request Jun 28, 2022
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.
beccadax added a commit to beccadax/swift that referenced this pull request Jun 28, 2022
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.
beccadax added a commit to beccadax/swift that referenced this pull request Jun 29, 2022
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.
sf->ObjCMethodList.push_back(method);
} else if (!isa<ProtocolDecl>(this) || !isAnAsyncAlternative(method, vec)) {
// We have a conflict.
sf->ObjCMethodConflicts.insert({ this, selector, isInstanceMethod });
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For those following along at home, I believe that the behavior change addressed in #59743 was caused by us now inserting conflicts into ObjCMethodConflicts when vec.size() >= 1 instead of only when vec.size() == 1. When a method had both completion handler and async variants, vec.size() would start off at 2 and there would be no way for a user-defined conflicting method to ever add an entry to ObjCMethodConflicts.

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