apache / pekko

Build highly concurrent, distributed, and resilient message-driven applications using Java/Scala
https://pekko.apache.org/
Apache License 2.0
1.22k stars 148 forks source link

Investigate fixing exhaustive match issues in Pekko codebase #265

Open mdedetrich opened 1 year ago

mdedetrich commented 1 year ago

In https://github.com/apache/incubator-pekko/pull/264/files#diff-676691b2e85cad6026adb107942b7c9325394e20e015b131fdcebd3b2b7794b0R132-R133 we had to silence pattern match exhaustiveness warnings because the strict-unsealed-patmat lint option was exposing cases such as this

[error] /Users/mdedetrich/github/incubator-pekko/docs/src/test/scala/typed/tutorial_4/DeviceGroup.scala:51:5: match may not be exhaustive.
[error] It would fail on the following input: (x: typed.tutorial_4.DeviceGroup.Command forSome x not in (typed.tutorial_4.DeviceGroup.DeviceTerminated, typed.tutorial_4.DeviceManager.RequestDeviceList, typed.tutorial_4.DeviceManager.RequestTrackDevice))
[error]     msg match {

We should investigate if we can just simply fix these exhaustiveness issues without creating any regressions, and if so then remove the silencing of this lint option.

He-Pin commented 1 year ago

how about add a

case _ => throw new IllegalStateException() // won't happen, compiler exhaustiveness check pleaser
mdedetrich commented 1 year ago

Sure we can do this, the point of making the issue is looking at it in more detail because there may be some legitimate situations where we should add the extra case and handle it rather than just doing case _ => throw ...).

I would also argue that if we are just going to add case _ => throw new IllegalStateException() in every single case (of which there are many, its not just one or two) its then just better leave the silencing of strict-unsealed-patmat just like what was done with infer-any

JD557 commented 1 year ago

Just to add my 2 cents regarding the case _ => throw new IllegalStateException() (especially the // won't happen, compiler exhaustiveness check pleaser):

The example linked was from a tutorial, so I think it would probably be better to have a "good example" there. That comment might make an user think "oh, it's OK to just ignore the compiler warnings". At least make it a case unknown => throw new IllegalStateException(s"Received unexpected message: $unknown").

Although personally I would prefer if the examples actually incentivized users to use sealed trait everywhere, I can see why that's not the case in tutorial 4 (if only union types were available... 😢 ).

mdedetrich commented 1 year ago

Indeed, this is one of the main reasons why I want to look at the cases specifically. As you rightly pointed out, this is from a tutorial and we shouldn't be encouraging people to throw exceptions like this.

mdedetrich commented 1 year ago

@pjfanning Can we remove this from the 1.0.0 project release? Its a nice to have but its not strictly blocking anything