Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bug#10511 Add total orderings for Float and Double #6410

Merged
merged 1 commit into from Jun 4, 2018

Conversation

@NthPortal
Copy link
Contributor

commented Mar 12, 2018

Add total orderings for Float and Double, so that there
are two implicit orderings for each in scope: one
consistent with a total ordering, and one consistent with
IEEE spec.

Fixes scala/bug#10511

@NthPortal

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2018

review @Ichoran

@scala-jenkins scala-jenkins requested a review from Ichoran Mar 12, 2018

@NthPortal

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2018

@SethTisue could you run a community build on this, to see how much code assumes that there is a non-ambiguous implicit Ordering for Float and Double in scope? Thanks :)

@NthPortal NthPortal force-pushed the NthPortal:bug#10511/R6 branch 3 times, most recently from 0a05589 to b69d410 Mar 12, 2018

@SethTisue

This comment has been minimized.

Copy link
Member

commented Mar 13, 2018

@NthPortal

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2018

Problems:

  • scalaz (tests only)
  • scala-js
  • singleton-ops (but doesn't seem to be from this)
  • scoverage
  • scala-java8-compat (tests only)

scala-js is the one I'm most concerned about

@dwijnand

This comment has been minimized.

Copy link
Member

commented Mar 23, 2018

is two orderings (of the same type) in implicit scope better than no orderings in implicit scope?

@sjrd

This comment has been minimized.

Copy link
Member

commented Mar 23, 2018

scala-js is the one I'm most concerned about

[scala-js] [error] /home/jenkins/workspace/scala-2.13.x-integrate-community-build/target-0.9.11/project-builds/scala-js-9a26f4316928c442ef5f745301eae97d16ffe241/javalib/src/main/scala/java/util/Arrays.scala:52: error: ambiguous implicit values:
[scala-js] [error]  both object FloatTotalOrdering in object Ordering of type scala.math.Ordering.FloatTotalOrdering.type
[scala-js] [error]  and object FloatIeeeOrdering in object Ordering of type scala.math.Ordering.FloatIeeeOrdering.type
[scala-js] [error]  match expected type Ordering[Float]
[scala-js] [error]     sortImpl(a)
[scala-js] [error]             ^
[scala-js] [error] /home/jenkins/workspace/scala-2.13.x-integrate-community-build/target-0.9.11/project-builds/scala-js-9a26f4316928c442ef5f745301eae97d16ffe241/javalib/src/main/scala/java/util/Arrays.scala:55: error: ambiguous implicit values:
[scala-js] [error]  both object FloatTotalOrdering in object Ordering of type scala.math.Ordering.FloatTotalOrdering.type
[scala-js] [error]  and object FloatIeeeOrdering in object Ordering of type scala.math.Ordering.FloatIeeeOrdering.type
[scala-js] [error]  match expected type Ordering[Float]
[scala-js] [error]     sortRangeImpl[Float](a, fromIndex, toIndex)
[scala-js] [error]                         ^
[scala-js] [error] /home/jenkins/workspace/scala-2.13.x-integrate-community-build/target-0.9.11/project-builds/scala-js-9a26f4316928c442ef5f745301eae97d16ffe241/javalib/src/main/scala/java/util/Arrays.scala:58: error: ambiguous implicit values:
[scala-js] [error]  both object DoubleTotalOrdering in object Ordering of type scala.math.Ordering.DoubleTotalOrdering.type
[scala-js] [error]  and object DoubleIeeeOrdering in object Ordering of type scala.math.Ordering.DoubleIeeeOrdering.type
[scala-js] [error]  match expected type Ordering[Double]
[scala-js] [error]     sortImpl(a)
[scala-js] [error]             ^
[scala-js] [error] /home/jenkins/workspace/scala-2.13.x-integrate-community-build/target-0.9.11/project-builds/scala-js-9a26f4316928c442ef5f745301eae97d16ffe241/javalib/src/main/scala/java/util/Arrays.scala:61: error: ambiguous implicit values:
[scala-js] [error]  both object DoubleTotalOrdering in object Ordering of type scala.math.Ordering.DoubleTotalOrdering.type
[scala-js] [error]  and object DoubleIeeeOrdering in object Ordering of type scala.math.Ordering.DoubleIeeeOrdering.type
[scala-js] [error]  match expected type Ordering[Double]
[scala-js] [error]     sortRangeImpl[Double](a, fromIndex, toIndex)
[scala-js] [error]                          ^
[scala-js] [error] four errors found

That can be addressed by adding the explicit Ordering that corresponds to the old behavior. Nothing to significantly worry about.

@NthPortal

This comment has been minimized.

Copy link
Contributor Author

commented Mar 23, 2018

@dwijnand having them both in scope means that users know what they are called and thus where their documentation is (hopefully helping them choose); if none were in scope, they wouldn't know where to start, and would just be confused why there's no Ordering for Double/Float

@NthPortal

This comment has been minimized.

Copy link
Contributor Author

commented Mar 23, 2018

@sjrd Because the sorts use Ordering.lteq, I'm going to recommend FloatTotalOrdering and DoubleTotalOrdering.

The behaviour in 2.12 is that of FloatIeeeOrdering and DoubleIeeeOrdering, but it is broken and leads to non-deterministic/wrong sorts if NaN is present (which is the primary reason for this PR).

@dwijnand

This comment has been minimized.

Copy link
Member

commented Mar 23, 2018

@NthPortal interesting.. you're right.

@Ichoran

Ichoran approved these changes Apr 1, 2018

Copy link
Contributor

left a comment

LGTM! I'm not sure if there's a way to get a custom message in there in case of the collision (that would be ideal), but I think it's a good compromise between discoverability and safety.

All the details look fine to me.

@hrhino

This comment has been minimized.

Copy link
Member

commented Apr 1, 2018

if there's a way to get a custom message in there in case of the collision

wouldn't @implicitAmbiguous work here?

@Ichoran

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2018

@hrhino - Oh, yes! It would. I hadn't noticed that one. @NthPortal - Can you provide a more solution-oriented message using the annotation? (E.g. suggest passing the ordering directly, or explicitly importing the one that they mean in the context where they mean it?)

@NthPortal

This comment has been minimized.

Copy link
Contributor Author

commented Apr 1, 2018

I did not even know that annotation existed!

I will absolutely work on coming up with a custom message for that

@NthPortal

This comment has been minimized.

Copy link
Contributor Author

commented Apr 1, 2018

Is the following message too long?

"The behaviour of Double specified by IEEE is not consistent with a total ordering when dealing with NaN, so there are two orderings defined for Double: DoubleTotalOrdering, which is consistent with a total ordering, and DoubleIeeeOrdering, which is consistent as much as possible with IEEE spec and floating point operations defined in scala.math"

@Ichoran

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2018

@NthPortal - Yeah, that does seem too long, and it is missing the critical section that tells the receiver of the message what to do. The scaladoc on the instances should have the wordier explanation of the difference between the orderings. If you could come up with something more brief that would be great. The most important thing is to mention that the user needs to select (by import, assignment, or explicit passing) one of the two orderings. This may be the first intentional collision between implicits that the user sees, so we should walk them through solving it.

@NthPortal

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2018

"The behaviour of IEEE Doubles is not always consistent with a total ordering, so there are two orderings defined for Double. You can import the one which best suits your needs, or pass it explicitly as an argument."

@Ichoran thoughts?

I can add the names of the two instances to the message, but the compiler should list them anyway as part of the error message.

@NthPortal

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2018

Thinking of possibly holding off on merging this until #6468 is merged, as it will simplify the changes needed for this PR

@dwijnand

This comment has been minimized.

Copy link
Member

commented Apr 3, 2018

I'd say terser is better (image a few pages of this ambiguity in a build log somewhere), favouring info about how to resolve and deferring further context to another source.

so perhaps something like:

Import, or pass explicitly, the instance of Ordering according to whether total ordering or IEEE compliance is of greater importance. See xxx for more info.

maybe?

</bikeshed>

@Ichoran

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2018

I like the heads-up about the problem, but I agree that terseness is a virtue. Maybe

There is more than one way to order Doubles!  Specify one by using a local import,
assigning an implicit val, or passing the ordering explicitly.  See the documentation
for details.

(And the corresponding one for Float.)

Then the documentation would give examples of each, and discuss the problem at more length.

@NthPortal

This comment has been minimized.

Copy link
Contributor Author

commented Apr 3, 2018

@dwijnand I have literally requested bikeshedding at this point xD

@NthPortal

This comment has been minimized.

Copy link
Contributor Author

commented Apr 3, 2018

I was mistaken - using @implicitAmbiguous entirely overrides the original error message, so listing the implicits' names is required.

@scala-jenkins scala-jenkins requested a review from Ichoran Apr 3, 2018

@NthPortal

This comment has been minimized.

Copy link
Contributor Author

commented Apr 3, 2018

The message used in the latest commit is:

"There are multiple ways to order Doubles (Ordering.DoubleTotalOrdering, Ordering.DoubleIeeeOrdering). Specify one by using a local import, assigning an implicit val, or passing it explicitly. See the documentation for details."

If you can think of any reasonable way to shorten that further, I'm happy to do so, but I can't think of a good way to do so without losing important information.

@dwijnand

This comment has been minimized.

Copy link
Member

commented Apr 3, 2018

lgtm!

@SethTisue

This comment has been minimized.

Copy link
Member

commented May 26, 2018

Are your misgivings resolved?

not really, it still seems pretty bad to me that I can't just sort some numbers.

(though I'm not going to stand in the way if there is otherwise a consensus on merging this.)

it sounds like the plan to do something about it is:

I'd prefer to eventually make the total ordering the default but this will require a transition period

can someone spell out what that path looks like?

@Ichoran

This comment has been minimized.

Copy link
Contributor

commented May 26, 2018

I agree that it's bad that you can't just sort some numbers.

Can we figure out a way to get a warning if you don't import the one that you mean, but to make it work anyway (with the total ordering)? Does that seem like a reasonable compromise, @SethTisue ?

@NthPortal

This comment has been minimized.

Copy link
Contributor Author

commented May 27, 2018

Can we figure out a way to get a warning if you don't import the one that you mean, but to make it work anyway (with the total ordering)?

Should be doable - I'll have something up shortly

@NthPortal

This comment has been minimized.

Copy link
Contributor Author

commented May 27, 2018

What should the deprecation message be? the same as @implicitAmbiguous?

@lrytz

lrytz approved these changes May 28, 2018

Copy link
Member

left a comment

👍 👍

@martijnhoekstra

This comment has been minimized.

Copy link
Contributor

commented May 28, 2018

Is a deprecation warning right? That is, is this usage slated for removal? If it is, what would the removal path look like (per Seth's comment)? If it isn't, is this the local optimum in lieu of the @lint annotation we don't have? Tangent: if it is, should we have one?

@lrytz

This comment has been minimized.

Copy link
Member

commented May 28, 2018

It is a bit of a misuse of @deprecated, but for me it's fine. The repl seems to shows deprecations by default. There's also @migration, but there you only get a warnign if you use -Xmigration.

@NthPortal

This comment has been minimized.

Copy link
Contributor Author

commented May 28, 2018

@martijnhoekstra I would also like some warning annotations as alternatives to @deprecated, but they don't currently exist

@lrytz

This comment has been minimized.

Copy link
Member

commented May 28, 2018

I'm not at all against, but don't think it needs to hold up this PR.

bug#10511 Add total orderings for Float and Double
Add total orderings for Float and Double, so that there
are two implicit orderings for each in scope: one
consistent with a total ordering, and one consistent with
IEEE spec.

@NthPortal NthPortal force-pushed the NthPortal:bug#10511/R6 branch from 42d121e to b686173 Jun 3, 2018

@NthPortal

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2018

squashed

@lrytz

lrytz approved these changes Jun 4, 2018

Copy link
Member

left a comment

🎈

@lrytz lrytz merged commit b6982c8 into scala:2.13.x Jun 4, 2018

3 checks passed

cla @NthPortal signed the Scala CLA. Thanks!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
validate-main [2129] SUCCESS. Took 38 min.
Details

@NthPortal NthPortal deleted the NthPortal:bug#10511/R6 branch Jun 4, 2018

@SethTisue

This comment has been minimized.

Copy link
Member

commented Jun 6, 2018

just to show anyone interested what we actually ended up with:

scala> List(3.0, 2.0, 1.0).sorted
warning: object DeprecatedDoubleOrdering in object Ordering is deprecated (since 2.13.0): There are multiple ways to order Doubles (Ordering.Double.TotalOrdering, Ordering.Double.IeeeOrdering). Specify one by using a local import, assigning an implicit val, or passing it explicitly. See the documentation for details.

@Ichoran

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2018

I like that IeeeOrdering sounds like a wail of despair, which is generally what happens to anyone who uses floating point for too long.

What do you think about the usability of this lengthy error message as the default interaction with sorting, Seth? It still feels nonideal to me, but having undetected wrong assumptions also seems quite nonideal.

@dwijnand

This comment has been minimized.

Copy link
Member

commented Jun 7, 2018

IMO "See the documentation for details" could use some precision - which documentation? The documentation of {Float,Double}.{TotalOrdering,IeeeOrdering} it turns out. So maybe "See their documentation for details"?

@SethTisue

This comment has been minimized.

Copy link
Member

commented Jun 7, 2018

What do you think about the usability of this lengthy error message as the default interaction with sorting, Seth? It still feels nonideal to me, but having undetected wrong assumptions also seems quite nonideal.

idk... there is no ideal solution to this. if it had been just up to me, I would probably have just provided the additional orderings, updated the documentation to refer to them, not deprecated anything, and call it good, and let those with higher standards use a linter to enforce them.

but perhaps I am insufficiently devoted to lawfulness and correctness.

@NthPortal

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2018

What do you think about the usability of this lengthy error message

I would love to cut down on the length of the error message. If anyone has ideas of how to do so without losing significant information, I would be 100% behind that.

So maybe "See their documentation for details"

@dwijnand sounds good to me. I'll wait a little to see if anyone has thoughts on shortening the message in general before PRing the change though.

@nafg

This comment has been minimized.

Copy link
Contributor

commented Aug 10, 2018

I think it's not necessary to say "Specify one by using a local import, assigning an implicit val, or passing it explicitly." That's general-purpose scala knowledge. Especially for a warning which may occur a lot of times.

How about,
"To avoid this warning, use Ordering.Double.TotalOrdering or Ordering.Double.IeeeOrdering instead. Their documentation has more info."

If it's not obvious enough from the context that they're implicits, how about this:
"Instead, use the Ordering.Double.TotalOrdering or Ordering.Double.IeeeOrdering implicit."

Anyway, a newbie has been told to see their docs. Perhaps a (shortened?) link could be included. Anyway the docs would have all the information a newbie needs. And for everyone else it should be self-explanatory.

@nafg

This comment has been minimized.

Copy link
Contributor

commented Aug 10, 2018

(the docs could remind the reader of the various ways of using implicit Orderings)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.