Permalink
Browse files

no null check for type-tested unapply arg

pattern matching on case classes where pattern
is not known to be a subclass of the unapply's argument type
used to result in code like:

```
if (x1.isInstanceOf[Foo]) {
  val x2 = x1.asInstanceOf[Foo]
  if (x2 != null) { // redundant
    ...
  }
}
```

this wastes byte code on the redundant null check
with this patch, when previous type tests imply
the variable cannot be null, there's no null check
  • Loading branch information...
1 parent 62b37dd commit 71ea3e8278aad030cbe8c9093fe49790a4e419cb @adriaanm adriaanm committed Jan 25, 2013
@@ -409,14 +409,19 @@ trait PatternMatching extends Transform with TypingTransformers with ast.TreeDSL
// example check: List[Int] <:< ::[Int]
// TODO: extractor.paramType may contain unbound type params (run/t2800, run/t3530)
+ // `patBinderOrCasted` is assigned the result of casting `patBinder` to `extractor.paramType`
val (typeTestTreeMaker, patBinderOrCasted, binderKnownNonNull) =
if (needsTypeTest(patBinder.info.widen, extractor.paramType)) {
// chain a type-testing extractor before the actual extractor call
// it tests the type, checks the outer pointer and casts to the expected type
// TODO: the outer check is mandated by the spec for case classes, but we do it for user-defined unapplies as well [SPEC]
// (the prefix of the argument passed to the unapply must equal the prefix of the type of the binder)
val treeMaker = TypeTestTreeMaker(patBinder, patBinder, extractor.paramType, extractor.paramType)(pos, extractorArgTypeTest = true)
- (List(treeMaker), treeMaker.nextBinder, false)
+
+ // check whether typetest implies patBinder is not null,
+ // even though the eventual null check will be on patBinderOrCasted
+ // it'll be equal to patBinder casted to extractor.paramType anyway (and the type test is on patBinder)
+ (List(treeMaker), treeMaker.nextBinder, treeMaker.impliesBinderNonNull(patBinder))
} else {
// no type test needed, but the tree maker relies on `patBinderOrCasted` having type `extractor.paramType` (and not just some type compatible with it)
// SI-6624 shows this is necessary because apparently patBinder may have an unfortunate type (.decls don't have the case field accessors)
@@ -773,11 +778,16 @@ trait PatternMatching extends Transform with TypingTransformers with ast.TreeDSL
def resultType = tpe.finalResultType
def isSeq = extractorCall.symbol.name == nme.unapplySeq
- /** Create the TreeMaker that embodies this extractor call
- *
- * `binder` has been casted to `paramType` if necessary
- * `binderKnownNonNull` is not used in this subclass
- */
+ /** Create the TreeMaker that embodies this extractor call
+ *
+ * `binder` has been casted to `paramType` if necessary
+ * `binderKnownNonNull` is not used in this subclass
+ *
+ * TODO: implement review feedback by @retronym:
+ * Passing the pair of values around suggests:
+ * case class Binder(sym: Symbol, knownNotNull: Boolean).
+ * Perhaps it hasn't reached critical mass, but it would already clean things up a touch.
+ */
def treeMaker(patBinderOrCasted: Symbol, binderKnownNonNull: Boolean, pos: Position): TreeMaker = {
// the extractor call (applied to the binder bound by the flatMap corresponding to the previous (i.e., enclosing/outer) pattern)
val extractorApply = atPos(pos)(spliceApply(patBinderOrCasted))
@@ -1177,6 +1187,17 @@ trait PatternMatching extends Transform with TypingTransformers with ast.TreeDSL
def eqTest(pat: Tree, testedBinder: Symbol): Result = false
def and(a: Result, b: Result): Result = false // we don't and type tests, so the conjunction must include at least one false
}
+
+ def nonNullImpliedByTestChecker(binder: Symbol) = new TypeTestCondStrategy {
+ type Result = Boolean
+
+ def typeTest(testedBinder: Symbol, expectedTp: Type): Result = testedBinder eq binder
+ def outerTest(testedBinder: Symbol, expectedTp: Type): Result = false
+ def nonNullTest(testedBinder: Symbol): Result = testedBinder eq binder
+ def equalsTest(pat: Tree, testedBinder: Symbol): Result = false // could in principle analyse pat and see if it's statically known to be non-null
+ def eqTest(pat: Tree, testedBinder: Symbol): Result = false // could in principle analyse pat and see if it's statically known to be non-null
+ def and(a: Result, b: Result): Result = a || b
+ }
}
/** implements the run-time aspects of (§8.2) (typedPattern has already done the necessary type transformations)
@@ -1260,6 +1281,8 @@ trait PatternMatching extends Transform with TypingTransformers with ast.TreeDSL
// is this purely a type test, e.g. no outer check, no equality tests (used in switch emission)
def isPureTypeTest = renderCondition(pureTypeTestChecker)
+ def impliesBinderNonNull(binder: Symbol) = renderCondition(nonNullImpliedByTestChecker(binder))
+
override def toString = "TT"+(expectedTp, testedBinder.name, nextBinderTp)
}
@@ -0,0 +1 @@
+bytecode identical
@@ -0,0 +1 @@
+-optimize
@@ -0,0 +1,25 @@
+// this class's bytecode, compiled under -optimize is analyzed by the test
+// method a's bytecode should be identical to method b's bytecode
+case class Foo(x: Any)
+
+class SameBytecode {
+ def a =
+ (Foo(1): Any) match {
+ case Foo(_: String) =>
+ }
+
+ // there's no null check
+ def b: Unit = {
+ val x1: Any = Foo(1)
+ if (x1.isInstanceOf[Foo]) {
+ val x3 = x1.asInstanceOf[Foo]
+ if (x3.x.isInstanceOf[String]) {
+ val x4 = x3.x.asInstanceOf[String]
+ val x = ()
+ return
+ }
+ }
+
+ throw new MatchError(x1)
+ }
+}
@@ -0,0 +1,8 @@
+import scala.tools.partest.BytecodeTest
+
+object Test extends BytecodeTest {
+ def show: Unit = {
+ val classNode = loadClassNode("SameBytecode")
+ sameBytecode(getMethod(classNode, "a"), getMethod(classNode, "b"))
+ }
+}
Oops, something went wrong.

0 comments on commit 71ea3e8

Please sign in to comment.