Missing warning for insensible comparison

[scabug]

Something is up with these; I swear some of them used to warn. Really, I can't spot "Nil == 0" ?

scala> val x = scala.collection.mutable.Map[String, List[String]]() withDefaultValue Nil
x: scala.collection.mutable.Map[String,List[String]] = Map()

scala> x("bippy") == 0
res0: Boolean = false

scala> 0 == x("bippy")
<console>:9: warning: comparing values of types Int and List[String] using `==' will always yield false
              0 == x("bippy")
                ^
res1: Boolean = false

scala> (null: List[String]) == 0
res2: Boolean = false

scala> Nil == 0
res3: Boolean = false

[scabug]

Imported From: https://issues.scala-lang.org/browse/SI-6593?orig=1
Reporter: @paulp
Affected Versions: 2.10.0

[scabug]

Lee Mighdoll (mighdoll) said (edited on May 2, 2013 3:27:53 AM UTC):
I just tripped on comparing a List to an Int. I missed the nicety of a 'will always yield false' warning.

$ ./scala 
Welcome to Scala version 2.11.0-20130430-161342-13cf0481d2 (Java HotSpot(TM) 64-Bit Server VM, Java 1.7.0_11).

scala> List("bah") == 1
res1: Boolean = false

(warns nicely if reversed)

scala> 1 == List("foo")
<console>:8: warning: comparing values of types Int and List[String] using `==' will always yield false
              1 == List("foo")
                ^
res0: Boolean = false

Realizing that it's hard to know for certain if someone has overridden equals, I suppose this is tricky. Maybe an annotation on some library equals methods: @neverEqualsPrimitive. Not clear to me how often this comes up or whether there's anything more reasonable to be done.

[scabug]

@som-snytt said:

$ scala -Xlint
Welcome to Scala version 2.11.7 (Java HotSpot(TM) 64-Bit Server VM, Java 1.6.0_65).
Type in expressions to have them evaluated.
Type :help for more information.

scala> def f(c: Char) = c == "o"
<console>:10: warning: comparing values of types Char and String using `==' will always yield false
       def f(c: Char) = c == "o"
                          ^
f: (c: Char)Boolean

scala> def f(s: String) = s.exists(_ == "o")
f: (s: String)Boolean

scala> def f(s: String) = s.exists((c: Char) => c == "o")
f: (s: String)Boolean

[scabug]

Ivan Jager (aij) said (edited on Jun 20, 2016 1:18:10 PM UTC):
Here are more examples that don't warn, without using exists:

scala> val f = (c: Char) => c == "ooo"
f: Char => Boolean = <function1>

scala> val f = 1 == (_:Char)
f: Char => Boolean = <function1>

scala> val f = "ooo" == (_:Char)
f: Char => Boolean = <function1>

scala> val f = (_:Char) == "ooo"
f: Char => Boolean = <function1>

scala> val f: Char => Boolean = _ == "ooo"
f: Char => Boolean = <function1>

Maybe it's a matter of methods vs. functions?

[som-snytt]

Note some expected warnings are emitted now:

scala> def f(s: String) = s.exists(_ == "o")
                                     ^
       warning: comparing values of types Char and String using `==` will always yield false
def f(s: String): Boolean

scala> val f = (c: Char) => c == "ooo"
                              ^
       warning: comparing values of types Char and String using `==` will always yield false
val f: Char => Boolean = $Lambda$1151/0x0000000801086c70@47b33e07

Not the ones scared off by Object.equals.

[lrytz]

@som-snytt what do you think about assuming symmetry in checkSensible and check both directions (Nil == 0 would warn because 0 == Nil does)?

Also we could hard code assumptions about String.equals, comparing with strings by accident seems one of the more likely scenarios.

[lrytz]

@lrytz it's not that simple.

I took a look at what we have.

A "warnable equals" is (based on the receiver type):

• Any/AnyRef.equals
• case class equals

The current comparison is "warnable" if it uses either

• eq / ne
• or == / != with a warnable equals

There are two warning types

• non-sensible: comparing ... will always yield false
• unrelated: types ... are unrelated: they will most likely never compare equal

---

The "non-sensible" warning (will always yield true) is issued when

• If comparison is "warnable" and not case-equals: if the two types unrelated and both are final
• some special cases if the receiver is primitive, Number, etc
• some special cases for case-class-equals

Generally, the "non-sensible" warning is not an approximation, when it shows up it's correct. However not in this case:

scala> case class C(x: Int)
scala> class CC extends C(1) { override def equals(o: Any) = true }
scala> (new CC: C) == 42
                   ^
       warning: comparing values of types C and Int using `==` will always yield false
val res0: Boolean = true

We could remove the warning or tone down its confidence for case-equals that are not final.

---

The "unrelated" warning is issued

• if the comparison is "warnable" and the common parent is AnyRef

That warning can be incorrect:

scala> class C
scala> class D extends C { override def equals(o: Any) = true }
scala> class E // unrelated

scala> val c = new C; val d = new D; val e = new E

scala> d == e
val res1: Boolean = true

scala> (d: C) == e
              ^
       warning: C and E are unrelated: they will most likely never compare equal
val res2: Boolean = true

---

Now the question is, how can we argue for warning about List(1) == 1?

It's not a "warnable equals", the method is overridden and we know nothing about its implementation. So we should probably not issue a "non-sensible" warning (which is supposed to be correct).

I see two options:

1. We could argue that the opposite direction 1 == List(1) issues a "non-sensible" warning, which is always correct. So the comparison is suspicious, as == is supposed to be symmetric.

2. Simplifying the example to remove primitives, the opposite direction would issue an "unrelated" warning (see below). We could argue that the "unrelated" warning, which is already an approximation, could be extended to also check the inverse direction.

scala> class C { override def equals(o: Any) = super.equals(o) }
     | class D
     | val c  = new C
     | val d = new D

scala> d == c
         ^
       warning: D and C are unrelated: they will most likely never compare equal
val res0: Boolean = false

scala> c == d
val res1: Boolean = false

There are cases where only 1) or only 2) would trigger a warning. For example, with scala/scala#10850, now List(1) == Symbol(1) warns "non-sensical" because of built-in knowledge about collections. The opposite doesn't warn at all, "unrelated" is not triggered because Symbol overrides equals. Maybe the original idea is not that bad after all: do 1) and 2), i.e., always warn if the opposite direction would warn?