Warning when "overriding" an extension method that's not in the protocol


(Jesse Rusak) #1

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

Thanks!
Jesse


(Douglas Gregor) #2

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

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.

  - Doug

···

On Dec 31, 2015, at 3:15 PM, Jesse Rusak <me@jesserusak.com> wrote:


(Lily Ballard) #3

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

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

···

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@jesserusak.com> wrote:


(Lily Ballard) #4

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

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

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

···

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@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@jesserusak.com> wrote:


(Matthew Johnson) #5

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

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

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.

···

Sent from my iPad

On Jan 3, 2016, at 1:22 AM, Kevin Ballard via swift-dev <swift-dev@swift.org> wrote:

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@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@jesserusak.com> wrote:

-Kevin Ballard

_______________________________________________
swift-dev mailing list
swift-dev@swift.org
https://lists.swift.org/mailman/listinfo/swift-dev


(Jesse Rusak) #6

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

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

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

···

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


(Lily Ballard) #7

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

···

On Sun, Jan 3, 2016, at 09:30 AM, Jesse Rusak wrote:


(Jesse Rusak) #8

Hi Kevin,

Thanks for your thoughts. In general, I think I’m going to put off thinking more about the more general extension-shadowing warning until the related proposal on swift-evolution is decided upon, as decisions made there will undoubtedly impact what will trigger this warning and how to resolve it.

I’ll continue to work on the more specialized warning Doug brought up.

A few more comments below for when if/when we circle back to this.

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.

My intent was that you could move the conflicting member into an extension rather than the conformance in the case when the main class declaration conforms to a protocol or inherits it from its superclass. (Same proviso about stored properties, of course.)

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

Sure, we might miss that. I will add it to the “we should try to get this” pile.

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.

Thanks for this! I will definitely be looking through the rest of the standard lib for other examples.

- Jesse

···

On Jan 3, 2016, at 7:33 PM, Kevin Ballard <kevin@sb.org> wrote:
On Sun, Jan 3, 2016, at 09:30 AM, Jesse Rusak wrote: