SI-8939 warn about non-sensible equals in anonymous functions

[lloydmeta]

Adds a new Xlint Ywarn that warns on comparison of mismatched types.

I understand that this is a bit of a ham-fisted approach, but something like this, in the vein of warn-me-if-i-am-comparing-any-unequal-types, seems to be something that (a) people want but doesn't exist and (b) would solve a lot of issues when combined with fatal warnings.

Closes scala/bug#8939 (https://issues.scala-lang.org/browse/SI-8939)

[som-snytt]

Possibly this is too ham-fisted for -Xlint but might be useful to someone as -Ywarn-strict-equals where "strict" means narrower than "universal" equality.

I've tried to help tweak these warnings, and options are limited for assuming too much.

A third-party linter could lint things like comparing Option to junk. Linting common sealed types like that would make a great contribution to scalafix.

-- Wait, isn't scalafix a tool for fixing syntax?
-- Yes, but it isn't only for fixing any more! It's a fixer and a linter!
-- Kewl! I'm going to start scalafixing today!

[hrhino]

Yes, this is pretty harsh indeed:

Welcome to Scala 2.13.0-20180402-083257-6f4848e (Java HotSpot(TM) 64-Bit Server VM, Java 1.8.0_161).
Type in expressions for evaluation. Or try :help.

scala> final case class UserId(value: Int) ; final case class User(id: Option[UserId], other: Int)
defined class UserId
defined class User

scala> val l: List[User] = List(User(Some(UserId(1)), 2))
l: List[User] = List(User(Some(UserId(1)),2))

scala> l.find(_.id == Some(UserId(1)))
                   ^
       warning: Option[UserId] and Some[UserId] are unrelated: they will most likely never compare equal
res0: Option[User] = Some(User(Some(UserId(1)),2))

It's also got a lenient side:

scala> l.find(_.id == Option(1))
res1: Option[User] = None

[lloydmeta]

@som-snytt

Thanks for chiming in; I'm totally fine with making something like this -Ywarn-strict-equals and have gone ahead and done so.

A third-party linter could lint things like comparing Option to junk. Linting common sealed types like that would make a great contribution to scalafix.

@hrhino

Good eye and nice testing :) I've gone and updated this PR so the second scenario will get caught. As for the first (comparing Some[T] and Option[T]), I'm thinking it's ok to be a bit harsher there because I kind of want to avoid the slippery slope of dealing with inheritance and getting caught a bunch of complexity (and contra/covariance), but also because it's relatively easy to fix something like that using a type annotation (can recommend this in the lint description)

That said, in the last commit, I've allowed upwards class hierarchy search, which I think handles comparing Some[T] and Option[T], but I must admit I'm not super confident about it handling all corner cases.

[som-snytt]

There are issues issued for other comparisons. Wow, for instance, "" == 42 does not warn.

It's like on the Mission Impossible tv series where the tape promises to disavow all knowledge of your actions if you are caught or killed. String? Who's that?

I can't speak authoritatively, but I suspect they might want to limit the depth of the rabbit hole for this linter check, and prefer to externalize it. They might like the -Y option of limited scope but avoid something that results in noise, complaints, and further tweaking maintenance.

Edit: but Option as a fundamental type of sorts might be an appealing check anyway.

[lloydmeta]

Wow, for instance, "" == 42 does not warn.

Weird, I just added another check and it shows up:

t8939.scala:16: warning: String and Int are unrelated: they will most likely never compare equal
  val c7 = "" == 42
              ^

EDIT: ah you meant by default, without the lint added in this PR?

[adriaanm]

@dwijnand perhaps you'd like to take a look?

[dwijnand]

Please treat all boolean settings with respect. There's no need to shove them to the right. 😉

[som-snytt]

The whole world is moving to the right.

[dwijnand]

Beyoncé isn't, she prefers

To the left, to the left
Everything you own in the box to the left

[dwijnand]

My personal stance is that we should avoid putting more linting in the compiler and instead implement them in Scalafix where they can (1) grow, multiply and evolve outside of the release cadence of Scala, and (2) they have a chance to be rewrite rules (as opposed to just warning/notifying rules).

(Also expressed some of these thoughts in scala/scala-dev#430 (comment))

As such my vote is for ❌ rejecting this PR (which isn't a GitHub review option..)

[lrytz]

I agree this breaks too easily for it to be included in the compiler. IMO warnings should be implemented in the compiler if there are no (few) false positives, and it's difficult to implement the same in a lint tool.

As an example, in the collections, types that are not subtypes of each other can be equal

scala> import collection.immutable._
scala> HashSet(1) == TreeSet(1)
                  ^
       warning: scala.collection.immutable.HashSet[Int] and scala.collection.immutable.TreeSet[Int] are unrelated: they will most likely never compare equal
res11: Boolean = true