Introducing role keywords to reduce hard-to-find bugs

Evolution Pitches
41

This has been covered in detail elsewhere (and probably in this thread) but there are several issues with making this a compile-time error, e.g. around library evolution, retroactive conformances, etc. I think it is generally agreed, however, that warnings are appropriate for situations like these (near misses with protocol methods, perhaps some sort of annotation that can provide warnings, etc).

One thing I am interested in hearing is how these default implementations have been “dangerous” in practice. You mention that this has hit you several times at Tumblr, but your example is a contrived one to demonstrate the general issue and not a specific practical example. In my head, a default implementation should only be provided if the implementation is obvious and can be implemented semantically correctly solely by composition of the other protocol requirements. I see overriding the default implementation as a way of optimising the implementation for a specific type, rather than doing something semantically entirely different. I think this is in line with the use of default implementations in the standard library and the accepted view of protocols in Swift as specifying semantics rather than just syntax. I think if you agree with this viewpoint then these issues become mainly “just” issues with run-time performance, rather than “dangerous bugs”.

I guess in your example you are saying that you might want to insert some analytics code around the implementation, so wanting to perform some side effect that is totally unrelated to the semantics of the protocol is perhaps one example. Are there other cases you have come across?

2 Likes
42

@jawbroken One specific example I have in mind is with my Reusable pod, which, in short, provides a default implementation to conforming UITableViewCells for reuseIdentifier:

public protocol Reusable: class {
  static var reuseIdentifier: String { get }
}

public extension UITableView {
  final func dequeueReusableCell<T: UITableViewCell>(for indexPath: IndexPath, cellType: T.Type = T.self) -> T where T: Reusable {
      guard let cell = self.dequeueReusableCell(withIdentifier: cellType.reuseIdentifier, for: indexPath) as? T else {
        fatalError("Failed to dequeue a cell with identifier \(cellType.reuseIdentifier) matching type \(cellType.self). Check that the reuseIdentifier is set properly in your XIB/Storyboard and that you registered the cell beforehand")
      }
      return cell
  }
}

This mixin allows you to do extension MyTableViewCell: Reusable { } and then magically be able to let cell = tableView.dequeueReusableCell(for: indexPath) as MyTableViewCell. But if you need to use an alternative reuseIdentifier for one of your custom cell you can override the default implementation.

The issue I had here is that I previously named that static var identifier: String { get } in the protocol, and used that in a bunch of projects. Then I later decided to rename that property reuseIdentifier instead.

But then suddenly every extension MyCustomTableViewCell: Reusable { static var identifier = "CustomID" } which intended to override their identifier ended up generating a fatalError and crash only at runtime (because it used the default implementation instead of the specialisation)… instead of having a compiler error or warning to tell me that the name mismatched, which would have saved me a lot of crashes

This is a real use case where missing that error is not just a missing opportunity for optimisation, but clearly introducing a new bug.

1 Like
43

For the benefit of others reading this, I dug up the code in question and there actually is a default implementation not shown above

public extension Reusable {
  /// By default, use the name of the class as String for its reuseIdentifier
  static var reuseIdentifier: String {
    return String(describing: self)
  }
}

I'm a little unclear as to how this fundamentally caused the bug, though. Your registration code (e.g. in UICollectionView+Reusable.swift) also uses reuseIdentifier to register the cell, so failing to override the default implementation would still seem to work fine (albeit with an unexpected reuse identifier) unless you were mixing protocol functionality with direct registration, which is probably generally a bad idea regardless of default implementations. Thanks for the example.

44

@jawbroken Whoops you're right I forgot to quote the default impl, thanks for quoting it!

45

Are there other cases you have come across?

Many times that this has hit us has been during Swift migrations - where the function grammar changes (especially Swift 3) and has left us with hanging implementations that are never called.

Other times it's usually just programmer error and its hard to reason about in a codebase like ours (~190k lines of Swift code). It is also hard to debug these errors since the function signature can look almost identical leaving engineers pulling their hair out wondering why their implementation isn't the one being called but the default one is (or, in extreme cases - the opposite is totally possible by complete accident!)

1 Like
46

Apparently, I replied to this thread in June 2017 with lots of thoughts. I have no idea what this conversation is about anymore, and I suspect few others do. On mobile, I can’t even see the title of the thread.

Instead of resurrecting an extensive old conversation and referring to all of it as “this,” it’d be nice to have a new thread summarizing what’s already been said.

1 Like
47

Instead of resurrecting an extensive old conversation and referring to all of it as “this,” it’d be nice to have a new thread summarizing what’s already been said.

I am totally down to do so. What does everyone think about that?

1 Like
48

This is a great proposal, wish to see these become required keywords which perhaps would help the compiler fix the subclass issue too. I especially love example 4 about orphaned implementations, which are an issue even in non-default implementation conformances. I think an even safer and Swiftier approach is to require protocol conformance methods to be annotated as such with their protocol (similar to the override keyword for subclass methods).

3 Likes
49

If such an annotation would be mandatory, what about retroactive conformance?

50

This has been the crux of the issue ever since this topic was first raised. Retroactive conformance must be possible and nobody has proposed an acceptable design that allows for it.

51

One possibility I remember being raised a long time ago, is to let developers mark *extensions* with some annotation, whereby anything declared within that extension must be either a requirement of a certain protocol, or a helper method with restricted visibility. For example:

@exclusivelyForProtocol(SomeProtocol)
extension Foo {
  // Everything in here must be part of SomeProtocol
  // or a private helper method.
  // Anything else is an error.
}
52

I believe this is the main word. IMO It is possible to suggest a solution here, which respects the retroactive conformance, the question is if such suggestions will have "acceptable" requirements. Is new keyword acceptable? Is new attribute acceptable? Is new modifier acceptable? etc
Personally I believe the discussed problem worth new keyword/new attribute etc, but not sure what others think.

53

Unfortunately, I am at an impasse. My C++ skills are low to nonexistent. I cannot move forward without a prototype implementation and this is quite a complex task for anyone who is not an Swift information silo (thanks @Joe_Groff for the terminology). I haven't the slightest idea how to find someone to help build this, so I'm limited in my proposal pull requests to items I have actually coded.

I think this proposal could have a significant contribution to language safety.

6 Likes
54

I can not thank Erica enough for all the ideas during past 2-3 years. I also do support the idea behind role keywords and I would want to see someone to implement a mandatory default keyword for protocol default implementations.

It would be really great if we could have a pinned page where we'd have a table or a list that defines different parts of the Swift project (different parts of the compiler, stdlib, etc. etc.). Then contributors, the core team members and other follow developer from Apple could tell everyone where they can help and what area they can work on. It does not mean they should have any time to help if they're part of this list, but it would allow us, who cannot implement a pitched idea, to contact those people and ask them friendly if they're interested and can find some time to help us out.

3 Likes
55

Here is the latest version of the proposal: roles2.md · GitHub

As you can tell, implementing it will be a major undertaking

56

Wow, this is a really great proposal update, thank you!

I think there's an additional opportunity here to address one long-standing issue with protocol-oriented Swift: naming collisions.

protocol testProtocolOne {
    var param: Int { get set }
}

protocol testProtocolTwo {
    var param: Int { get set }
}

class implementingClass: testProtocolOne, testProtocolTwo
{
    // this woudn't compile due to naming collisions
    required var param: Int {
       ...
    }
}

This is solved by using protocol extensions instead, but it might be a nicer pattern to require conformance in a nested extension (if the implementer wants all conformances in the class definition):

class implementingClass: testProtocolOne, testProtocolTwo
{
    // ... other class and instance properties, methods, etc.
    // ...
    // ...

    extension testProtocolOne {
        required var param: Int { return 500 }
    }

    extension testProtocolTwo {
        required var param: Int { return 1 }
    }
}

This not only solves collisions, but makes the compiler's job easier for this proposal, makes the ubiquitous // MARK: superfluous in many cases, and groups code into a consistent manner across Swift.

57

I'm not sure I entirely understand the issue with naming collisions. Are you saying that default implementations might exist for both protocols and they conflict when both are conformed to?

58

Yes, that is one possible collision here.

59

I have no problem with this proposal as such (and it may well the correct way forward), but I'm troubled by a couple of things in this area.

Firstly, I'm a bit horrified by the snowdrifts of modifiers that are building up at the start of declarations, especially since they tend to solve very specific (and sometimes inscrutable to general users) problems. While reading the proposal, I understood what the 4 new keywords were for, but I was left with the feeling that I still would have to think long and hard about which one to use. The compiler will help with that, of course, but we're back to the "Press Fixit to win!" game that we've disliked elsewhere.

Secondly, I'm inclined to think that most of the declaration modifiers are alerts to a defect in the underlying Swift design (or what we would call a defect, except we can't admit we don't know how to fix it). It's not that simple, of course, but before papering over some new cracks, I'd like to be sure we've actually understood what the problem is.

The largest underlying problem, I think, is that the idea of a protocol "requirement" has become something of a lie. A protocol "requirement" with a default implementation is not a requirement at all. It is what, in a different thread, @Nevin (IIRC) called a "customization point". Protocols seem to have bifurcated into actual requirements and customization points, and the current syntax just doesn't seem capable of keeping up.

(It could be argued that this is actually a trifurcation, since we already have unconformable-protocol-as-constraining-existential wandering off in its own direction.)

Has there been any discussion of enhancing the language to explicitly recognize a "consumption protocol" (i.e. a set of behaviors that clients of the protocol can assume to exist) and "production protocol" (i.e. a set of requirements on what conformers to the protocol must bring to the party).

It could be argued that this proposal kind of goes in that direction, but it does so in an after-the-fact sort of way. Is there another discussion that needs to be had first (or has already been had)?

4 Likes
60

Agreed. And this is probably our last chance ever to fix this.

2 Likes