RFC: "Near-miss" checking for defaulted protocol requirements


(Douglas Gregor) #1

Hi all,

A common complaint with protocol conformance checking is that it’s easy to make a little mistake when trying to implement a protocol requirement that has a default implementation. Here is a silly example:

protocol P {
  func foo(value: Float)
}

extension P {
  func foo(value: Float) { } // default implementation of P.foo(value:)
}

struct S { }

extension S : P {
  func foo(value: Double) { } // intended-but-incorrect attempt to satisfy the requirement P.foo(value:)
}

S satisfies the requirement for P.foo(value:) using the implementation from the protocol extension, although it certainly looks like the user intended to provide a different implementation.

This kind of problem has prompted repeated calls for some kind of “implements” keyword to indicate that one is intending to implement a protocol requirement. I, personally, *really* don’t want yet another decorator keyword to indicate the intent here, because I believe the user has already indicated intent by stating conformance to the protocol P. I’ve recently committed a number of changes that provide “near-miss” checking for optional requirements of @objc protocols (which have the same problem).

It might be worth enabling this functionality for cases like the above as well. The Swift compiler patch to do so is attached, and will produce the following warning for the code above:

t2.swift:12:8: warning: instance method 'foo(value:)' nearly matches optional requirement 'foo(value:)' of protocol 'P'
  func foo(value: Double) { }
       ^
t2.swift:12:8: note: candidate has non-matching type '(value: Double) -> ()'
  func foo(value: Double) { }
       ^
t2.swift:12:8: note: move 'foo(value:)' to another extension to silence this warning
  func foo(value: Double) { }
       ^
t2.swift:12:8: note: make 'foo(value:)' private to silence this warning
  func foo(value: Double) { }
       ^
  private
t2.swift:2:8: note: requirement 'foo(value:)' declared here
  func foo(value: Float)
       ^

It’s unfortunate that it’s a lot of notes. The first says what is wrong with the candidate (and there is much we could do to improve the precision of this diagnostic!), while the next two are mitigation strategies: move it to another extension (which implies that it’s not part of the conformance to P) or explicitly mark it as having less visibility than the conformance (in this case, private), which feels like a good way to state “I intend this to be a helper”. This might nudge Swift developers toward a style where they write one conformance per extension, but I don’t think that’s necessarily a bad thing: it’s a fine way to organize code.

Naturally, this handles typos as well, e.g.,

t2.swift:12:8: warning: instance method 'foob(value:)' nearly matches optional requirement 'foo(value:)' of protocol 'P'
  func foob(value: Float) { }
       ^
t2.swift:12:8: note: rename to 'foo(value:)' to satisfy this requirement
  func foob(value: Float) { }
       ^~~~
       foo

Running this on the standard library produces a number of results:

/Users/dgregor/Projects/swift/swift/stdlib/public/core/Arrays.swift.gyb:726:24: warning: instance method 'removeLast()' nearly matches optional requirement 'removeFirst()' of protocol 'RangeReplaceableCollection'
  public mutating func removeLast() -> Element {
                       ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/Arrays.swift.gyb:726:24: note: rename to 'removeFirst()' to satisfy this requirement
  public mutating func removeLast() -> Element {
                       ^~~~~~~~~~
                       removeFirst
/Users/dgregor/Projects/swift/swift/stdlib/public/core/Arrays.swift.gyb:726:24: note: move 'removeLast()' to another extension to silence this warning
  public mutating func removeLast() -> Element {
                       ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/RangeReplaceableCollection.swift:158:17: note: requirement 'removeFirst()' declared here
  mutating func removeFirst() -> Iterator.Element
                ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/Arrays.swift.gyb:726:24: warning: instance method 'removeLast()' nearly matches optional requirement 'removeFirst()' of protocol 'RangeReplaceableCollection'
  public mutating func removeLast() -> Element {
                       ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/Arrays.swift.gyb:726:24: note: rename to 'removeFirst()' to satisfy this requirement
  public mutating func removeLast() -> Element {
                       ^~~~~~~~~~
                       removeFirst
/Users/dgregor/Projects/swift/swift/stdlib/public/core/Arrays.swift.gyb:726:24: note: move 'removeLast()' to another extension to silence this warning
  public mutating func removeLast() -> Element {
                       ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/RangeReplaceableCollection.swift:158:17: note: requirement 'removeFirst()' declared here
  mutating func removeFirst() -> Iterator.Element
                ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/Arrays.swift.gyb:726:24: warning: instance method 'removeLast()' nearly matches optional requirement 'removeFirst()' of protocol 'RangeReplaceableCollection'
  public mutating func removeLast() -> Element {
                       ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/Arrays.swift.gyb:726:24: note: rename to 'removeFirst()' to satisfy this requirement
  public mutating func removeLast() -> Element {
                       ^~~~~~~~~~
                       removeFirst
/Users/dgregor/Projects/swift/swift/stdlib/public/core/Arrays.swift.gyb:726:24: note: move 'removeLast()' to another extension to silence this warning
  public mutating func removeLast() -> Element {
                       ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/RangeReplaceableCollection.swift:158:17: note: requirement 'removeFirst()' declared here
  mutating func removeFirst() -> Iterator.Element
                ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/HashedCollections.swift.gyb:1077:10: warning: subscript 'subscript' nearly matches optional requirement 'subscript' of protocol 'Collection'
  public subscript(key: Key) -> Value? {
         ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/HashedCollections.swift.gyb:1077:10: note: candidate has non-matching type 'Key -> Value?' [with Iterator = DictionaryIterator<Key, Value>, SubSequence = Slice<Dictionary<Key, Value>>]
  public subscript(key: Key) -> Value? {
         ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/HashedCollections.swift.gyb:1077:10: note: move 'subscript' to an extension to silence this warning
  public subscript(key: Key) -> Value? {
         ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/Collection.swift:147:3: note: requirement 'subscript' declared here
  subscript(bounds: Range<Index>) -> SubSequence { get }
  ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/Range.swift:116:10: warning: subscript 'subscript' nearly matches optional requirement 'subscript' of protocol 'Collection'
  public subscript(_: Element._DisabledRangeIndex) -> Element {
         ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/Range.swift:116:10: note: candidate has non-matching type 'Element._DisabledRangeIndex -> Element' [with Iterator = RangeIterator<Element>, SubSequence = Slice<Range<Element>>]
  public subscript(_: Element._DisabledRangeIndex) -> Element {
         ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/Range.swift:116:10: note: move 'subscript' to an extension to silence this warning
  public subscript(_: Element._DisabledRangeIndex) -> Element {
         ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/Collection.swift:147:3: note: requirement 'subscript' declared here
  subscript(bounds: Range<Index>) -> SubSequence { get }
  ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/ExistentialCollection.swift.gyb:508:15: warning: instance method '_distance(to:)' nearly matches optional requirement 'distance(to:)' of protocol 'ForwardIndex'
  public func _distance(to other: AnyForwardIndex) -> AnyForwardIndex.Distance {
              ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/ExistentialCollection.swift.gyb:508:15: note: rename to 'distance(to:)' to satisfy this requirement [with _DisabledRangeIndex = _DisabledRangeIndex_]
  public func _distance(to other: AnyForwardIndex) -> AnyForwardIndex.Distance {
              ^~~~~~~~~
              distance
/Users/dgregor/Projects/swift/swift/stdlib/public/core/ExistentialCollection.swift.gyb:508:15: note: move '_distance(to:)' to an extension to silence this warning
  public func _distance(to other: AnyForwardIndex) -> AnyForwardIndex.Distance {
              ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/Index.swift:180:8: note: requirement 'distance(to:)' declared here
  func distance(to end: Self) -> Distance
       ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/ExistentialCollection.swift.gyb:508:15: warning: instance method '_distance(to:)' nearly matches optional requirement 'distance(to:)' of protocol 'ForwardIndex'
  public func _distance(to other: AnyBidirectionalIndex) -> AnyBidirectionalIndex.Distance {
              ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/ExistentialCollection.swift.gyb:508:15: note: rename to 'distance(to:)' to satisfy this requirement [with _DisabledRangeIndex = _DisabledRangeIndex_]
  public func _distance(to other: AnyBidirectionalIndex) -> AnyBidirectionalIndex.Distance {
              ^~~~~~~~~
              distance
/Users/dgregor/Projects/swift/swift/stdlib/public/core/ExistentialCollection.swift.gyb:508:15: note: move '_distance(to:)' to an extension to silence this warning
  public func _distance(to other: AnyBidirectionalIndex) -> AnyBidirectionalIndex.Distance {
              ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/Index.swift:180:8: note: requirement 'distance(to:)' declared here
  func distance(to end: Self) -> Distance
       ^

It’s somewhat frustrating that these are *all* false positives. However, they seem like “reasonable” false positives, in the sense that they’re close enough to the requirement to be justifiable, and the suggested recovery strategies look acceptable.

Thoughts? Should we turn this on?

  - Doug

default-impl-near-miss.patch (627 Bytes)


(John McCall) #2

Would a word-by-word edit-distance heuristic work better? That is, removeFirst is not a plausible typo for removeLast because First is not a plausible typo for Last.

Admittedly, you might need to do something else if the word counts don't match.

John.

···

On Apr 22, 2016, at 3:33 PM, Douglas Gregor via swift-dev <swift-dev@swift.org> wrote:
Hi all,

A common complaint with protocol conformance checking is that it’s easy to make a little mistake when trying to implement a protocol requirement that has a default implementation. Here is a silly example:

protocol P {
  func foo(value: Float)
}

extension P {
  func foo(value: Float) { } // default implementation of P.foo(value:)
}

struct S { }

extension S : P {
  func foo(value: Double) { } // intended-but-incorrect attempt to satisfy the requirement P.foo(value:)
}

S satisfies the requirement for P.foo(value:) using the implementation from the protocol extension, although it certainly looks like the user intended to provide a different implementation.

This kind of problem has prompted repeated calls for some kind of “implements” keyword to indicate that one is intending to implement a protocol requirement. I, personally, *really* don’t want yet another decorator keyword to indicate the intent here, because I believe the user has already indicated intent by stating conformance to the protocol P. I’ve recently committed a number of changes that provide “near-miss” checking for optional requirements of @objc protocols (which have the same problem).

It might be worth enabling this functionality for cases like the above as well. The Swift compiler patch to do so is attached, and will produce the following warning for the code above:

t2.swift:12:8: warning: instance method 'foo(value:)' nearly matches optional requirement 'foo(value:)' of protocol 'P'
  func foo(value: Double) { }
       ^
t2.swift:12:8: note: candidate has non-matching type '(value: Double) -> ()'
  func foo(value: Double) { }
       ^
t2.swift:12:8: note: move 'foo(value:)' to another extension to silence this warning
  func foo(value: Double) { }
       ^
t2.swift:12:8: note: make 'foo(value:)' private to silence this warning
  func foo(value: Double) { }
       ^
  private
t2.swift:2:8: note: requirement 'foo(value:)' declared here
  func foo(value: Float)
       ^

It’s unfortunate that it’s a lot of notes. The first says what is wrong with the candidate (and there is much we could do to improve the precision of this diagnostic!), while the next two are mitigation strategies: move it to another extension (which implies that it’s not part of the conformance to P) or explicitly mark it as having less visibility than the conformance (in this case, private), which feels like a good way to state “I intend this to be a helper”. This might nudge Swift developers toward a style where they write one conformance per extension, but I don’t think that’s necessarily a bad thing: it’s a fine way to organize code.

Naturally, this handles typos as well, e.g.,

t2.swift:12:8: warning: instance method 'foob(value:)' nearly matches optional requirement 'foo(value:)' of protocol 'P'
  func foob(value: Float) { }
       ^
t2.swift:12:8: note: rename to 'foo(value:)' to satisfy this requirement
  func foob(value: Float) { }
       ^~~~
       foo

Running this on the standard library produces a number of results:

/Users/dgregor/Projects/swift/swift/stdlib/public/core/Arrays.swift.gyb:726:24: warning: instance method 'removeLast()' nearly matches optional requirement 'removeFirst()' of protocol 'RangeReplaceableCollection'
  public mutating func removeLast() -> Element {
                       ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/Arrays.swift.gyb:726:24: note: rename to 'removeFirst()' to satisfy this requirement
  public mutating func removeLast() -> Element {
                       ^~~~~~~~~~
                       removeFirst
/Users/dgregor/Projects/swift/swift/stdlib/public/core/Arrays.swift.gyb:726:24: note: move 'removeLast()' to another extension to silence this warning
  public mutating func removeLast() -> Element {
                       ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/RangeReplaceableCollection.swift:158:17: note: requirement 'removeFirst()' declared here
  mutating func removeFirst() -> Iterator.Element
                ^


(Erica Sadun) #3

With apologies, I do not see near miss checks as a pressing need:

* Quick Help lists of required members (including associated types and inherited members) would be far more valuable to me than "near miss" detection.
* I'd like the compiler to check for unsatisfied conformances and emit a list of required conformances including whether they are found and their origin. This would tangentially address the "near miss" case but only for unsatisfied conformances. It would not help when conformances are satisfied.
* I believe "near miss" is less important than "intentional override", requiring a signature of intent as in inheritance.

Here's what you said previously on the topic of default implementation overrides that brought me here:

This is a commonly-requested feature that I don’t think we need. The TL;DR version is that I feel like specifying the conformance explicitly (my type Foo conforms to protocol P) already expresses intent, and the compiler should help with the rest. I’ve recently been working on providing better warnings for cases where one has tried to implement an optional requirement for a protocol (but got the declaration wrong), and I think we can turn it on for cases where one got a default implementation instead:

  http://thread.gmane.org/gmane.comp.lang.swift.devel/1799

-- Erica

···

On Apr 22, 2016, at 4:33 PM, Douglas Gregor via swift-dev <swift-dev@swift.org> wrote:

Hi all,

A common complaint with protocol conformance checking is that it’s easy to make a little mistake when trying to implement a protocol requirement that has a default implementation. Here is a silly example:

protocol P {
  func foo(value: Float)
}

extension P {
  func foo(value: Float) { } // default implementation of P.foo(value:)
}

struct S { }

extension S : P {
  func foo(value: Double) { } // intended-but-incorrect attempt to satisfy the requirement P.foo(value:)
}

S satisfies the requirement for P.foo(value:) using the implementation from the protocol extension, although it certainly looks like the user intended to provide a different implementation.

This kind of problem has prompted repeated calls for some kind of “implements” keyword to indicate that one is intending to implement a protocol requirement. I, personally, *really* don’t want yet another decorator keyword to indicate the intent here, because I believe the user has already indicated intent by stating conformance to the protocol P. I’ve recently committed a number of changes that provide “near-miss” checking for optional requirements of @objc protocols (which have the same problem).

It might be worth enabling this functionality for cases like the above as well. The Swift compiler patch to do so is attached, and will produce the following warning for the code above:

t2.swift:12:8: warning: instance method 'foo(value:)' nearly matches optional requirement 'foo(value:)' of protocol 'P'
  func foo(value: Double) { }
       ^
t2.swift:12:8: note: candidate has non-matching type '(value: Double) -> ()'
  func foo(value: Double) { }
       ^
t2.swift:12:8: note: move 'foo(value:)' to another extension to silence this warning
  func foo(value: Double) { }
       ^
t2.swift:12:8: note: make 'foo(value:)' private to silence this warning
  func foo(value: Double) { }
       ^
  private
t2.swift:2:8: note: requirement 'foo(value:)' declared here
  func foo(value: Float)
       ^

It’s unfortunate that it’s a lot of notes. The first says what is wrong with the candidate (and there is much we could do to improve the precision of this diagnostic!), while the next two are mitigation strategies: move it to another extension (which implies that it’s not part of the conformance to P) or explicitly mark it as having less visibility than the conformance (in this case, private), which feels like a good way to state “I intend this to be a helper”. This might nudge Swift developers toward a style where they write one conformance per extension, but I don’t think that’s necessarily a bad thing: it’s a fine way to organize code.

Naturally, this handles typos as well, e.g.,

t2.swift:12:8: warning: instance method 'foob(value:)' nearly matches optional requirement 'foo(value:)' of protocol 'P'
  func foob(value: Float) { }
       ^
t2.swift:12:8: note: rename to 'foo(value:)' to satisfy this requirement
  func foob(value: Float) { }
       ^~~~
       foo

Running this on the standard library produces a number of results:

/Users/dgregor/Projects/swift/swift/stdlib/public/core/Arrays.swift.gyb:726:24: warning: instance method 'removeLast()' nearly matches optional requirement 'removeFirst()' of protocol 'RangeReplaceableCollection'
  public mutating func removeLast() -> Element {
                       ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/Arrays.swift.gyb:726:24: note: rename to 'removeFirst()' to satisfy this requirement
  public mutating func removeLast() -> Element {
                       ^~~~~~~~~~
                       removeFirst
/Users/dgregor/Projects/swift/swift/stdlib/public/core/Arrays.swift.gyb:726:24: note: move 'removeLast()' to another extension to silence this warning
  public mutating func removeLast() -> Element {
                       ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/RangeReplaceableCollection.swift:158:17: note: requirement 'removeFirst()' declared here
  mutating func removeFirst() -> Iterator.Element
                ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/Arrays.swift.gyb:726:24: warning: instance method 'removeLast()' nearly matches optional requirement 'removeFirst()' of protocol 'RangeReplaceableCollection'
  public mutating func removeLast() -> Element {
                       ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/Arrays.swift.gyb:726:24: note: rename to 'removeFirst()' to satisfy this requirement
  public mutating func removeLast() -> Element {
                       ^~~~~~~~~~
                       removeFirst
/Users/dgregor/Projects/swift/swift/stdlib/public/core/Arrays.swift.gyb:726:24: note: move 'removeLast()' to another extension to silence this warning
  public mutating func removeLast() -> Element {
                       ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/RangeReplaceableCollection.swift:158:17: note: requirement 'removeFirst()' declared here
  mutating func removeFirst() -> Iterator.Element
                ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/Arrays.swift.gyb:726:24: warning: instance method 'removeLast()' nearly matches optional requirement 'removeFirst()' of protocol 'RangeReplaceableCollection'
  public mutating func removeLast() -> Element {
                       ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/Arrays.swift.gyb:726:24: note: rename to 'removeFirst()' to satisfy this requirement
  public mutating func removeLast() -> Element {
                       ^~~~~~~~~~
                       removeFirst
/Users/dgregor/Projects/swift/swift/stdlib/public/core/Arrays.swift.gyb:726:24: note: move 'removeLast()' to another extension to silence this warning
  public mutating func removeLast() -> Element {
                       ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/RangeReplaceableCollection.swift:158:17: note: requirement 'removeFirst()' declared here
  mutating func removeFirst() -> Iterator.Element
                ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/HashedCollections.swift.gyb:1077:10: warning: subscript 'subscript' nearly matches optional requirement 'subscript' of protocol 'Collection'
  public subscript(key: Key) -> Value? {
         ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/HashedCollections.swift.gyb:1077:10: note: candidate has non-matching type 'Key -> Value?' [with Iterator = DictionaryIterator<Key, Value>, SubSequence = Slice<Dictionary<Key, Value>>]
  public subscript(key: Key) -> Value? {
         ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/HashedCollections.swift.gyb:1077:10: note: move 'subscript' to an extension to silence this warning
  public subscript(key: Key) -> Value? {
         ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/Collection.swift:147:3: note: requirement 'subscript' declared here
  subscript(bounds: Range<Index>) -> SubSequence { get }
  ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/Range.swift:116:10: warning: subscript 'subscript' nearly matches optional requirement 'subscript' of protocol 'Collection'
  public subscript(_: Element._DisabledRangeIndex) -> Element {
         ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/Range.swift:116:10: note: candidate has non-matching type 'Element._DisabledRangeIndex -> Element' [with Iterator = RangeIterator<Element>, SubSequence = Slice<Range<Element>>]
  public subscript(_: Element._DisabledRangeIndex) -> Element {
         ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/Range.swift:116:10: note: move 'subscript' to an extension to silence this warning
  public subscript(_: Element._DisabledRangeIndex) -> Element {
         ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/Collection.swift:147:3: note: requirement 'subscript' declared here
  subscript(bounds: Range<Index>) -> SubSequence { get }
  ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/ExistentialCollection.swift.gyb:508:15: warning: instance method '_distance(to:)' nearly matches optional requirement 'distance(to:)' of protocol 'ForwardIndex'
  public func _distance(to other: AnyForwardIndex) -> AnyForwardIndex.Distance {
              ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/ExistentialCollection.swift.gyb:508:15: note: rename to 'distance(to:)' to satisfy this requirement [with _DisabledRangeIndex = _DisabledRangeIndex_]
  public func _distance(to other: AnyForwardIndex) -> AnyForwardIndex.Distance {
              ^~~~~~~~~
              distance
/Users/dgregor/Projects/swift/swift/stdlib/public/core/ExistentialCollection.swift.gyb:508:15: note: move '_distance(to:)' to an extension to silence this warning
  public func _distance(to other: AnyForwardIndex) -> AnyForwardIndex.Distance {
              ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/Index.swift:180:8: note: requirement 'distance(to:)' declared here
  func distance(to end: Self) -> Distance
       ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/ExistentialCollection.swift.gyb:508:15: warning: instance method '_distance(to:)' nearly matches optional requirement 'distance(to:)' of protocol 'ForwardIndex'
  public func _distance(to other: AnyBidirectionalIndex) -> AnyBidirectionalIndex.Distance {
              ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/ExistentialCollection.swift.gyb:508:15: note: rename to 'distance(to:)' to satisfy this requirement [with _DisabledRangeIndex = _DisabledRangeIndex_]
  public func _distance(to other: AnyBidirectionalIndex) -> AnyBidirectionalIndex.Distance {
              ^~~~~~~~~
              distance
/Users/dgregor/Projects/swift/swift/stdlib/public/core/ExistentialCollection.swift.gyb:508:15: note: move '_distance(to:)' to an extension to silence this warning
  public func _distance(to other: AnyBidirectionalIndex) -> AnyBidirectionalIndex.Distance {
              ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/Index.swift:180:8: note: requirement 'distance(to:)' declared here
  func distance(to end: Self) -> Distance
       ^

It’s somewhat frustrating that these are *all* false positives. However, they seem like “reasonable” false positives, in the sense that they’re close enough to the requirement to be justifiable, and the suggested recovery strategies look acceptable.

Thoughts? Should we turn this on?

  - Doug

<default-impl-near-miss.patch>
_______________________________________________
swift-dev mailing list
swift-dev@swift.org
https://lists.swift.org/mailman/listinfo/swift-dev


(Jordan Rose) #4

I, personally, *really* don’t want yet another decorator keyword to indicate the intent here, because I believe the user has already indicated intent by stating conformance to the protocol P.

I don't find this so compelling in a language with defaulted requirements. The place where we're seeing this problem the most is optional requirements in ObjC protocols (because of all the name changes), but it's really the same thing: it's hard to know if a particular member is intended to be part of a protocol or just something that happens to be similar.

It’s somewhat frustrating that these are *all* false positives. However, they seem like “reasonable” false positives, in the sense that they’re close enough to the requirement to be justifiable, and the suggested recovery strategies look acceptable.

I agree that these are all reasonable, but I don't think moving decls to extensions is a great answer. A lot of these close names might be close because they're helper functions, or (in the case of removeLast) because they implement API that's in the same family as the protocol.

Jordan


(Douglas Gregor) #5

A word-by-word edit distance seems to imply that if *any* word is too far off, reject. I’m a bit concerned that it would create false negatives.

One possibility in this space would be to remove common words from consideration. That way, only the mismatching words will be used to do the edit-distance computation, so the one-mistake-per-N-characters-typed heuristic wouldn’t consider the completely-matching parts.

In defense of the warning in this case: RangeReplaceableCollection has a “removeFirst” but not a “removeLast”; the conforming type here is implementing “removeLast” but not “removeFirst”. It is *so easy* to imagine this as programmer error that the warning feels justified.

  - Doug

···

On Apr 22, 2016, at 6:08 PM, John McCall <rjmccall@apple.com> wrote:

On Apr 22, 2016, at 3:33 PM, Douglas Gregor via swift-dev <swift-dev@swift.org <mailto:swift-dev@swift.org>> wrote:
Hi all,

A common complaint with protocol conformance checking is that it’s easy to make a little mistake when trying to implement a protocol requirement that has a default implementation. Here is a silly example:
[snip]

Naturally, this handles typos as well, e.g.,

t2.swift:12:8: warning: instance method 'foob(value:)' nearly matches optional requirement 'foo(value:)' of protocol 'P'
  func foob(value: Float) { }
       ^
t2.swift:12:8: note: rename to 'foo(value:)' to satisfy this requirement
  func foob(value: Float) { }
       ^~~~
       foo

Running this on the standard library produces a number of results:

/Users/dgregor/Projects/swift/swift/stdlib/public/core/Arrays.swift.gyb:726:24: warning: instance method 'removeLast()' nearly matches optional requirement 'removeFirst()' of protocol 'RangeReplaceableCollection'
  public mutating func removeLast() -> Element {
                       ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/Arrays.swift.gyb:726:24: note: rename to 'removeFirst()' to satisfy this requirement
  public mutating func removeLast() -> Element {
                       ^~~~~~~~~~
                       removeFirst
/Users/dgregor/Projects/swift/swift/stdlib/public/core/Arrays.swift.gyb:726:24: note: move 'removeLast()' to another extension to silence this warning
  public mutating func removeLast() -> Element {
                       ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/RangeReplaceableCollection.swift:158:17: note: requirement 'removeFirst()' declared here
  mutating func removeFirst() -> Iterator.Element
                ^

Would a word-by-word edit-distance heuristic work better? That is, removeFirst is not a plausible typo for removeLast because First is not a plausible typo for Last.


(Douglas Gregor) #6

With apologies, I do not see near miss checks as a pressing need:

* Quick Help lists of required members (including associated types and inherited members) would be far more valuable to me than "near miss" detection.

Understood.

* I'd like the compiler to check for unsatisfied conformances and emit a list of required conformances including whether they are found and their origin. This would tangentially address the "near miss" case but only for unsatisfied conformances. It would not help when conformances are satisfied.

Yes, the experience here is terrible. What the compiler *should* do is give a serious of errors, each of which corresponds to a single requirement and has a Fix-It to stub out the appropriate method/property/etc.

* I believe "near miss" is less important than "intentional override", requiring a signature of intent as in inheritance.

Here's what you said previously on the topic of default implementation overrides that brought me here:

This is a commonly-requested feature that I don’t think we need. The TL;DR version is that I feel like specifying the conformance explicitly (my type Foo conforms to protocol P) already expresses intent, and the compiler should help with the rest. I’ve recently been working on providing better warnings for cases where one has tried to implement an optional requirement for a protocol (but got the declaration wrong), and I think we can turn it on for cases where one got a default implementation instead:

  http://thread.gmane.org/gmane.comp.lang.swift.devel/1799

My opinion is that the signature of intent is the explicitly-stated conformance to the protocol, but this is not universally agreed upon (even within the core team). It suffices to say that a proposal to add some kind of “implements” keyword won’t come from me :slight_smile:

  - Doug

···

On Apr 25, 2016, at 1:13 PM, Erica Sadun <erica@ericasadun.com> wrote:

-- Erica

On Apr 22, 2016, at 4:33 PM, Douglas Gregor via swift-dev <swift-dev@swift.org <mailto:swift-dev@swift.org>> wrote:

Hi all,

A common complaint with protocol conformance checking is that it’s easy to make a little mistake when trying to implement a protocol requirement that has a default implementation. Here is a silly example:

protocol P {
  func foo(value: Float)
}

extension P {
  func foo(value: Float) { } // default implementation of P.foo(value:)
}

struct S { }

extension S : P {
  func foo(value: Double) { } // intended-but-incorrect attempt to satisfy the requirement P.foo(value:)
}

S satisfies the requirement for P.foo(value:) using the implementation from the protocol extension, although it certainly looks like the user intended to provide a different implementation.

This kind of problem has prompted repeated calls for some kind of “implements” keyword to indicate that one is intending to implement a protocol requirement. I, personally, *really* don’t want yet another decorator keyword to indicate the intent here, because I believe the user has already indicated intent by stating conformance to the protocol P. I’ve recently committed a number of changes that provide “near-miss” checking for optional requirements of @objc protocols (which have the same problem).

It might be worth enabling this functionality for cases like the above as well. The Swift compiler patch to do so is attached, and will produce the following warning for the code above:

t2.swift:12:8: warning: instance method 'foo(value:)' nearly matches optional requirement 'foo(value:)' of protocol 'P'
  func foo(value: Double) { }
       ^
t2.swift:12:8: note: candidate has non-matching type '(value: Double) -> ()'
  func foo(value: Double) { }
       ^
t2.swift:12:8: note: move 'foo(value:)' to another extension to silence this warning
  func foo(value: Double) { }
       ^
t2.swift:12:8: note: make 'foo(value:)' private to silence this warning
  func foo(value: Double) { }
       ^
  private
t2.swift:2:8: note: requirement 'foo(value:)' declared here
  func foo(value: Float)
       ^

It’s unfortunate that it’s a lot of notes. The first says what is wrong with the candidate (and there is much we could do to improve the precision of this diagnostic!), while the next two are mitigation strategies: move it to another extension (which implies that it’s not part of the conformance to P) or explicitly mark it as having less visibility than the conformance (in this case, private), which feels like a good way to state “I intend this to be a helper”. This might nudge Swift developers toward a style where they write one conformance per extension, but I don’t think that’s necessarily a bad thing: it’s a fine way to organize code.

Naturally, this handles typos as well, e.g.,

t2.swift:12:8: warning: instance method 'foob(value:)' nearly matches optional requirement 'foo(value:)' of protocol 'P'
  func foob(value: Float) { }
       ^
t2.swift:12:8: note: rename to 'foo(value:)' to satisfy this requirement
  func foob(value: Float) { }
       ^~~~
       foo

Running this on the standard library produces a number of results:

/Users/dgregor/Projects/swift/swift/stdlib/public/core/Arrays.swift.gyb:726:24: warning: instance method 'removeLast()' nearly matches optional requirement 'removeFirst()' of protocol 'RangeReplaceableCollection'
  public mutating func removeLast() -> Element {
                       ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/Arrays.swift.gyb:726:24: note: rename to 'removeFirst()' to satisfy this requirement
  public mutating func removeLast() -> Element {
                       ^~~~~~~~~~
                       removeFirst
/Users/dgregor/Projects/swift/swift/stdlib/public/core/Arrays.swift.gyb:726:24: note: move 'removeLast()' to another extension to silence this warning
  public mutating func removeLast() -> Element {
                       ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/RangeReplaceableCollection.swift:158:17: note: requirement 'removeFirst()' declared here
  mutating func removeFirst() -> Iterator.Element
                ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/Arrays.swift.gyb:726:24: warning: instance method 'removeLast()' nearly matches optional requirement 'removeFirst()' of protocol 'RangeReplaceableCollection'
  public mutating func removeLast() -> Element {
                       ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/Arrays.swift.gyb:726:24: note: rename to 'removeFirst()' to satisfy this requirement
  public mutating func removeLast() -> Element {
                       ^~~~~~~~~~
                       removeFirst
/Users/dgregor/Projects/swift/swift/stdlib/public/core/Arrays.swift.gyb:726:24: note: move 'removeLast()' to another extension to silence this warning
  public mutating func removeLast() -> Element {
                       ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/RangeReplaceableCollection.swift:158:17: note: requirement 'removeFirst()' declared here
  mutating func removeFirst() -> Iterator.Element
                ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/Arrays.swift.gyb:726:24: warning: instance method 'removeLast()' nearly matches optional requirement 'removeFirst()' of protocol 'RangeReplaceableCollection'
  public mutating func removeLast() -> Element {
                       ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/Arrays.swift.gyb:726:24: note: rename to 'removeFirst()' to satisfy this requirement
  public mutating func removeLast() -> Element {
                       ^~~~~~~~~~
                       removeFirst
/Users/dgregor/Projects/swift/swift/stdlib/public/core/Arrays.swift.gyb:726:24: note: move 'removeLast()' to another extension to silence this warning
  public mutating func removeLast() -> Element {
                       ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/RangeReplaceableCollection.swift:158:17: note: requirement 'removeFirst()' declared here
  mutating func removeFirst() -> Iterator.Element
                ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/HashedCollections.swift.gyb:1077:10: warning: subscript 'subscript' nearly matches optional requirement 'subscript' of protocol 'Collection'
  public subscript(key: Key) -> Value? {
         ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/HashedCollections.swift.gyb:1077:10: note: candidate has non-matching type 'Key -> Value?' [with Iterator = DictionaryIterator<Key, Value>, SubSequence = Slice<Dictionary<Key, Value>>]
  public subscript(key: Key) -> Value? {
         ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/HashedCollections.swift.gyb:1077:10: note: move 'subscript' to an extension to silence this warning
  public subscript(key: Key) -> Value? {
         ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/Collection.swift:147:3: note: requirement 'subscript' declared here
  subscript(bounds: Range<Index>) -> SubSequence { get }
  ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/Range.swift:116:10: warning: subscript 'subscript' nearly matches optional requirement 'subscript' of protocol 'Collection'
  public subscript(_: Element._DisabledRangeIndex) -> Element {
         ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/Range.swift:116:10: note: candidate has non-matching type 'Element._DisabledRangeIndex -> Element' [with Iterator = RangeIterator<Element>, SubSequence = Slice<Range<Element>>]
  public subscript(_: Element._DisabledRangeIndex) -> Element {
         ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/Range.swift:116:10: note: move 'subscript' to an extension to silence this warning
  public subscript(_: Element._DisabledRangeIndex) -> Element {
         ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/Collection.swift:147:3: note: requirement 'subscript' declared here
  subscript(bounds: Range<Index>) -> SubSequence { get }
  ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/ExistentialCollection.swift.gyb:508:15: warning: instance method '_distance(to:)' nearly matches optional requirement 'distance(to:)' of protocol 'ForwardIndex'
  public func _distance(to other: AnyForwardIndex) -> AnyForwardIndex.Distance {
              ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/ExistentialCollection.swift.gyb:508:15: note: rename to 'distance(to:)' to satisfy this requirement [with _DisabledRangeIndex = _DisabledRangeIndex_]
  public func _distance(to other: AnyForwardIndex) -> AnyForwardIndex.Distance {
              ^~~~~~~~~
              distance
/Users/dgregor/Projects/swift/swift/stdlib/public/core/ExistentialCollection.swift.gyb:508:15: note: move '_distance(to:)' to an extension to silence this warning
  public func _distance(to other: AnyForwardIndex) -> AnyForwardIndex.Distance {
              ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/Index.swift:180:8: note: requirement 'distance(to:)' declared here
  func distance(to end: Self) -> Distance
       ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/ExistentialCollection.swift.gyb:508:15: warning: instance method '_distance(to:)' nearly matches optional requirement 'distance(to:)' of protocol 'ForwardIndex'
  public func _distance(to other: AnyBidirectionalIndex) -> AnyBidirectionalIndex.Distance {
              ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/ExistentialCollection.swift.gyb:508:15: note: rename to 'distance(to:)' to satisfy this requirement [with _DisabledRangeIndex = _DisabledRangeIndex_]
  public func _distance(to other: AnyBidirectionalIndex) -> AnyBidirectionalIndex.Distance {
              ^~~~~~~~~
              distance
/Users/dgregor/Projects/swift/swift/stdlib/public/core/ExistentialCollection.swift.gyb:508:15: note: move '_distance(to:)' to an extension to silence this warning
  public func _distance(to other: AnyBidirectionalIndex) -> AnyBidirectionalIndex.Distance {
              ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/Index.swift:180:8: note: requirement 'distance(to:)' declared here
  func distance(to end: Self) -> Distance
       ^

It’s somewhat frustrating that these are *all* false positives. However, they seem like “reasonable” false positives, in the sense that they’re close enough to the requirement to be justifiable, and the suggested recovery strategies look acceptable.

Thoughts? Should we turn this on?

  - Doug

<default-impl-near-miss.patch>
_______________________________________________
swift-dev mailing list
swift-dev@swift.org <mailto:swift-dev@swift.org>
https://lists.swift.org/mailman/listinfo/swift-dev


(Jordan Rose) #7

This is a bit of a tangent, but as far as I know no one has objected to this. It's just that no one has written up a proposal that includes retroactive modeling.

Having an "intentionally implements a protocol" keyword is good, but doesn't obviate the use for near-miss checking, particularly in migration from Swift 2.2.

Jordan

···

On Apr 25, 2016, at 13:13, Erica Sadun via swift-evolution <swift-evolution@swift.org> wrote:

* I believe "near miss" is less important than "intentional override", requiring a signature of intent as in inheritance.


(Douglas Gregor) #8

I, personally, *really* don’t want yet another decorator keyword to indicate the intent here, because I believe the user has already indicated intent by stating conformance to the protocol P.

I don't find this so compelling in a language with defaulted requirements.

If we didn’t have defaulted/optional requirements, this wouldn’t be an issue at all.

The place where we're seeing this problem the most is optional requirements in ObjC protocols (because of all the name changes), but it's really the same thing: it's hard to know if a particular member is intended to be part of a protocol or just something that happens to be similar.

It’s somewhat frustrating that these are *all* false positives. However, they seem like “reasonable” false positives, in the sense that they’re close enough to the requirement to be justifiable, and the suggested recovery strategies look acceptable.

I agree that these are all reasonable, but I don't think moving decls to extensions is a great answer. A lot of these close names might be close because they're helper functions,

You can explicitly give them less visibility than the conformance, which makes it clear that they’re helper functions and disables the diagnostic.

or (in the case of removeLast) because they implement API that's in the same family as the protocol.

It seems less onerous to move removeLast out to a different extension than it is to splat some “implements” keyword on a bunch of declarations within an extension whose purpose is to conform to a protocol P:

  extension X : P {
    // this is how X conforms to P
  }

  - Doug

···

On Apr 25, 2016, at 3:30 PM, Jordan Rose <jordan_rose@apple.com> wrote:


(John McCall) #9

Hi all,

A common complaint with protocol conformance checking is that it’s easy to make a little mistake when trying to implement a protocol requirement that has a default implementation. Here is a silly example:
[snip]

Naturally, this handles typos as well, e.g.,

t2.swift:12:8: warning: instance method 'foob(value:)' nearly matches optional requirement 'foo(value:)' of protocol 'P'
  func foob(value: Float) { }
       ^
t2.swift:12:8: note: rename to 'foo(value:)' to satisfy this requirement
  func foob(value: Float) { }
       ^~~~
       foo

Running this on the standard library produces a number of results:

/Users/dgregor/Projects/swift/swift/stdlib/public/core/Arrays.swift.gyb:726:24: warning: instance method 'removeLast()' nearly matches optional requirement 'removeFirst()' of protocol 'RangeReplaceableCollection'
  public mutating func removeLast() -> Element {
                       ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/Arrays.swift.gyb:726:24: note: rename to 'removeFirst()' to satisfy this requirement
  public mutating func removeLast() -> Element {
                       ^~~~~~~~~~
                       removeFirst
/Users/dgregor/Projects/swift/swift/stdlib/public/core/Arrays.swift.gyb:726:24: note: move 'removeLast()' to another extension to silence this warning
  public mutating func removeLast() -> Element {
                       ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/RangeReplaceableCollection.swift:158:17: note: requirement 'removeFirst()' declared here
  mutating func removeFirst() -> Iterator.Element
                ^

Would a word-by-word edit-distance heuristic work better? That is, removeFirst is not a plausible typo for removeLast because First is not a plausible typo for Last.

A word-by-word edit distance seems to imply that if *any* word is too far off, reject. I’m a bit concerned that it would create false negatives.

Any shift in the heuristic will eliminate false positives at the risk of creating false negatives.

A word-by-word heuristic allows you to catch a large number of typos in a long method name without matching completely different words just because the method name is long. That seems like the right trade-off to me.

One possibility in this space would be to remove common words from consideration. That way, only the mismatching words will be used to do the edit-distance computation, so the one-mistake-per-N-characters-typed heuristic wouldn’t consider the completely-matching parts.

I think you have fallen a bit too in love with dictionaries.

In defense of the warning in this case: RangeReplaceableCollection has a “removeFirst” but not a “removeLast”; the conforming type here is implementing “removeLast” but not “removeFirst”. It is *so easy* to imagine this as programmer error that the warning feels justified.

This does not feel like it leads to a reasonable general principle. At best it calls for some way to opt in to a warning about implementing only "related" methods, analogous the related-type warnings in ObjC ARC bridging.

John.

···

On Apr 25, 2016, at 9:57 AM, Douglas Gregor <dgregor@apple.com> wrote:

On Apr 22, 2016, at 6:08 PM, John McCall <rjmccall@apple.com <mailto:rjmccall@apple.com>> wrote:

On Apr 22, 2016, at 3:33 PM, Douglas Gregor via swift-dev <swift-dev@swift.org <mailto:swift-dev@swift.org>> wrote:


(Jordan Rose) #10

"no one except Doug"

···

On Apr 25, 2016, at 15:16, Jordan Rose via swift-evolution <swift-evolution@swift.org> wrote:

On Apr 25, 2016, at 13:13, Erica Sadun via swift-evolution <swift-evolution@swift.org <mailto:swift-evolution@swift.org>> wrote:

* I believe "near miss" is less important than "intentional override", requiring a signature of intent as in inheritance.

This is a bit of a tangent, but as far as I know no one has objected to this. It's just that no one has written up a proposal that includes retroactive modeling.


(Douglas Gregor) #11

By “common words” I mean “words that are common to both names”, not “common English words” :slight_smile:

  - Doug

···

On Apr 25, 2016, at 10:09 AM, John McCall <rjmccall@apple.com> wrote:

On Apr 25, 2016, at 9:57 AM, Douglas Gregor <dgregor@apple.com <mailto:dgregor@apple.com>> wrote:

On Apr 22, 2016, at 6:08 PM, John McCall <rjmccall@apple.com <mailto:rjmccall@apple.com>> wrote:

On Apr 22, 2016, at 3:33 PM, Douglas Gregor via swift-dev <swift-dev@swift.org <mailto:swift-dev@swift.org>> wrote:
Hi all,

A common complaint with protocol conformance checking is that it’s easy to make a little mistake when trying to implement a protocol requirement that has a default implementation. Here is a silly example:
[snip]

Naturally, this handles typos as well, e.g.,

t2.swift:12:8: warning: instance method 'foob(value:)' nearly matches optional requirement 'foo(value:)' of protocol 'P'
  func foob(value: Float) { }
       ^
t2.swift:12:8: note: rename to 'foo(value:)' to satisfy this requirement
  func foob(value: Float) { }
       ^~~~
       foo

Running this on the standard library produces a number of results:

/Users/dgregor/Projects/swift/swift/stdlib/public/core/Arrays.swift.gyb:726:24: warning: instance method 'removeLast()' nearly matches optional requirement 'removeFirst()' of protocol 'RangeReplaceableCollection'
  public mutating func removeLast() -> Element {
                       ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/Arrays.swift.gyb:726:24: note: rename to 'removeFirst()' to satisfy this requirement
  public mutating func removeLast() -> Element {
                       ^~~~~~~~~~
                       removeFirst
/Users/dgregor/Projects/swift/swift/stdlib/public/core/Arrays.swift.gyb:726:24: note: move 'removeLast()' to another extension to silence this warning
  public mutating func removeLast() -> Element {
                       ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/RangeReplaceableCollection.swift:158:17: note: requirement 'removeFirst()' declared here
  mutating func removeFirst() -> Iterator.Element
                ^

Would a word-by-word edit-distance heuristic work better? That is, removeFirst is not a plausible typo for removeLast because First is not a plausible typo for Last.

A word-by-word edit distance seems to imply that if *any* word is too far off, reject. I’m a bit concerned that it would create false negatives.

Any shift in the heuristic will eliminate false positives at the risk of creating false negatives.

A word-by-word heuristic allows you to catch a large number of typos in a long method name without matching completely different words just because the method name is long. That seems like the right trade-off to me.

One possibility in this space would be to remove common words from consideration. That way, only the mismatching words will be used to do the edit-distance computation, so the one-mistake-per-N-characters-typed heuristic wouldn’t consider the completely-matching parts.

I think you have fallen a bit too in love with dictionaries.


(Kate Stone) #12

Hi all,

A common complaint with protocol conformance checking is that it’s easy to make a little mistake when trying to implement a protocol requirement that has a default implementation. Here is a silly example:
[snip]

Naturally, this handles typos as well, e.g.,

t2.swift:12:8: warning: instance method 'foob(value:)' nearly matches optional requirement 'foo(value:)' of protocol 'P'
  func foob(value: Float) { }
       ^
t2.swift:12:8: note: rename to 'foo(value:)' to satisfy this requirement
  func foob(value: Float) { }
       ^~~~
       foo

Running this on the standard library produces a number of results:

/Users/dgregor/Projects/swift/swift/stdlib/public/core/Arrays.swift.gyb:726:24: warning: instance method 'removeLast()' nearly matches optional requirement 'removeFirst()' of protocol 'RangeReplaceableCollection'
  public mutating func removeLast() -> Element {
                       ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/Arrays.swift.gyb:726:24: note: rename to 'removeFirst()' to satisfy this requirement
  public mutating func removeLast() -> Element {
                       ^~~~~~~~~~
                       removeFirst
/Users/dgregor/Projects/swift/swift/stdlib/public/core/Arrays.swift.gyb:726:24: note: move 'removeLast()' to another extension to silence this warning
  public mutating func removeLast() -> Element {
                       ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/RangeReplaceableCollection.swift:158:17: note: requirement 'removeFirst()' declared here
  mutating func removeFirst() -> Iterator.Element
                ^

Would a word-by-word edit-distance heuristic work better? That is, removeFirst is not a plausible typo for removeLast because First is not a plausible typo for Last.

A word-by-word edit distance seems to imply that if *any* word is too far off, reject. I’m a bit concerned that it would create false negatives.

Any shift in the heuristic will eliminate false positives at the risk of creating false negatives.

A word-by-word heuristic allows you to catch a large number of typos in a long method name without matching completely different words just because the method name is long. That seems like the right trade-off to me.

Common mistakes may well include omitting a word or inserting an extra word. The resulting cascade of word-by-word mismatches is something I’d expect our algorithm to deal with more gracefully than purely word index-based comparisons could handle.

···

On Apr 25, 2016, at 10:09 AM, John McCall via swift-dev <swift-dev@swift.org> wrote:

On Apr 25, 2016, at 9:57 AM, Douglas Gregor <dgregor@apple.com <mailto:dgregor@apple.com>> wrote:

On Apr 22, 2016, at 6:08 PM, John McCall <rjmccall@apple.com <mailto:rjmccall@apple.com>> wrote:

On Apr 22, 2016, at 3:33 PM, Douglas Gregor via swift-dev <swift-dev@swift.org <mailto:swift-dev@swift.org>> wrote:

One possibility in this space would be to remove common words from consideration. That way, only the mismatching words will be used to do the edit-distance computation, so the one-mistake-per-N-characters-typed heuristic wouldn’t consider the completely-matching parts.

I think you have fallen a bit too in love with dictionaries.

In defense of the warning in this case: RangeReplaceableCollection has a “removeFirst” but not a “removeLast”; the conforming type here is implementing “removeLast” but not “removeFirst”. It is *so easy* to imagine this as programmer error that the warning feels justified.

This does not feel like it leads to a reasonable general principle. At best it calls for some way to opt in to a warning about implementing only "related" methods, analogous the related-type warnings in ObjC ARC bridging.

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


(John McCall) #13

Oh, I see, of course. Yes, that would probably go a long way towards minimizing the false positives.

John.

···

On Apr 25, 2016, at 10:12 AM, Douglas Gregor <dgregor@apple.com> wrote:

On Apr 25, 2016, at 10:09 AM, John McCall <rjmccall@apple.com <mailto:rjmccall@apple.com>> wrote:

On Apr 25, 2016, at 9:57 AM, Douglas Gregor <dgregor@apple.com <mailto:dgregor@apple.com>> wrote:

On Apr 22, 2016, at 6:08 PM, John McCall <rjmccall@apple.com <mailto:rjmccall@apple.com>> wrote:

On Apr 22, 2016, at 3:33 PM, Douglas Gregor via swift-dev <swift-dev@swift.org <mailto:swift-dev@swift.org>> wrote:
Hi all,

A common complaint with protocol conformance checking is that it’s easy to make a little mistake when trying to implement a protocol requirement that has a default implementation. Here is a silly example:
[snip]

Naturally, this handles typos as well, e.g.,

t2.swift:12:8: warning: instance method 'foob(value:)' nearly matches optional requirement 'foo(value:)' of protocol 'P'
  func foob(value: Float) { }
       ^
t2.swift:12:8: note: rename to 'foo(value:)' to satisfy this requirement
  func foob(value: Float) { }
       ^~~~
       foo

Running this on the standard library produces a number of results:

/Users/dgregor/Projects/swift/swift/stdlib/public/core/Arrays.swift.gyb:726:24: warning: instance method 'removeLast()' nearly matches optional requirement 'removeFirst()' of protocol 'RangeReplaceableCollection'
  public mutating func removeLast() -> Element {
                       ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/Arrays.swift.gyb:726:24: note: rename to 'removeFirst()' to satisfy this requirement
  public mutating func removeLast() -> Element {
                       ^~~~~~~~~~
                       removeFirst
/Users/dgregor/Projects/swift/swift/stdlib/public/core/Arrays.swift.gyb:726:24: note: move 'removeLast()' to another extension to silence this warning
  public mutating func removeLast() -> Element {
                       ^
/Users/dgregor/Projects/swift/swift/stdlib/public/core/RangeReplaceableCollection.swift:158:17: note: requirement 'removeFirst()' declared here
  mutating func removeFirst() -> Iterator.Element
                ^

Would a word-by-word edit-distance heuristic work better? That is, removeFirst is not a plausible typo for removeLast because First is not a plausible typo for Last.

A word-by-word edit distance seems to imply that if *any* word is too far off, reject. I’m a bit concerned that it would create false negatives.

Any shift in the heuristic will eliminate false positives at the risk of creating false negatives.

A word-by-word heuristic allows you to catch a large number of typos in a long method name without matching completely different words just because the method name is long. That seems like the right trade-off to me.

One possibility in this space would be to remove common words from consideration. That way, only the mismatching words will be used to do the edit-distance computation, so the one-mistake-per-N-characters-typed heuristic wouldn’t consider the completely-matching parts.

I think you have fallen a bit too in love with dictionaries.

By “common words” I mean “words that are common to both names”, not “common English words” :slight_smile: