Skip to content
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

[SR-7629] Fix wrong fixit when the type doesn't conform to a public protocol #17083

Merged

Conversation

naru-jpn
Copy link
Contributor

@naru-jpn naru-jpn commented Jun 9, 2018

Wrong fixit occurred when initializer is memberwiseInitializer. So I disabled fixit only in that case.

Resolves SR-7629.

@beccadax
Copy link
Contributor

beccadax commented Jun 10, 2018

This looks great! Can you add a test that confirms this works correctly? Off the top of my head, I would do this in test/attr/accessibility.swift:

  1. Make a public protocol with an initializer.
  2. Make a conforming type with an explicit internal initializer and confirm that you get both the error and the fix-it.
  3. Make a conforming type with a memberwise initializer and confirm that you only get the error, not the fix-it.

Like many files in the test suite, attr/accessibility.swift is run in a special mode where the Swift compiler compares the diagnostics emitted by the file to specially-formatted comments. Take a look at other parts of the file to see how it's done. Any diagnostic that isn't mentioned in a comment causes the test to fail, so when you're testing the memberwise behavior, simply not mentioning the fix-it diagnostic in the comments ensures the test will fail if it's emitted.

@naru-jpn
Copy link
Contributor Author

naru-jpn commented Jun 10, 2018

@brentdax
Thank you for your great advice and I will try to add test!

@naru-jpn
Copy link
Contributor Author

naru-jpn commented Jun 14, 2018

I wrote tests and checked to pass all tests.

@jrose-apple
Copy link
Contributor

Brent, if you're good with this version so am I; feel free to run tests and merge when you're satisfied with it. (Though I'm sad isImplicit isn't good enough.)

@@ -2773,6 +2773,12 @@ ConformanceChecker::resolveWitnessViaLookup(ValueDecl *requirement) {
requiredAccess,
protoAccessScope.accessLevelForDiagnostics(),
proto->getName());
if (auto *decl = dyn_cast<AbstractFunctionDecl>(witness)) {
auto isMemberwiseInitializer = decl->getBodyKind() == AbstractFunctionDecl::BodyKind::MemberwiseInitializer;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor nit: can you clang-format this change?

@jrose-apple
Copy link
Contributor

@brentdax, any further comments?

@swift-ci Please test

@swiftlang swiftlang deleted a comment from swift-ci Jun 20, 2018
@swiftlang swiftlang deleted a comment from swift-ci Jun 20, 2018
@naru-jpn
Copy link
Contributor Author

@jrose-apple @brentdax Are there other things that you think I should do?

@jrose-apple
Copy link
Contributor

I guess we can take this, and if Brent has further follow-up comments he can still provide them after the merge.

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