[stdlib]Custom message for recursive Strideable witness #11623
Conversation
|
@swift-ci please smoke test |
|
/cc @natecook1000 |
There was a problem hiding this comment.
As discussed in person. The problem with StdlibUnittest style test is that it would succeed even without your patch. Validating the message is essential in this case, and so it should be a file-check style test.
A minor nitpick: all the non-mutating binary operators in InvalidStrideable are not needed, since there are default implementations for them.
moiseev
changed the title
stdlib/public/core/Stride.swift.gyb
Outdated
| @@ -23,6 +23,9 @@ | |||
| }% | |||
| /// Conforming types are notionally continuous, one-dimensional | |||
| /// values that can be offset and measured. | |||
| /// | |||
| /// - Warning: If 'Stride == Self' you must provide an implementation of | |||
| /// 'Equatable' and 'Comparable'. | |||
There was a problem hiding this comment.
Could we use this language instead for now?
/// - Important: The `Strideable` protocol provides default implementations for
/// the equal-to (`==`) and less-than (`<`) operators that depend on the
/// `Stride` type's implementations. If a type conforming to `Strideable`
/// is its own `Stride` type, it must provide concrete implementations of
/// the two operators to avoid infinite recursion.
stdlib/public/core/Stride.swift.gyb
Outdated
| fatalError(""" | ||
| Strideable conformance where 'Stride == Self' requires user-defined | ||
| implementation of Equatable and Comparable. | ||
| """) |
There was a problem hiding this comment.
We should be specific about what needs to be implemented here.
fatalError("""
Strideable conformance where 'Stride == Self' requires user-defined
implementation of the '<' operator.
""")
Also, probably indent the trailing """ so the message isn't indented.
|
@moiseev It seems that the mutating and non-mutating operators are required, otherwise I get ambiguities with requirements inherited, unsurprisingly, from Strideable extensions in Stdlib. Going to move this to a validation test to test the crash message |
|
@swift-ci please test |
|
This change adds even more overloads to an already crowded space. Worth timing stdlib compile times... Just to be sure. |
CodaFi
changed the title
|
Out of curiosity, did we consider removing |
|
I originally wanted to, but doing so is source-breaking. FWIW, these are correct witnesses in every case but the one in the validation test/radar. You really have to struggle with basically re-implementing a numeric type to come close to this trap. |
|
(Or, in my case, break overload resolution for all the numeric types and wonder why LLVM was inserting infinite loops in odd places) |
|
Hm. This is a little unfortunate because they become part of the ABI, but I guess they're tiny. Can you also mark them deprecated, so that the user will get a warning at compile time too? (Unfortunately they can't be unavailable because that affects overload resolution.) |
|
The ABI impact could be nullified by making them inlinable, when we have that. |
Strideable declares a default witness for Equatable and Comparable
extension Strideable {
@_inlineable
public static func < (x: Self, y: Self) -> Bool {
return x.distance(to: y) > 0
}
@_inlineable
public static func == (x: Self, y: Self) -> Bool {
return x.distance(to: y) == 0
}
}
This witness is implemented recursively because the expectation
is that Stride != Self. However, it is possible to implement
SignedNumeric and Strideable together and inherit this conformance
which will cause undefined behavior - usually a crash.
Provide an overload when Stride == Self and crash with custom message
that mentions that Equatable and Comparable have to be implemented
in this case.
- It's better than nothing.
|
@swift-ci please test |
|
@swift-ci please test source compatibility |
|
⛵️ |
The wording for the errors and the docs needs to be shopped, but otherwise this should do it.
Improves QoI for rdar://33668987