warning precisely on (more) nonsense equality comparisons

Here's my thinking on warning about a wider set of nonsense equality
comparisons, in a way that avoids false positives and therefore that
we can cheerfully turn on by default.

The broadest warning in RefChecks.scala is this one:

// Whether def equals(other: Any) is overridden
def isUsingDefaultEquals = ...
def isWarnable = isReferenceOp ||
(isUsingDefaultEquals && isUsingDefaultScalaOp)
...
else if (isWarnable) {
...
else if (receiver.isFinal && !(receiver isSubClass actual))
{ // object X, Y; X == Y
if (isEitherNullable)
nonSensible("non-null ", false)
else
nonSensible("", false)
}
}

IOW, we try to prove (a) that the equality method in question is the
default one which just checks reference equality, and (b) that the
types on the two sides have no values in common other than null, and
if we can prove both (a) and (b) then we warn.

In the case of Option[Int] and Int, (b) is true, but a bit trickier to
prove because Option is not final. Fortunately Option is sealed, so
all of its immediate subclasses are known, and its immediate
subclasses Some and None are final, so indeed all of its subclasses
are known. More complicated is (a). Some and None, as case classes,
actually do have their own 'equals' methods, injected by
typechecker/SyntheticMethods.scala. These synthetic case-class
'equals' methods still have straightforward semantics when the
argument is of some alien type -- so long as the argument's
isInstanceOf method is also straightforward and returns false in that
case.

So I think it should be feasible to prove both (b) and a version of
(a) in a wider set of cases that would include Option[Int] vs Int. My
rough plan would be something like

* extend isFinal to a broader predicate along the lines of
lazy val isNearFinal = isFinal || (isSealed && for (cls <- /*
immediate subclasses */) cls.isNearFinal)
which allows us to prove (b);

* have SyntheticMethods set a flag when it supplies 'equals', so we
can distinguish its 'equals' from a user-supplied 'equals';

* when isWarnable and receiver.isNearFinal, warn if !(T isSubClass
actual) for all subclasses T of receiver;

* otherwise if we're using the synthetic case-class 'equals' and
receiver.isNearFinal, check that actual.isNearFinal and that we'll be
using the default 'isInstanceOf', and warn unless one of the
subclasses of receiver is also a subclass of actual.

Does this plan make sense?

Greg

On Tue, Sep 6, 2011 at 2:42 AM, 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) ...
> 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)
>                  ^
>