Skip to content
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

Missing exhaustivity when pattern matching on nested case classes #13003

Closed
martingd opened this issue Jul 2, 2021 · 1 comment · Fixed by #13030
Closed

Missing exhaustivity when pattern matching on nested case classes #13003

martingd opened this issue Jul 2, 2021 · 1 comment · Fixed by #13030

Comments

@martingd
Copy link

martingd commented Jul 2, 2021

When pattern matching on a nested case class pattern like, case One(Two(Some(...))) the Scala 3 compiler does not detect the match as being non-exhaustive – unless option -Ycheck-all-patmat is given.

Compiler version

I have tested on compiler version 3.0.2-RC1-bin-20210701-9f97b0b-NIGHTLY (as well as on 3.0.0 where even more cases go undetected.)

Minimized code

Given this code…

case class One(two: Two)
case class Two(o: Option[Int])

def matchOneTwo(one: One) = one match
    case One(Two(Some(i))) => "match!"

def matchTwo(two: Two) = two match
    case Two(Some(i)) => "match!"

def matchOO(oo: Option[Option[Int]]) = oo match
    case Some(Some(i)) => "match!"

def matchOOO(ooo: Option[Option[Option[Int]]]) = ooo match
    case Some(Some(Some(i))) => "match!"

…the compiler only emits warnings about non-exhaustive matches for matchTwo(), matchOO(), and matchOOO() but not for matchOneTwo() – unless compiler option -Ycheck-all-patmat is given.

Output

Without option -Ycheck-all-patmat the output is:

-- [E029] Pattern Match Exhaustivity Warning: /path/to/Example.scala:7:25 
7 |def matchTwo(two: Two) = two match
  |                         ^^^
  |                         match may not be exhaustive.
  |
  |                         It would fail on pattern case: Two(None)
-- [E029] Pattern Match Exhaustivity Warning: /path/to/Example.scala:10:39 
10 |def matchOO(oo: Option[Option[Int]]) = oo match
   |                                       ^^
   |                         match may not be exhaustive.
   |
   |                         It would fail on pattern case: None, Some(None)
-- [E029] Pattern Match Exhaustivity Warning: /path/to/Example.scala:13:49 
13 |def matchOOO(ooo: Option[Option[Option[Int]]]) = ooo match
   |                                                 ^^^
   |       match may not be exhaustive.
   |
   |       It would fail on pattern case: None, Some(None), Some(Some(None))
three warnings found
three warnings found

Adding compiler option -Ycheck-all-patmat causes the missing case to be detected:

4 |def matchOneTwo(one: One) = one match
  |                            ^^^
  |                            match may not be exhaustive.
  |
  |                            It would fail on pattern case: One(Two(None))
-- [E029] Pattern Match Exhaustivity Warning: /path/to/Example.scala:7:25 

However, from the description of the compiler option:

-Ycheck-all-patmat          Check exhaustivity and redundancy of all pattern
                            matching (used for testing the algorithm).

I would not expect this option to be one to use “in production” – I might be wrong here.

Expectation

I would expect the compiler to detect all four cases without the use of compiler option -Ycheck-all-patmat.

@dwijnand
Copy link
Member

dwijnand commented Jul 5, 2021

Without knowing the code, it seems to me like the "checkable" test isn't correctly considering case classes nested in case classes:

private def exhaustivityCheckable(sel: Tree): Boolean = {
  // Possible to check everything, but be compatible with scalac by default
  def isCheckable(tp: Type): Boolean =
    val tpw = tp.widen.dealias
    val classSym = tpw.classSymbol
    classSym.is(Sealed) ||
    tpw.isInstanceOf[OrType] ||
    (tpw.isInstanceOf[AndType] && {
      val and = tpw.asInstanceOf[AndType]
      isCheckable(and.tp1) || isCheckable(and.tp2)
    }) ||
    tpw.isRef(defn.BooleanClass) ||
    classSym.isAllOf(JavaEnumTrait)

  val res = !sel.tpe.hasAnnotation(defn.UncheckedAnnot) && {
    ctx.settings.YcheckAllPatmat.value
    || isCheckable(sel.tpe)
    || {
      val tpw = sel.tpe.widen.dealias
      val classSym = tpw.classSymbol
      classSym.is(Case) && productSelectorTypes(tpw, sel.srcPos).exists(isCheckable(_))
    }
  }

  debug.println(s"exhaustivity checkable: ${sel.show} = $res")
  res
}

So looking to shuffling that classSym.is(Case) && ... part into isCheckable and looking at how the various patmat tests react would be a good next step.

@dwijnand dwijnand self-assigned this Jul 5, 2021
dwijnand added a commit to dwijnand/scala3 that referenced this issue Jul 8, 2021
Fixes scala#13003, by refixing scala#12485 (PR scala#12488).

Part of the issue is that isCheckable behaves differently under
-Ycheck-all-patmat and our tests only run under that flag.  So for
starters I added a test variant where that flag isn't used.  I'd like to
understand why that flag exists to see if we could remove it from
guarding the logic and the tests.
dwijnand added a commit to dwijnand/scala3 that referenced this issue Jul 9, 2021
Fixes scala#13003, by refixing scala#12485 (PR scala#12488).

Part of the issue is that isCheckable behaves differently under
-Ycheck-all-patmat and our tests only run under that flag.  So for
starters I added a test variant where that flag isn't used.  I'd like to
understand why that flag exists to see if we could remove it from
guarding the logic and the tests.

Also make sure that any patmat test that throws an exception fails the
test...
@dwijnand dwijnand added this to the 3.0.2-RC1 milestone Jul 12, 2021
BarkingBad pushed a commit to BarkingBad/dotty that referenced this issue Jul 23, 2021
Fixes scala#13003, by refixing scala#12485 (PR scala#12488).

Part of the issue is that isCheckable behaves differently under
-Ycheck-all-patmat and our tests only run under that flag.  So for
starters I added a test variant where that flag isn't used.  I'd like to
understand why that flag exists to see if we could remove it from
guarding the logic and the tests.

Also make sure that any patmat test that throws an exception fails the
test...
@Kordyjan Kordyjan modified the milestones: 3.0.2-RC1, 3.0.2 Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants