[swift-dev] Warning when "overriding" an extension method that's not in the protocol

[Kevin Ballard]

On Sat, Jan 2, 2016, at 11:39 AM, Douglas Gregor via swift-dev wrote:
>
>
>> On Dec 31, 2015, at 3:15 PM, Jesse Rusak <me at jesserusak.com> wrote:
>>
>> Hi Doug,
>>
>> I’ve been playing around with an implementation of the warning you
>> referenced here:
>> https://lists.swift.org/pipermail/swift-evolution/Week-of-Mon-20151207/001584.html
>>
>> Would it be helpful for me to take this on?
>
> Yes, absolutely!
>
>> If so, is there any detail in the radar assigned to you about what
>> exactly should trigger such a warning? For example, I have it
>> currently triggering whenever there’s a method with a matching name,
>> ignoring parameter/return types; it’s not obvious to me how closely
>> they should have to match, if at all, to trigger the warning.
> I just realized that I wrote up a big discussion of a related-but-not-
> identical warning. In either case, there is some kind of radar, and
> neither gives a lot of detail.
>
> In general: just matching on name alone feels like it might produce
> too many false positives, but exactly matching parameter/return types
> feels like it might miss cases where this warning would be important,
> so… I think it’s going to come down to coming up with cases where you
> do/don’t want to see the warning and tuning the warning to do the
> right thing. It might be that you want to do some simplistic matching
> (perhaps akin to what lib/Sema/TypeCheckProtocol.cpp does when
> inferring type witnesses) that ignores uses of associated types that
> might have been deduced differently from what the user expected.
>
> That leads to my #1 example I’d love to get a warning for, which is
> when you intended to provide something to satisfy a protocol
> requirement, but you ended up getting a default implementation:
>
>> struct MyGenerator {  mutating func next() -> Int? { return nil } }
>>
>> struct MyCollection : CollectionType {  typealias Index = Int
>>
>> var startIndex: Int { return 0 }  var endIndex: Int { return 10 }
>>
>> subscript (index: Int) -> Int {    return index  }
>>
>> func generate() -> MyGenerator {    print("using MyGenerator")
>> return MyGenerator()  } }
>>
>> func foo<C: CollectionType>(c: C) {  c.generate() }
>>
>> foo(MyCollection())
>
> Note that there is no output from this program, although one would
> expect to see “using MyGenerator”.
>
> The root of the problem is annoying simple (I “forgot” to state that
> MyGenerator conforms to GeneratorType). The result is that the implied
> SequenceType conformance gets a default implementation of “generate”
> from a protocol extension in the standard library (that produces
> default generator for any SequenceType that is also a CollectionType).
> Our place to warn about this is at the point where we decide to use a
> “generate” from a protocol extension rather than the “generate” in the
> same “struct” that declares conformance to CollectionType. Obviously,
> lots of bonus points if we could say why the generate() in the struct
> wasn’t picked :)
>
> That brings up another point about warnings: it’s useful to have a way
> to suppress them. Let’s say we got a warning for my example above
> (huzzah!) but I wanted to silence it. A fairly natural way to do so
> would be to move the “generate” function I defined into a separate
> extension, so it’s away from where we state conformance to
> CollectionType:
>
>
>> struct MyGenerator {  mutating func next() -> Int? { return nil } }
>>
>> struct MyCollection : CollectionType {  typealias Index = Int
>>
>> var startIndex: Int { return 0 }  var endIndex: Int { return 10 }
>>
>> subscript (index: Int) -> Int {    return index  } }
>>
>> extension MyCollection {  func generate() -> MyGenerator { // no
>> warning    print("using MyGenerator")    return MyGenerator()  } }
>>
> Effectively, we’re using the declaration of conformance to a protocol
> as indicating user intent that the contents of this particular
> definition/extension involve that conformance.

This isn't going to work well for cases where the protocol declares a
property (with a default computed property impl) and a conforming type
attempts to use a stored property. Stored properties must be declared in
the initial type declaration, not in extensions. The only way to
suppress it then would be to suppress the warning in general for members
provided in the initial type declaration when the protocol conformance
is part of an extension (sort of the opposite of the suggested way of
suppressing warnings), and maybe that's fine, but it does mean that
there's no way to suppress the warning for a single stored property
without suppressing it for all stored properties.

> The actual warning you are talking about is very, very similar, and
> would likely use most of the same logic. The part that differs is the
> trigger: whenever one declares something within a type, perform a
> lookup in that type to determine whether there are any similar
> members of extensions of one of the protocols that type conforms to.
> You'll want to think about how to suppress the warning when the user
> wants to.

The warning you documented in most of this email seems perfectly
reasonable; if I declare a function that looks like it's meant to
match a protocol method, there's a good chance I expected it to
actually do so (and, as you suggested, there's a reasonable way to
suppress it for methods).

But the warning that Jesse was talking about, the one that was discussed
in the older threads, I think is completely different. If I declare a
method on my type that matches a protocol extension method, it is *not*
safe to assume I was trying to override the (non-overridable) method.
It's very likely that I'm simply providing a specialization of the
method that will be called by anyone who's working with my type directly
(with the acknowledgement that working with the type generically won't
call my type's version). Not only that, but it's not even possible by
looking at the definition of the protocol to determine if any given
method I add to my type will even trigger a warning! The warning will be
triggered by the existence of any visible extension to the protocol,
even one the programmer isn't aware of.

I think a much better approach to handling this is simply to update the
documentation to make it much more obvious which methods/properties are
"part of" the protocol and which are extensions. The current stdlib docs
(in the iOS Developer Library) adds the text "Default implementation"
next to any default method, but it doesn't actually separate out the
default implementations from the required ones. This also means that
when Xcode shows the method list in the documentation viewer there's no
indication as to which ones are default and which are not.
http://swiftdoc.org provides a *much* better display where "Instance
methods" are in a separate section from "Default Implementations" (see
http://swiftdoc.org/v2.1/protocol/SequenceType/ for an example).

-Kevin Ballard
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.swift.org/pipermail/swift-dev/attachments/20160102/b06828cc/attachment.html>