Provide heterogeneous floating-point comparisons, remove old compare hooks. #14987

stephentyrone · 2018-03-05T18:51:42Z

This patch makes two fairly significant changes to the FloatingPoint and BinaryFloatingPoint protocols and concrete types.

Provides heterogeneous comparison between members conforming to BinaryFloatingPoint. This was part of the original SE-0067, but was not implementable at the time because we didn't have SE-0104 yet.
Makes the isEqual(to:), isLess(than:) and isLessThanOrEqualTo() methods on FloatingPoint and concrete types obsolete. At the time that these were added, this was planned to be the means of conforming to Equatable / Comparable, but we ended up going in a different direction with the static operators ==, <, <= instead.

Of particular relevance, the second change is breaking, I don't think that much code uses these methods directly, but if people have types that conform to FloatingPoint or BinaryFloatingPoint, they may have used these as the implementation hooks, as that was the original documented design. I'm not sure if we can actually make this change, and if we can I'm not precisely sure what affordances we need to make, so feedback on this point is particularly welcome.

…e methods. This patch makes two fairly significant changes to the FloatingPoint and BinaryFloatingPoint protocols and concrete types. - Provides heterogeneous comparison between members conforming to BinaryFloatingPoint. This was part of the original SE-0067, but was not implementable at the time because we didn't have SE-0104 yet. - Makes the `isEqual(to:)`, `isLess(than:)` and `isLessThanOrEqualTo()` methods on FloatingPoint and concrete types obsolete. At the time that these were added, this was planned to be the means of conforming to Equatable / Comparable, but we ended up going in a different direction with the static operators ==, <, <= instead. Of particular relevance, the second change is breaking, I don't think that much code uses these methods directly, but if people have types that conform to FloatingPoint or BinaryFloatingPoint, they may have used these as the implementation hooks, as that was the original documented design. I'm not sure if we can actually make this change, and if we can I'm not precisely sure what affordances we need to make, so feedback on this point is particularly welcome.

xwu · 2018-03-05T20:00:50Z

Mmm. The isEqual(to:) trampolines were nice for another reason:

For ordinary methods, if end users have already implemented an identically named method, that implementation correctly shadows any newly added standard library implementation. Overload resolution is different for operators, however. If end users have implemented a conflicting ==, vending a standard library implementation leads to ambiguities at the call site for which there's no workaround.

The @_implements hack allowed us to avoid this problem for synthesized conformance to Equatable. For conditional conformances to Equatable, this hack doesn't work due to compiler deficiencies.

As it happened, SwiftPM vended its own conditional == implementation, and to work around it I've rigged up an underscored _isEqual(to:) that the core team no doubt doesn't like one bit.

But this is all to say that even if end users never directly call isEqual(to:), removing it can be source breaking in other ways you may not have anticipated.

While implementing generic floating-point conversions, I considered enabling generic comparisons but did not (and I have the full, non-FIXME version ready to go that works with subnormals) because of an unresolved issue I'm very concerned about.

There's a bug encountered on the integer side which makes heterogeneous comparison expose a potential footgun; @moiseev hacked around the problem in concrete code by putting in concrete implementations of homogeneous comparison operators in each type. The problem is much worse with floating-point code:

If one operand is a literal, overload resolution will favor heterogeneous comparison with FloatLiteralType over homogeneous comparison.

With integer literals, this now only shows up in generic code (where concrete overloads can't hack around the problem), thanks to @moiseev's hack, and now only becomes apparent when you write ~0 and get the wrong bit pattern. (Though it seems rare, that actually has been a source of error even within the Swift project itself.) With float literals, there will be unintentional imprecision with Float80 comparisons every time a literal value is used because it'll be interpreted as Double.

We still do not have a solution to this problem. The integer variation of the issue is tracked as SR-4984. In my book, the unintentional imprecision problem causes me to consider heterogeneous floating-point comparison to be "unimplementable" still.

stephentyrone · 2018-03-05T20:11:53Z

If one operand is a literal, overload resolution will favor heterogeneous comparison with FloatLiteralType over homogeneous comparison.

This doesn't actually seem to be the case:

(swift) let xf = 1.1 as Float
// xf : Float = 1.10000002
(swift) let xd = 1.1 as Double
// xd : Double = 1.1000000000000001
(swift) let xl = 1.1 as Float80
// xl : Float80 = 1.10000000000000000002
(swift) xf == 1.1
// r7 : Bool = true
(swift) xd == 1.1
// r8 : Bool = true
(swift) xl == 1.1
// r9 : Bool = true
(swift) xf == xd
// r10 : Bool = false
(swift) xf == xl
// r11 : Bool = false
(swift) xd == xl
// r12 : Bool = false

Or am I misinterpreting your comment?

stephentyrone · 2018-03-05T20:16:30Z

Ah, I see, you mean in a generic context. Interesting.

xwu · 2018-03-05T20:17:29Z

With concrete implementations present on the individual types, there's a hack that causes this footgun not to strike in concrete code. The same does not help in generic code. Try this on ToT today, then in your branch:

func isOnePointTwo<T : BinaryFloatingPoint>(_ value: T) -> Bool {
  return value == 1.2
}

isOnePointTwo(1.2 as Float80)

Yeah, the result becomes different. Subtle, scary.
(See here for @moiseev's version for integers.)

stephentyrone · 2018-03-05T20:41:19Z

Well, this just ended up pointing me to the fact that float literal parsing is fundamentally broken anyway. For whatever reason I hadn't noticed this previously, but the way _ExpressibleByBuiltinFloatLiteral is implemented leads invariably to double rounding. For example, on x86:

1> let x = 1.000000000000000111076512571139929264063539449125528335571
x: Double = 1

This is incorrect; the correct result would be 1.0000000000000002.

Explanation: the literal is just slightly greater than 1 + Double.ulpOfOne/2, so it should round up to 1 + Double.ulpOfOne. But what actually happens is that it is first rounded to _MaxBuiltinFloatType, AKA Float80, which results in exactly 1 + Double.ulpOfOne/2; because this is an exact halfway case, it rounds to the value with the most trailing zeros, 1.

Sigh.

stephentyrone · 2018-03-05T21:02:07Z

https://bugs.swift.org/browse/SR-7124

xwu · 2018-03-05T21:09:06Z

Sigh. Definitely not great, but at least that bug is messing up only ("only") a smaller set of values.

I'm eager to hear about any progress as to the sorts of discussions @dabrahams mentioned regarding the interaction of literal parsing and operators. IMHO, if that can be resolved, then there's no impediment to this change here, but at the moment we're promoting these powerful generic facilities and then shipping subtle footguns.

natecook1000

Thanks @stephentyrone! A few edits and a Q for you.

natecook1000 · 2018-03-05T19:02:03Z

stdlib/public/core/FloatingPoint.swift.gyb

  ///     // true
-  ///     x.isEqual(to: .nan)
+  ///     x == .nan


Just checking — with the heterogenous operators, do these expressions still type check properly? i.e. it correctly assumes Double for the .nan property?

natecook1000 · 2018-03-05T21:15:26Z

stdlib/public/core/FloatingPoint.swift.gyb

@@ -1011,94 +1011,85 @@ public protocol FloatingPoint: SignedNumeric, Strideable, Hashable {
  /// - If `x` is `-greatestFiniteMagnitude`, then `x.nextDown` is `-infinity`.
  var nextDown: Self { get }

-  /// Returns a Boolean value indicating whether this instance is equal to the
-  /// given value.
+  /// Returns a Boolean value indicating whether `lhs` is equal to `rhs`.


Can all the abstracts follow this format:

Returns a Boolean value indicating whether (two values are equal / one value is less than another).
empty line
Start any discussion here...

natecook1000 · 2018-03-05T21:20:36Z

stdlib/public/core/FloatingPoint.swift.gyb

+  /// - rhs: The second value to compare.
+  /// - Returns: `true` if `lhs` and `rhs` represent the same extended real
+  ///   number value; otherwise, `false`. If either `lhs` or `rhs` is NaN,
+  ///   the result of this oeprator is `false`.


oeprator —> operator (looks like this shows up four times)

natecook1000 · 2018-03-05T21:29:41Z

stdlib/public/core/FloatingPoint.swift.gyb

-  /// - `-infinity` compares less than all values except for itself and NaN.
-  /// - Every value except for NaN and `+infinity` compares less than
-  ///   `+infinity`.
+  /// - Because NaN is incomparable with any value, this method returns `false`


Instead of this method returns `false` when let's say the result of this comparison is `false` when, throughout.

stephentyrone · 2018-03-07T13:29:37Z

Closing out this PR until we have a coherent plan for literals.

stephentyrone requested review from natecook1000 and moiseev March 5, 2018 18:51

natecook1000 reviewed Mar 5, 2018

View reviewed changes

stephentyrone closed this Mar 7, 2018

stephentyrone deleted the fp-comparison-operators-only branch March 24, 2018 20:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Provide heterogeneous floating-point comparisons, remove old compare hooks. #14987

Provide heterogeneous floating-point comparisons, remove old compare hooks. #14987

Uh oh!

stephentyrone commented Mar 5, 2018

Uh oh!

xwu commented Mar 5, 2018 •

edited

Loading

Uh oh!

stephentyrone commented Mar 5, 2018 •

edited

Loading

Uh oh!

stephentyrone commented Mar 5, 2018

Uh oh!

xwu commented Mar 5, 2018 •

edited

Loading

Uh oh!

stephentyrone commented Mar 5, 2018

Uh oh!

stephentyrone commented Mar 5, 2018

Uh oh!

xwu commented Mar 5, 2018

Uh oh!

natecook1000 left a comment

Uh oh!

natecook1000 Mar 5, 2018

Uh oh!

natecook1000 Mar 5, 2018

Uh oh!

natecook1000 Mar 5, 2018

Uh oh!

natecook1000 Mar 5, 2018

Uh oh!

stephentyrone commented Mar 7, 2018

Uh oh!

Uh oh!

Provide heterogeneous floating-point comparisons, remove old compare hooks. #14987

Provide heterogeneous floating-point comparisons, remove old compare hooks. #14987

Uh oh!

Conversation

stephentyrone commented Mar 5, 2018

Uh oh!

xwu commented Mar 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stephentyrone commented Mar 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stephentyrone commented Mar 5, 2018

Uh oh!

xwu commented Mar 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stephentyrone commented Mar 5, 2018

Uh oh!

stephentyrone commented Mar 5, 2018

Uh oh!

xwu commented Mar 5, 2018

Uh oh!

natecook1000 left a comment

Choose a reason for hiding this comment

Uh oh!

natecook1000 Mar 5, 2018

Choose a reason for hiding this comment

Uh oh!

natecook1000 Mar 5, 2018

Choose a reason for hiding this comment

Uh oh!

natecook1000 Mar 5, 2018

Choose a reason for hiding this comment

Uh oh!

natecook1000 Mar 5, 2018

Choose a reason for hiding this comment

Uh oh!

stephentyrone commented Mar 7, 2018

Uh oh!

Uh oh!

xwu commented Mar 5, 2018 •

edited

Loading

stephentyrone commented Mar 5, 2018 •

edited

Loading

xwu commented Mar 5, 2018 •

edited

Loading