Skip to content

Commit

Permalink
Avoid spurious custom extractor warnings
Browse files Browse the repository at this point in the history
The pattern matcher converts code like:

    Box(Option("hello")) match {
      case Box(Some(s)) =>
      case Box(None)    =>
    }

into two invocations of Box.unapply, because it can't trust that
Box.unapply is idempotent.  For case classes (like Some) it can, and
moreso it can just access the underlying values directly ("x1.value").
The exhaustivity/unreachability analysis builds on this (e.g
"pointsToBound.exists(sym => t.exists(_.symbol == sym)").

With the previous commits making the exhaustivity/unreachability checker
consider custom extractors this would result in multiple symbols for
each extraction (instead of all being "x1.value", for example) which
results in multiple SAT variables which results, basically, in  warnings
that the value returned from the first extraction could None and the
second value could be Some - which is only true for a non-idempotent
Box.unapply.  The intent of the previous commits (and the whole PR) is
to make the exhaustivity checker provide warnings that were previous
missing, but warning for fear of non-idempotent custom extractors would
produce more noise than signal.

Now, within a minor release like 2.13.4 we can't go and change the code
generation and thus the code semantics (Dotty can and apparently does),
but we can do the analysis with the assumption of idempotency.  That's
what this does: in MatchApproximator's TreeMakersToProps use a cache for
the binders so when we see two extractions that are equivalent we reuse
the same binder, so it translates into the same SAT variable.
  • Loading branch information
dwijnand committed Oct 9, 2020
1 parent 0aa80e4 commit 789c827
Show file tree
Hide file tree
Showing 2 changed files with 64 additions and 12 deletions.
40 changes: 28 additions & 12 deletions src/compiler/scala/tools/nsc/transform/patmat/MatchAnalysis.scala
Original file line number Diff line number Diff line change
Expand Up @@ -68,13 +68,14 @@ trait TreeAndTypeAnalysis extends Debugging {
}

def equivalentTree(a: Tree, b: Tree): Boolean = (a, b) match {
case (Select(qual1, _), Select(qual2, _)) => equivalentTree(qual1, qual2) && a.symbol == b.symbol
case (Ident(_), Ident(_)) => a.symbol == b.symbol
case (Literal(c1), Literal(c2)) => c1 == c2
case (This(_), This(_)) => a.symbol == b.symbol
case (Apply(fun1, args1), Apply(fun2, args2)) => equivalentTree(fun1, fun2) && args1.corresponds(args2)(equivalentTree)
// Those are the only cases we need to handle in the pattern matcher
case _ => false
case (Select(qual1, _), Select(qual2, _)) => equivalentTree(qual1, qual2) && a.symbol == b.symbol
case (Ident(_), Ident(_)) => a.symbol == b.symbol
case (Literal(c1), Literal(c2)) => c1 == c2
case (This(_), This(_)) => a.symbol == b.symbol
case (Apply(fun1, args1), Apply(fun2, args2)) => equivalentTree(fun1, fun2) && args1.corresponds(args2)(equivalentTree)
case (TypeApply(fun1, args1), TypeApply(fun2, args2)) => equivalentTree(fun1, fun2) && args1.corresponds(args2)(equivalentTree)
case (a @ TypeTree(), b @ TypeTree()) => a.tpe =:= b.tpe
case _ => false // Those are the only cases we need to handle in the pattern matcher
}

trait CheckableTreeAndTypeAnalysis {
Expand Down Expand Up @@ -238,8 +239,9 @@ trait MatchApproximation extends TreeAndTypeAnalysis with ScalaLogic with MatchT
uniqueTypeProps.getOrElseUpdate((testedPath, pt), Eq(Var(testedPath), TypeConst(checkableType(pt))))

// a variable in this set should never be replaced by a tree that "does not consist of a selection on a variable in this set" (intuitively)
private val pointsToBound = mutable.HashSet(root)
private val trees = mutable.HashSet.empty[Tree]
private val pointsToBound = mutable.HashSet(root)
private val trees = mutable.HashSet.empty[Tree]
private val extractBinders = mutable.HashMap.empty[Tree, Symbol]

// the substitution that renames variables to variables in pointsToBound
private var normalize: Substitution = EmptySubstitution
Expand Down Expand Up @@ -282,7 +284,21 @@ trait MatchApproximation extends TreeAndTypeAnalysis with ScalaLogic with MatchT
// binderToUniqueTree uses the type of the first symbol that was encountered as the type for all future binders
abstract class TreeMakerToProp extends (TreeMaker => Prop) {
// requires(if (!substitutionComputed))
def updateSubstitution(subst: Substitution): Unit = {
def updateSubstitution(tm: TreeMaker): Unit = {
val subst = tm.subPatternsAsSubstitution

tm match {
case x @ ExtractorTreeMaker(_, None, binder) =>
val extractor = accumSubst(normalize(x.extractor))
extractBinders.collectFirst {
case (t, reuseBinder) if equivalentTree(t, extractor) => reuseBinder
} match {
case Some(reuseBinder) => normalize >>= Substitution(binder, binderToUniqueTree(reuseBinder))
case None => extractBinders(extractor) = binder
}
case _ =>
}

// find part of substitution that replaces bound symbols by new symbols, and reverse that part
// so that we don't introduce new aliases for existing symbols, thus keeping the set of bound symbols minimal

Expand All @@ -307,7 +323,7 @@ trait MatchApproximation extends TreeAndTypeAnalysis with ScalaLogic with MatchT

val okSubst = Substitution(unboundFrom.toList, unboundTo.toList) // it's important substitution does not duplicate trees here -- it helps to keep hash consing simple, anyway
foreach2(okSubst.from, okSubst.to){(f, t) =>
if (pointsToBound exists (sym => t.exists(_.symbol == sym)))
if (pointsToBound.exists(sym => t.exists(_.symbol == sym)) || tm.isInstanceOf[ExtractorTreeMaker])
pointsToBound += f
}
// debug.patmat("pointsToBound: "+ pointsToBound)
Expand All @@ -328,7 +344,7 @@ trait MatchApproximation extends TreeAndTypeAnalysis with ScalaLogic with MatchT
* TODO: don't ignore outer-checks
*/
def apply(tm: TreeMaker): Prop = {
if (!substitutionComputed) updateSubstitution(tm.subPatternsAsSubstitution)
if (!substitutionComputed) updateSubstitution(tm)

tm match {
case ttm@TypeTestTreeMaker(prevBinder, testedBinder, pt, _) =>
Expand Down
36 changes: 36 additions & 0 deletions test/files/pos/t10502.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
// scalac: -Xfatal-warnings

final class Box[A](val unwrap: A)

object Box {
def apply[A](x: A): Box[A] = new Box[A](x)
def unapply[A](x: Box[A]): Some[A] = Some(x.unwrap)
}

object Perhaps {
def unapply[A](oa: Option[A]): Some[Option[A]] = Some(oa)
}

class Test {
def test() = {
List(Option("hello")) match {
case Perhaps(Some(s)) :: _ =>
case Perhaps(None ) :: _ =>
case Nil =>
}
}

def justOption() = {
Option("hello") match {
case Perhaps(Some(s)) =>
case Perhaps(None) =>
}
}

def boxTest() = {
Box(Option("hello")) match {
case Box(Some(s)) =>
case Box(None) =>
}
}
}

0 comments on commit 789c827

Please sign in to comment.