improve Unreachable case error message

[He-Pin]

Compiler version

3.3.1

Minimized code

Not minimized yet.

1. clone incubator-pekko
2. change the scala3 version to 3.3.1
3. compile the multi-node-testkit with scala 3.3.1

Output

[error] -- [E030] Match case Unreachable Error: C:\Users\hepin\IdeaProjects\incubator-pekko\multi-node-testkit\src\main\scala\org\apache\pekko\remote\testconductor\Player.scala:218:14
[error] 218 |    case Event(_: ServerOp, _) =>
[error]     |         ^^^^^^^^^^^^^^^^^^^^^
[error]     |         Unreachable case
[error] one error found

Expectation

I just check the code, the branches will be matched with

    case Event(msg: NetworkOp, _) =>

first , so I think the compiler is right.

Better with an output:

[error] 218 |    case Event(_: ServerOp, _) =>
[error]     |         ^^^^^^^^^^^^^^^^^^^^^
[error]     |         Unreachable case, all case has been matched with `case Event(Done, _) =>` and `case Event(msg: NetworkOp, _)`
[error] one error found

[He-Pin]

@dwijnand I think the compiler is right, can you check it too, thanks.

[bishabosha]

I also checked this independently when I was testing pekko with pipeline compilation support, in the source file multi-node-testkit/src/main/scala/org/apache/pekko/remote/testconductor/DataTypes.scala all ServerOp also extended NetworkOp, so therefore would be covered by NetworkOp which is in the case above.

However for the project maintainers perhaps they should swap the cases.

[WojciechMazur]

I've tested it when investigating nightly failures in Open Community Build for akka/pekko and it's an improvement and not a regression. As @bishabosha already mentioned all the cases for ServerOp have been already covered and this code is in fact unreachable.

[He-Pin]

That's true.

[He-Pin]

Thank you everyone.

[He-Pin]

@bishabosha Is it possible to improve the error message, I mean you can reopen this issue with a different tile, or start a new issue.

[bishabosha]

to summarise, the error message for "unreachable case" could be expanded to mention which cases already cover the "unreachable" case mentioned. This could help the programmer re-order the code if they really need to do something different when that case matches,

e.g. in the Pekko code mentioned, there is meant to be special behaviour specifically for the ServerOp case, and its a bug in the Pekko code that the branch would never match

[bishabosha]

minimisation:

sealed trait ServerOp
sealed trait NetworkOp

case object Done extends ServerOp with NetworkOp
case class Foo() extends ServerOp with NetworkOp

def foo(x: Matchable) = x match
  case Done => ???
  case _: NetworkOp => ???
  case _: ServerOp => ??? // warning: Unreachable case 

[dwijnand]

to summarise, the error message for "unreachable case" could be expanded to mention which cases already cover the "unreachable" case mentioned.

How? By trying all the combinations of all the cases to find the smallest set of cases that make the case redundant? Currently we don't compare the case to individual previous cases, we just take all previous cases and consider whether the current case is redundant. I don't think we should spend the CPU time doing more, unless it's an opt-in option.

[bishabosha]

This is just a thought without much knowledge of the implementation, but could you keep a reference to the original tree that the space element comes from, then whenever some part covers the case then you can recover which cases were responsible?

[dwijnand]

The calculation is isSubspace(covered, prev), is the space of values covered by this case a subspace of the sum of all previous spaces? Which is a more efficient way to ask covered - prev == Empty. So I guess perhaps we could try doing val covered{n} = covered{n-1} - prev{n} and see if the result is ne covered{n-1}. That would, btw, only tell you if one singular previous case covers the redundant case, so any more complicated than that and you're back to nothing.

[odersky]

There's also the issue of inlining. For inlined code we don't even see the pattern and the match, so "unreachable case" is particularly unhelpful.