[swift-dev] Warning when "overriding" an extension method that's not in the protocol
Matthew Johnson
matthew at anandabits.com
Sun Jan 3 09:03:09 CST 2016
Sent from my iPad
> On Jan 3, 2016, at 1:22 AM, Kevin Ballard via swift-dev <swift-dev at swift.org> wrote:
>
> (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).
The issue of extension naming has already popped up once or twice. I wouldn't be surprised if we end up adding the ability to name them eventually. One use case would be selective import. Another use case might be initializing extensions with stored properties that require nontrivial initialization (i.e. contain properties that do not have initial values).
IMO the behavior discussed in this thread definitely contains potential for unexpected results even when you do know exactly how the language works. Maybe developer 1 takes advantage of a protocol extension method in cases where the static type is T (the conforming type). Then developer 2 comes along unaware of that code or the protocol extension method and adds a method with the identical signature, but possibly different behavior, to the type itself. The original code is now using developer 2's code. I don't think this is an unrealistic scenario.
In the case of subclassing this problem does not exist. We are required to specify 'overrides' when overriding and are not allowed to shadow 'final' methods.
The issue is more complex with protocols, especially given the need for retroactive modeling. The rules that prevent these issues from arising with inheritance won't work. At the same time, I don't think Swift as it exists today does enough to help us avoid them.
I've given this some thought and don't have a clear opinion on what the right solution is. But I do believe something more is necessary.
>
> -Kevin Ballard
>
> _______________________________________________
> swift-dev mailing list
> swift-dev at swift.org
> https://lists.swift.org/mailman/listinfo/swift-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.swift.org/pipermail/swift-dev/attachments/20160103/0cc9087b/attachment.html>
More information about the swift-dev
mailing list