-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Fix warning message for matching on redundant nulls #21850
Conversation
I changed the checkfile for tests/patmat/null.check as the result did seem to be wrong to me. It seems that there has been some history on this issue, with a commit which specifically does the opposite of this PR, removing null computations because of expensive computation costs. Would this fix be unnecessary because it incurs a large performance cost? |
Maybe I need to clarify my suggestion. I mean we always project a wildcard pattern to nullable space if the type is nullable. For example: Like in sealed trait T_B[C, D]
case class CC_A()
case class CC_B[A, C](a: A) extends T_B[C, CC_A]
case class CC_C[C, D](a: T_B[C, D]) extends T_B[Int, CC_A]
case class CC_E(a: CC_C[Char, Byte])
val v_a: T_B[Int, CC_A] = CC_B(CC_E(CC_C(null)))
v_a match
case CC_B(CC_E(CC_C(_))) => println(0) // was warn: unreachable
case _ => println(1) If we run the code, we can see the first case is executed, so it is not unreachable. Then for the
I tried this approach, I think the code should be more cleaner: https://github.com/dotty-staging/dotty/blob/381c197ebbc1feb2f801c9df31d06e6d4d97fa6a/compiler/src/dotty/tools/dotc/transform/patmat/Space.scala Although this may contradict to what I said in def f3(s: String | Null) = s match
case s2: String =>
case _ => // warn NullOnly or not? |
Maybe you need a rebase? |
ce53f6e
to
d53a9ce
Compare
Looks like there's code in the other tests that use a wildcard to catch a null. Somehow that didn't throw a warning before? |
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.
Except the weird diff, otherwise LGTM. Can you try to git rebase scala3/main
to see if you can get rid of the issue?
a1f3993
to
0800e0f
Compare
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.
Looks great! Thanks.
Fixes #21724