From c619f94a9cfbddc12c9c5df3affb4636f8982a0a Mon Sep 17 00:00:00 2001 From: Jason Zaugg Date: Sat, 8 Sep 2012 18:37:59 +0200 Subject: SI-6331 Avoid typing an If tree with a constant type. The fast path in typedIf added in 8552740b avoided lubbing the if/else branch types if they are identical, but this fails to deconst the type. This could lead to the entire if expression being replaced by a constant. Also introduces a new tool in partest for nicer checkfiles. // in Test.scala trace(if (t) -0d else 0d) // in Test.check trace> if (Test.this.t) -0.0 else 0.0 res: Double = -0.0 --- .../scala/tools/nsc/typechecker/Typers.scala | 2 +- src/partest/scala/tools/partest/package.scala | 27 +++++++++++++++++++ test/files/run/t6331b.check | 30 ++++++++++++++++++++++ test/files/run/t6331b.scala | 20 +++++++++++++++ 4 files changed, 78 insertions(+), 1 deletion(-) create mode 100644 test/files/run/t6331b.check create mode 100644 test/files/run/t6331b.scala diff --git a/src/compiler/scala/tools/nsc/typechecker/Typers.scala b/src/compiler/scala/tools/nsc/typechecker/Typers.scala index 9cf5d42e00..c878828aad 100644 --- a/src/compiler/scala/tools/nsc/typechecker/Typers.scala +++ b/src/compiler/scala/tools/nsc/typechecker/Typers.scala @@ -4067,7 +4067,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/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 ())) +} -- cgit v1.2.3 From 815f60ff9c50d22a23fbc9d980570fb6941a7d71 Mon Sep 17 00:00:00 2001 From: Jason Zaugg Date: Sat, 8 Sep 2012 18:43:22 +0200 Subject: Refine equality of Constant types over floating point values. The constant types for 0d and -0d should not be equal. This is implemented by checking equality of the result of doubleToRawLongBits / floatToRawIntBits, which also correctly considers two NaNs of the same flavour to be equal. Followup to SI-6331. --- src/reflect/scala/reflect/internal/Constants.scala | 12 ++++- test/files/run/t6331.check | 23 ++++++++ test/files/run/t6331.scala | 63 ++++++++++++++++++++++ 3 files changed, 96 insertions(+), 2 deletions(-) create mode 100644 test/files/run/t6331.check create mode 100644 test/files/run/t6331.scala diff --git a/src/reflect/scala/reflect/internal/Constants.scala b/src/reflect/scala/reflect/internal/Constants.scala index e5a543da46..4d512e3864 100644 --- a/src/reflect/scala/reflect/internal/Constants.scala +++ b/src/reflect/scala/reflect/internal/Constants.scala @@ -83,8 +83,16 @@ trait Constants extends api.Constants { */ override def equals(other: Any): Boolean = other match { case that: Constant => - this.tag == that.tag && - (this.value == that.value || this.isNaN && that.isNaN) + // Consider two NaNs to be identical, despite non-equality + // Consider -0d to be distinct from 0d, despite equality + import java.lang.Double.doubleToRawLongBits + import java.lang.Float.floatToRawIntBits + + this.tag == that.tag && ((value, that.value) match { + case (f1: Float, f2: Float) => floatToRawIntBits(f1) == floatToRawIntBits(f2) + case (d1: Double, d2: Double) => doubleToRawLongBits(d1) == doubleToRawLongBits(d2) + case (v1, v2) => v1 == v2 + }) case _ => false } 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..e9ed96fad3 --- /dev/null +++ b/test/files/run/t6331.scala @@ -0,0 +1,63 @@ +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 equal = Constant(c1) == Constant(c2) + 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)}") + } + + 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) + } +} -- cgit v1.2.3 From aedb8db47338637430672b145cfc11e8d89441b9 Mon Sep 17 00:00:00 2001 From: Jason Zaugg Date: Sun, 9 Sep 2012 11:16:24 +0200 Subject: Test for consistency of Constant#{equals, hashCode}. For the examples I've constructed, they are consistent, but I put this down to good luck, rather than good management. The next commit will address this. --- test/files/run/t6331.scala | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/test/files/run/t6331.scala b/test/files/run/t6331.scala index e9ed96fad3..4e43a7686e 100644 --- a/test/files/run/t6331.scala +++ b/test/files/run/t6331.scala @@ -15,7 +15,9 @@ object Test extends DirectTest { import global._ def check(c1: Any, c2: Any): Unit = { - val equal = Constant(c1) == Constant(c2) + 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" @@ -26,6 +28,12 @@ object Test extends DirectTest { }) 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((), ()) -- cgit v1.2.3 From 21bd081540413a8625247d2e40506112cc1ea218 Mon Sep 17 00:00:00 2001 From: Jason Zaugg Date: Sun, 9 Sep 2012 13:04:31 +0200 Subject: Improve Constant#hashCode - Incorporate `tag`, which is considered by equals, to reduce collisions. - Use the result of floatToRawIntBits(value) / doubleToRawLongBits(value), rather than value. This wasn't strictly necessary as (0d.## == (-0d).##) but this is more obviously correct. --- src/reflect/scala/reflect/internal/Constants.scala | 40 ++++++++++++++++------ 1 file changed, 29 insertions(+), 11 deletions(-) diff --git a/src/reflect/scala/reflect/internal/Constants.scala b/src/reflect/scala/reflect/internal/Constants.scala index 4d512e3864..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,18 +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 => - // Consider two NaNs to be identical, despite non-equality - // Consider -0d to be distinct from 0d, despite equality - import java.lang.Double.doubleToRawLongBits - import java.lang.Float.floatToRawIntBits - - this.tag == that.tag && ((value, that.value) match { - case (f1: Float, f2: Float) => floatToRawIntBits(f1) == floatToRawIntBits(f2) - case (d1: Double, d2: Double) => doubleToRawLongBits(d1) == doubleToRawLongBits(d2) - case (v1, v2) => v1 == v2 - }) + this.tag == that.tag && equalHashValue == that.equalHashValue case _ => false } @@ -244,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 -- cgit v1.2.3