Incorrect match inexhaustive warnings in Scala 2.13.4

[robby-phd]

reproduction steps

using Scala 2.13.4 and scalameta 4.3.24,

import scala.meta._

object Test extends App {
  "".parse[Source] match {
    case Parsed.Success(_) => println("Success")
    case Parsed.Error(_, _, _) => println("Error")
  }
}

problem

The Scala 2.13.4 compiler produces the following message:

.../Test.scala:4:11: match may not be exhaustive.
[warn] It would fail on the following inputs: Error(), Success()
[warn]   "".parse[Source] match {
[warn]           ^
[warn] one warning found
[info] done compiling

The Scala 2.13.3 compiler does not produce this warning.

This might be related to issue #12186.

Attached is a zip file to ease reproducing the issue.
scala-2.13.4-match-issue.zip

[dwijnand]

Minimised:

$ m Test.scala
object Test {
  sealed trait Parsed2[+T]

  object Parsed2 {
    class Success[+T](val tree: T)                                         extends Parsed2[T]
    class Error(val pos: Int, val message: String, val details: Exception) extends Parsed2[Nothing]

    object Success {
      def apply[T](tree: T): Success[T]        = new Success[T](tree)
      def unapply[T](x: Success[T]): Option[T] = if (x == null) None else Some(x.tree)
    }

    object Error {
      def apply(pos: Int, message: String, details: Exception): Error = new Error(pos, message, details)
      def unapply(x: Error): Option[(Int, String, Exception)]         = if (x == null) None else Some((x.pos, x.message, x.details))
    }
  }

  def m[Source](p: Parsed2[Source]) = p match {
    case Parsed2.Success(_)     => println("Success")
    case Parsed2.Error(_, _, _) => println("Error")
  }
}

$ scalac -2.13.4 Test.scala
Test.scala:19: warning: match may not be exhaustive.
It would fail on the following inputs: Error(), Success()
  def m[Source](p: Parsed2[Source]) = p match {
                                      ^
1 warning

The problem is that Success and Error aren't case classes, which means that the pattern matcher uses the custom unapply as its extractor and that extractor isn't irrefutable. For case classes it knows it is, because it knows from the initial type test it does (if (x.isInstanceOf[Success[_]])) that it's not null. Which leaves two refutable custom extractors, which are actually complementary (ignoring null...) but the compiler doesn't know that either...

Basically the compiler is saying that the unhandled cases are those where Success.unapply and Error.unapply return None, which we know to be for null. In general the exhaustivity checker ignores warning on the null case, but now it doesn't know that's root cause of the Nones... Ideally we could capture that in the type... def unapply(x: Success[T] @orNull): Option[T]... (Or even more ideally null was quarantined from socialising with the rest of the type system community...)

One way this could be avoided is by changing the custom extractors to return Some[T] and Some[(Int, String, Exception)] instead of Option, at the cost of those unapplies no longer being safe if some other (i.e not from a pattern match case) call is made to unapply.

I no longer think this is a regression and I'm not even sure it's a bug any more... Everything's pretty much working as intended, given what is provided to the compiler. Making this better would required enhancing things in some way...

[smarter]

def unapply[T](x: Success[T]): Option[T] = if (x == null) None else Some(x.tree)

null is never matched by an extractor since scala/scala#6485, so this can safely be replaced by def unapply[T](x: Success[T]): Some[T] = Some(x.tree) to regain exhaustiveness checking.

[dwijnand]

That's what I said, except it makes it unsafe for random other unapply calls. Perhaps that's acceptable (I wouldn't hesitate to do that). Could be the macro implementation is from before that change.

[smarter]

Ah ok I misunderstood, yeah I don't think null-safety of explicitly called unapply is something anyone really cares about.

[dwijnand]

Just to confirm myself:

$ m Test.scala
object Test {
  sealed trait Parsed2[+T]

  object Parsed2 {
    class Success[+T](val tree: T)                                         extends Parsed2[T]
    class Error(val pos: Int, val message: String, val details: Exception) extends Parsed2[Nothing]

    object Success {
      def apply[T](tree: T): Success[T]      = new Success[T](tree)
      def unapply[T](x: Success[T]): Some[T] = Some(x.tree)
    }

    object Error {
      def apply(pos: Int, message: String, details: Exception): Error = new Error(pos, message, details)
      def unapply(x: Error): Some[(Int, String, Exception)]           = Some((x.pos, x.message, x.details))
    }
  }

  def m[Source](p: Parsed2[Source]) = p match {
    case Parsed2.Success(_)     => println("Success")
    case Parsed2.Error(_, _, _) => println("Error")
  }
}
$ scalac Test.scala
$

Given that, I'm going to call this one not a bug.

[robby-phd]

How to avoid the warning for extractors returning Boolean?

object Test {
  sealed trait Foo
  final class Bar extends Foo

  object Bar {
    def unapply(o: Bar) /* : true */ = true
  }

  def f(foo: Foo) = foo match {
    case Bar() => println("Bar")
  }
}

Test.scala:9: warning: match may not be exhaustive.
It would fail on the following input: Bar()
  def f(foo: Foo) = foo match {
                    ^
1 warning

Uncommenting the true literal return type gives the following error:

Test.scala:10: error: The result type of an unapply method must contain a member `get` to be used as an extractor pattern, no such member exists in true
    case Bar() => println("Bar")
         ^
1 error

[dwijnand]

That came up in #12240, which @martijnhoekstra is trying to get working in scala/scala#9343.

[martijnhoekstra]

It's close, but not quite: 9343 is doing irrefutable name-based extractors.

This is an irrefutable boolean extractor that's not working.

Irrefutable extractors (whether the Some variety that already works, the name based variety in the PR or the boolean variety as Robby shows) are not part of the spec, which is annoying. I'll file a separate bug for that.

[martijnhoekstra]

It was so close, that it's now quite. I made a separate issue for it but included the fix in the same PR

[blast-hardcheese]

I'm still seeing an issue with this in 2.13.6

Minimal case:

abstract class LA
class SL extends LA

class LanguageParameter[L <: LA](
  val in: Boolean
)

object LanguageParameter {
  def unapply[L <: LA](param: LanguageParameter[L]): Option[Boolean] =
    Some(param.in)
}

object App {
  def param: Option[LanguageParameter[SL]] = Some(new LanguageParameter[SL](true))

  val z: List[Boolean] =
    param.toList.flatMap {
      case LanguageParameter(in) =>
        Some(in)
    }
}

yields

[warn] .../src/main/scala/App.scala:17:26: match may not be exhaustive.
[warn]     param.toList.flatMap {
[warn]                          ^
[warn] one warning found

[som-snytt]

@blast-hardcheese the example says to use def unapply(x): Some[_] to mean "irrefutable". Not sure if this is documented. Probably you want to ask questions on discord or gitter.

[blast-hardcheese]

@som-snytt My apologies, I falsely presumed this was related to some type bounded unapply and just got excited there was already an issue related to it. Thanks for pointing me in the right direction, confirmed that Some[...] resolves my issue as well.

[blast-hardcheese]

Follow-up, for those curious and or following the trail. The change that introduced this behaviour is scala/scala@c54081e#diff-2215b486f4bacbe3dcddde189788c570d51345473b306e22ced6cce6ca4c11edL508-L517

diff --git a/src/compiler/scala/tools/nsc/transform/patmat/MatchAnalysis.scala b/src/compiler/scala/tools/nsc/transform/patmat/MatchAnalysis.scala
index 301d2dc32a..cca38266e4 100644
--- a/src/compiler/scala/tools/nsc/transform/patmat/MatchAnalysis.scala
+++ b/src/compiler/scala/tools/nsc/transform/patmat/MatchAnalysis.scala
@@ -505,7 +505,6 @@ trait MatchAnalysis extends MatchApproximation {
       // - back off (to avoid crying exhaustive too often) in unhandled cases
       val start = if (StatisticsStatics.areSomeColdStatsEnabled) statistics.startTimer(statistics.patmatAnaExhaust) else null
       var backoff = false
-      val strict = settings.XstrictPatmatAnalysis.value

       val approx = new TreeMakersToProps(prevBinder)
       val symbolicCases = approx.approximateMatch(cases, approx.onUnknown { tm =>
@@ -514,8 +513,9 @@ trait MatchAnalysis extends MatchApproximation {
           case ExtractorTreeMaker(_, _, _)
              | ProductExtractorTreeMaker(_, _)
              | GuardTreeMaker(_) =>
-            if (strict) False else True
+            False  // <-- This used to be True by default, you had to opt-in to this "strict" behaviour.
-          case _ => // debug.patmat("backing off due to "+ tm)
+          case _ =>
+            debug.patmat("backing off due to "+ tm)
             backoff = true
             False
         })

Setting that value back to True doesn't cause any tests in the scala/scala repo to fail, but presumably there would be more unpleasant ramifications of that were done on a wider scale.

There does appear to be a bit of a bit of incorrect reporting around this, fwiw, an example from scalameta's codebase:

  private def fqRef(fqName: String, isTerm: Boolean): Tree = {
    def loop(parts: List[String]): Tree = parts match {
      case Nil :+ part => q"${TermName(part)}"
      case rest :+ part => q"${loop(rest)}.${TermName(part)}"
    }
    val prefix :+ last = fqName.split("\\.").toList
    if (isTerm) q"${loop(prefix)}.${TermName(last)}" else tq"${loop(prefix)}.${TypeName(last)}"
  }

reports

[warn] /home/runner/work/scalameta/scalameta/scalameta/common/shared/src/main/scala/org/scalameta/internal/MacroHelpers.scala:79:43: match may not be exhaustive.
[warn] It would fail on the following inputs: List(_), Nil
[warn]     def loop(parts: List[String]): Tree = parts match {
[warn]                                           ^

despite Nil :+ part obviously satisfying List(_). Rewriting from :+ to ::...

    def loop(parts: List[String]): Tree = parts match {
      case part :: Nil => q"${TermName(part)}"
      case part :: rest => q"${loop(rest)}.${TermName(part)}"
    }

correctly identifies

[warn] It would fail on the following input: Nil
[warn]     def loop(parts: List[String]): Term = parts match {
[warn]                                           ^

If there are no objections, I think it makes sense to report these kinds of edge cases in this issue, despite it already being closed, as there's already so much context here already.

If there are objections, I'm happy to open a new issue.

[blast-hardcheese]

I don't think null-safety of explicitly called unapply is something anyone really cares about.

Amusingly, scalameta does, up until now I hadn't realized this whole issue pertained to scalameta in the first place. Perhaps macro/compiler internals are an edge case due to their more... eccentric requirements?

[blast-hardcheese]

I've also got something of a usability question around this; is there a way, without case _, to catch the None case?

Previously, if we were not strict in unapply, we would have exhaustiveness checking in the compiler by fully expressing all fields (example). Now, I don't seem to have something I can put to satisfy the compiler warning while also offering totality checking for sealed traits.

Given the warning

[warn] .../guardrail/modules/java-spring-mvc/src/main/scala/dev/guardrail/generators/java/springMvc/SpringMvcServerGenerator.scala:61:62: match may not be exhaustive.
[warn] It would fail on the following inputs: BinaryContent(), TextContent()
[warn]   private def toSpringMediaType: ContentType => Expression = {
[warn]                                                              ^
[warn] one warning found

a user may be inclined to trust the construction suggested by the compiler:

@@ -65,6 +66,7 @@ class SpringMvcServerGenerator private (implicit Cl: CollectionsLibTerms[JavaLan
     case OctetStream         => new FieldAccessExpr(new NameExpr("MediaType"), "APPLICATION_OCTET_STREAM_VALUE")
     case TextContent(name)   => new StringLiteralExpr(name)
     case BinaryContent(name) => new StringLiteralExpr(name)
+    case BinaryContent()     => ??? // TODO: What do we do if we get here?
   }

we're greeted by the (expected)...

[info] compiling 1 Scala source to .../guardrail/modules/java-spring-mvc/target/scala-2.13/classes ...
[error] .../guardrail/modules/java-spring-mvc/src/main/scala/dev/guardrail/generators/java/springMvc/SpringMvcServerGenerator.scala:70:10: not enough patterns for object BinaryContent offering String: expected 1, found 0
[error]     case BinaryContent()     => ???
[error]          ^
[error] one error found

Is there a way to address the looseness of def unapply(...) = None while also maintaining exhaustivity checks in the compiler?

[som-snytt]

Extractors don't need to deal with null. The spec says "An extractor pattern cannot match the value null. The implementation ensures that the unapply/unapplySeq method is not applied to null." I was made aware of this because of regex extractors. IIRC it wasn't always true. Relatedly, type test case _: C is an instanceof and never matches null.

[som-snytt]

@blast-hardcheese You have something like the following. The revised unapply does not warn, TIL. Apparently, the narrower param type suffices to express "I cover this case".

There are other discussions about adding an annotation to say what an extractor covers, but I did not follow them. Again, you'd get a wider and smarter audience on the forum.

sealed trait T
sealed class C(val c: String) extends T
case object X extends T
case object Y extends C("y")

object Extract {
  // incurs "It would fail on the following inputs: C(), Y"
  def unapply0(t: T): Option[String] =
    t match {
      case c: C => Some(c.c)
      case _ => None
    }
  def unapply(t: C): Some[String] = Some(t.c)
}

object Test extends App {
  val t: T = Y
  def f(x: T) =
    x match {
      case X => "x"
      case Extract(y) => y
    }
  println(f(t))
}

but wait, you want a warning if cases are added, so better would be

  def unapply(t: C): Some[String] =
    t match {
      case y: Y.type => Some(y.c)
    }

but that actually warns about C() still, although the only C is the Y singleton.

[som-snytt]

this is also doesn't warn and is closer to the sample. I guess if you want separate extractors for Y and Z that would also work.

sealed trait T

case object X extends T
sealed abstract class C(val c: String) extends T
sealed class Y(x: String) extends C(x)
case object Y0 extends Y("y0")
case object Y1 extends Y("y1")
/*
sealed class Z(x: String) extends C(x)
case object Z0 extends Z("z0")
*/

object Extract {
  def unapply(c: C): Some[String] =
    c match {
      case y: Y => Some(y.c)
    }
}

object Test extends App {
  val t: T = Y1
  def f(x: T) =
    x match {
      case X => "x"
      case Extract(y) => y
    }
  println(f(t))
}

Edit: I just went, wait did I accidentally close the issue? before remembering we're chatting on a closed ticket. Probably any of discord, discourse, gitter, or a separate ticket (for buggy behavior) would be better, putting on my Dale hat and Seth t-shirt.

[dwijnand]

If there are no objections, I think it makes sense to report these kinds of edge cases in this issue, despite it already being closed, as there's already so much context here already.

The difference between :: and :+ is that :: is the case class name, while :+ is a custom extractor. Being a case class name (and exhausting the entire space of its head and tail parameters) all that's left is Nil. But that knowledge isn't reachable (no pun intended!) by the match analysis of using :+.

[som-snytt]

There are open links for the extractor coverage issue on dotty.

[coreywoodfield]

Found another issue in 2.13.6 (and presumably in 2.13.4) which works fine in 2.13.3:

def foo(bool: Boolean): Boolean = {
  val bar: Boolean = true
  bar match {
    case false => false
    case true if bool => true
    case true if !bool => true
  }
}

This match is clearly exhaustive, but the compiler errors (with fatal warnings on):

[Error] ... match may not be exhaustive.
[Error] It would fail on the following input: true

Is it appropriate to discuss this here or should I make a new issue?

[som-snytt]

@coreywoodfield my naive expectation is that the guards break exhaustiveness, but I also expect the breakage to be "false negative" (no warn) instead of false positive (warn incorrectly). I saw a similar false pos on another ticket. (I'm not following the latest work here.) I would be surprised if it accurately warned (or not) if two guards are identical except for negation, although that would be clever.

To answer the direct question, I think a discussion on the forum would be most productive as a first step, as it is not clearly a bug.

[coreywoodfield]

I checked 2.13.3 again, and it looks like the false negative route was taken there (so

def foo(bool: Boolean): Boolean = {
  val bar: Boolean = true
  bar match {
    case false => false
    case true if bool => true
  }
}

doesn't warn even though it's not exhaustive) so it seems like 2.13.4 just switched to the false positive, which is overall safer so a good change. It would be nice if it could recognize negated guards though. Further discussion here: https://users.scala-lang.org/t/exhaustive-match-checking-warns-incorrectly-when-cases-are-exhaustive-but-include-guards/7916

[SethTisue]

I've responded on Discourse.

Please use Discourse for questions. (There's also Stack Overflow, Discord, and so on.) These questions are perfectly good questions, but we are adamant about keeping scala/bug tickets focused on specific individual bugs, not free-for-alls for discussing entire general areas of functionality (for example, exhaustiveness checking).