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

non-exhaustiveness warning is too specific and too general #7746

Closed
scabug opened this issue Aug 12, 2013 · 6 comments
Closed

non-exhaustiveness warning is too specific and too general #7746

scabug opened this issue Aug 12, 2013 · 6 comments
Assignees
Labels
Milestone

Comments

@scabug
Copy link

scabug commented Aug 12, 2013

After mentioning None, the next example lets a wildcard escape, the next is too specific, and the last is too broad. The things not covered should be some variation on: None, Some(x forSome x not in Some(5))

scala> def f[T](x: Option[T]) = x match { case Some(Some(5)) => true }
<console>:7: warning: match may not be exhaustive.
It would fail on the following inputs: None, Some((x: T forSome x not in Some[?])), Some(Some((x: Any forSome x not in 5))), Some(_)
       def f[T](x: Option[T]) = x match { case Some(Some(5)) => true }
                                ^
f: [T](x: Option[T])Boolean
@scabug
Copy link
Author

scabug commented Aug 12, 2013

Imported From: https://issues.scala-lang.org/browse/SI-7746?orig=1
Reporter: @paulp
See #8430

@scabug
Copy link
Author

scabug commented Aug 13, 2013

@retronym said:
Focusing initially on the spurious Some(_) case:

This counter example is rendered from the model:

(0)  = {scala.Tuple2@3801}"(V1=Some[?]#8,true)"
(1)  = {scala.Tuple2@3803}"(V3=5#10,false)"

V1 represents the scrutinee; V3 the contents of the nested Some.

This comes from the forced part of findAllModels. I've removed that here: retronym/scala@scala:2.10.x...ticket/7746

@scabug
Copy link
Author

scabug commented Aug 16, 2013

@retronym said:

Tracing out SI-7746:

def f[T](x: Option[T]) = x match {
    case Some(Some(5)) => true // It would fail on the following inputs: None, Some((x: T forSome x not in Some[?])), Some(Some((x: Any forSome x not in 5))), Some(_)
  }
unassigned List(V1=None.type#11, V3=5#10) in Map(V1=Some[?]#8 -> true, V2=Some[?]#9 -> false)

...

dropUnit(
  ArrayBuffer(Set(V1=None.type#11, V1=Some[?]#8), Set(-V1=Some[?]#8, -V2=Some[?]#9, -V3=5#10), Set(V1=Some[?]#8, -V1=None.type#11)),
  V3=5#10
) = ArrayBuffer(Set(V1=None.type#11, V1=Some[?]#8), Set(-V1=Some[?]#8, -V2=Some[?]#9), Set(V1=Some[?]#8, -V1=None.type#11))
DPLL
 V1=None.type#11 \/   V1=Some[?]#8    /\
  -V1=Some[?]#8  \/  -V2=Some[?]#9    /\
  V1=Some[?]#8   \/ -V1=None.type#11
DPLL
 V1=None.type#11 \/   V1=Some[?]#8    /\
  V1=Some[?]#8   \/ -V1=None.type#11
DPLL

dropUnit(
  ArrayBuffer(Set(V1=None.type#11, V1=Some[?]#8), Set(-V1=Some[?]#8, -V2=Some[?]#9, -V3=5#10), Set(V1=Some[?]#8, -V1=None.type#11)),
  -V3=5#10
) = ArrayBuffer(Set(V1=None.type#11, V1=Some[?]#8), Set(V1=Some[?]#8, -V1=None.type#11))
DPLL
 V1=None.type#11 \/   V1=Some[?]#8    /\
  V1=Some[?]#8   \/ -V1=None.type#11
DPLL

forcedYes(V3=5#10) Map(V1=Some[?]#8 -> true, V2=Some[?]#9 -> false, V3=5#10 -> true)
forcedNo(V3=5#10) Map(V1=Some[?]#8 -> true, V3=5#10 -> false)
...
var assignment for model Map(V1=Some[?]#8 -> true, V3=5#10 -> false):
V1(=x1: Option[?]) == (Some[?])  != ()
V3(=x1.x.x: Any) == ()  != (5)
describing (V1,List(Some[?]),List(),Map(),class Some,true)
described as: Some(_)
Taking the unassigned V3=5 as false leads us to simplify the model to a point where we suggest Some(_) as a counter example. But for V3 to be assignable at all (as i understand things), we need V2 = Some[?] to be true.

Is Map(V1=Some[?]#8 -> true, V3=5#10 -> false): a valid counter example that is wrongly described, or an invalid counter example? (To me, it seems like the latter.)

 adriaanm commented 2 days ago 
I'm very open to improvements to the counter example algorithm (though I'm
getting more anxious about changing any of this in 2.10 due to the risk of
undetected regressions). I really wish I had more time to work on this
right now, but I can't. I'd be more at ease if this was happening in
master. I have budgeted time during the polish milestones to work on this
kind of stuff (sept-oct).
…

 adriaanm commented 16 hours ago 
Is Map(V1=Some[?]#8 -> true, V3=5#10 -> false): a valid counter example that is wrongly described, or an invalid counter example? (To me, it seems like the latter.)
Yeah, the whole logic of dependencies between variables' types is not encoded in the formula. That's why the counter examples need to be pruned (https://github.com/scala/scala/blob/master/src/compiler/scala/tools/nsc/transform/patmat/MatchAnalysis.scala#L352)

@scabug
Copy link
Author

scabug commented Feb 1, 2014

@paulp said:
Amusingly this has found a way to get worse. It now throws an "Option()" in with the others.

scala> def f[T](x: Option[T]) = x match { case Some(Some(5)) => true }
<console>:7: warning: match may not be exhaustive.
It would fail on the following inputs: None, Option(), Some((x: T forSome x not in Some[?])), Some(Some((x: Any forSome x not in 5))), Some(_)
       def f[T](x: Option[T]) = x match { case Some(Some(5)) => true }
                                ^
f: [T](x: Option[T])Boolean

@scabug
Copy link
Author

scabug commented Mar 21, 2014

@retronym said (edited on Mar 21, 2014 11:15:03 AM UTC):
Another test case from #8430. I've just submitted a PR to fix the non-determinism aspect of that, but I think the incomplete counter example list is a variant of this bug.

$ cat Test.scala
{code}
package test
 
sealed trait CL3Literal
case object IntLit extends CL3Literal
case object CharLit extends CL3Literal
case object BooleanLit extends CL3Literal
case object UnitLit extends CL3Literal
 
 
sealed trait Tree
case class LetL(value: CL3Literal) extends Tree
case object LetP extends Tree
case object LetC extends Tree
case object LetF extends Tree
 
object Test {
  def transform(tree: Tree) : Any = tree match {
    case LetL(CharLit) =>
      ???
  }
}
{code}

$ scalac Test.scala
Test.scala:17: warning: match may not be exhaustive.
It would fail on the following inputs: ??, LetC, LetF, LetL(IntLit), LetP
def transform(tree: Tree) : Any = tree match {
^
one warning found
$ scalac Test.scala
Test.scala:17: warning: match may not be exhaustive.
It would fail on the following inputs: ??, LetC, LetF, LetL(BooleanLit), LetL(IntLit), LetP
def transform(tree: Tree) : Any = tree match {
^
one warning found
Running "scalac Test.scala" will output randomly one of the two warning messages above. One of them is more complete than the other, but is still missing the LetL(UnitLit) case.
{noformat}

@scabug
Copy link
Author

scabug commented Oct 6, 2014

@gkossakowski said:
scala/scala#3954

@scabug scabug closed this as completed Oct 6, 2014
@scabug scabug added the patmat label Apr 7, 2017
@scabug scabug added this to the 2.11.3 milestone Apr 7, 2017
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