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

== returns true when comparing a boxed NaN with itself (the same boxed instance) #11698

Open
sjrd opened this issue Aug 21, 2019 · 6 comments
Open
Milestone

Comments

@sjrd
Copy link
Member

sjrd commented Aug 21, 2019

I know the topic has been brought up several times in the past (#10112, scala/scala-dev#329, #6492, #2574), and typically discarded as as-designed, but I think found one case that is genuinely incorrect, as a result of an incorrect optimization:

$ scala
Welcome to Scala 2.12.8 (OpenJDK 64-Bit Server VM, Java 1.8.0_222).Type in expressions for evaluation. Or try :help.

scala> def eqeq(x: Any, y: Any): Boolean = x == y
eqeq: (x: Any, y: Any)Boolean

scala> eqeq(Double.NaN, Double.NaN)
res0: Boolean = false

scala> def eqeqItself(x: Any): Boolean = eqeq(x, x)
eqeqItself: (x: Any)Boolean

scala> eqeqItself(Double.NaN)
res1: Boolean = true

In fact I think this distilled problem was part of the report in #6492, but it was mixed with other things and thus too easily dismissed.

The issue here is that, although a boxed NaN is supposed to compare with a boxed NaN as not ==, there is one way to make it return true: if we compare it with itself, as in, the exact same instance of java.lang.Double.

The cause of this issue is that BoxesRunTime.equals starts by short-circuiting the case x == y as true (this is Java code, so == is reference equality) at
https://github.com/scala/scala/blob/935ff010e1404062dadcfb39b23e99d637fe57c7/src/library/scala/runtime/BoxesRunTime.java#L118
That optimization is not correct when x and y are both the same boxed NaN.

I'm not sure how to fix this without affecting the performance of ==. If performance considerations are not an issue, an easy fix would be to replace the true by a call to isNotNaN(x) defined as

private static boolean isNotNaN(Object x) {
  if (x instanceof Double) return !((Double) x).isNaN();
  if (x instanceof Float) return !((Float) x).isNaN();
  return true;
}
@sjrd
Copy link
Member Author

sjrd commented Aug 21, 2019

OK, this is worse than I thought. And that's when you look more attentively to #6492. Because if any data structure x contains a NaN, then x == x is going to return true, even though it would be structurally not equal to any y that is a "copy" of it:

scala> eqeq(List(Double.NaN), List(Double.NaN))
res2: Boolean = false

scala> eqeqItself(List(Double.NaN))
res3: Boolean = true

And at that point, the only way to fix it is to completely remove the fast path for x == y (Java code).

That is probably not acceptable, and this is what was discussed 10 years ago at https://www.scala-lang.org/old/node/4254.

So I guess that's a won't fix?

Ah! If only we got rid of cooperative equality for good! This issue, along many others, would be gone.

@NthPortal
Copy link

I'm inclined to agree that it's a won't fix.

NaN makes me sad... :(

@eed3si9n

This comment was marked as outdated.

@sjrd
Copy link
Member Author

sjrd commented Aug 25, 2019

@eed3si9n I'm afraid you're missing the point. == in Scala is specced to behave the same regardless of whether operands are boxed or not. This is known as cooperative equality.

Here we see a violation of this principle: NaN == NaN is false when the operands' types are primitives. And it also usually is false when they are boxed. Except (and this is a bug, whether won't fix or not) when both sides are the same box (as opposed to two different boxes of the same value).

@nogurenn

This comment has been minimized.

@Atry
Copy link

Atry commented Feb 15, 2023

@specialized could change the behavior:

@noinline def equalsToSelf[A](a: A) = a == a
@noinline def specializedEqualsToSelf[@specialized A](a: A) = a == a

println(equalsToSelf(Double.NaN)) // true
println(specializedEqualsToSelf(Double.NaN)) // false

@SethTisue SethTisue added this to the Backlog milestone Feb 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants