Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

SI-6943 warn on value class miscomparison.

There's a very dangerous situation running around when you
combine universal equality with value classes:

  // All over your code
  val x = "abc"
  if (x == "abc") ...

  // Hey let's make x a value class
  val x = new ValueClass("abc")
  // Uh-oh

There was until now no warning when comparing a value class
with something else. Now there is.
  • Loading branch information...
commit 8448beb2ad6346d1d04a1feedb3de4d77f61024d 1 parent 2e0079e
@paulp paulp authored
View
32 src/compiler/scala/tools/nsc/typechecker/RefChecks.scala
@@ -1102,12 +1102,15 @@ abstract class RefChecks extends InfoTransform with scala.reflect.internal.trans
val s = fn.symbol
(s == Object_==) || (s == Object_!=) || (s == Any_==) || (s == Any_!=)
}
+ def haveSubclassRelationship = (actual isSubClass receiver) || (receiver isSubClass actual)
+
// Whether the operands+operator represent a warnable combo (assuming anyrefs)
// Looking for comparisons performed with ==/!= in combination with either an
// equals method inherited from Object or a case class synthetic equals (for
// which we know the logic.)
def isWarnable = isReferenceOp || (isUsingDefaultScalaOp && isUsingWarnableEquals)
def isEitherNullable = (NullClass.tpe <:< receiver.info) || (NullClass.tpe <:< actual.info)
+ def isEitherValueClass = actual.isDerivedValueClass || receiver.isDerivedValueClass
def isBoolean(s: Symbol) = unboxedValueClass(s) == BooleanClass
def isUnit(s: Symbol) = unboxedValueClass(s) == UnitClass
def isNumeric(s: Symbol) = isNumericValueClass(unboxedValueClass(s)) || isAnyNumber(s)
@@ -1122,6 +1125,12 @@ abstract class RefChecks extends InfoTransform with scala.reflect.internal.trans
// unused
def possibleNumericCount = onSyms(_ filter (x => isNumeric(x) || isMaybeValue(x)) size)
val nullCount = onSyms(_ filter (_ == NullClass) size)
+ def isNonsenseValueClassCompare = (
+ !haveSubclassRelationship
+ && isUsingDefaultScalaOp
+ && isEitherValueClass
+ && !isCaseEquals
+ )
def nonSensibleWarning(what: String, alwaysEqual: Boolean) = {
val msg = alwaysEqual == (name == nme.EQ || name == nme.eq)
@@ -1133,10 +1142,13 @@ abstract class RefChecks extends InfoTransform with scala.reflect.internal.trans
def nonSensiblyNeq() = nonSensible("", false)
def nonSensiblyNew() = nonSensibleWarning("a fresh object", false)
+ def unrelatedMsg = name match {
+ case nme.EQ | nme.eq => "never compare equal"
+ case _ => "always compare unequal"
+ }
def unrelatedTypes() = {
- val msg = if (name == nme.EQ || name == nme.eq)
- "never compare equal" else "always compare unequal"
- unit.warning(pos, typesString + " are unrelated: they will most likely " + msg)
+ val weaselWord = if (isEitherValueClass) "" else " most likely"
+ unit.warning(pos, s"$typesString are unrelated: they will$weaselWord $unrelatedMsg")
}
if (nullCount == 2) // null == null
@@ -1175,15 +1187,19 @@ abstract class RefChecks extends InfoTransform with scala.reflect.internal.trans
}
}
+ // warn if one but not the other is a derived value class
+ // this is especially important to enable transitioning from
+ // regular to value classes without silent failures.
+ if (isNonsenseValueClassCompare)
+ unrelatedTypes()
// possibleNumericCount is insufficient or this will warn on e.g. Boolean == j.l.Boolean
- if (isWarnable && nullCount == 0 && !(isSpecial(receiver) && isSpecial(actual))) {
+ else if (isWarnable && nullCount == 0 && !(isSpecial(receiver) && isSpecial(actual))) {
// better to have lubbed and lost
def warnIfLubless(): Unit = {
val common = global.lub(List(actual.tpe, receiver.tpe))
if (ObjectClass.tpe <:< common)
unrelatedTypes()
}
- def eitherSubclasses = (actual isSubClass receiver) || (receiver isSubClass actual)
// warn if actual has a case parent that is not same as receiver's;
// if actual is not a case, then warn if no common supertype, as below
if (isCaseEquals) {
@@ -1196,14 +1212,12 @@ abstract class RefChecks extends InfoTransform with scala.reflect.internal.trans
//else
// if a class, it must be super to thisCase (and receiver) since not <: thisCase
if (!actual.isTrait && !(receiver isSubClass actual)) nonSensiblyNeq()
- else if (!eitherSubclasses) warnIfLubless()
+ else if (!haveSubclassRelationship) warnIfLubless()
case _ =>
}
}
- else if (actual isSubClass receiver) ()
- else if (receiver isSubClass actual) ()
// warn only if they have no common supertype below Object
- else {
+ else if (!haveSubclassRelationship) {
warnIfLubless()
}
}
View
34 test/files/neg/t5663-badwarneq.check
@@ -1,22 +1,40 @@
-t5663-badwarneq.scala:42: error: comparing case class values of types Some[Int] and None.type using `==' will always yield false
+t5663-badwarneq.scala:47: error: comparing case class values of types Some[Int] and None.type using `==' will always yield false
println(new Some(1) == None) // Should complain on type, was: spuriously complains on fresh object
^
-t5663-badwarneq.scala:43: error: comparing case class values of types Some[Int] and Thing using `==' will always yield false
+t5663-badwarneq.scala:48: error: comparing case class values of types Some[Int] and Thing using `==' will always yield false
println(Some(1) == new Thing(1)) // Should complain on type, was: spuriously complains on fresh object
^
-t5663-badwarneq.scala:51: error: ThingOne and Thingy are unrelated: they will most likely never compare equal
+t5663-badwarneq.scala:56: error: ThingOne and Thingy are unrelated: they will most likely never compare equal
println(t1 == t2) // true, but apparently unrelated, a compromise warning
^
-t5663-badwarneq.scala:52: error: ThingThree and Thingy are unrelated: they will most likely never compare equal
+t5663-badwarneq.scala:57: error: ThingThree and Thingy are unrelated: they will most likely never compare equal
println(t4 == t2) // true, complains because ThingThree is final and Thingy not a subclass, stronger claim than unrelated
^
-t5663-badwarneq.scala:55: error: comparing case class values of types ThingTwo and Some[Int] using `==' will always yield false
+t5663-badwarneq.scala:60: error: comparing case class values of types ThingTwo and Some[Int] using `==' will always yield false
println(t3 == Some(1)) // false, warn on different cases
^
-t5663-badwarneq.scala:56: error: comparing values of types ThingOne and Cousin using `==' will always yield false
+t5663-badwarneq.scala:61: error: comparing values of types ThingOne and Cousin using `==' will always yield false
println(t1 == c) // should warn
^
-t5663-badwarneq.scala:64: error: comparing case class values of types Simple and SimpleSibling.type using `==' will always yield false
+t5663-badwarneq.scala:69: error: comparing case class values of types Simple and SimpleSibling.type using `==' will always yield false
println(new Simple() == SimpleSibling) // like Some(1) == None, but needn't be final case
^
-7 errors found
+t5663-badwarneq.scala:72: error: ValueClass1 and Int are unrelated: they will never compare equal
+ println(new ValueClass1(5) == 5) // bad
+ ^
+t5663-badwarneq.scala:74: error: comparing values of types Int and ValueClass1 using `==' will always yield false
+ println(5 == new ValueClass1(5)) // bad
+ ^
+t5663-badwarneq.scala:78: error: ValueClass2[String] and String are unrelated: they will never compare equal
+ println(new ValueClass2("abc") == "abc") // bad
+ ^
+t5663-badwarneq.scala:79: error: ValueClass2[Int] and ValueClass1 are unrelated: they will never compare equal
+ println(new ValueClass2(5) == new ValueClass1(5)) // bad - different value classes
+ ^
+t5663-badwarneq.scala:81: error: comparing values of types ValueClass3 and ValueClass2[Int] using `==' will always yield false
+ println(ValueClass3(5) == new ValueClass2(5)) // bad
+ ^
+t5663-badwarneq.scala:82: error: comparing values of types ValueClass3 and Int using `==' will always yield false
+ println(ValueClass3(5) == 5) // bad
+ ^
+13 errors found
View
18 test/files/neg/t5663-badwarneq.scala
@@ -17,6 +17,11 @@ class SimpleParent
case class Simple() extends SimpleParent
case object SimpleSibling extends SimpleParent
+// value classes
+final class ValueClass1(val value: Int) extends AnyVal
+final class ValueClass2[T](val value: T) extends AnyVal
+final case class ValueClass3(val value: Int) extends AnyVal
+
/* It's not possible to run partest without -deprecation.
* Since detecting the warnings requires a neg test with
* -Xfatal-warnings, and deprecation terminates the compile,
@@ -63,6 +68,19 @@ object Test {
println(new Simple() == SimpleSibling) // like Some(1) == None, but needn't be final case
+ println(new ValueClass1(5) == new ValueClass1(5)) // ok
+ println(new ValueClass1(5) == 5) // bad
+ println(new ValueClass1(5) == (5: Any)) // ok, have to let it through
+ println(5 == new ValueClass1(5)) // bad
+ println((5: Any) == new ValueClass1(5) == (5: Any)) // ok
+
+ println(new ValueClass2("abc") == new ValueClass2("abc")) // ok
+ println(new ValueClass2("abc") == "abc") // bad
+ println(new ValueClass2(5) == new ValueClass1(5)) // bad - different value classes
+ println(ValueClass3(5) == new ValueClass3(5)) // ok
+ println(ValueClass3(5) == new ValueClass2(5)) // bad
+ println(ValueClass3(5) == 5) // bad
+
/*
val mine = new MyThing
val some = new SomeThing
Please sign in to comment.
Something went wrong with that request. Please try again.