Skip to content

Incorrect exhaustiveness warning when pattern matching on Nil.type #12186

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

Closed
NthPortal opened this issue Oct 12, 2020 · 18 comments · Fixed by scala/scala#9313
Closed

Incorrect exhaustiveness warning when pattern matching on Nil.type #12186

NthPortal opened this issue Oct 12, 2020 · 18 comments · Fixed by scala/scala#9313
Assignees
Labels
dealias compiler isn't dealiasing when it should, or vice versa has PR patmat regression
Milestone

Comments

@NthPortal
Copy link

reproduction steps

using Scala 2.13.4-20201011-114304-1c1fa0c (equivalent roughly to HEAD a few days ago)

scala> def test(list: List[Int]): Boolean = list match {
     |   case h :: t => true
     |   case _: Nil.type => false
     | }
                                            ^
       warning: match may not be exhaustive.
       It would fail on the following input: Nil
def test(list: List[Int]): Boolean

scala> test(Nil)
val res0: Boolean = false

scala> def test(list: List[Int]): Boolean = list match {
     |   case h :: t => true
     |   case _: scala.collection.immutable.Nil.type => false
     | }
def test(list: List[Int]): Boolean

problem

Nil.type is the type of Nil, so the match is exhaustive.

In 2.13.3, the same code does not raise an exhaustiveness warning.

Note: it only warns if you use the value alias of Nil (from the scala package, but presumably any value alias as well), and does not warn if you use the fully qualified type.

@dwijnand
Copy link
Member

That sucks, and 100% going to be due to scala/scala#9208.

@dwijnand dwijnand added this to the 2.13.4 milestone Oct 12, 2020
@NthPortal
Copy link
Author

I checked the Jenkins output, and you are correct. the bootstrap introduced an exhaustiveness warning (which we never saw, because we don't have fatal warnings yet).

technically, that PR didn't break it so much as expose it, as in principle this can happen for any sealed sum type for which you create a value alias

@dwijnand dwijnand self-assigned this Oct 13, 2020
@dwijnand
Copy link
Member

Some notes:

Compare with the REPL:

scala> typeOf[Nil.type] =:= typeOf[scala.collection.immutable.Nil.type]
val res1: Boolean = true

One difference is scala.collection.immutable.Nil.type a UniqueSingleType in the REPL, but a ModuleTypeRef where it's checked in patmat (Const#unique). normalizePlus converts UniqueSingleTypes into ModuleTypeRefs, but only if the symbol is a module and ... 🥁 the symbol Nil in scala.Nil.type is a MethodSymbol - not a module 😞 (it's not even a isVal, surprisingly to me).

@lrytz
Copy link
Member

lrytz commented Oct 16, 2020

the symbol Nil in scala.Nil.type is a MethodSymbol - not a module

Ah, this rings a bell...

https://github.com/scala/scala/blob/92fa9be7e88c81bceae91494b45297616c0f1728/src/reflect/scala/reflect/internal/Symbols.scala#L778-L782

Unfortunately, phase travel doesn't revert that flag...

scala> :power

scala> class C { object T }
scala> val t = typeOf[C].member(TermName("T"))
val t: $r.intp.global.Symbol = object T

scala> t.isMethod
val res0: Boolean = false

scala> enteringJVM(t.info)
val res1: $r.intp.global.Type = (): T.type

scala> t.isMethod
val res2: Boolean = true

@lrytz
Copy link
Member

lrytz commented Oct 16, 2020

Though the symbol doesn't change here, and you're saying you see a MethodSymbol, so there is something else going on

scala> t.getClass
val res5: Class[_ <: $r.intp.global.Symbol] = class scala.reflect.internal.Symbols$ModuleSymbol

@dwijnand
Copy link
Member

Yeah, because my reference is the "val alias" scala.Nil, so more like:

scala> object Scala { val Nil: scala.collection.immutable.Nil.type = scala.collection.immutable.Nil }
object Scala

scala> val t = typeOf[Scala.type].member(TermName("Nil"))
val t: $r.intp.global.Symbol = value Nil

scala> t.getClass
val res0: Class[_ <: $r.intp.global.Symbol] = class scala.reflect.internal.Symbols$MethodSymbol

scala> t.isMethod
val res1: Boolean = true

@lrytz
Copy link
Member

lrytz commented Oct 16, 2020

Ah of course, sorry for the confusion

@adamw
Copy link

adamw commented Dec 15, 2020

I turned on nonexhaustive errors using -Wconf:cat=other-match-analysis:error and on 2.13.4 I'm now getting a lot of problems like this (simplified code from tapir)

def matchPath(pathInputs: Vector[String]): Unit = {
    pathInputs match {
      case Vector() => ???
      case _ :+ last => ???
    }
  }

causes:

[error] DecodeInputs.scala:122:5: match may not be exhaustive.
[error] It would fail on the following inputs: Vector0, Vector1(), Vector2(), Vector3(), Vector4(), Vector5(), Vector6()
[error]     pathInputs match {
[error]     ^

is this the same bug? I can create another one, but if that's already covered, there's no point.

Using 2.13.3 causes the code above to compile w/o error or warning.

@martijnhoekstra
Copy link

@adamw The issue you describe is that multiple extractors can't be shown to be exhaustive together. At the risk of stating the obvious, you can workaround by matching the extractor that extracts a value first, and use a fallback for the other:

case _ :+ last =>
case _ =>

@NthPortal
Copy link
Author

@adamw that is not the same bug; it is part of either #12243 or #12240 I believe

@adamw
Copy link

adamw commented Dec 16, 2020

@NthPortal thanks, though I suppose @martijnhoekstra says it's a feature, not a bug? :)

I'd prefer to avoid matching on _ - that's something different than matching on an empty/non-empty vector. Though, as I understand proving exhaustiveness basing on extractors is not possible. And I suppose that's correct, as the compiler can't possibly know what's extracted when

@martijnhoekstra
Copy link

I wouldn't want to call it a feature, just that scala doesn't have a feature to declare extractors that are exhaustive together.

Without matching on _, you could write a method that extracts Option[(init, last)], and match on that, or if #12240 gets fixed match on .view.reverse with case Seq() => ; case Seq(last, _*) =>

@adamw
Copy link

adamw commented Dec 16, 2020

@martijnhoekstra This makes perfect sense, thank you for the explanation!

@adamw
Copy link

adamw commented Dec 16, 2020

@martijnhoekstra In fact, isn't checking head & tail such a common operation on sequences, that it might be justified to add sth like a headAndTail/peel deconstructing method?

@martijnhoekstra
Copy link

Yes, potentially. They exist in :+.unapply and +:.unapply, but using them directly is ugly, in much the same way as #12243 (comment)

@adamw
Copy link

adamw commented Dec 16, 2020

uncons, that's the name I was looking for! I knew it had a name just couldn't remember it. Thanks again :)

@dwijnand
Copy link
Member

Here you were only using the last element if available, so matching lastOption is also a... option 😄.

@adamw
Copy link

adamw commented Dec 17, 2020

@dwijnand yes true, there was a number of errors I started getting with 2.13.4, and I copied the least representative one of course ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dealias compiler isn't dealiasing when it should, or vice versa has PR patmat regression
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants