SI-5663: Tweak warnings on case class equals

Re-enable testing the sensibility of comparing instances
of two case classes.  In particular, warn if we detect that
the two objects inherit from different case classes.  In
addition, avoid emitting misleading warnings when comparing "new C".

4 files changed, 142 insertions, 22 deletions

diff --git a/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala b/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala
index 806ee480f0..c4b08f6ea2 100644
--- a/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala
+++ b/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala
@@ -1059,12 +1059,10 @@ abstract class RefChecks extends InfoTransform with reflect.internal.transform.R
         // equals.
         def isUsingWarnableEquals = {
           val m = receiver.info.member(nme.equals_)
-          def n = actual.info.member(nme.equals_)
-          (   (m == Object_equals)
-           || (m == Any_equals)
-           || (m.isSynthetic && m.owner.isCase && !n.owner.isCase)
-          )
+          ((m == Object_equals) || (m == Any_equals) || isMethodCaseEquals(m))
         }
+        def isMethodCaseEquals(m: Symbol) = m.isSynthetic && m.owner.isCase
+        def isCaseEquals = isMethodCaseEquals(receiver.info.member(nme.equals_))
         // Whether this == or != is one of those defined in Any/AnyRef or an overload from elsewhere.
         def isUsingDefaultScalaOp = {
           val s = fn.symbol
@@ -1087,9 +1085,11 @@ abstract class RefChecks extends InfoTransform with reflect.internal.transform.R
           val msg = alwaysEqual == (name == nme.EQ || name == nme.eq)
           unit.warning(pos, "comparing "+what+" using `"+name.decode+"' will always yield " + msg)
         }
-
         def nonSensible(pre: String, alwaysEqual: Boolean) =
           nonSensibleWarning(pre+"values of types "+typesString, alwaysEqual)
+        def nonSensiblyEq() = nonSensible("", true)
+        def nonSensiblyNeq() = nonSensible("", false)
+        def nonSensiblyNew() = nonSensibleWarning("a fresh object", false)
 
         def unrelatedTypes() = {
           val msg = if (name == nme.EQ || name == nme.eq)
@@ -1097,52 +1097,73 @@ abstract class RefChecks extends InfoTransform with reflect.internal.transform.R
           unit.warning(pos, typesString + " are unrelated: they will most likely " + msg)
         }
 
-        if (nullCount == 2)
-          nonSensible("", true)  // null == null
+        if (nullCount == 2) // null == null
+          nonSensiblyEq()
         else if (nullCount == 1) {
           if (onSyms(_ exists isPrimitiveValueClass)) // null == 5
-            nonSensible("", false)
+            nonSensiblyNeq()
           else if (onTrees( _ exists isNew)) // null == new AnyRef
-            nonSensibleWarning("a fresh object", false)
+            nonSensiblyNew()
         }
         else if (isBoolean(receiver)) {
           if (!isBoolean(actual) && !isMaybeValue(actual))    // true == 5
-            nonSensible("", false)
+            nonSensiblyNeq()
         }
         else if (isUnit(receiver)) {
           if (isUnit(actual)) // () == ()
-            nonSensible("", true)
+            nonSensiblyEq()
           else if (!isUnit(actual) && !isMaybeValue(actual))  // () == "abc"
-            nonSensible("", false)
+            nonSensiblyNeq()
         }
         else if (isNumeric(receiver)) {
           if (!isNumeric(actual) && !forMSIL)
             if (isUnit(actual) || isBoolean(actual) || !isMaybeValue(actual))   // 5 == "abc"
-              nonSensible("", false)
+              nonSensiblyNeq()
         }
-        else if (isWarnable) {
+        else if (isWarnable && !isCaseEquals) {
           if (isNew(qual)) // new X == y
-            nonSensibleWarning("a fresh object", false)
+            nonSensiblyNew()
           else if (isNew(args.head) && (receiver.isEffectivelyFinal || isReferenceOp))   // object X ; X == new Y
-            nonSensibleWarning("a fresh object", false)
+            nonSensiblyNew()
           else if (receiver.isEffectivelyFinal && !(receiver isSubClass actual)) {  // object X, Y; X == Y
             if (isEitherNullable)
               nonSensible("non-null ", false)
             else
-              nonSensible("", false)
+              nonSensiblyNeq()
           }
         }
 
         // possibleNumericCount is insufficient or this will warn on e.g. Boolean == j.l.Boolean
         if (isWarnable && nullCount == 0 && !(isSpecial(receiver) && isSpecial(actual))) {
-          if (actual isSubClass receiver) ()
-          else if (receiver isSubClass actual) ()
-          // warn only if they have no common supertype below Object
-          else {
+          // 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) {
+            def thisCase = receiver.info.member(nme.equals_).owner
+            actual.info.baseClasses.find(_.isCase) match {
+              case Some(p) if (p != thisCase) => nonSensible("case class ", false)
+              case None =>
+                // stronger message on (Some(1) == None)
+                //if (receiver.isCase && receiver.isEffectivelyFinal && !(receiver isSubClass actual)) nonSensiblyNeq()
+                //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()
+              case _ =>
+            }
+          }
+          else if (actual isSubClass receiver) ()
+          else if (receiver isSubClass actual) ()
+          // warn only if they have no common supertype below Object
+          else {
+            warnIfLubless()
+          }
         }
       case _ =>
     }
diff --git a/test/files/neg/t5663-badwarneq.check b/test/files/neg/t5663-badwarneq.check
new file mode 100644
index 0000000000..00c2234e9d
--- /dev/null
+++ b/test/files/neg/t5663-badwarneq.check
@@ -0,0 +1,22 @@
+t5663-badwarneq.scala:42: 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
+    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
+    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
+    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
+    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
+    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
+    println(new Simple() == SimpleSibling) // like Some(1) == None, but needn't be final case
+                         ^
+7 errors found
diff --git a/test/files/neg/t5663-badwarneq.flags b/test/files/neg/t5663-badwarneq.flags
new file mode 100644
index 0000000000..85d8eb2ba2
--- /dev/null
+++ b/test/files/neg/t5663-badwarneq.flags
@@ -0,0 +1 @@
+-Xfatal-warnings
diff --git a/test/files/neg/t5663-badwarneq.scala b/test/files/neg/t5663-badwarneq.scala
new file mode 100644
index 0000000000..56ec389c03
--- /dev/null
+++ b/test/files/neg/t5663-badwarneq.scala
@@ -0,0 +1,76 @@
+
+// alias
+trait Thingy
+
+class Gramps
+
+// sibling classes that extend a case class
+case class Thing(i: Int) extends Gramps
+class ThingOne(x:Int) extends Thing(x)
+class ThingTwo(y:Int) extends Thing(y) with Thingy
+final class ThingThree(z:Int) extends Thing(z)
+
+// not case cousin
+class Cousin extends Gramps
+
+class SimpleParent
+case class Simple() extends SimpleParent
+case object SimpleSibling extends SimpleParent
+
+/* 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,
+ * we'll just comment out the nasty part.  The point was
+ * just to show there's nothing special about a trait
+ * that extends a case class, which is only permitted
+ * (deprecatingly) by omitting the parens.
+ *
+// common ancestor is something else
+class AnyThing
+case class SomeThing extends AnyThing // deprecation
+class OtherThing extends AnyThing
+
+// how you inherit caseness doesn't matter
+trait InThing extends SomeThing
+class MyThing extends InThing
+*/
+
+object Test {
+  def main(a: Array[String]) {
+    // nothing to do with Gavin
+    println(new Some(1) == new Some(1)) // OK, true
+    println(new Some(1) == None) // Should complain on type, was: spuriously complains on fresh object
+    println(Some(1) == new Thing(1)) // Should complain on type, was: spuriously complains on fresh object
+
+    val t1 = new ThingOne(11)
+    val t2: Thingy = new ThingTwo(11)
+    val t3 = new ThingTwo(11)
+    val t4 = new ThingThree(11)
+    val c = new Cousin
+
+    println(t1 == t2) // true, but apparently unrelated, a compromise warning
+    println(t4 == t2) // true, complains because ThingThree is final and Thingy not a subclass, stronger claim than unrelated
+    println(t2 == t3) // OK, two Thingy
+    println(t3 == t2) // ditto with case receiver
+    println(t3 == Some(1)) // false, warn on different cases
+    println(t1 == c) // should warn
+
+    // don't warn on fresh cases
+    println(new ThingOne(11) == t1) // OK, was: two cases not warnable on trunk
+    println(new ThingTwo(11) == t2) // true, was: spuriously complains on fresh object
+    println(new ThingOne(11) == t3) // two cases not warnable on trunk
+    println(new ThingTwo(11) == t3) // ditto
+
+    println(new Simple() == SimpleSibling) // like Some(1) == None, but needn't be final case
+
+    /*
+    val mine = new MyThing
+    val some = new SomeThing
+    val other = new OtherThing
+    println(mine == some) // OK, two Something
+    println(some == mine)
+    println(mine == other) // OK, two Anything?
+    println(mine == t1) // false
+    */
+  }
+}