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

unapplySeq silences all exhaustivity warnings #9232

Closed
scabug opened this issue Mar 17, 2015 · 5 comments
Closed

unapplySeq silences all exhaustivity warnings #9232

scabug opened this issue Mar 17, 2015 · 5 comments
Assignees
Labels
Milestone

Comments

@scabug
Copy link

@scabug scabug commented Mar 17, 2015

The following code compiles without any warning:

package test

final class Foo(val value: Int)
object Foo {
  def unapplySeq(foo: Foo): Some[Seq[Int]] = Some(List(foo.value))
  /*def unapply(foo: Foo): Some[Int] = Some(foo.value)*/
}

sealed trait Tree
case class Node1(foo: Foo) extends Tree
case class Node2() extends Tree

object Test {
  def transformTree(tree: Tree): Any = tree match {
    case Node1(Foo(1)) => ???
  }
}

But if you uncomment unapply, you get:

test2.scala:14: warning: match may not be exhaustive.
It would fail on the following inputs: Node1(_), Node2()
  def transformTree(tree: Tree): Any = tree match {
                                       ^
one warning found

It'd be nice to give the same warnings when only unapplySeq is used, for example with List.

@scabug
Copy link
Author

@scabug scabug commented Mar 17, 2015

Imported From: https://issues.scala-lang.org/browse/SI-9232?orig=1
Reporter: Guillaume Martres (Smarter)
Affected Versions: 2.11.6
See #5365

@scabug
Copy link
Author

@scabug scabug commented Mar 18, 2015

Guillaume Martres (Smarter) said:
I did not mention it explicitely but replacing transformTree with:

  def transformTree(tree: Tree): Any = tree match {
    case Node1(_) => ???
  }

Also produces the warnings you'd expect:

test2.scala:14: warning: match may not be exhaustive.
It would fail on the following input: Node2()
  def transformTree(tree: Tree): Any = tree match {
                                       ^
one warning found

Which is why I'm saying that having unapplySeq without unapply somehow silences even unrelated exhaustivity warnings

@scabug
Copy link
Author

@scabug scabug commented Mar 24, 2015

@retronym said:
Extractor patterns within a pattern match typically disable analysis of exhaustivity and reachability. There is a special case for unapply methods that are irrefutable, that is, they statically return Some[T] rather than just Option[T]. Such patterns are considered to always match for the purposes of analysis.

Guards similarly hamper analysis:

object Test {
  def noWarningHere(x: Option[Any]): Any = x match {
    case Some(1) if "".isEmpty => ???
  }
}

@scabug
Copy link
Author

@scabug scabug commented Mar 24, 2015

Guillaume Martres (Smarter) said:

Extractor patterns within a pattern match typically disable analysis of exhaustivity and reachability.

Oh, I knew that this happened with guard but not with extractors. I think that two things need to be done at least:

  • Have a warning (even an optional one) when the exhaustivity/reachability analysis is disabled.
  • Try to do "best effort" analysis, e.g. in your example, replace case Some(1) if "".isEmpty by case Some(1) to do a partial analysis, or in my example replace case Node1(Foo(1)) by case Node1(_), this would already improve things a lot.

The warning is especially important because a type system is much more useful when it is reliable, it's hard to rely on the exhaustiveness checker because it's so easy to accidentally disable it.

@scabug
Copy link
Author

@scabug scabug commented Apr 29, 2015

Richard Bradley (richard.bradley) said:

Guards similarly hamper analysis:
...
[we should] have a warning (even an optional one) when the exhaustivity/reachability analysis is disabled.
it's hard to rely on the exhaustiveness checker because it's so easy to accidentally disable it.

I agree.
See also #5365 re guards disabling exhaustivity analysis. I don't see why they need to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 participants