SI-5365 Exhaustivity of extractors, guards, and unsealed traits. #5617
Conversation
Rather than disabling the analysis altogether, rewrite
guard to `true` (by default) or to `false` under a new
compiler option.
The default option errs on the side of avoiding spurious
warnings where the guard writer knows better. However,
there was a convincing argument made in the ticket that
in these cases it would not be to onerous to require
the code to be rewritten from
case P if g1 =>
case P if g2 =>
to:
case P if g1 =>
case P =>
Or to:
(scrut: @unchecked) match {
case P if g1 =>
case P if g2 =>
So perhaps we can turn on the strict version by
default, after running against the community build
as a sanity check.
Extractor patterns had the same limitation as guards.
This commit also enables the analysis for them in the
same way as done for guards. However, this still not
done for `unapplySeq` extractors, as the counter example
generator can't handle these yet.
| @@ -888,7 +893,7 @@ trait MatchAnalysis extends MatchApproximation { | |||
| // if uniqueEqualTo contains more than one symbol of the same domain | |||
| // then we can safely ignore these counter examples since we will eventually encounter | |||
| // both counter examples separately | |||
| case _ if inSameDomain => None | |||
| case _ if inSameDomain => Some(WildcardExample) | |||
There was a problem hiding this comment.
There is probably a better way to handle this case. I’m not sure what should ever hit the previous version of this – no tests failed, but I’m sure I’m overlooking something.
|
I signed the CLA. |
|
Some tests have changed the output: From https://scala-ci.typesafe.com/job/scala-2.12.x-validate-test/3948/consoleFull, you can run this to update the expectations ("checkfiles" in partest parlance), and then see if this is as you expect. |
|
@retronym Thanks. I must have been running the wrong test command locally, since I missed the failures. Hopefully one of the failures will point me in the right direction for fixing my confusion in |
Handle exhaustivity for `unapplySeq` extractors and restrict a `WildcardExample` case to when strict pattern matching is enabled.
|
As @durban pointed out on typelevel#129, these changes affect |
|
I'm not following the convo, but I had an old PR to treat catches exactly as PartialFunctions. |
|
@som-snytt Yeah, that sounds like exactly what I want. Should that PR be revived, or should I steal the part I need? (The parsing/hygiene stuff sounds a bit intimidating, so I doubt I’d pick up the whole PR, and @retronym suggested splitting it up anyway). |
|
FYI: this has been merged in Typelevel Scala 2.12.1. |
|
I see there is a failure on the “combined” check. Does that mean I should squash failing commits so that everything in the history builds cleanly? |
yes please. as a rule, before final merge we ask that every commit be green (and that unnecessary merge commits be avoided) |
|
Another strangeness (tested with Scalatl 2.12.1, scala> import scala.reflect.runtime.universe._
import scala.reflect.runtime.universe._
scala> (NoSymbol : Symbol) match {
| case NoSymbol => "nosym"
| case sym: Symbol if sym.isMethod => "meth"
| case x: Symbol => "fallback"
| }
<console>:15: warning: match may not be exhaustive.
It would fail on the following input: (x: reflect.runtime.universe.Symbol forSome x not in NoSymbol)
(NoSymbol : Symbol) match {
^
error: No warnings can be incurred under -Xfatal-warnings.However with a simple trait, it seems to work fine: scala> trait Foo
defined trait Foo
scala> val x: Foo = new Foo {}
x: Foo = $anon$1@10bd5fb8
scala> (x : Foo) match {
| case `x` => "x"
| case foo: Foo if scala.util.Random.nextBoolean() => "rnd"
| case foo: Foo => "fallback"
| }
res6: String = x
scala> |
|
@durban Thanks. This is an interesting case. It only happens with both flags enabled, unlike the other ones that only required |
|
@durban |
|
@sellout Ok, thanks, that makes (some) sense. So the behavior (while somewhat surprising) seems to be correct. Those cases indeed aren't exhaustive, so the compiler is right. As to why a (syntactically) type pattern uses an extractor, this comment on /** A ClassTag[T] can serve as an extractor that matches only objects of type T.
*
* The compiler tries to turn unchecked type tests in pattern matches into checked ones
* by wrapping a `(_: T)` type pattern as `ct(_: T)`, where `ct` is the `ClassTag[T]` instance.
* Type tests necessary before calling other extractors are treated similarly.
* `SomeExtractor(...)` is turned into `ct(SomeExtractor(...))` if `T` in `SomeExtractor.unapply(x: T)`
* is uncheckable, but we have an instance of `ClassTag[T]`.
*/So, if I understand correctly, this is to work around type erasure, and make more type patterns work (somewhat) safely. And it seems, that the reflection/macro API uses this feature extensively. Fair enough. |
|
@durban Well, your comment just helped me understand it better than the docs I’ve been reading all morning 😆 However, I wonder if we can avoid using the extractor in the case where the type annotation is the same as the scrutinee’s type? Since then you don’t really need to use the unapply. The type doesn’t change – i.e., |
|
In b4 @paulp explains why that doesn’t work. |
|
@sellout 🤔 |
|
TIL. |
|
@sellout The typechecker rewrites uncheckable type tests as class tag extractor calls before the pattern matcher reasons about whether a type test is redundant (due to the type of the scrutinee and also the knowledge that previous cases haven't matched). So the pattern matcher just sees an opaque extractor pattern and emits the In other words (inb4 @paulp again; TIAL), "an artifact of the implementation" |
| @@ -761,6 +765,7 @@ trait MatchAnalysis extends MatchApproximation { | |||
| def chop(path: Tree): List[Symbol] = path match { | |||
| case Ident(_) => List(path.symbol) | |||
| case Select(pre, name) => chop(pre) :+ path.symbol | |||
| case Apply(fun, args) => chop(fun) :+ path.symbol | |||
There was a problem hiding this comment.
Could you please rebase this change into a separate commit to help understand its impact (ie, is a no-op without the new flag enabled?) and comment this line? I'm assuming we know must account for getting an extractor call in the tree path under the new option.
| @@ -56,7 +56,7 @@ class TestSealedExhaustive { // compile only | |||
| case Ga => | |||
| } | |||
|
|
|||
| def ma6() = List(1,2) match { // give up | |||
There was a problem hiding this comment.
What change caused this test case to progress?
|
As a sidenote/question: pattern exhaustivity of destructuring |
|
Happy to reschedule for 2.12.4 once feel we can push this forward in time. |
|
closing for inactivity. anyone want to try and carry this work forward...? |
|
I'll pick this one up too. |
|
I'm going to close this one for now. @milessabin: happy to receive your resubmit when ready! |
|
happy ending at #9140 |
|
Two follow-ups to the thread are -Xlint enables check on "destructuring vals" and the "catch takes a function" is almost in #4400 but I haven't looked at whether checks are applied. |
This builds on @retronym’s #4929.
It handles the unsealed trait case (via an additional
-Xlint:strict-unsealed-patmatflag) and the extractor-only case (both raised by @durban).It still defaults to lax checking by default. I don’t know what’s involved in testing it against the community build to see if we can do strict-by-default.