Case Seq() is declared unreachable when matching a Vector

[ansvonwa]

Compiler version

3.1.1-RC1, 3.1.2-RC1-bin-20211102-82172ed-NIGHTLY but not in 3.1.0

Minimized code

Vector() match {
  case Seq() => println("empty")
  case _ => println("non-empty")
}

Output

[warn] 4 |      case Seq() => println("empty")
[warn]   |           ^^^^^
[warn]   |           Unreachable case
[warn] one warning found
[warn] one warning found
[info] running run 
empty

Expectation

No warning

[SethTisue]

We should try hard to fix this regression — it seems pretty fundamental to me.

Note that it's not something about Vector specifically; IndexedSeq() match ... also fails

even IndexedSeq() match { case IndexedSeq() => ... fails

so does IndexedSeq(1) match { case IndexedSeq(1) => ...

[SethTisue]

it would be nice to have a reproduction that is self-contained in the sense of not bringing in complicated collections classes like Vector and IndexedSeq, but my efforts so far to construct one have failed

[dwijnand]

Looks like quite a tricky one... Looks like taking out one part fixes these and breaks some others: master...dwijnand:reachabarity. Check the comment in tests/patmat/exhaustive_heuristics.scala: I think what I'm taking out is doing that list unapply to cons build rewrite, but doing that breaks the 3 cases here.

[dwijnand]

See also scala/bug#12252, where I thought I came to learn that in Scala 3 the arity problem is properly dealt with. Looks like it does so by eagerly rewriting all seq's to cons lists, and that breaks these cases.

Also, something seems funky in master, because my patternMatcher logging is being emitted in "elimOpaque" phase.

[smarter]

is being emitted in "elimOpaque" phase.

I think that's normal even if it's confusing, the phase in the context is used to determine which denotation transformers should be run when accessing a denotation in the current context: it will be the denotation transformers of all phases less than ctx.phase, and tree transformers of a given (mega-)phase are always set to have the denotation transformer of that phase already run (for megaphases this is set in https://github.com/lampepfl/dotty/blob/d2b6bd5df40e93ed457295ba7b3aa0f6d689bd60/compiler/src/dotty/tools/dotc/transform/MegaPhase.scala#L460). To help deal with that you can do -Ylog:phaseName+ which is equivalent to -Ylog:phaseName,phaseAfterThat

[dwijnand]

Ah. I understand what you say, thank you very much for explaining. That's really unfortunate and annoying... That's not how it is in nsc, is it? Can we change this?

[smarter]

The phaseName+ thing is adequate I'd say, cause you do want to see the logs of the denotation transformer of phaseName itself too. To do something more precise you'd need to only enable the printer depending on the tree transformer maybe?

[odersky]

What is the commit that broke this? I believe this needs to be fixed before we ship 3.1.1.

[odersky]

It seems to be a commit from #13485

We should revert the PR unless we can figure this out right now and fix it. 3.1.1 RC 2 is overdue. We should not let regressions like this stay in the codebase.

[dwijnand]

It seems to be a commit from #13485

We should revert the PR unless we can figure this out right now and fix it. 3.1.1 RC 2 is overdue. We should not let regressions like this stay in the codebase.

I'll look closer on Monday, but if we have to revert it would only have to be the emitting of the deferred warnings, not the rest of the PR. As in:

-        for (pat <- deferred.reverseIterator)
-          report.warning(MatchCaseUnreachable(), pat.srcPos)

[dwijnand]

I'll PR the revert and a backport of the revert, and then fix this afterwards.