-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Check exhaustivity of any case class #22604
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@@ -1,4 +1,4 @@ | |||
//> using options -Xfatal-warnings -deprecation -feature | |||
//> using options -deprecation -feature | |||
|
|||
abstract class Foo { | |||
def bar(): Unit = this match { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why doesn't bar
also get a non-exhaustive warning as the baz
below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because Foo isn't sealed, we just don't run exhaustivity on it. But Tuple2 is sealed, so we do (and then comprehend that there are more possible values of Foo after Foo1 and Foo2)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree we shouldn't check exhaustivity for Foo
. In this sense, the selector type is not sealed, then matching any subtype of it would be considered as "(maybe) covering the whole space".
Therefore, the tuple arguments should also cover the whole space. Tuple2
is sealed only means the tuple itself need to be covered, and the content should not be affected, so we should not check exhaustivity for the tuple arguments here as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So are you saying that baz shouldn't warn?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, because the two matching do look identical to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hesitate to do that, as I fear it would be classified a "regression" that we're no longer warning there. I understand the position the dilemma, though.
Fixes #22590