Skip to content

Suggest @objc for overrides of declarations from/in extensions. #13417

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
Dec 14, 2017

Conversation

DougGregor
Copy link
Member

The Swift class model does not support overriding declarations where either
the overridden declaration or the overriding declaration are in an extension.
However, the Objective-C class model does, so marking the declaration as
@objc (when possible) will work around the limitation.

Customize the "cannot override declaration in extension" diagnostic to
suggest adding @objc to the overridden declaration in cases where
@objc is permitted. Fixes SR-6512 / rdar://problem/35787914.

The Swift class model does not support overriding declarations where either
the overridden declaration or the overriding declaration are in an extension.
However, the Objective-C class model does, so marking the declaration as
@objc (when possible) will work around the limitation.

Customize the "cannot override declaration in extension" diagnostic to
suggest adding @objc to the overridden declaration in cases where
@objc is permitted. Fixes SR-6512 / rdar://problem/35787914.
@DougGregor
Copy link
Member Author

@swift-ci please smoke test and merge

@@ -67,7 +67,7 @@ class A {
set { }
}

func overriddenInExtension() {} // expected-note {{overridden declaration is here}}
func overriddenInExtension() {} // expected-note {{overri}}
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a typo?

class func ef2() {} // expected-note {{overri}}
class func ef3() {} // expected-note {{overri}}
class func ef4() {} // expected-note {{overri}}
class func ef5() {} // expected-note {{overri}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Or is this a magic string somehow?

Copy link
Contributor

Choose a reason for hiding this comment

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

If the rest of the diagnostic text is ancillary, you can match on a known prefix with the verifier.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a string that's common to both the "we have Objective-C interop" and "we don't have Objective-C interop" cases. If I tried to match the whole thing, the tests would fail on either Linux or Darwin, because the produce different diagnostics depending on whether that particular declaration could be made @objc.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ha, smart :)

@swift-ci swift-ci merged commit a24121a into swiftlang:master Dec 14, 2017
@DougGregor DougGregor deleted the override-objc-diag branch December 14, 2017 00:14
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.

4 participants