Pattern match induces "will most likely never compare equal"

[NthPortal]

There is a case in s.c.m.Shrinkable and a case in s.c.m.Growable where we check if a {Growable|Shrinkable} is eq to an IterableOnce. As they do not share any type below Any, the compiler previously warned that they would "most likely never compare equal", and we added a @nowarn annotation because the compiler is wrong (mutable Sets, Maps and Buffers all extend IterableOnce, Shrinkable and Growable).

At some point the compiler stopped warning for these cases, which came up at scala/scala#9301 (comment) when Seth was doing some cleanup.

Are the missing warnings a regression, or the compiler getting smarter somehow (does it track all types inheriting from both of them?)?

---

• remove annotations

[som-snytt]

When I check out your commit and remove (revert) the nowarn from Shrinkable, I don't see a ("does not suppress") warning there, but I do see the warning on Growable:

[warn] /.../scala/src/library/scala/collection/mutable/Growable.scala:59:4: @nowarn annotation does not suppress any warnings
[warn]   @nowarn("msg=will most likely never compare equal")

That is both straight and bootstrapped. Is it possible it never warned on the current edit of subtractAll?

scala> import collection.mutable.Shrinkable
import collection.mutable.Shrinkable

scala> import collection.IterableOnce
import collection.IterableOnce

scala> (null: IterableOnce[Int]).asInstanceOf[AnyRef] eq (null: Shrinkable[Int])
val res0: Boolean = true

scala>

contrast

scala> (null: collection.mutable.Growable[Int]) eq (null: Shrinkable[Int])
                                                ^
       warning: scala.collection.mutable.Growable[Int] and scala.collection.mutable.Shrinkable[Int] are unrelated: they will most likely never compare equal

By contrast, the following message does disappear on bootstrap, which demonstrates that the warning was newly introduced:

[warn] /.../scala/src/library/scala/runtime/NonLocalReturnControl.scala:18:2: @nowarn annotation does not suppress any warnings
[warn] @annotation.nowarn("cat=lint-unit-specialization")

[SethTisue]

Are the missing warnings a regression, or the compiler getting smarter somehow (does it track all types inheriting from both of them?)?

I don't recall any PR that aimed to add any smarts like that. I think the next investigation step here would be to bisect, using published nightlies, to identify the PR where the behavior changed. At that point it should be obvious whether it's a progression or regression.

[som-snytt]

My previous comment was that reverting the nowarn at the time it was committed does not induce a warning, but leaving it in does induce the "suppresses nothing".

I verified that no 2.13 release warns on the comparison, which has the form:

scala> val xs: collection.IterableOnce[Int] = List(1,2,3)
val xs: scala.collection.IterableOnce[Int] = List(1, 2, 3)

scala> val ys: collection.mutable.Shrinkable[Int] = collection.mutable.ListBuffer(1,2,3)
val ys: scala.collection.mutable.Shrinkable[Int] = ListBuffer(1, 2, 3)

scala> xs.asInstanceOf[AnyRef] eq ys
val res0: Boolean = false

scala> "" eq ys  // sanity check
          ^
       warning: String and scala.collection.mutable.Shrinkable[Int] are unrelated: they will most likely never compare equal
val res1: Boolean = false

I don't if there was a warning between releases. I don't think you can bisect if the behavior never changed.

[NthPortal]

I believe the PR ended up getting rebased several times, so it's possible that the commit that ended up getting merged didn't need them. however, the annotations were definitely needed by the original commit (this actually preceded fatal warnings in the library module)

[NthPortal]

@som-snytt see NthPortal/scala@b5c8443 (the original commit introducing the annotations) and NthPortal/scala@04a311e (a commit I just made removing the annotations, that warns). parent commit on scala/scala is scala/scala@7b85389

[som-snytt]

I get the same result building what you say is the parent commit. Oh wait, now I see what you saw. The line it warns on from your repo is different from what I cited in scala/scala:

➜  scala git:(test/sd-747) ✗ sdk use scala 2.13.3

Using scala version 2.13.3 in this shell.
➜  scala git:(test/sd-747) ✗ scalac -d /tmp ./src/library/scala/collection/mutable/Growable.scala
./src/library/scala/collection/mutable/Growable.scala:61: warning: scala.collection.IterableOnce[A] and scala.collection.mutable.Growable[A] are unrelated: they will most likely never compare equal
      case xs: AnyRef if xs eq this => addAll(Buffer.from(xs)) // avoid mutating under our own iterator
                            ^
1 warning

Cf https://github.com/scala/scala/blame/2.13.x/src/library/scala/collection/mutable/Growable.scala#L61

I don't actually know what the type is in the pattern guard. Presumably AnyRef with IterableOnce[A]? It is not the same as the cast because it knows the scrutinee.

I would also cite this as a reason I don't like the "shadow a name as more precise type in pattern match" because it is in fact hard to read.

In short, you never committed the code that warns.

[NthPortal]

huh, interesting. I actually changed the code as per Jason's request (scala/scala#9174 (comment)), so... 🤷‍♀️ I guess we can get rid of the annotations and leave it at that then

[NthPortal]

thanks for catching that @som-snytt

[som-snytt]

It warns in 2.13.0:

scala> trait X { def f(t: Runnable) = t match { case that if that eq this => } }
                                                                  ^
       warning: Runnable and X are unrelated: they will most likely never compare equal
defined trait X

where that is a refinement. Some previous refinement tweaking touched the (dubious) refcheck in 2.12.7. Maybe something related was retweaked for 2.13.0.

[som-snytt]

Extraneous nowarn deleted at scala/scala#9442 which incurred some serious déjà vu.

[NthPortal]

I'm not sure there's actually a bug here. I think the warning on a pattern match is correct, as the compiler has no way of knowing the types are related (since Growable and Shrinkable don't extend IterableOnce)

[som-snytt]

The warning tries to be conservative and not noisy. It's supposed to emit some advice only when it has something useful to say, like Mr Ed the talking horse. So I would say definitely undesired behavior; but I'm not sure the feature is worth investing much time in.