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

[Kevin Ballard]

(CCing the list again as I believe you omitted it accidentally)

On Sat, Jan 2, 2016, at 10:10 PM, Douglas Gregor wrote:
> On Jan 2, 2016, at 6:58 PM, Kevin Ballard via swift-dev <swift-
> dev at swift.org> wrote:
>> 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.
>
> We've discussed a change to allow stored properties within extensions
> of the nominal type that occur in the same module, by if we don't get
> that (or we limit it somehow), you're right.

We did discuss that, but we haven't agreed on it yet (and last I
checked it seemed there was more support for limiting it just to
classes anyway).

>> 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.
>
> Right. I'm okay with that kind of limitation: we don't and can't catch
> all problems, but we can help a lot without adding any burden to the
> 99% case.

I agree, I was just pointing out the limitations, as well as hopefully
serving as a reminder to any implementors not to overlook the issue with
properties (better to lose out on valid warnings in some cases than to
end up with false positive warnings that you can't suppress).

>>> 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).
>
> Even if you're doing it intentionally, there's a potential surprise
> lurking here: your type will behave differently when used as a
> concrete type vs. as a generic type argument or an existential. That
> still feels worthy of a warning, and might even be something you as
> the author of that method would want to document for your own users.

It's only a surprise to anyone who doesn't understand how protocol
extension methods work. And any such documentation would be done in the
doc comment, which is orthogonal to this warning.

> Yes, we definitely need a suppression mechanism when this is
> intentional, and I don't have a natural one in mind.
>
>> 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.
>
> Sure, but that's where warnings are most helpful, right? When it's
> telling you about a potential problem that you didn't even realize you
> needed to look for?

It's only a problem if you believe the method belongs to the protocol to
begin with. If you never think the method belongs to the protocol at
all, then you'd never expect to override the protocol method since it's
not actually a protocol method.

Basically, the warning we're discussing is the same thing as saying "if
you have a misconception about how protocol extension methods work, then
you might be attempting to do something that won't work", which is
basically just a punishment for people who don't have that misconception
at all because it means their perfectly legitimate code is throwing
warnings because someone happened to declare a protocol extension with
the same method. They may not even be aware the extension methods exists
at all, which makes the warning even more surprising.

>>
>> 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).
> We could certainly improve the presentation, but I don't think it
> solves the problem adequately because reference documentation doesn't
> convey the potential problems with introducing such a specialization.
> And this is a difficult enough area that I don't feel like Swift
> programmers should "just know" to be on the lookout for this.

My belief is that a warning in this case would end up being unwanted in
a disproportionate number of cases. I believe it's better to clearly
differentiate between protocol methods and extension methods and how
they behave in our documentation and education for programmers than to
add spurious warnings that will annoy everyone who does understand the
distinction.

Another concern with this warning is it means that adding a simple
`import Foo` to the top of a file can cause my otherwise-valid type
definition to sprout warnings (because Foo may declare an extension to a
protocol I'm using). I realize there's already cases where adding an
import can cause errors (e.g. ambiguity errors), but I think we should
think very carefully before adding anything else that causes imports to
cause problems. And with the recently-proposed changes to import syntax
we can even resolve ambiguity errors at the import line, but this
proposed warning couldn't possibly be fixed that way (you can't name
extensions so importing a module must pull in all its defined
extensions).

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