Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.Sign up
warning on a wider set of provable nonsense equality comparisons #4979
[This is an RFC on a patch I would go off and try to write -- not a request for someone else to do the work.]
BACKGROUND: 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
PROBLEM: This comparison will always fail, even in the case where the programmer intended it to succeed, but scalac currently emits no warning. In #4978 I suggested enabling a comprehensive but imprecise test under -Xlint; that'd help, but it'd be better if we can have a warning for this that is always right so we can turn it on unconditionally.
SOLUTION PROPOSAL: In nsc/typechecker/RefChecks.scala we have a number of fairly specific checks that try to warn in cases like this. Here's the broadest warning so far:
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
Does this plan make sense? If it does (or for some version of it that does), I'll go try to carry it out and come back with a patch.
Imported From: https://issues.scala-lang.org/browse/SI-4979?orig=1
The algorithm is: if neither operand is a subclass of the other, then calculate the lub. If the lub >: Object, warn. That will catch truly unrelated types, but allows "siblings" like B == C where B and C are both subclasses of A. And it sidesteps the complicated analysis.
I'll check it in shortly, let me know if you have examples you think can be caught which it doesn't catch.
Greg Price (price) said:
Relative to the warning without the lub check, out of the 15 or so examples I know of, it eliminates some false positives and also one real error.
It eliminates false positives where, e.g., the two sides are subclasses of GenSeqLike, which overrides equals() so that many of its otherwise unrelated subclasses can compare equal. This covers the only false positive in Quora's codebase, and if I recall correctly the lub check would also cut out the two false positives in scalac itself.
A false positive is still possible; there's no reason in principle why I might not declare class A overriding equals() to accept instances of B, with ScalaObject the lub. In practice I probably want B's equals() to also accept A, and then perhaps I want to define that equals() only once, which would lead me to give A and B a common base class. If people follow that logic, then this is a pretty sharp heuristic.
But then the lub check does also eliminate one of the two real errors which the warning found in scalac itself:
Here the lub is scala.tools.nsc.interpreter.session.History. I don't see such a tight software-engineering argument as I gave above to say why this shouldn't happen; I suspect the lub check will suppress a decent number of real bugs in practice.
That could argue for an option to make this warning skip the lub check.
Thanks! Glad you found a simple version that's precise enough to turn on by default.
The history comparison is wrong but it's going to be the exception that one can sensibly warn about a once-valid comparison which becomes invalid after inheritance refines one of the types. I believe that would be a parade of false positives.
Greg Price (price) said:
Yeah, I agree, the majority of the cases that the lub check cuts out are likely to be false positives. I think the description in terms of a once-valid comparison followed by inheritance refining one of the types is narrower than the full set of bugs that the lub check would hide in practice, though. For example, the one place where the warning with the lub check fires in scalac is here:
This is a real bug. As it happens, these two classes come from fairly separate parts of the codebase, so their lub is ScalaObject. But the authors of this code clearly think of them as related classes, or they wouldn't make this mistake; they both belong to the MSIL subsystem and are things that describe chunks of MSIL code; so it could have happened that the two classes shared some generic utility class like "MSILItem" that was used by that subsystem generally. In that case, the lub check would have suppressed this warning.
Perhaps an option to issue this warning without the lub check would fit under -Ywarn-all; the SNR (excluding what the lub check does let through) is probably too low for -Xlint.