warning on nonsense equality comparisons

On Tue, Sep 6, 2011 at 06:42, Greg Price wrote:
> Hi,
>
> My colleagues and I are porting a large codebase to Scala, and we're
> finding we often use Option for fields or variables that are
> "optional". (In the Python original of the codebase, these would be
> variables where None is an acceptable value.) This results in a lot of
> comparisons like
>  if (someUid == someOtherUid) ...

scala> val a = 5; val b = Option(5); if (a == b) println("Equal")
:7: warning: comparing values of types Int and Option[Int]
using `==' will always yield false
val a = 5; val b = Option(5); if (a == b) println("Equal")
^

That's Scala 2.8.1. The same is true on 2.9.0:

scala> val a = 5; val b = Option(5); if (a != b) println("Equal")
:9: warning: comparing values of types Int and Option[Int]
using `!=' will always yield true
val a = 5; val b = Option(5); if (a != b) println("Equal")
^

It is also true on trunk. I don't have a 2.9.1 around to test against,
but I'd be surprised if it were disabled there. So the warning _is_
present -- I don't understand what else do you want.

> where it's easy to forget that, e.g., someUid is of type Option[Int]
> while someOtherUid is of type Int. This comparison will always fail,
> even in the case where the programmer intended it to succeed. I'm
> therefore interested in compiler warnings that would help Scala
> programmers avoid this kind of bug.
>
> In nsc/typechecker/RefChecks.scala, we have a number of warnings along
> these lines, but none of them quite manage to catch this case.  There
> is, however, this crude, effective, but disabled warning:
>
>        // Warning on types without a parental relationship.  Uncovers a lot of
>        // bugs, but not always right to warn.
>        if (false) {
>          if (nullCount == 0 && possibleNumericCount < 2 &&
>              !(receiver isSubClass actual) && !(actual isSubClass receiver))
>            unrelatedTypes()
>        }
>
> This warning is valuable enough, despite some false positives, that I
> want to use it. I have a short patch which turns it on with a flag
> -Ywarn-compare-unrelated and adds the flag to -Xlint. I'm submitting
> the patch as a Github pull request
> (https://github.com/scala/scala/pull/84), because that's what we did
> at the documentation day at Scalathon; let me know if there's a more
> preferred way to do so and I'll be happy to accommodate.
>
> I believe it should also be possible to warn on cases like Option[Int]
> vs Int without any false positives, though it will take some work.
> More on that in a follow-up message.
>
> Greg
>
>
>
> PS - The warning is useful enough, in fact, that of four places where
> it fires in scalac itself on trunk, I believe two of them are real
> bugs:
>
> /scratch/src/scala/src/compiler/scala/tools/nsc/backend/msil/GenMSIL.scala:1266:
> warning: GenMSIL.this.global.icodes.BasicBlock and
> ch.epfl.lamp.compiler.msil.emit.Label are unrelated: probably always
> compare unequal
>            if (next != defaultTarget)
>                     ^
>
> /scratch/src/scala/src/compiler/scala/tools/nsc/interpreter/JLineReader.scala:57:
> warning: scala.tools.nsc.interpreter.session.JLineHistory and object
> scala.tools.nsc.interpreter.session.NoHistory are unrelated: probably
> always compare unequal
>      if (history ne NoHistory)
>                  ^
>

On 9/6/11 8:12 AM, Daniel Sobral wrote:
> scala> val a = 5; val b = Option(5); if (a != b) println("Equal")
> :9: warning: comparing values of types Int and Option[Int]
> using `!=' will always yield true
> val a = 5; val b = Option(5); if (a != b) println("Equal")
> ^
>
> It is also true on trunk. I don't have a 2.9.1 around to test against,
> but I'd be surprised if it were disabled there. So the warning _is_
> present -- I don't understand what else do you want.

It's present when the lhs is an Int because the compiler has exhaustive
knowledge of Ints. It's not present in reverse.

scala> Some(5) == 5
res0: Boolean = false

To original poster, I'm the guy who wrote and disabled the warning you
found. I can revive it in some form: my world of warnings is better
organized now.

On 9/6/11 8:12 AM, Daniel Sobral wrote:

scala>  val a = 5; val b = Option(5); if (a != b) println("Equal")
<console>:9: warning: comparing values of types Int and Option[Int]
using `!=' will always yield true
       val a = 5; val b = Option(5); if (a != b) println("Equal")
                                           ^

It is also true on trunk. I don't have a 2.9.1 around to test against,
but I'd be surprised if it were disabled there. So the warning _is_
present -- I don't understand what else do you want.

It's present when the lhs is an Int because the compiler has exhaustive knowledge of Ints.  It's not present in reverse.

scala> Some(5) == 5
res0: Boolean = false

To original poster, I'm the guy who wrote and disabled the warning you found.  I can revive it in some form: my world of warnings is better organized now.

On Tue, Sep 6, 2011 at 8:46 AM, Paul Phillips wrote:
> To original poster, I'm the guy who wrote and disabled the warning you
> found.  I can revive it in some form: my world of warnings is better
> organized now.

Perfect. Yep, I saw you had written the original code there in r23643;
that's why I cc'd you directly. Now I see where you organized the
warning options into settings/Warnings.scala in r25172; makes sense
that it's easier to put this in now than it was before that commit.

Naturally I think the easiest and best way to do so is to merge my
patch. =) (But if not, I'd be interested in knowing how so just for
feedback.)

Greg