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

check exhaustivity involving user-defined extractors with precise return type #4691

Closed
scabug opened this issue Jun 13, 2011 · 13 comments
Closed
Assignees
Labels
Milestone

Comments

@scabug
Copy link

scabug commented Jun 13, 2011

The following code fails to produce a non-exhaustive pattern warning when compiled with Scala 2.9.0-RC3 and above. The warning does get emitted with versions 2.9.0-RC2 and 2.8.1.

sealed trait Node

class NodeType1 (val a:Int) extends Node
class NodeType2 (val b:Int) extends Node

object NodeType1 {
  def unapply (x : NodeType1) : Some[Int] = Some(x.a)
}

object NodeType2 {
  def unapply (x : NodeType2) : Some[Int] = Some(x.b)
}

object Test {
  def test (x: Node) = x match {
    case NodeType1(a) => "got node type 1 " + a 
  }
}
@scabug
Copy link
Author

scabug commented Jun 13, 2011

Imported From: https://issues.scala-lang.org/browse/SI-4691?orig=1
Reporter: Arseniy Alekseyev (rotsor)
Affected Versions: 2.9.0

@scabug
Copy link
Author

scabug commented Nov 28, 2011

@Sciss said:
I stumbled across this, too (in Scala 2.9.1). Sealed traits are very important because you they can, as opposed to an abstract class, participate in multiple sealed hierarchies.

object Order {
   sealed trait EntryOption
   case object EmptyEntry extends EntryOption
   trait Entry extends EntryOption
   
   // this doesn't emit a warning:
   def isEmpty( a: EntryOption ) : Boolean = a match {
      case EmptyEntry => true
//    case _: Entry   => false
   }
}

@scabug
Copy link
Author

scabug commented Nov 28, 2011

@Sciss said:
Another example:

trait Order {
   sealed trait EntryOption
   case object EmptyEntry extends EntryOption
   sealed trait Entry extends EntryOption
   private final class EntryImpl extends Entry
       
   def isEmpty( a: EntryOption ) : Boolean = a match {
      case EmptyEntry => true
//    case _: Entry   => false
   }
}

This code above does emit the warning. But the following variation not:

trait Order {
   sealed trait EntryOption
   case object EmptyEntry extends EntryOption
   sealed trait Entry extends EntryOption
       
   def isEmpty( a: EntryOption ) : Boolean = a match {
      case EmptyEntry => true
//    case _: Entry   => false
   }
}

trait OrderImpl extends Order {
   private final class EntryImpl extends Entry
}

@scabug
Copy link
Author

scabug commented Jun 11, 2012

@adriaanm said:
This report is a bit of a mixed bag of not a bug and fixed.
User-defined extractors cause the exhaustivity checker to back off.
We could in principle inline the extractors, but otherwise, what can you do?
The valid examples do produce exhaustivity warnings.
The one where EntryImpl is factored out to OrderImpl does not warn, since we do not do a whole-program analysis.
If you move the match to OrderImpl, you will get a warning.

@scabug
Copy link
Author

scabug commented Jun 11, 2012

Arseniy Alekseyev (rotsor) said (edited on Jun 11, 2012 2:33:43 PM UTC):
The problem is not caused by user-defined extractors: as you can see, the user-defined extractors are total (have the return type of Some[Int]). The non-totality of this pattern match is caused by Scala implicitly matching on the runtime type. The non-exhaustiveness of such a match should be evident to the compiler.

Anyway, the warnings need to become conservative to be useful. Let's wait for that to happen first.

@scabug
Copy link
Author

scabug commented Jun 11, 2012

@dcsobral said:
@Arseniy:

I think you misunderstood what Adriaan said. You said that user-defined extractors are total, but Scala does not and cannot check that (for the general case anyway -- it's theoretically possible to detect trivial cases). Since it can never decide whether a pattern with user-defined extractors is exhaustive or not, it simply turns that feature off, therefore not warning in cases where it could make a statement to the effect that the matching is definitely not exhaustive.

Anyway, given that the pattern matcher cannot warn non-exhaustiveness correctly, it doing so in some cases might lead people to think that absence of a warning as an assertion of exhaustiveness (which it is not).

@scabug
Copy link
Author

scabug commented Jun 11, 2012

@adriaanm said:
Thanks Daniel for clarifying -- yes, that's what I meant. Since we can't check extractors in general, we never do.

Also, the exhaustivity/unreachability warnings are supposed to be conservative. The analyses have been rewritten from scratch.
(Available as of recent nightlies or M4).

Spurious warnings are a bug, please assign them directly to me.

@scabug
Copy link
Author

scabug commented Jun 11, 2012

@paulp said:
I agree with Arseniy, and "we can't warn about everything so we will warn about nothing" is a completely bogus line of argumentation which nobody believes with any consistency, or we'd have no warnings.

There is no reason f2 and f3 in the following can't give the same warning which f1 does. We have exactly the same information about what is covered and what is not in each of them. It would be very useful if this worked.

sealed trait Foo
class Bar1 extends Foo
class Bar2 extends Foo
class Bar3 extends Foo

object Baz1 {
  def unapply(x: Bar1): Some[Int] = Some(1)
}
object Baz2 {
  def unapply(x: Bar2): Some[Int] = Some(2)
}


object Test {
  def f1(x: Foo) = x match {
    case _: Bar1 => 1
    case _: Bar2 => 2
  }
  def f2(x: Foo) = x match {
    case _: Bar1 => 1
    case Baz2(x) => x
  }
  def f3(x: Foo) = x match {
    case Baz1(x) => x
    case Baz2(x) => x
  }
}

@scabug
Copy link
Author

scabug commented Jun 11, 2012

@paulp said:
I apologize for reopening this given that I've seen you deal with like 150 tickets today, but I need more convincing and I don't want it get buried.

@scabug
Copy link
Author

scabug commented Jun 11, 2012

@adriaanm said:
Thanks. I agree that the Some[T] result type (which I missed -- my vision went a bit blurry around #139) should tell us something. We can only hope it's not Some(Math.random) they're returning.

@scabug
Copy link
Author

scabug commented Jun 11, 2012

@paulp said:
They can return Math.random if they want. An extractor covers Bar1 because the method parameter type is Bar1 and the return is Some[T]. The values are immaterial. We can hope against Math.random on general principles though if you like.

@scabug
Copy link
Author

scabug commented Jun 11, 2012

@adriaanm said:
Yes. General principles. That's what I was thinking of. And sleep.

@scabug
Copy link
Author

scabug commented Jul 5, 2012

@adriaanm said:
scala/scala#824

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants