Navigation Menu

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

Remove final in protocol extensions #8010

Conversation

KingOfBrian
Copy link
Contributor

This PR generates an error when the final attribute is added to a function or variable inside of a protocol extension. This matches the current semantic model -- final only disables overrides for class types, and it has no effect on protocol extensions. This makes final syntax consistent across struct, enum and protocol types.

This PR also cleans up the reporting of the message by adding a fixit, and only reporting the error once in variable declarations with child declarations. For instance, the error message is currently reported multiple times if a final variable declaration has implicit getters and setters.

This error message also uncovered a lot of usage in the test suite, and a few places where final was used in extensions of a Error protocols in Foundation.

Resolves SR-1762.

@slavapestov
Copy link
Member

Can you make it a warning in Swift 3 mode and add a testcase to test/Compatibility/?

@KingOfBrian
Copy link
Contributor Author

It is a warning in Swift 3 and I added test/Compatibility/final_protocol_extension.swift which checks the warning in Swift 3 mode. Did I miss a case there?

@slavapestov
Copy link
Member

Oh sorry I missed that you already did that. Looks good, thanks!

@slavapestov
Copy link
Member

@jrose-apple Does this look ok to you?

@slavapestov
Copy link
Member

@swift-ci Please smoke test

@jrose-apple
Copy link
Contributor

I mentioned on the bug (and Joe concurred) that this should still go through swift-evolution, since it's an additional break between Swift 3 and Swift 4. It's tiny but that doesn't automatically mean we should just do it.

@@ -1,4 +1,4 @@
// RUN: %target-typecheck-verify-swift
// RUN: %target-typecheck-verify-swift -swift-version 4
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not a change we should make just yet. We're using our existing test suite as source compatibility tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you referring to the -swift-version 4 or to the changes to the tests themselves? I thought that all of the source compatibility tests (for v3) were being put int test/Compatibility. I added -swift-version 4 here in an attempt to follow the pattern I saw in test/attr_[escaping|inlineable|objc], but glad to change anything as needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

-swift-version 4. You only get to add that if everything in the file is also being tested in Compatibility. (Right, @slavapestov?)

@jrose-apple
Copy link
Contributor

We can certainly put the "cleanup" changes in to remove final where it's not necessary, and we can add the warning unconditionally for now.

@KingOfBrian
Copy link
Contributor Author

The attr->invalidate() that I added to remove duplicate warnings was causing the ASTVerifier to fail when comparing the func decl final state to the storage decl final state. Instead of invalidating the attribute, I am removing it which should make sure that the func and storage final states are consistent, and that the error message only occurs once.

I also made a PR to foundation(apple/swift-corelibs-foundation#917) to remove the two places where this warning would occur.

@alblue
Copy link
Contributor

alblue commented Mar 13, 2017

@swift-ci please smoke test

@jrose-apple jrose-apple merged commit f12afd2 into apple:master Mar 13, 2017
@jrose-apple
Copy link
Contributor

Thanks, Brian!

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.

None yet

4 participants