Skip to content

Missing warning for insensible comparison #6593

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
scabug opened this issue Oct 30, 2012 · 7 comments
Open

Missing warning for insensible comparison #6593

scabug opened this issue Oct 30, 2012 · 7 comments

Comments

@scabug
Copy link

scabug commented Oct 30, 2012

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
Copy link
Author

scabug commented Oct 30, 2012

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

@scabug
Copy link
Author

scabug commented May 2, 2013

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
Copy link
Author

scabug commented Jun 20, 2016

@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
Copy link
Author

scabug commented Jun 20, 2016

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
Copy link

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.

@SethTisue SethTisue added this to the Backlog milestone Aug 25, 2022
@lrytz
Copy link
Member

lrytz commented Jun 20, 2024

@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
Copy link
Member

lrytz commented Aug 29, 2024

@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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants