Unreachable case warning could point to previous matching case

[smarter]

Given:

trait Foo
class One extends Foo
class Two extends Foo
class Bla extends One

object Test {
  def test(f: Foo) = f match {
    case f: One =>
    case f: Two =>
    case f: Bla =>
  }
}

We get the warning:

-- [E120] Only null matched Warning: try/caseun.scala:10:9 ---------------------
10 |    case f: Bla =>
   |         ^^^^^^
   |         Only null is matched. Consider using `case null =>` instead.

This is confusing since it doesn't explain why only null is matched. Here it's easy to see that this is because we matched on One previously and that Bla is a subtype of One, but in a long series of case this might be very hard to understand. A better warning message would explain all of this.

[abeln]

If we actually run this code and set f to null, we get a match error: https://scastie.scala-lang.org/B12dNkjASH2bVIiBMBb4qA

The reason we incorrectly, I think, get this warning here is that after we project the pattern Bla we intersect the resulting space with Foo, and that gives us a nullable space Or(Bla, null). This leads us to mistakenly believe that the last pattern matches null; hence the warning.

That said, what would we want the "better warning message" to look like? If every case in a match is reachable, then if the last case matches only null it's because all the previous cases contributed to that.

[liufengyun]

I totally misunderstood this issue and #4661, I thought null was covered by the last case -- and we have a friendly null-coverage warning only for the last case.

Thanks a lot @abeln .

[liufengyun]

@abeln Your PR #4869 also fixed this issue. Could you create another PR adding this test case?

[Blaisorblade]

What's fixed is that now we get the correct warning, but it still doesn't explain the reasoning as asked in the OP, so this enhancement issue shouldn't be closed. But it already has all the correct labels.

It might also not be hard to do better: The debugging messages already show the internal reasoning, they're just not designed for end users and can't be enabled without recompiling Dotty to change the relevant printer.

[liufengyun]

I incline to close this with stat:revisit, as the OP was fooled by the incorrect warning. For the correct warning, it's not easy to do better unless we are willing to spend more CPU time on exhaustivity check.

[abeln]

@Blaisorblade which debugging messages did you have in mind to add to the error message?

[Blaisorblade]

@abeln replace val exhaustivity: Printer = noPrinter with val exhaustivity: Printer = default in https://github.com/lampepfl/dotty/blob/61cba970fec1017223ceb37da05f241f011c14d4/compiler/src/dotty/tools/dotc/config/Printers.scala#L21. Ditto for any other printer there.
I'd vote for a -Ylog: setting, or at least for docs, but haven't had time to shave this particular yak.

[abeln]

I see. That feels like a change with larger implications, though. I'd agree with Fengyun that we either put on hold or close this, since we now have the correct error message that makes more sense. We could file a separate bug for "-Ylog".

[Blaisorblade]

Created #4953 for -Ylog, and closing with stat:revisit.