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

[Kevin Ballard]

On Sun, Jan 3, 2016, at 09:30 AM, Jesse Rusak wrote:
> Some things from Kevin:
>
>> 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).
>
> Can I ask for examples of cases where you’d want to do this
> intentionally? In those cases, would moving such members (or the
> protocol conformance) to an extension be a hassle? The point about
> stored properties is one way this is awkward; are there others?

The suggested way to suppress the warning wasn't for this warning. It
was for the warning about trying to conform to a protocol but having a
member that doesn't actually match the protocol requirements (e.g.
having a CollectionType with a `func generate()` that returns a type
that doesn't actually conform to `GeneratorType`).

The warning we're discussing here, of attempting to override a protocol
extension method even though it's not a member of the protocol, can't
work with this suppression mechanism unless the warning simply doesn't
work at all for subclasses who inherit the protocol from their
superclass. If you inherit the protocol declaration, then you can't
possibly put the protocol conformance into its own extension because
you're not declaring the conformance to begin with. Even if you ignore
classes, the suppression mechanism doesn't really work for this warning
anyway as it would suppress a lot of the cases that you actually want to
warn about. I say that because it seems quite reasonable for people to
declare things like `var last` in an extension on a type (which would
trigger this suppression mechanism). This is doubly true for members
declared with where clauses, because those are required to be in
extensions (and can't declare protocol conformance). So e.g. if I say

struct MyCollection<Base : CollectionType> : CollectionType {    // ...
}

extension MyCollection where Base.Index : BidirectionalIndexType {
var last: Generator.Element? {        // ...    } }

then, assuming you think the warning is valid to begin with, you'd want
to warn about that `var last`.

>> 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.
>
> It’s true that if you’re sophisticated enough to understand the
> behavior and want to take advantage of it, you’d be inconvenienced by
> a warning here. I think it comes down to the frequency of people
> intending to do this (who’d have to move some code around?) versus the
> frequency of people not intending to do this (who'd get potentially-
> surprising behavior).

Since the discussed suppression behavior won't work for this particular
warning, it's going to more than just "mov[ing] some code around" to
work around it.

>> 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.
>
> How often are you doing this intentionally in your code? If it’s
> frequent, we should see if there’s a way to suppress it for your
> common cases.

I don't currently have any examples of this in my code. But I've also
never had any cases where the warning would have triggered either. But
the standard library provides examples of this!

SequenceType has an extension that declares a `var lazy`. LazySequence
also declares `var lazy`. It's obviously not overriding SequenceType's
`lazy` property (especially because the type is slightly different), but
it is very intentionally trying to shadow it, to make it so anyone who
types `foo.lazy.lazy` gets back a `LazySequence<Foo>` instead of a
`LazySequence<LazySequence<Foo>>`.

Same thing also happens with `SequenceType.flatMap` and
`LazySequence.flatMap`.

CollectionType and LazyCollection is another example. At the very least
the `lazy` property is an example. I'm not sure offhand if there's any
other examples with those types but I wouldn't be surprised to find that
there are.

Set defines a method `contains(_ element:)` that's an exact match for
the SequenceType extension method `contains(_ element:)`, but that's not
actually an override. It also has `indexOf(_:)` which matches
CollectionType's extension method `indexOf(_:)`.

There may be others, I didn't do an exhaustive search of the standard
library. But these should illustrate legitimate use-cases for shadowing
protocol extension methods.

>> 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).
> One way to prevent this warning in general (if we go with the above
> silencing mechanism) is to declare each protocol conformance in a
> separate extension with just the methods that implement that protocol.
> (I think many people do this already for organizational reasons,
> though with the same caveat about stored properties.) Then importing
> new modules won't cause additional warnings.

As talked about above, this suppression mechanism doesn't work well for
this warning, because it will suppress far more than is intended.

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