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-1762] Using "final" keyword in protocol extension doesn't prevent overriding #44371

Closed
devioustree opened this issue Jun 14, 2016 · 17 comments

Comments

@devioustree
Copy link

devioustree commented Jun 14, 2016

Previous ID SR-1762
Radar rdar://problem/31674946
Original Reporter @devioustree
Type Bug
Status Resolved
Resolution Done
Additional Detail from JIRA
Votes 1
Component/s Compiler
Labels Bug, StarterBug
Assignee KingOfBrian (JIRA)
Priority Medium

md5: 8fafa4fede6ff32b86141ecfa4a70feb

is duplicated by:

  • SR-2018 Compiler should not allow final on functions defined in protocol extensions.

Issue Description:

If I implement a protocol method in a protocol extension, and mark it as final, it seems that I am able to always override it. Should the compiler allow this?

import UIKit

var str = "Hello, playground"

protocol P : class {
    func f()
}

extension P {
    final func f() {} // final has no effect
}

class A : P {}

class B : A {
    func f() {}
}
@belkadan
Copy link
Contributor

belkadan commented Jun 20, 2016

This is left over from the first implementation of protocol extensions, where they never got to satisfy requirements and thus were "final" in that sense. We should probably remove it at this point, which will require a small language proposal.

@swift-ci
Copy link
Collaborator

swift-ci commented Jan 22, 2017

Comment by Brian (JIRA)

I looked into a fix for this but it's being a bit tricky. The desired error is generated in `lib/Sema/TypeCheckAttr.cpp` guarded behind `!D->getDeclContext()->getAsProtocolExtensionContext()`, so this seemed like a pretty easy fix. But, when I removed that condition and added some test cases, it would still not generate any error message. Given this test case:

protocol P {
final func bar()
}
extension P {
final func foo() {}
}

It turns out that `FD->getAttrs()` is empty in the extension, and populated in the initial protocol declaration when `TypeChecker::checkDeclAttributes` is invoked. I suspected that the parser was handling a special case, but `parseNewDeclAttribute` is adding the decl in both cases. I grepped in the dark for a few trying to figure out where the decl was being removed but didn't have any luck. It would be nice to be able to dump the representation of the SILModule so I could hone in on where the decl was being dropped, but `dump(false)` was failing on a symbol not existing. Any tricks here that might help?

@belkadan
Copy link
Contributor

belkadan commented Jan 23, 2017

I don't understand what you're trying to do. Either we parse but ignore the 'final' attribute (a problem in Parse), or we parse and then mark it invalid without diagnosing (a problem in Sema, under TypeChecker somewhere), or possibly both. This all happens well before SIL.

@swift-ci
Copy link
Collaborator

swift-ci commented Jan 23, 2017

Comment by Brian (JIRA)

I'm trying to figure out what is causing the FinalDecl to not appear in the attributes inside `TypeChecker::checkDeclAttributes`. I was trying to dump the AST between parseIntoSourceFile and performTypeChecking to debug things, and was dumping the SILModule(which was obviously the wrong thing to dump) and the SF.Decl , both of which reported symbol not found errors.

But I think I figured out how to keep debugging, I realized that most decl's do dump fine, so I grab the address in the parse phase and am able to see the representation transform from

{{(func_decl "foo()" final
  (parameter_list
    (parameter "self"))
  (parameter_list)
  (brace_stmt))

to

(func_decl "foo()" interface type='<Self where Self : F> (Self) -> () -> ()' access=internal
  (parameter_list
    (parameter "self" type='Self' interface type='Self'))
  (parameter_list)
  (brace_stmt))
}}

I should be able to zero in on where final is being dropped from here.

@swift-ci
Copy link
Collaborator

swift-ci commented Jan 24, 2017

Comment by Brian (JIRA)

Turns out it was getting removed in `AttributeEarlyChecker`. I have a branch (KingOfBrian@247b109) that now generates an error in Swift 3, a warning in Swift 4 and adds fixits for all of the 'final not supported' errors.

Should I open a PR or does the SE proposal need to get approved first? I'd gladly write one up but I'm not sure I understand the design criteria of the initial protocol implementation to explain the why sufficiently.

@swift-ci
Copy link
Collaborator

swift-ci commented Feb 2, 2017

Comment by Brian (JIRA)

Hey @belkadan, I wrote a draft of a SE proposal for this bug: https://gist.github.com/KingOfBrian/6f20c566114ac0ef54c8092d80e54ee7. I believe it should be included in Phase 1 since it impacts source compatibility. I'm not sure if this is right so I thought I'd get a set of eyes on it before sending an email to the SE mailing list.

I'm aware this is probably pretty low priority, but thought I'd try to push it forward.

@belkadan
Copy link
Contributor

belkadan commented Mar 10, 2017

Thanks!

In the original protocol model of Swift, a developer could use the final keyword when declaring a function in a protocol extension to ensure the function could not be overridden.

It didn't actually affect behavior at all then either. We just originally required it to signify that there was no dynamic dispatch going on. Once we started allowing protocol extension methods to fulfill requirements, it became more confusing than useful.

I don't think we even shipped a release of Swift with required-final on protocol extension methods. I think this was Swift 2.0 WWDC beta vs. Swift 2.0 GM.

@swift-ci
Copy link
Collaborator

swift-ci commented Mar 10, 2017

Comment by Brian (JIRA)

Interesting! I started using swift in the 2.2 days, so I was just trying to make up an origin story that made sense. I'll update the proposal if SE wants a proposal, but Slava was thinking it was more of a bug against the specification than a change in the specification. I pushed a PR up with a fix: #8010

@belkadan
Copy link
Contributor

belkadan commented Mar 10, 2017

I did see the PR. :-) I'd like a core team member's blessing that this wouldn't have to go through swift-evolution. @DougGregor, @jckarter?

@jckarter
Copy link
Member

jckarter commented Mar 10, 2017

I didn't know we still allowed this. We should probably put it through evolution since it would be a source break from 3.0 at this point IMO.

@swift-ci
Copy link
Collaborator

swift-ci commented Mar 10, 2017

Comment by Brian (JIRA)

I sent the proposal here to the list yesterday and got some good feedback from Erica on the language. Glad to incorporate that and a more accurate historical representation. Should I close the PR for now?

@belkadan
Copy link
Contributor

belkadan commented Mar 10, 2017

I think it's fine to leave it in limbo. We actually have a label for things pending -evolution discussion, though I don't remember what it is offhand.

@DougGregor
Copy link
Member

DougGregor commented Mar 10, 2017

We should probably require `override` here, but IMO it's a swift-evolution topic. (And I'm glad to see the swift-evolution PR on this)

@tkremenek
Copy link
Member

tkremenek commented Apr 18, 2017

@swift-ci create

@ematejska
Copy link
Mannequin

ematejska mannequin commented Apr 18, 2017

KingOfBrian (JIRA User), now that SE-0164 has been accepted, do you have that pull request? 🙂

@swift-ci
Copy link
Collaborator

swift-ci commented Apr 18, 2017

Comment by Brian (JIRA)

Yea, working on tests now. The core functionality is there, it's just upgrading a warning to an error in Swift 4 mode.

@swift-ci
Copy link
Collaborator

swift-ci commented Apr 21, 2017

Comment by Brian (JIRA)

Resolved by #8890

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants