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

[Jesse Rusak]

Thanks for the replies, all. Some inline responses below.

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 <mailto:me at jesserusak.com>> wrote:
>> 
>>  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. 
> 
> 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.


Sounds good. I’ll try to come up with positive/negative examples and see if I can get some from other folks, then take a pass at what kind of type-matching makes sense.

> 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 :)


Right, that makes sense to me. This sounds like essentially the same warning. My prototype warning currently triggers when:

1) You declare a method M on a nominal type.
2) That type conforms to protocol with an extension having a (visible) method with the same name as M.
3) The protocol itself does not declare a method with the same name as M.

If we change #3 to be “The protocol itself does not declare a method of the same name as M which M successfully implements” then I think this triggers for your example, too. (All of the above will also be updated to use whatever type comparisons we decide on.)

> 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.
>  

> 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. 


I think your suggestion is also a good way to suppress the original warning as well; either move the conformance or the conflicting members into a separate extension. 

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?

> 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).

> 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. 

> 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.

- Jesse

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