Merge pull request #1271 from retronym/ticket/6331

SI-6331 deconst If type / refine equality of floating point Constant types.

7 files changed, 201 insertions, 4 deletions

diff --git a/src/compiler/scala/tools/nsc/typechecker/Typers.scala b/src/compiler/scala/tools/nsc/typechecker/Typers.scala
index 738dde7895..d0722f7b98 100644
--- a/src/compiler/scala/tools/nsc/typechecker/Typers.scala
+++ b/src/compiler/scala/tools/nsc/typechecker/Typers.scala
@@ -4086,7 +4086,7 @@ trait Typers extends Modes with Adaptations with Tags {
             if ( opt.virtPatmat && !isPastTyper
               && thenp1.tpe.annotations.isEmpty && elsep1.tpe.annotations.isEmpty // annotated types need to be lubbed regardless (at least, continations break if you by pass them like this)
               && thenTp =:= elseTp
-               ) (thenp1.tpe, false) // use unpacked type
+               ) (thenp1.tpe.deconst, false) // use unpacked type. Important to deconst, as is done in ptOrLub, otherwise `if (???) 0 else 0` evaluates to 0 (SI-6331)
             // TODO: skolemize (lub of packed types) when that no longer crashes on files/pos/t4070b.scala
             else ptOrLub(thenp1.tpe :: elsep1.tpe :: Nil, pt)
 
diff --git a/src/partest/scala/tools/partest/package.scala b/src/partest/scala/tools/partest/package.scala
index 08934ef143..49d3ed301c 100644
--- a/src/partest/scala/tools/partest/package.scala
+++ b/src/partest/scala/tools/partest/package.scala
@@ -73,4 +73,31 @@ package object partest {
 
   def isPartestDebug: Boolean =
     propOrEmpty("partest.debug") == "true"
+
+
+  import language.experimental.macros
+
+  /**
+   * `trace("".isEmpty)` will return `true` and as a side effect print the following to standard out.
+   * {{{
+   *   trace> "".isEmpty
+   *   res: Boolean = true
+   *
+   * }}}
+   *
+   * An alternative to [[scala.tools.partest.ReplTest]] that avoids the inconvenience of embedding
+   * test code in a string.
+   */
+  def trace[A](a: A) = macro traceImpl[A]
+
+  import scala.reflect.macros.Context
+  def traceImpl[A: c.AbsTypeTag](c: Context)(a: c.Expr[A]): c.Expr[A] = {
+    import c.universe._
+    val exprCode = c.literal(show(a.tree))
+    val exprType = c.literal(show(a.actualType))
+    reify {
+      println(s"trace> ${exprCode.splice}\nres: ${exprType.splice} = ${a.splice}\n")
+      a.splice
+    }
+  }
 }
diff --git a/src/reflect/scala/reflect/internal/Constants.scala b/src/reflect/scala/reflect/internal/Constants.scala
index e5a543da46..b434be64a3 100644
--- a/src/reflect/scala/reflect/internal/Constants.scala
+++ b/src/reflect/scala/reflect/internal/Constants.scala
@@ -31,6 +31,9 @@ trait Constants extends api.Constants {
   final val EnumTag    = 13
 
   case class Constant(value: Any) extends ConstantApi {
+    import java.lang.Double.doubleToRawLongBits
+    import java.lang.Float.floatToRawIntBits
+
     val tag: Int = value match {
       case null         => NullTag
       case x: Unit      => UnitTag
@@ -81,10 +84,10 @@ trait Constants extends api.Constants {
 
     /** We need the equals method to take account of tags as well as values.
      */
+    // !!! In what circumstance could `equalHashValue == that.equalHashValue && tag != that.tag` be true?
     override def equals(other: Any): Boolean = other match {
       case that: Constant =>
-        this.tag == that.tag &&
-        (this.value == that.value || this.isNaN && that.isNaN)
+        this.tag == that.tag && equalHashValue == that.equalHashValue
       case _ => false
     }
 
@@ -236,7 +239,30 @@ trait Constants extends api.Constants {
     def typeValue: Type     = value.asInstanceOf[Type]
     def symbolValue: Symbol = value.asInstanceOf[Symbol]
 
-    override def hashCode: Int = value.## * 41 + 17
+    /**
+     * Consider two `NaN`s to be identical, despite non-equality
+     * Consider -0d to be distinct from 0d, despite equality
+     *
+     * We use the raw versions (i.e. `floatToRawIntBits` rather than `floatToIntBits`)
+     * to avoid treating different encodings of `NaN` as the same constant.
+     * You probably can't express different `NaN` varieties as compile time
+     * constants in regular Scala code, but it is conceivable that you could
+     * conjure them with a macro.
+     */
+    private def equalHashValue: Any = value match {
+      case f: Float  => floatToRawIntBits(f)
+      case d: Double => doubleToRawLongBits(d)
+      case v         => v
+    }
+
+    override def hashCode: Int = {
+      import scala.util.hashing.MurmurHash3._
+      val seed = 17
+      var h = seed
+      h = mix(h, tag.##) // include tag in the hash, otherwise 0, 0d, 0L, 0f collide.
+      h = mix(h, equalHashValue.##)
+      finalizeHash(h, length = 2)
+    }
   }
 
   object Constant extends ConstantExtractor
diff --git a/test/files/run/t6331.check b/test/files/run/t6331.check
new file mode 100644
index 0000000000..9bf3f7823a
--- /dev/null
+++ b/test/files/run/t6331.check
@@ -0,0 +1,23 @@
+          () == ()
+        true == true
+        true != false
+       false != true
+    0.toByte == 0.toByte
+    0.toByte != 1.toByte
+   0.toShort == 0.toShort
+   0.toShort != 1.toShort
+           0 == 0
+           0 != 1
+          0L == 0L
+          0L != 1L
+        0.0f == 0.0f
+        0.0f != -0.0f
+       -0.0f != 0.0f
+        NaNf == NaNf
+        0.0d == 0.0d
+        0.0d != -0.0d
+       -0.0d != 0.0d
+        NaNd == NaNd
+           0 != 0.0d
+           0 != 0L
+        0.0d != 0.0f
diff --git a/test/files/run/t6331.scala b/test/files/run/t6331.scala
new file mode 100644
index 0000000000..4e43a7686e
--- /dev/null
+++ b/test/files/run/t6331.scala
@@ -0,0 +1,71 @@
+import scala.tools.partest._
+import java.io._
+import scala.tools.nsc._
+import scala.tools.nsc.util.CommandLineParser
+import scala.tools.nsc.{Global, Settings, CompilerCommand}
+import scala.tools.nsc.reporters.ConsoleReporter
+
+// Test of Constant#equals, which must must account for floating point intricacies.
+object Test extends DirectTest {
+
+  override def code = ""
+
+  override def show() {
+    val global = newCompiler()
+    import global._
+
+    def check(c1: Any, c2: Any): Unit = {
+      val const1 = Constant(c1)
+      val const2 = Constant(c2)
+      val equal = const1 == const2
+      def show(a: Any) = "" + a + (a match {
+        case _: Byte   => ".toByte"
+        case _: Short  => ".toShort"
+        case _: Long   => "L"
+        case _: Float  => "f"
+        case _: Double => "d"
+        case _ => ""
+      })
+      val op = if (equal) "==" else "!="
+      println(f"${show(c1)}%12s $op ${show(c2)}")
+
+      val hash1 = const1.hashCode
+      val hash2 = const2.hashCode
+      val hashesEqual = hash1 == hash2
+      val hashBroken = equal && !hashesEqual
+      if (hashBroken) println(f"$hash1%12s != $hash2 // hash codes differ for equal objects!!")
+    }
+
+    check((), ())
+
+    check(true, true)
+    check(true, false)
+    check(false, true)
+
+    check(0.toByte, 0.toByte)
+    check(0.toByte, 1.toByte)
+
+    check(0.toShort, 0.toShort)
+    check(0.toShort, 1.toShort)
+
+    check(0, 0)
+    check(0, 1)
+
+    check(0L, 0L)
+    check(0L, 1L)
+
+    check(0f, 0f)
+    check(0f, -0f)
+    check(-0f, 0f)
+    check(Float.NaN, Float.NaN)
+
+    check(0d, 0d)
+    check(0d, -0d)
+    check(-0d, 0d)
+    check(Double.NaN, Double.NaN)
+
+    check(0, 0d)
+    check(0, 0L)
+    check(0d, 0f)
+  }
+}
diff --git a/test/files/run/t6331b.check b/test/files/run/t6331b.check
new file mode 100644
index 0000000000..6ca09e3814
--- /dev/null
+++ b/test/files/run/t6331b.check
@@ -0,0 +1,30 @@
+trace> if (Test.this.t)
+  -0.0
+else
+  0.0
+res: Double = -0.0
+
+trace> if (Test.this.t)
+  0.0
+else
+  -0.0
+res: Double = 0.0
+
+trace> Test.this.intercept.apply[Any](if (scala.this.Predef.???)
+  -0.0
+else
+  0.0)
+res: Any = class scala.NotImplementedError
+
+trace> Test.this.intercept.apply[Any](if (scala.this.Predef.???)
+  0.0
+else
+  0.0)
+res: Any = class scala.NotImplementedError
+
+trace> Test.this.intercept.apply[Any](if (scala.this.Predef.???)
+  ()
+else
+  ())
+res: Any = class scala.NotImplementedError
+
diff --git a/test/files/run/t6331b.scala b/test/files/run/t6331b.scala
new file mode 100644
index 0000000000..f966abea51
--- /dev/null
+++ b/test/files/run/t6331b.scala
@@ -0,0 +1,20 @@
+import scala.tools.partest._
+import java.io._
+import scala.tools.nsc._
+import scala.tools.nsc.util.CommandLineParser
+import scala.tools.nsc.{Global, Settings, CompilerCommand}
+import scala.tools.nsc.reporters.ConsoleReporter
+
+import scala.tools.partest.trace
+import scala.util.control.Exception._
+
+
+object Test extends App {
+  def intercept = allCatch.withApply(_.getClass)
+  val t: Boolean = true
+  trace(if (t) -0d else 0d)
+  trace(if (t) 0d else -0d)
+  trace(intercept(if (???) -0d else 0d))
+  trace(intercept(if (???) 0d else 0d))
+  trace(intercept(if (???) () else ()))
+}