Missing unchecked warning

[dwijnand]

Compiler version

3.2.1

Minimized code

case class Wrapper[A](value: A)
enum Color:
  case Red, Green

def test_wrong(x: Wrapper[Color]): Option[Wrapper[Color.Red.type]] =
  x match
    case x: Wrapper[Color.Red.type] => Some(x)
    case _                          => None

Output

No warnings

Expectation

Unchecked warnings.

Notes

This is rereporting #16451, which we reverted the fix for in #18303.

[SethTisue]

The difficulty here that the unchecked warning is the really important warning to emit, but it's emitted by a later phase (erasure) than the conceptually secondary exhaustivity/unreachability warnings (that come during pattern analysis), so it ends up being suppressed because the spans match and there is logic in the compiler (in UniqueMessagePositions) not to emit multiple warnings for the same span. (The motivation being that the later warnings are likely to be redundant or spurious.)

Dale and I considered two possible fix plans here.

The more ambitious fix plan would be: during pattern analysis, in order to do exhaustivity and reachability analysis correctly, we need to know the erased type, since the erased type is what is actually will be used at runtime. So during pattern analysis, we would need to time travel forward to erasure to get the erased type. Then if an unchecked warning needs to be issued, we would record that fact in a new attachment. Then compilation would proceed, eventually reach the (non-time-traveled) erasure phase, and at that time we'd consult the attachment to emit the unchecked warning.

That plan is arguably the “right” fix, but we're somewhat reluctant to introduce additional complexity and time travel, both because getting it right would be work, and because it would be additional machinery that would complicate everyone's mental model of how this stuff works and would need to be maintained indefinitely

The narrower fix plan is to leave the incorrect reachability/exhaustivity analysis alone, considering it an unfortunate but understandable consequence of the user having written an uncheckable type test, and just un-suppress the unchecked warning.

[SethTisue]

Is the narrower fix plan sufficient, at least for the time being?

What makes me hesitate the most is the behavior if the user enables fatal warnings. By design — a design that can be questioned, but it's the design we have at present — is that if fatal warnings are enabled, the first fatal warning terminates compilation. So the user will never see the unchecked warning, which means the bug isn't really fixed for those users.

And there is a related issue around testing — we don't currently have a good way to write a test for multiple warnings. Currently in vulpix, as in Scala 2's partest, pos tests cannot have check files, so the way to test for a warning being emitted is to enable fatal warnings and make the test a neg test instead. But as described above, enabling fatal warnings means we'll never see the unchecked warning, so we can't put it in the check file. We might want to fix vulpix to allow pos tests to have an optional check file. If no check file is present, the code would still be allowed to emit warnings, they just wouldn't be checked. So the status quo for existing tests would not be affected. But new pos tests could opt in to including a check file. (This would match how neg tests already work: a neg test can have an // error comment if we only care that an error is present and on which line, but a neg test can optionally have a .check file with the full message.)

[SethTisue]

Dale has found a showAlways: Boolean method that we might be able to use to prevent the suppression...?

Yes, that seems to work. (Though it doesn't improve the behavior under fatal warnings. Dale cares less about that than I do. I'm trying to see it Dale's way 😀)

Currently showAlways = true is only being used by ImplicitSearchTooLargeWarning. It's not clear to me what design principle should guide which warnings should have it? But neither Dale or I can think of a reason not to turn showAlways on for unchecked-type-test warnings.

And then we just need to do the vulpix change so we can test it.

[SethTisue]

PR with the less ambitious plan (including the vulpix change): #18377

(But we remain interested in also attempting the more ambitious plan.)