From a07555f015939412c6e4421860abce6e5f90f710 Mon Sep 17 00:00:00 2001 From: Adriaan Moors Date: Wed, 30 Jan 2013 14:57:05 -0800 Subject: bytecode diffing support in ByteCodeTest use sameByteCode(methodNode1, methodNode2) to check methods compile to identical bytecode (line number info is not taken into account) --- src/partest/scala/tools/partest/BytecodeTest.scala | 77 ++++++++++++++++++++-- 1 file changed, 73 insertions(+), 4 deletions(-) (limited to 'src') diff --git a/src/partest/scala/tools/partest/BytecodeTest.scala b/src/partest/scala/tools/partest/BytecodeTest.scala index 93183c2095..1f43ee7d2e 100644 --- a/src/partest/scala/tools/partest/BytecodeTest.scala +++ b/src/partest/scala/tools/partest/BytecodeTest.scala @@ -8,11 +8,11 @@ import asm.tree.{ClassNode, MethodNode, InsnList} import java.io.InputStream /** - * Providies utilities for inspecting bytecode using ASM library. + * Provides utilities for inspecting bytecode using ASM library. * * HOW TO USE * 1. Create subdirectory in test/files/jvm for your test. Let's name it $TESTDIR. - * 2. Create $TESTDIR/BytecodeSrc_1.scala that contains Scala source file which you + * 2. Create $TESTDIR/BytecodeSrc_1.scala that contains Scala source file that you * want to inspect the bytecode for. The '_1' suffix signals to partest that it * should compile this file first. * 3. Create $TESTDIR/Test.scala: @@ -35,11 +35,23 @@ abstract class BytecodeTest { def main(args: Array[String]): Unit = show +// asserts + def sameBytecode(methA: MethodNode, methB: MethodNode) = { + val isa = instructions.fromMethod(methA) + val isb = instructions.fromMethod(methB) + if (isa == isb) println("bytecode identical") + else (isa, isb).zipped.foreach { case (a, b) => + if (a == b) println("OK : "+ a) + else println("DIFF: "+ a +" <=> "+ b) + } + } + +// loading protected def getMethod(classNode: ClassNode, name: String): MethodNode = classNode.methods.asScala.find(_.name == name) getOrElse sys.error(s"Didn't find method '$name' in class '${classNode.name}'") - protected def loadClassNode(name: String): ClassNode = { + protected def loadClassNode(name: String, skipDebugInfo: Boolean = true): ClassNode = { val classBytes: InputStream = (for { classRep <- classpath.findClass(name) binary <- classRep.binary @@ -47,7 +59,7 @@ abstract class BytecodeTest { val cr = new ClassReader(classBytes) val cn = new ClassNode() - cr.accept(cn, 0) + cr.accept(cn, if (skipDebugInfo) ClassReader.SKIP_DEBUG else 0) cn } @@ -58,4 +70,61 @@ abstract class BytecodeTest { val containers = DefaultJavaContext.classesInExpandedPath(Defaults.javaUserClassPath) new JavaClassPath(containers, DefaultJavaContext) } + + // wrap ASM's instructions so we get case class-style `equals` and `toString` + object instructions { + def fromMethod(meth: MethodNode): List[instructions.Instruction] = { + val insns = meth.instructions + val asmToScala = new AsmToScala{ def labelIndex(l: asm.tree.AbstractInsnNode) = insns.indexOf(l) } + + asmToScala.mapOver(insns.iterator.asScala.toList).asInstanceOf[List[instructions.Instruction]] + } + + sealed abstract class Instruction { def opcode: Int } + case class Field (opcode: Int, desc: String, name: String, owner: String) extends Instruction + case class Incr (opcode: Int, incr: Int, `var`: Int) extends Instruction + case class Op (opcode: Int) extends Instruction + case class IntOp (opcode: Int, operand: Int) extends Instruction + case class Jump (opcode: Int, label: Label) extends Instruction + case class Ldc (opcode: Int, cst: Any) extends Instruction + case class LookupSwitch (opcode: Int, dflt: Label, keys: List[Integer], labels: List[Label]) extends Instruction + case class TableSwitch (opcode: Int, dflt: Label, max: Int, min: Int, labels: List[Label]) extends Instruction + case class Method (opcode: Int, desc: String, name: String, owner: String) extends Instruction + case class NewArray (opcode: Int, desc: String, dims: Int) extends Instruction + case class TypeOp (opcode: Int, desc: String) extends Instruction + case class VarOp (opcode: Int, `var`: Int) extends Instruction + case class Label (opcode: Int, offset: Int) extends Instruction + case class FrameEntry (opcode: Int, local: List[Any], stack: List[Any]) extends Instruction + case class LineNumber (opcode: Int, line: Int, start: Label) extends Instruction + } + + abstract class AsmToScala { + import instructions._ + def labelIndex(l: asm.tree.AbstractInsnNode): Int + + def mapOver(is: List[Any]): List[Any] = is map { + case i: asm.tree.AbstractInsnNode => apply(i) + case x => x + } + + def lst[T](xs: java.util.List[T]): List[T] = if (xs == null) Nil else xs.asScala.toList + def apply(l: asm.tree.LabelNode): Label = this(l: asm.tree.AbstractInsnNode).asInstanceOf[Label] + def apply(x: asm.tree.AbstractInsnNode): Instruction = x match { + case i: asm.tree.FieldInsnNode => Field (i.getOpcode: Int, i.desc: String, i.name: String, i.owner: String) + case i: asm.tree.IincInsnNode => Incr (i.getOpcode: Int, i.incr: Int, i.`var`: Int) + case i: asm.tree.InsnNode => Op (i.getOpcode: Int) + case i: asm.tree.IntInsnNode => IntOp (i.getOpcode: Int, i.operand: Int) + case i: asm.tree.JumpInsnNode => Jump (i.getOpcode: Int, this(i.label)) + case i: asm.tree.LdcInsnNode => Ldc (i.getOpcode: Int, i.cst: Any) + case i: asm.tree.LookupSwitchInsnNode => LookupSwitch (i.getOpcode: Int, this(i.dflt), lst(i.keys), mapOver(lst(i.labels)).asInstanceOf[List[Label]]) + case i: asm.tree.TableSwitchInsnNode => TableSwitch (i.getOpcode: Int, this(i.dflt), i.max: Int, i.min: Int, mapOver(lst(i.labels)).asInstanceOf[List[Label]]) + case i: asm.tree.MethodInsnNode => Method (i.getOpcode: Int, i.desc: String, i.name: String, i.owner: String) + case i: asm.tree.MultiANewArrayInsnNode => NewArray (i.getOpcode: Int, i.desc: String, i.dims: Int) + case i: asm.tree.TypeInsnNode => TypeOp (i.getOpcode: Int, i.desc: String) + case i: asm.tree.VarInsnNode => VarOp (i.getOpcode: Int, i.`var`: Int) + case i: asm.tree.LabelNode => Label (i.getOpcode: Int, labelIndex(x)) + case i: asm.tree.FrameNode => FrameEntry (i.getOpcode: Int, mapOver(lst(i.local)), mapOver(lst(i.stack))) + case i: asm.tree.LineNumberNode => LineNumber (i.getOpcode: Int, i.line: Int, this(i.start): Label) + } + } } -- cgit v1.2.3 From 415becdab8aae63470deac5a6e122fa7a697cbe0 Mon Sep 17 00:00:00 2001 From: Adriaan Moors Date: Thu, 31 Jan 2013 10:20:32 -0800 Subject: support testing bytecode similarity in ByteCodeTest one similarity measure comes free of charge: it ignores which variable is stored/loaded, everything else must be identical like this: `similarBytecode(methNodeA, methNodeB, equalsModuloVar)` also implemented prettier diffing --- .../scala/tools/partest/ASMConverters.scala | 71 ++++++++++++++++ src/partest/scala/tools/partest/BytecodeTest.scala | 94 ++++++++-------------- 2 files changed, 104 insertions(+), 61 deletions(-) create mode 100644 src/partest/scala/tools/partest/ASMConverters.scala (limited to 'src') diff --git a/src/partest/scala/tools/partest/ASMConverters.scala b/src/partest/scala/tools/partest/ASMConverters.scala new file mode 100644 index 0000000000..d618e086f4 --- /dev/null +++ b/src/partest/scala/tools/partest/ASMConverters.scala @@ -0,0 +1,71 @@ +package scala.tools.partest + +import scala.collection.JavaConverters._ +import scala.tools.asm +import asm.tree.{ClassNode, MethodNode, InsnList} + +/** Makes using ASM from ByteCodeTests more convenient. + * + * Wraps ASM instructions in case classes so that equals and toString work + * for the purpose of bytecode diffing and pretty printing. + */ +trait ASMConverters { + // wrap ASM's instructions so we get case class-style `equals` and `toString` + object instructions { + def fromMethod(meth: MethodNode): List[Instruction] = { + val insns = meth.instructions + val asmToScala = new AsmToScala{ def labelIndex(l: asm.tree.AbstractInsnNode) = insns.indexOf(l) } + + asmToScala.mapOver(insns.iterator.asScala.toList).asInstanceOf[List[Instruction]] + } + + sealed abstract class Instruction { def opcode: String } + case class Field (opcode: String, desc: String, name: String, owner: String) extends Instruction + case class Incr (opcode: String, incr: Int, `var`: Int) extends Instruction + case class Op (opcode: String) extends Instruction + case class IntOp (opcode: String, operand: Int) extends Instruction + case class Jump (opcode: String, label: Label) extends Instruction + case class Ldc (opcode: String, cst: Any) extends Instruction + case class LookupSwitch (opcode: String, dflt: Label, keys: List[Integer], labels: List[Label]) extends Instruction + case class TableSwitch (opcode: String, dflt: Label, max: Int, min: Int, labels: List[Label]) extends Instruction + case class Method (opcode: String, desc: String, name: String, owner: String) extends Instruction + case class NewArray (opcode: String, desc: String, dims: Int) extends Instruction + case class TypeOp (opcode: String, desc: String) extends Instruction + case class VarOp (opcode: String, `var`: Int) extends Instruction + case class Label (offset: Int) extends Instruction { def opcode: String = "" } + case class FrameEntry (local: List[Any], stack: List[Any]) extends Instruction { def opcode: String = "" } + case class LineNumber (line: Int, start: Label) extends Instruction { def opcode: String = "" } + } + + abstract class AsmToScala { + import instructions._ + + def labelIndex(l: asm.tree.AbstractInsnNode): Int + + def mapOver(is: List[Any]): List[Any] = is map { + case i: asm.tree.AbstractInsnNode => apply(i) + case x => x + } + + def op(i: asm.tree.AbstractInsnNode) = if (asm.util.Printer.OPCODES.isDefinedAt(i.getOpcode)) asm.util.Printer.OPCODES(i.getOpcode) else "?" + def lst[T](xs: java.util.List[T]): List[T] = if (xs == null) Nil else xs.asScala.toList + def apply(l: asm.tree.LabelNode): Label = this(l: asm.tree.AbstractInsnNode).asInstanceOf[Label] + def apply(x: asm.tree.AbstractInsnNode): Instruction = x match { + case i: asm.tree.FieldInsnNode => Field (op(i), i.desc: String, i.name: String, i.owner: String) + case i: asm.tree.IincInsnNode => Incr (op(i), i.incr: Int, i.`var`: Int) + case i: asm.tree.InsnNode => Op (op(i)) + case i: asm.tree.IntInsnNode => IntOp (op(i), i.operand: Int) + case i: asm.tree.JumpInsnNode => Jump (op(i), this(i.label)) + case i: asm.tree.LdcInsnNode => Ldc (op(i), i.cst: Any) + case i: asm.tree.LookupSwitchInsnNode => LookupSwitch (op(i), this(i.dflt), lst(i.keys), mapOver(lst(i.labels)).asInstanceOf[List[Label]]) + case i: asm.tree.TableSwitchInsnNode => TableSwitch (op(i), this(i.dflt), i.max: Int, i.min: Int, mapOver(lst(i.labels)).asInstanceOf[List[Label]]) + case i: asm.tree.MethodInsnNode => Method (op(i), i.desc: String, i.name: String, i.owner: String) + case i: asm.tree.MultiANewArrayInsnNode => NewArray (op(i), i.desc: String, i.dims: Int) + case i: asm.tree.TypeInsnNode => TypeOp (op(i), i.desc: String) + case i: asm.tree.VarInsnNode => VarOp (op(i), i.`var`: Int) + case i: asm.tree.LabelNode => Label (labelIndex(x)) + case i: asm.tree.FrameNode => FrameEntry (mapOver(lst(i.local)), mapOver(lst(i.stack))) + case i: asm.tree.LineNumberNode => LineNumber (i.line: Int, this(i.start): Label) + } + } +} \ No newline at end of file diff --git a/src/partest/scala/tools/partest/BytecodeTest.scala b/src/partest/scala/tools/partest/BytecodeTest.scala index 1f43ee7d2e..41329a8264 100644 --- a/src/partest/scala/tools/partest/BytecodeTest.scala +++ b/src/partest/scala/tools/partest/BytecodeTest.scala @@ -28,7 +28,7 @@ import java.io.InputStream * See test/files/jvm/bytecode-test-example for an example of bytecode test. * */ -abstract class BytecodeTest { +abstract class BytecodeTest extends ASMConverters { /** produce the output to be compared against a checkfile */ protected def show(): Unit @@ -40,9 +40,38 @@ abstract class BytecodeTest { val isa = instructions.fromMethod(methA) val isb = instructions.fromMethod(methB) if (isa == isb) println("bytecode identical") - else (isa, isb).zipped.foreach { case (a, b) => - if (a == b) println("OK : "+ a) - else println("DIFF: "+ a +" <=> "+ b) + else diffInstructions(isa, isb) + } + + import instructions._ + // bytecode is equal modulo local variable numbering + def equalsModuloVar(a: Instruction, b: Instruction) = (a, b) match { + case _ if a == b => true + case (VarOp(op1, _), VarOp(op2, _)) if op1 == op2 => true + case _ => false + } + + def similarBytecode(methA: MethodNode, methB: MethodNode, similar: (Instruction, Instruction) => Boolean) = { + val isa = fromMethod(methA) + val isb = fromMethod(methB) + if (isa == isb) println("bytecode identical") + else if ((isa, isb).zipped.forall { case (a, b) => similar(a, b) }) println("bytecode similar") + else diffInstructions(isa, isb) + } + + def diffInstructions(isa: List[Instruction], isb: List[Instruction]) = { + val len = Math.max(isa.length, isb.length) + if (len > 0 ) { + val width = isa.map(_.toString.length).max + val lineWidth = len.toString.length + (1 to len) foreach { line => + val isaPadded = isa.map(_.toString) orElse Stream.continually("") + val isbPadded = isb.map(_.toString) orElse Stream.continually("") + val a = isaPadded(line-1) + val b = isbPadded(line-1) + + println(s"""$line${" " * (lineWidth-line.toString.length)} ${if (a==b) "==" else "<>"} $a${" " * (width-a.length)} | $b""") + } } } @@ -70,61 +99,4 @@ abstract class BytecodeTest { val containers = DefaultJavaContext.classesInExpandedPath(Defaults.javaUserClassPath) new JavaClassPath(containers, DefaultJavaContext) } - - // wrap ASM's instructions so we get case class-style `equals` and `toString` - object instructions { - def fromMethod(meth: MethodNode): List[instructions.Instruction] = { - val insns = meth.instructions - val asmToScala = new AsmToScala{ def labelIndex(l: asm.tree.AbstractInsnNode) = insns.indexOf(l) } - - asmToScala.mapOver(insns.iterator.asScala.toList).asInstanceOf[List[instructions.Instruction]] - } - - sealed abstract class Instruction { def opcode: Int } - case class Field (opcode: Int, desc: String, name: String, owner: String) extends Instruction - case class Incr (opcode: Int, incr: Int, `var`: Int) extends Instruction - case class Op (opcode: Int) extends Instruction - case class IntOp (opcode: Int, operand: Int) extends Instruction - case class Jump (opcode: Int, label: Label) extends Instruction - case class Ldc (opcode: Int, cst: Any) extends Instruction - case class LookupSwitch (opcode: Int, dflt: Label, keys: List[Integer], labels: List[Label]) extends Instruction - case class TableSwitch (opcode: Int, dflt: Label, max: Int, min: Int, labels: List[Label]) extends Instruction - case class Method (opcode: Int, desc: String, name: String, owner: String) extends Instruction - case class NewArray (opcode: Int, desc: String, dims: Int) extends Instruction - case class TypeOp (opcode: Int, desc: String) extends Instruction - case class VarOp (opcode: Int, `var`: Int) extends Instruction - case class Label (opcode: Int, offset: Int) extends Instruction - case class FrameEntry (opcode: Int, local: List[Any], stack: List[Any]) extends Instruction - case class LineNumber (opcode: Int, line: Int, start: Label) extends Instruction - } - - abstract class AsmToScala { - import instructions._ - def labelIndex(l: asm.tree.AbstractInsnNode): Int - - def mapOver(is: List[Any]): List[Any] = is map { - case i: asm.tree.AbstractInsnNode => apply(i) - case x => x - } - - def lst[T](xs: java.util.List[T]): List[T] = if (xs == null) Nil else xs.asScala.toList - def apply(l: asm.tree.LabelNode): Label = this(l: asm.tree.AbstractInsnNode).asInstanceOf[Label] - def apply(x: asm.tree.AbstractInsnNode): Instruction = x match { - case i: asm.tree.FieldInsnNode => Field (i.getOpcode: Int, i.desc: String, i.name: String, i.owner: String) - case i: asm.tree.IincInsnNode => Incr (i.getOpcode: Int, i.incr: Int, i.`var`: Int) - case i: asm.tree.InsnNode => Op (i.getOpcode: Int) - case i: asm.tree.IntInsnNode => IntOp (i.getOpcode: Int, i.operand: Int) - case i: asm.tree.JumpInsnNode => Jump (i.getOpcode: Int, this(i.label)) - case i: asm.tree.LdcInsnNode => Ldc (i.getOpcode: Int, i.cst: Any) - case i: asm.tree.LookupSwitchInsnNode => LookupSwitch (i.getOpcode: Int, this(i.dflt), lst(i.keys), mapOver(lst(i.labels)).asInstanceOf[List[Label]]) - case i: asm.tree.TableSwitchInsnNode => TableSwitch (i.getOpcode: Int, this(i.dflt), i.max: Int, i.min: Int, mapOver(lst(i.labels)).asInstanceOf[List[Label]]) - case i: asm.tree.MethodInsnNode => Method (i.getOpcode: Int, i.desc: String, i.name: String, i.owner: String) - case i: asm.tree.MultiANewArrayInsnNode => NewArray (i.getOpcode: Int, i.desc: String, i.dims: Int) - case i: asm.tree.TypeInsnNode => TypeOp (i.getOpcode: Int, i.desc: String) - case i: asm.tree.VarInsnNode => VarOp (i.getOpcode: Int, i.`var`: Int) - case i: asm.tree.LabelNode => Label (i.getOpcode: Int, labelIndex(x)) - case i: asm.tree.FrameNode => FrameEntry (i.getOpcode: Int, mapOver(lst(i.local)), mapOver(lst(i.stack))) - case i: asm.tree.LineNumberNode => LineNumber (i.getOpcode: Int, i.line: Int, this(i.start): Label) - } - } } -- cgit v1.2.3 From 62b37dd9a87afd17a67752c6c1b174987817b3e9 Mon Sep 17 00:00:00 2001 From: Adriaan Moors Date: Fri, 25 Jan 2013 10:57:49 -0800 Subject: refactor: prepare null check redundancy analysis this commit doesn't change any behavior --- .../tools/nsc/typechecker/PatternMatching.scala | 51 ++++++++++++++++------ 1 file changed, 38 insertions(+), 13 deletions(-) (limited to 'src') diff --git a/src/compiler/scala/tools/nsc/typechecker/PatternMatching.scala b/src/compiler/scala/tools/nsc/typechecker/PatternMatching.scala index b0745b4c09..9b2898741e 100644 --- a/src/compiler/scala/tools/nsc/typechecker/PatternMatching.scala +++ b/src/compiler/scala/tools/nsc/typechecker/PatternMatching.scala @@ -409,14 +409,14 @@ 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) - val (typeTestTreeMaker, patBinderOrCasted) = + 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) + (List(treeMaker), treeMaker.nextBinder, false) } 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) @@ -426,10 +426,10 @@ trait PatternMatching extends Transform with TypingTransformers with ast.TreeDSL if (settings.developer.value && !(patBinder.info =:= extractor.paramType)) devWarning(s"resetting info of $patBinder: ${patBinder.info} to ${extractor.paramType}") */ - (Nil, patBinder setInfo extractor.paramType) + (Nil, patBinder setInfo extractor.paramType, false) } - withSubPats(typeTestTreeMaker :+ extractor.treeMaker(patBinderOrCasted, pos), extractor.subBindersAndPatterns: _*) + withSubPats(typeTestTreeMaker :+ extractor.treeMaker(patBinderOrCasted, binderKnownNonNull, pos), extractor.subBindersAndPatterns: _*) } @@ -622,8 +622,13 @@ trait PatternMatching extends Transform with TypingTransformers with ast.TreeDSL // to which type should the previous binder be casted? def paramType : Type - // binder has been casted to paramType if necessary - def treeMaker(binder: Symbol, pos: Position): TreeMaker + /** Create the TreeMaker that embodies this extractor call + * + * `binder` has been casted to `paramType` if necessary + * `binderKnownNonNull` indicates whether the cast implies `binder` cannot be null + * when `binderKnownNonNull` is `true`, `ProductExtractorTreeMaker` does not do a (redundant) null check on binder + */ + def treeMaker(binder: Symbol, binderKnownNonNull: Boolean, pos: Position): TreeMaker // `subPatBinders` are the variables bound by this pattern in the following patterns // subPatBinders are replaced by references to the relevant part of the extractor's result (tuple component, seq element, the result as-is) @@ -731,8 +736,13 @@ trait PatternMatching extends Transform with TypingTransformers with ast.TreeDSL def isSeq: Boolean = rawSubPatTypes.nonEmpty && isRepeatedParamType(rawSubPatTypes.last) protected def rawSubPatTypes = constructorTp.paramTypes - // binder has type paramType - def treeMaker(binder: Symbol, pos: Position): TreeMaker = { + /** Create the TreeMaker that embodies this extractor call + * + * `binder` has been casted to `paramType` if necessary + * `binderKnownNonNull` indicates whether the cast implies `binder` cannot be null + * when `binderKnownNonNull` is `true`, `ProductExtractorTreeMaker` does not do a (redundant) null check on binder + */ + def treeMaker(binder: Symbol, binderKnownNonNull: Boolean, pos: Position): TreeMaker = { val paramAccessors = binder.constrParamAccessors // binders corresponding to mutable fields should be stored (SI-5158, SI-6070) val mutableBinders = @@ -741,7 +751,7 @@ trait PatternMatching extends Transform with TypingTransformers with ast.TreeDSL else Nil // checks binder ne null before chaining to the next extractor - ProductExtractorTreeMaker(binder, lengthGuard(binder))(subPatBinders, subPatRefs(binder), mutableBinders) + ProductExtractorTreeMaker(binder, lengthGuard(binder))(subPatBinders, subPatRefs(binder), mutableBinders, binderKnownNonNull) } // reference the (i-1)th case accessor if it exists, otherwise the (i-1)th tuple component @@ -763,7 +773,12 @@ trait PatternMatching extends Transform with TypingTransformers with ast.TreeDSL def resultType = tpe.finalResultType def isSeq = extractorCall.symbol.name == nme.unapplySeq - def treeMaker(patBinderOrCasted: Symbol, pos: Position): TreeMaker = { + /** Create the TreeMaker that embodies this extractor call + * + * `binder` has been casted to `paramType` if necessary + * `binderKnownNonNull` is not used in this subclass + */ + 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)) val binder = freshSym(pos, pureType(resultInMonad)) // can't simplify this when subPatBinders.isEmpty, since UnitClass.tpe is definitely wrong when isSeq, and resultInMonad should always be correct since it comes directly from the extractor's result type @@ -1081,7 +1096,8 @@ trait PatternMatching extends Transform with TypingTransformers with ast.TreeDSL case class ProductExtractorTreeMaker(prevBinder: Symbol, extraCond: Option[Tree])( val subPatBinders: List[Symbol], val subPatRefs: List[Tree], - val mutableBinders: List[Symbol]) extends FunTreeMaker with PreserveSubPatBinders { + val mutableBinders: List[Symbol], + binderKnownNonNull: Boolean) extends FunTreeMaker with PreserveSubPatBinders { import CODE._ val nextBinder = prevBinder // just passing through @@ -1092,8 +1108,17 @@ trait PatternMatching extends Transform with TypingTransformers with ast.TreeDSL def chainBefore(next: Tree)(casegen: Casegen): Tree = { val nullCheck = REF(prevBinder) OBJ_NE NULL - val cond = extraCond map (nullCheck AND _) getOrElse nullCheck - casegen.ifThenElseZero(cond, bindSubPats(substitution(next))) + val cond = + if (binderKnownNonNull) extraCond + else (extraCond map (nullCheck AND _) + orElse Some(nullCheck)) + + cond match { + case Some(cond) => + casegen.ifThenElseZero(cond, bindSubPats(substitution(next))) + case _ => + bindSubPats(substitution(next)) + } } override def toString = "P"+(prevBinder.name, extraCond getOrElse "", localSubstitution) -- cgit v1.2.3 From 71ea3e8278aad030cbe8c9093fe49790a4e419cb Mon Sep 17 00:00:00 2001 From: Adriaan Moors Date: Fri, 25 Jan 2013 14:16:27 -0800 Subject: 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 --- .../tools/nsc/typechecker/PatternMatching.scala | 35 +++- test/files/jvm/patmat_opt_no_nullcheck.check | 1 + test/files/jvm/patmat_opt_no_nullcheck.flags | 1 + .../jvm/patmat_opt_no_nullcheck/Analyzed_1.scala | 25 +++ test/files/jvm/patmat_opt_no_nullcheck/test.scala | 8 + test/files/run/inline-ex-handlers.check | 206 ++++++++++----------- 6 files changed, 167 insertions(+), 109 deletions(-) create mode 100644 test/files/jvm/patmat_opt_no_nullcheck.check create mode 100644 test/files/jvm/patmat_opt_no_nullcheck.flags create mode 100644 test/files/jvm/patmat_opt_no_nullcheck/Analyzed_1.scala create mode 100644 test/files/jvm/patmat_opt_no_nullcheck/test.scala (limited to 'src') diff --git a/src/compiler/scala/tools/nsc/typechecker/PatternMatching.scala b/src/compiler/scala/tools/nsc/typechecker/PatternMatching.scala index 9b2898741e..f32ca4bd8e 100644 --- a/src/compiler/scala/tools/nsc/typechecker/PatternMatching.scala +++ b/src/compiler/scala/tools/nsc/typechecker/PatternMatching.scala @@ -409,6 +409,7 @@ 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 @@ -416,7 +417,11 @@ trait PatternMatching extends Transform with TypingTransformers with ast.TreeDSL // 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) } diff --git a/test/files/jvm/patmat_opt_no_nullcheck.check b/test/files/jvm/patmat_opt_no_nullcheck.check new file mode 100644 index 0000000000..43f53aba12 --- /dev/null +++ b/test/files/jvm/patmat_opt_no_nullcheck.check @@ -0,0 +1 @@ +bytecode identical diff --git a/test/files/jvm/patmat_opt_no_nullcheck.flags b/test/files/jvm/patmat_opt_no_nullcheck.flags new file mode 100644 index 0000000000..1182725e86 --- /dev/null +++ b/test/files/jvm/patmat_opt_no_nullcheck.flags @@ -0,0 +1 @@ +-optimize \ No newline at end of file diff --git a/test/files/jvm/patmat_opt_no_nullcheck/Analyzed_1.scala b/test/files/jvm/patmat_opt_no_nullcheck/Analyzed_1.scala new file mode 100644 index 0000000000..1594eb523c --- /dev/null +++ b/test/files/jvm/patmat_opt_no_nullcheck/Analyzed_1.scala @@ -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) + } +} \ No newline at end of file diff --git a/test/files/jvm/patmat_opt_no_nullcheck/test.scala b/test/files/jvm/patmat_opt_no_nullcheck/test.scala new file mode 100644 index 0000000000..2927e763d5 --- /dev/null +++ b/test/files/jvm/patmat_opt_no_nullcheck/test.scala @@ -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")) + } +} diff --git a/test/files/run/inline-ex-handlers.check b/test/files/run/inline-ex-handlers.check index 282542a732..905dfc3ee7 100644 --- a/test/files/run/inline-ex-handlers.check +++ b/test/files/run/inline-ex-handlers.check @@ -26,54 +26,55 @@ --- > locals: value args, variable result, value ex6, value x4, value x5, value x 397c393 -< blocks: [1,2,3,4,5,8,11,13,14,16] +< blocks: [1,2,3,4,5,8,10,11,13] --- -> blocks: [1,2,3,5,8,11,13,14,16,17] +> blocks: [1,2,3,5,8,10,11,13,14] 421c417,426 < 103 THROW(MyException) --- > ? STORE_LOCAL(value ex6) -> ? JUMP 17 +> ? JUMP 14 > -> 17: +> 14: > 101 LOAD_LOCAL(value ex6) > 101 STORE_LOCAL(value x4) > 101 SCOPE_ENTER value x4 > 106 LOAD_LOCAL(value x4) > 106 IS_INSTANCE REF(class MyException) -> 106 CZJUMP (BOOL)NE ? 5 : 11 +> 106 CZJUMP (BOOL)NE ? 5 : 8 434,436d438 < 101 JUMP 4 < < 4: -450,453d451 +446,449d447 < 106 LOAD_LOCAL(value x5) < 106 CALL_METHOD MyException.message (dynamic) < 106 STORE_LOCAL(value message) < 106 SCOPE_ENTER value message -455c453,454 +451c449,450 < 106 LOAD_LOCAL(value message) --- > ? LOAD_LOCAL(value x5) > 106 CALL_METHOD MyException.message (dynamic) -527c526 +523c522 < blocks: [1,2,3,4,6,7,8,9,10] --- > blocks: [1,2,3,4,6,7,8,9,10,11,12,13] -556c555,560 +552c551 < 306 THROW(MyException) --- > ? JUMP 11 -> +553a553,557 > 11: > ? LOAD_LOCAL(variable monitor4) > 305 MONITOR_EXIT > ? JUMP 12 -562c566 +> +558c562 < ? THROW(Throwable) --- > ? JUMP 12 -568c572,579 +564c568,575 < ? THROW(Throwable) --- > ? STORE_LOCAL(value t) @@ -84,7 +85,7 @@ > 304 MONITOR_EXIT > ? STORE_LOCAL(value t) > ? JUMP 13 -583a595,606 +579a591,602 > 13: > 310 LOAD_MODULE object Predef > 310 CALL_PRIMITIVE(StartConcat) @@ -97,35 +98,35 @@ > 310 CALL_METHOD scala.Predef.println (dynamic) > 310 JUMP 2 > -592c615 +588c611 < catch (Throwable) in ArrayBuffer(7, 8, 9, 10) starting at: 6 --- > catch (Throwable) in ArrayBuffer(7, 8, 9, 10, 11) starting at: 6 -595c618 +591c614 < catch (Throwable) in ArrayBuffer(4, 6, 7, 8, 9, 10) starting at: 3 --- > catch (Throwable) in ArrayBuffer(4, 6, 7, 8, 9, 10, 11, 12) starting at: 3 -627c650 +623c646 < blocks: [1,2,3,4,5,6,7,9,10] --- > blocks: [1,2,3,4,5,6,7,9,10,11,12] -651c674,675 +647c670,671 < 78 THROW(IllegalArgumentException) --- > ? STORE_LOCAL(value e) > ? JUMP 11 -652a677,681 +648a673,677 > 11: > 81 LOAD_LOCAL(value e) > ? STORE_LOCAL(variable exc1) > ? JUMP 12 > -680c709,710 +676c705,706 < 81 THROW(Exception) --- > ? STORE_LOCAL(variable exc1) > ? JUMP 12 -696a727,739 +692a723,735 > 12: > 83 LOAD_MODULE object Predef > 83 CONSTANT("finally") @@ -139,88 +140,88 @@ > 84 LOAD_LOCAL(variable exc1) > 84 THROW(Throwable) > -702c745 +698c741 < catch () in ArrayBuffer(4, 6, 7, 9) starting at: 3 --- > catch () in ArrayBuffer(4, 6, 7, 9, 11) starting at: 3 -726c769 +722c765 < locals: value args, variable result, value ex6, variable exc2, value x4, value x5, value message, value x, value ex6, value x4, value x5, value message, value x --- > locals: value args, variable result, value ex6, variable exc2, value x4, value x5, value x, value ex6, value x4, value x5, value x -728c771 -< blocks: [1,2,3,4,5,6,9,12,14,17,18,19,22,25,27,28,30,31] +724c767 +< blocks: [1,2,3,4,5,6,9,11,14,15,16,19,21,22,24,25] --- -> blocks: [1,2,3,4,5,6,9,12,14,17,18,19,22,25,27,28,30,31,32,33,34] -752c795,802 +> blocks: [1,2,3,4,5,6,9,11,14,15,16,19,21,22,24,25,26,27,28] +748c791,798 < 172 THROW(MyException) --- > ? STORE_LOCAL(value ex6) -> ? JUMP 32 +> ? JUMP 26 > -> 32: +> 26: > 170 LOAD_LOCAL(value ex6) > 170 STORE_LOCAL(value x4) > 170 SCOPE_ENTER value x4 -> 170 JUMP 18 -799,802d848 +> 170 JUMP 15 +791,794d840 < 175 LOAD_LOCAL(value x5) < 175 CALL_METHOD MyException.message (dynamic) < 175 STORE_LOCAL(value message) < 175 SCOPE_ENTER value message -804c850,851 +796c842,843 < 176 LOAD_LOCAL(value message) --- > ? LOAD_LOCAL(value x5) > 176 CALL_METHOD MyException.message (dynamic) -808c855,856 +800c847,848 < 177 LOAD_LOCAL(value message) --- > ? LOAD_LOCAL(value x5) > 177 CALL_METHOD MyException.message (dynamic) -810c858,859 +802c850,851 < 177 THROW(MyException) --- > ? STORE_LOCAL(value ex6) -> ? JUMP 33 -814c863,864 +> ? JUMP 27 +806c855,856 < 170 THROW(Throwable) --- > ? STORE_LOCAL(value ex6) -> ? JUMP 33 -823a874,879 -> 33: +> ? JUMP 27 +815a866,871 +> 27: > 169 LOAD_LOCAL(value ex6) > 169 STORE_LOCAL(value x4) > 169 SCOPE_ENTER value x4 > 169 JUMP 5 > -838,841d893 +826,829d881 < 180 LOAD_LOCAL(value x5) < 180 CALL_METHOD MyException.message (dynamic) < 180 STORE_LOCAL(value message) < 180 SCOPE_ENTER value message -843c895,896 +831c883,884 < 181 LOAD_LOCAL(value message) --- > ? LOAD_LOCAL(value x5) > 181 CALL_METHOD MyException.message (dynamic) -847c900,901 +835c888,889 < 182 LOAD_LOCAL(value message) --- > ? LOAD_LOCAL(value x5) > 182 CALL_METHOD MyException.message (dynamic) -849c903,904 +837c891,892 < 182 THROW(MyException) --- > ? STORE_LOCAL(variable exc2) -> ? JUMP 34 -853c908,909 +> ? JUMP 28 +841c896,897 < 169 THROW(Throwable) --- > ? STORE_LOCAL(variable exc2) -> ? JUMP 34 -869a926,938 -> 34: +> ? JUMP 28 +857a914,926 +> 28: > 184 LOAD_MODULE object Predef > 184 CONSTANT("finally") > 184 CALL_METHOD scala.Predef.println (dynamic) @@ -233,159 +234,158 @@ > 185 LOAD_LOCAL(variable exc2) > 185 THROW(Throwable) > -875c944 -< catch (Throwable) in ArrayBuffer(17, 18, 19, 22, 25, 27, 28, 30) starting at: 4 +863c932 +< catch (Throwable) in ArrayBuffer(14, 15, 16, 19, 21, 22, 24) starting at: 4 --- -> catch (Throwable) in ArrayBuffer(17, 18, 19, 22, 25, 27, 28, 30, 32) starting at: 4 -878c947 -< catch () in ArrayBuffer(4, 5, 6, 9, 12, 17, 18, 19, 22, 25, 27, 28, 30) starting at: 3 +> catch (Throwable) in ArrayBuffer(14, 15, 16, 19, 21, 22, 24, 26) starting at: 4 +866c935 +< catch () in ArrayBuffer(4, 5, 6, 9, 14, 15, 16, 19, 21, 22, 24) starting at: 3 --- -> catch () in ArrayBuffer(4, 5, 6, 9, 12, 17, 18, 19, 22, 25, 27, 28, 30, 32, 33) starting at: 3 -902c971 +> catch () in ArrayBuffer(4, 5, 6, 9, 14, 15, 16, 19, 21, 22, 24, 26, 27) starting at: 3 +890c959 < locals: value args, variable result, value e, value ex6, value x4, value x5, value message, value x --- > locals: value args, variable result, value e, value ex6, value x4, value x5, value x -904c973 -< blocks: [1,2,3,6,7,8,11,14,16,17,19] +892c961 +< blocks: [1,2,3,6,7,8,11,13,14,16] --- -> blocks: [1,2,3,6,7,8,11,14,16,17,19,20] -928c997,1004 +> blocks: [1,2,3,6,7,8,11,13,14,16,17] +916c985,992 < 124 THROW(MyException) --- > ? STORE_LOCAL(value ex6) -> ? JUMP 20 +> ? JUMP 17 > -> 20: +> 17: > 122 LOAD_LOCAL(value ex6) > 122 STORE_LOCAL(value x4) > 122 SCOPE_ENTER value x4 > 122 JUMP 7 -957,960d1032 +941,944d1016 < 127 LOAD_LOCAL(value x5) < 127 CALL_METHOD MyException.message (dynamic) < 127 STORE_LOCAL(value message) < 127 SCOPE_ENTER value message -962c1034,1035 +946c1018,1019 < 127 LOAD_LOCAL(value message) --- > ? LOAD_LOCAL(value x5) > 127 CALL_METHOD MyException.message (dynamic) -991c1064 -< catch (IllegalArgumentException) in ArrayBuffer(6, 7, 8, 11, 14, 16, 17, 19) starting at: 3 +975c1048 +< catch (IllegalArgumentException) in ArrayBuffer(6, 7, 8, 11, 13, 14, 16) starting at: 3 --- -> catch (IllegalArgumentException) in ArrayBuffer(6, 7, 8, 11, 14, 16, 17, 19, 20) starting at: 3 -1015c1088 +> catch (IllegalArgumentException) in ArrayBuffer(6, 7, 8, 11, 13, 14, 16, 17) starting at: 3 +999c1072 < locals: value args, variable result, value ex6, value x4, value x5, value message, value x, value e --- > locals: value args, variable result, value ex6, value x4, value x5, value x, value e -1017c1090 -< blocks: [1,2,3,4,5,8,11,15,16,17,19] +1001c1074 +< blocks: [1,2,3,4,5,8,12,13,14,16] --- -> blocks: [1,2,3,5,8,11,15,16,17,19,20] -1041c1114,1123 +> blocks: [1,2,3,5,8,12,13,14,16,17] +1025c1098,1107 < 148 THROW(MyException) --- > ? STORE_LOCAL(value ex6) -> ? JUMP 20 +> ? JUMP 17 > -> 20: +> 17: > 145 LOAD_LOCAL(value ex6) > 145 STORE_LOCAL(value x4) > 145 SCOPE_ENTER value x4 > 154 LOAD_LOCAL(value x4) > 154 IS_INSTANCE REF(class MyException) -> 154 CZJUMP (BOOL)NE ? 5 : 11 -1062,1064d1143 +> 154 CZJUMP (BOOL)NE ? 5 : 8 +1046,1048d1127 < 145 JUMP 4 < < 4: -1078,1081d1156 +1058,1061d1136 < 154 LOAD_LOCAL(value x5) < 154 CALL_METHOD MyException.message (dynamic) < 154 STORE_LOCAL(value message) < 154 SCOPE_ENTER value message -1083c1158,1159 +1063c1138,1139 < 154 LOAD_LOCAL(value message) --- > ? LOAD_LOCAL(value x5) > 154 CALL_METHOD MyException.message (dynamic) -1300c1376 +1280c1356 < blocks: [1,2,3,4,5,7] --- > blocks: [1,2,3,4,5,7,8] -1324c1400,1401 +1304c1380,1387 < 38 THROW(IllegalArgumentException) --- > ? STORE_LOCAL(value e) > ? JUMP 8 -1325a1403,1408 +> > 8: > 42 LOAD_MODULE object Predef > 42 CONSTANT("IllegalArgumentException") > 42 CALL_METHOD scala.Predef.println (dynamic) > 42 JUMP 2 -> -1371c1454 +1351c1434 < locals: value args, variable result, value ex6, value x4, value x5, value message, value x --- > locals: value args, variable result, value ex6, value x4, value x5, value x -1373c1456 -< blocks: [1,2,3,4,5,8,11,13,14,16,17,19] +1353c1436 +< blocks: [1,2,3,4,5,8,10,11,13,14,16] --- -> blocks: [1,2,3,5,8,11,13,14,16,17,19,20] -1397c1480,1481 +> blocks: [1,2,3,5,8,10,11,13,14,16,17] +1377c1460,1461 < 203 THROW(MyException) --- > ? STORE_LOCAL(value ex6) -> ? JUMP 20 -1417c1501,1510 +> ? JUMP 17 +1397c1481,1490 < 209 THROW(MyException) --- > ? STORE_LOCAL(value ex6) -> ? JUMP 20 +> ? JUMP 17 > -> 20: +> 17: > 200 LOAD_LOCAL(value ex6) > 200 STORE_LOCAL(value x4) > 200 SCOPE_ENTER value x4 > 212 LOAD_LOCAL(value x4) > 212 IS_INSTANCE REF(class MyException) -> 212 CZJUMP (BOOL)NE ? 5 : 11 -1430,1432d1522 +> 212 CZJUMP (BOOL)NE ? 5 : 8 +1410,1412d1502 < 200 JUMP 4 < < 4: -1446,1449d1535 +1422,1425d1511 < 212 LOAD_LOCAL(value x5) < 212 CALL_METHOD MyException.message (dynamic) < 212 STORE_LOCAL(value message) < 212 SCOPE_ENTER value message -1451c1537,1538 +1427c1513,1514 < 213 LOAD_LOCAL(value message) --- > ? LOAD_LOCAL(value x5) > 213 CALL_METHOD MyException.message (dynamic) -1495c1582 +1471c1558 < blocks: [1,2,3,4,5,7] --- > blocks: [1,2,3,4,5,7,8] -1519c1606,1607 +1495c1582,1583 < 58 THROW(IllegalArgumentException) --- > ? STORE_LOCAL(value e) > ? JUMP 8 -1520a1609,1614 +1496a1585,1590 > 8: > 62 LOAD_MODULE object Predef > 62 CONSTANT("RuntimeException") > 62 CALL_METHOD scala.Predef.println (dynamic) > 62 JUMP 2 > -1568c1662 +1544c1638 < blocks: [1,2,3,4] --- > blocks: [1,2,3,4,5] -1588c1682,1687 +1564c1658,1663 < 229 THROW(MyException) --- > ? JUMP 5 @@ -394,19 +394,19 @@ > ? LOAD_LOCAL(variable monitor1) > 228 MONITOR_EXIT > 228 THROW(Throwable) -1594c1693 +1570c1669 < ? THROW(Throwable) --- > 228 THROW(Throwable) -1622c1721 +1598c1697 < locals: value args, variable result, variable monitor2, variable monitorResult1 --- > locals: value exception$1, value args, variable result, variable monitor2, variable monitorResult1 -1624c1723 +1600c1699 < blocks: [1,2,3,4] --- > blocks: [1,2,3,4,5] -1647c1746,1754 +1623c1722,1730 < 245 THROW(MyException) --- > ? STORE_LOCAL(value exception$1) @@ -418,7 +418,7 @@ > ? LOAD_LOCAL(variable monitor2) > 244 MONITOR_EXIT > 244 THROW(Throwable) -1653c1760 +1629c1736 < ? THROW(Throwable) --- > 244 THROW(Throwable) -- cgit v1.2.3 From 494ba94518c9b40b7bf600ec7b70561842375597 Mon Sep 17 00:00:00 2001 From: Adriaan Moors Date: Tue, 29 Jan 2013 00:13:14 -0800 Subject: don't store subpats bound to underscore also, tweak fix in place for SI-5158 to appease SI-6941 don't store mutable fields from scala.* as we can assume these classes are well-behaved and do not mutate their case class fields --- .../tools/nsc/typechecker/PatternMatching.scala | 64 ++++++++++++++++++---- test/files/jvm/patmat_opt_ignore_underscore.check | 1 + test/files/jvm/patmat_opt_ignore_underscore.flags | 1 + .../patmat_opt_ignore_underscore/Analyzed_1.scala | 30 ++++++++++ .../jvm/patmat_opt_ignore_underscore/test.scala | 15 +++++ test/files/run/t6288.check | 10 +--- 6 files changed, 102 insertions(+), 19 deletions(-) create mode 100644 test/files/jvm/patmat_opt_ignore_underscore.check create mode 100644 test/files/jvm/patmat_opt_ignore_underscore.flags create mode 100644 test/files/jvm/patmat_opt_ignore_underscore/Analyzed_1.scala create mode 100644 test/files/jvm/patmat_opt_ignore_underscore/test.scala (limited to 'src') diff --git a/src/compiler/scala/tools/nsc/typechecker/PatternMatching.scala b/src/compiler/scala/tools/nsc/typechecker/PatternMatching.scala index f32ca4bd8e..8ae2e0a3b8 100644 --- a/src/compiler/scala/tools/nsc/typechecker/PatternMatching.scala +++ b/src/compiler/scala/tools/nsc/typechecker/PatternMatching.scala @@ -647,6 +647,11 @@ trait PatternMatching extends Transform with TypingTransformers with ast.TreeDSL case bp => bp } + // never store these in local variables (for PreserveSubPatBinders) + lazy val ignoredSubPatBinders = (subPatBinders zip args).collect{ + case (b, PatternBoundToUnderscore()) => b + }.toSet + def subPatTypes: List[Type] = if(isSeq) { val TypeRef(pre, SeqClass, args) = seqTp @@ -750,13 +755,16 @@ trait PatternMatching extends Transform with TypingTransformers with ast.TreeDSL def treeMaker(binder: Symbol, binderKnownNonNull: Boolean, pos: Position): TreeMaker = { val paramAccessors = binder.constrParamAccessors // binders corresponding to mutable fields should be stored (SI-5158, SI-6070) + // make an exception for classes under the scala package as they should be well-behaved, + // to optimize matching on List val mutableBinders = - if (paramAccessors exists (_.isMutable)) + if (!binder.info.typeSymbol.hasTransOwner(ScalaPackageClass) && + (paramAccessors exists (_.isMutable))) subPatBinders.zipWithIndex.collect{ case (binder, idx) if paramAccessors(idx).isMutable => binder } else Nil // checks binder ne null before chaining to the next extractor - ProductExtractorTreeMaker(binder, lengthGuard(binder))(subPatBinders, subPatRefs(binder), mutableBinders, binderKnownNonNull) + ProductExtractorTreeMaker(binder, lengthGuard(binder))(subPatBinders, subPatRefs(binder), mutableBinders, binderKnownNonNull, ignoredSubPatBinders) } // reference the (i-1)th case accessor if it exists, otherwise the (i-1)th tuple component @@ -792,7 +800,7 @@ trait PatternMatching extends Transform with TypingTransformers with ast.TreeDSL // 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)) val binder = freshSym(pos, pureType(resultInMonad)) // can't simplify this when subPatBinders.isEmpty, since UnitClass.tpe is definitely wrong when isSeq, and resultInMonad should always be correct since it comes directly from the extractor's result type - ExtractorTreeMaker(extractorApply, lengthGuard(binder), binder)(subPatBinders, subPatRefs(binder), resultType.typeSymbol == BooleanClass, checkedLength, patBinderOrCasted) + ExtractorTreeMaker(extractorApply, lengthGuard(binder), binder)(subPatBinders, subPatRefs(binder), resultType.typeSymbol == BooleanClass, checkedLength, patBinderOrCasted, ignoredSubPatBinders) } override protected def seqTree(binder: Symbol): Tree = @@ -849,6 +857,16 @@ trait PatternMatching extends Transform with TypingTransformers with ast.TreeDSL } } + object PatternBoundToUnderscore { + def unapply(pat: Tree): Boolean = pat match { + case Bind(nme.WILDCARD, _) => true // don't skip when binding an interesting symbol! + case Ident(nme.WILDCARD) => true + case Alternative(ps) => ps forall (PatternBoundToUnderscore.unapply(_)) + case Typed(PatternBoundToUnderscore(), _) => true + case _ => false + } + } + object Bound { def unapply(t: Tree): Option[(Symbol, Tree)] = t match { case t@Bind(n, p) if (t.symbol ne null) && (t.symbol ne NoSymbol) => // pos/t2429 does not satisfy these conditions @@ -1016,10 +1034,17 @@ trait PatternMatching extends Transform with TypingTransformers with ast.TreeDSL trait PreserveSubPatBinders extends TreeMaker { val subPatBinders: List[Symbol] val subPatRefs: List[Tree] + val ignoredSubPatBinders: Set[Symbol] // unless `debugInfoEmitVars`, this set should contain the bare minimum for correctness // mutable case class fields need to be stored regardless (SI-5158, SI-6070) -- see override in ProductExtractorTreeMaker - def storedBinders: Set[Symbol] = if (debugInfoEmitVars) subPatBinders.toSet else Set.empty + // sub patterns bound to wildcard (_) are never stored as they can't be referenced + // dirty debuggers will have to get dirty to see the wildcards + lazy val storedBinders: Set[Symbol] = + (if (debugInfoEmitVars) subPatBinders.toSet else Set.empty) ++ extraStoredBinders -- ignoredSubPatBinders + + // e.g., mutable fields of a case class in ProductExtractorTreeMaker + def extraStoredBinders: Set[Symbol] def emitVars = storedBinders.nonEmpty @@ -1040,10 +1065,22 @@ trait PatternMatching extends Transform with TypingTransformers with ast.TreeDSL Substitution(subPatBinders, subPatRefs) >> super.subPatternsAsSubstitution import CODE._ - def bindSubPats(in: Tree): Tree = if (!emitVars) in + def bindSubPats(in: Tree): Tree = + if (!emitVars) in else { - val (subPatBindersStored, subPatRefsStored) = stored.unzip - Block(map2(subPatBindersStored.toList, subPatRefsStored.toList)(VAL(_) === _), in) + // binders in `subPatBindersStored` that are referenced by tree `in` + val usedBinders = new collection.mutable.HashSet[Symbol]() + // all potentially stored subpat binders + val potentiallyStoredBinders = stored.unzip._1.toSet + // compute intersection of all symbols in the tree `in` and all potentially stored subpat binders + in.foreach(t => if (potentiallyStoredBinders(t.symbol)) usedBinders += t.symbol) + + if (usedBinders.isEmpty) in + else { + // only store binders actually used + val (subPatBindersStored, subPatRefsStored) = stored.filter{case (b, _) => usedBinders(b)}.unzip + Block(map2(subPatBindersStored.toList, subPatRefsStored.toList)(VAL(_) === _), in) + } } } @@ -1063,7 +1100,11 @@ trait PatternMatching extends Transform with TypingTransformers with ast.TreeDSL val subPatRefs: List[Tree], extractorReturnsBoolean: Boolean, val checkedLength: Option[Int], - val prevBinder: Symbol) extends FunTreeMaker with PreserveSubPatBinders { + val prevBinder: Symbol, + val ignoredSubPatBinders: Set[Symbol] + ) extends FunTreeMaker with PreserveSubPatBinders { + + def extraStoredBinders: Set[Symbol] = Set() def chainBefore(next: Tree)(casegen: Casegen): Tree = { val condAndNext = extraCond match { @@ -1107,14 +1148,15 @@ trait PatternMatching extends Transform with TypingTransformers with ast.TreeDSL val subPatBinders: List[Symbol], val subPatRefs: List[Tree], val mutableBinders: List[Symbol], - binderKnownNonNull: Boolean) extends FunTreeMaker with PreserveSubPatBinders { + binderKnownNonNull: Boolean, + val ignoredSubPatBinders: Set[Symbol] + ) extends FunTreeMaker with PreserveSubPatBinders { import CODE._ val nextBinder = prevBinder // just passing through // mutable binders must be stored to avoid unsoundness or seeing mutation of fields after matching (SI-5158, SI-6070) - // (the implementation could be optimized by duplicating code from `super.storedBinders`, but this seems more elegant) - override def storedBinders: Set[Symbol] = super.storedBinders ++ mutableBinders.toSet + def extraStoredBinders: Set[Symbol] = mutableBinders.toSet def chainBefore(next: Tree)(casegen: Casegen): Tree = { val nullCheck = REF(prevBinder) OBJ_NE NULL diff --git a/test/files/jvm/patmat_opt_ignore_underscore.check b/test/files/jvm/patmat_opt_ignore_underscore.check new file mode 100644 index 0000000000..43f53aba12 --- /dev/null +++ b/test/files/jvm/patmat_opt_ignore_underscore.check @@ -0,0 +1 @@ +bytecode identical diff --git a/test/files/jvm/patmat_opt_ignore_underscore.flags b/test/files/jvm/patmat_opt_ignore_underscore.flags new file mode 100644 index 0000000000..1182725e86 --- /dev/null +++ b/test/files/jvm/patmat_opt_ignore_underscore.flags @@ -0,0 +1 @@ +-optimize \ No newline at end of file diff --git a/test/files/jvm/patmat_opt_ignore_underscore/Analyzed_1.scala b/test/files/jvm/patmat_opt_ignore_underscore/Analyzed_1.scala new file mode 100644 index 0000000000..ac59e41ba7 --- /dev/null +++ b/test/files/jvm/patmat_opt_ignore_underscore/Analyzed_1.scala @@ -0,0 +1,30 @@ +// this class's bytecode, compiled under -optimize is analyzed by the test +// method a's bytecode should be identical to method b's bytecode +// this is not the best test for shielding against regressing on this particular issue, +// but it sets the stage for checking the bytecode emitted by the pattern matcher and +// comparing it to manually tuned code using if/then/else etc. +class SameBytecode { + case class Foo(x: Any, y: String) + + def a = + Foo(1, "a") match { + case Foo(_: String, y) => y + } + + // this method's body holds the tree that should be generated by the pattern matcher for method a (-Xprint:patmat) + // the test checks that bytecode for a and b is identical (modulo line numbers) + // we can't diff trees as they are quite different (patmat uses jumps to labels that cannot be expressed in source, for example) + // note that the actual tree is quite bad: we do an unnecessary null check, isInstanceOf and local val (x3) + // some of these will be fixed soon (the initial null check is for the scrutinee, which is harder to fix in patmat) + def b: String = { + val x1 = Foo(1, "a") + if (x1.ne(null)) { + if (x1.x.isInstanceOf[String]) { + val x3 = x1.x.asInstanceOf[String] + return x1.y + } + } + + throw new MatchError(x1) + } +} \ No newline at end of file diff --git a/test/files/jvm/patmat_opt_ignore_underscore/test.scala b/test/files/jvm/patmat_opt_ignore_underscore/test.scala new file mode 100644 index 0000000000..6179101a7e --- /dev/null +++ b/test/files/jvm/patmat_opt_ignore_underscore/test.scala @@ -0,0 +1,15 @@ +import scala.tools.partest.BytecodeTest + +import scala.tools.nsc.util.JavaClassPath +import java.io.InputStream +import scala.tools.asm +import asm.ClassReader +import asm.tree.{ClassNode, InsnList} +import scala.collection.JavaConverters._ + +object Test extends BytecodeTest { + def show: Unit = { + val classNode = loadClassNode("SameBytecode") + sameBytecode(getMethod(classNode, "a"), getMethod(classNode, "b")) + } +} diff --git a/test/files/run/t6288.check b/test/files/run/t6288.check index af6bd5d269..4895c2c007 100644 --- a/test/files/run/t6288.check +++ b/test/files/run/t6288.check @@ -11,10 +11,7 @@ [64]case5()[84]{ [84] val o7: [84]Option[Int] = [84][84]Case3.unapply([84]x1); [84]if ([84]o7.isEmpty.unary_!) - [84]{ - [90]val nr: [90]Int = [90]o7.get; - [97][97]matchEnd4([97]()) - } + [97][97]matchEnd4([97]()) else [84][84]case6() }; @@ -38,10 +35,7 @@ [195] val o7: [195]Option[List[Int]] = [195][195]Case4.unapplySeq([195]x1); [195]if ([195]o7.isEmpty.unary_!) [195]if ([195][195][195][195]o7.get.!=([195]null).&&([195][195][195][195]o7.get.lengthCompare([195]1).==([195]0))) - [195]{ - [201]val nr: [201]Int = [201][201]o7.get.apply([201]0); - [208][208]matchEnd4([208]()) - } + [208][208]matchEnd4([208]()) else [195][195]case6() else -- cgit v1.2.3 From b47bb0fe1a0eb2f60f280ef0fa62a6f0be65580b Mon Sep 17 00:00:00 2001 From: Adriaan Moors Date: Tue, 29 Jan 2013 00:21:43 -0800 Subject: no type test if static type <:< primitive value class --- .../tools/nsc/typechecker/PatternMatching.scala | 44 +++++++++++++--------- test/files/jvm/patmat_opt_primitive_typetest.check | 1 + test/files/jvm/patmat_opt_primitive_typetest.flags | 1 + .../patmat_opt_primitive_typetest/Analyzed_1.scala | 25 ++++++++++++ .../jvm/patmat_opt_primitive_typetest/test.scala | 8 ++++ 5 files changed, 61 insertions(+), 18 deletions(-) create mode 100644 test/files/jvm/patmat_opt_primitive_typetest.check create mode 100644 test/files/jvm/patmat_opt_primitive_typetest.flags create mode 100644 test/files/jvm/patmat_opt_primitive_typetest/Analyzed_1.scala create mode 100644 test/files/jvm/patmat_opt_primitive_typetest/test.scala (limited to 'src') diff --git a/src/compiler/scala/tools/nsc/typechecker/PatternMatching.scala b/src/compiler/scala/tools/nsc/typechecker/PatternMatching.scala index 8ae2e0a3b8..c6f80c9b20 100644 --- a/src/compiler/scala/tools/nsc/typechecker/PatternMatching.scala +++ b/src/compiler/scala/tools/nsc/typechecker/PatternMatching.scala @@ -411,7 +411,17 @@ trait PatternMatching extends Transform with TypingTransformers with ast.TreeDSL // 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)) { + if (patBinder.info.widen <:< extractor.paramType) { + // 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) + // TODO: get to the bottom of this -- I assume it happens when type checking infers a weird type for an unapply call + // by going back to the parameterType for the extractor call we get a saner type, so let's just do that for now + /* TODO: uncomment when `settings.developer` and `devWarning` become available + if (settings.developer.value && !(patBinder.info =:= extractor.paramType)) + devWarning(s"resetting info of $patBinder: ${patBinder.info} to ${extractor.paramType}") + */ + (Nil, patBinder setInfo extractor.paramType, false) + } else { // 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] @@ -422,16 +432,6 @@ trait PatternMatching extends Transform with TypingTransformers with ast.TreeDSL // 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) - // TODO: get to the bottom of this -- I assume it happens when type checking infers a weird type for an unapply call - // by going back to the parameterType for the extractor call we get a saner type, so let's just do that for now - /* TODO: uncomment when `settings.developer` and `devWarning` become available - if (settings.developer.value && !(patBinder.info =:= extractor.paramType)) - devWarning(s"resetting info of $patBinder: ${patBinder.info} to ${extractor.paramType}") - */ - (Nil, patBinder setInfo extractor.paramType, false) } withSubPats(typeTestTreeMaker :+ extractor.treeMaker(patBinderOrCasted, binderKnownNonNull, pos), extractor.subBindersAndPatterns: _*) @@ -1176,9 +1176,6 @@ trait PatternMatching extends Transform with TypingTransformers with ast.TreeDSL override def toString = "P"+(prevBinder.name, extraCond getOrElse "", localSubstitution) } - // typetag-based tests are inserted by the type checker - def needsTypeTest(tp: Type, pt: Type): Boolean = !(tp <:< pt) - object TypeTestTreeMaker { // factored out so that we can consistently generate other representations of the tree that implements the test // (e.g. propositions for exhaustivity and friends, boolean for isPureTypeTest) @@ -1192,12 +1189,14 @@ trait PatternMatching extends Transform with TypingTransformers with ast.TreeDSL def equalsTest(pat: Tree, testedBinder: Symbol): Result def eqTest(pat: Tree, testedBinder: Symbol): Result def and(a: Result, b: Result): Result + def tru: Result } object treeCondStrategy extends TypeTestCondStrategy { import CODE._ type Result = Tree def and(a: Result, b: Result): Result = a AND b + def tru = TRUE_typed def typeTest(testedBinder: Symbol, expectedTp: Type) = codegen._isInstanceOf(testedBinder, expectedTp) def nonNullTest(testedBinder: Symbol) = REF(testedBinder) OBJ_NE NULL def equalsTest(pat: Tree, testedBinder: Symbol) = codegen._equals(pat, testedBinder) @@ -1228,6 +1227,7 @@ trait PatternMatching extends Transform with TypingTransformers with ast.TreeDSL def equalsTest(pat: Tree, testedBinder: Symbol): Result = false 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 tru = true } def nonNullImpliedByTestChecker(binder: Symbol) = new TypeTestCondStrategy { @@ -1239,6 +1239,7 @@ trait PatternMatching extends Transform with TypingTransformers with ast.TreeDSL 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 + def tru = false } } @@ -1308,10 +1309,16 @@ trait PatternMatching extends Transform with TypingTransformers with ast.TreeDSL // I think it's okay: // - the isInstanceOf test includes a test for the element type // - Scala's arrays are invariant (so we don't drop type tests unsoundly) - case _ if (expectedTp <:< AnyRefClass.tpe) && !needsTypeTest(testedBinder.info.widen, expectedTp) => - // do non-null check first to ensure we won't select outer on null - if (outerTestNeeded) and(nonNullTest(testedBinder), outerTest(testedBinder, expectedTp)) - else nonNullTest(testedBinder) + case _ if testedBinder.info.widen <:< expectedTp => + // if the expected type is a primitive value type, it cannot be null and it cannot have an outer pointer + // since the types conform, no further checking is required + if (expectedTp.typeSymbol.isPrimitiveValueClass) tru + // have to test outer and non-null only when it's a reference type + else if (expectedTp <:< AnyRefClass.tpe) { + // do non-null check first to ensure we won't select outer on null + if (outerTestNeeded) and(nonNullTest(testedBinder), outerTest(testedBinder, expectedTp)) + else nonNullTest(testedBinder) + } else default case _ => default } @@ -1823,6 +1830,7 @@ trait PatternMatching extends Transform with TypingTransformers with ast.TreeDSL def nonNullTest(testedBinder: Symbol) = NonNullCond(binderToUniqueTree(testedBinder)) def equalsTest(pat: Tree, testedBinder: Symbol) = EqualityCond(binderToUniqueTree(testedBinder), unique(pat)) def eqTest(pat: Tree, testedBinder: Symbol) = EqualityCond(binderToUniqueTree(testedBinder), unique(pat)) // TODO: eq, not == + def tru = TrueCond } ttm.renderCondition(condStrategy) case EqualityTestTreeMaker(prevBinder, patTree, _) => EqualityCond(binderToUniqueTree(prevBinder), unique(patTree)) diff --git a/test/files/jvm/patmat_opt_primitive_typetest.check b/test/files/jvm/patmat_opt_primitive_typetest.check new file mode 100644 index 0000000000..43f53aba12 --- /dev/null +++ b/test/files/jvm/patmat_opt_primitive_typetest.check @@ -0,0 +1 @@ +bytecode identical diff --git a/test/files/jvm/patmat_opt_primitive_typetest.flags b/test/files/jvm/patmat_opt_primitive_typetest.flags new file mode 100644 index 0000000000..49d036a887 --- /dev/null +++ b/test/files/jvm/patmat_opt_primitive_typetest.flags @@ -0,0 +1 @@ +-optimize diff --git a/test/files/jvm/patmat_opt_primitive_typetest/Analyzed_1.scala b/test/files/jvm/patmat_opt_primitive_typetest/Analyzed_1.scala new file mode 100644 index 0000000000..c5b8811449 --- /dev/null +++ b/test/files/jvm/patmat_opt_primitive_typetest/Analyzed_1.scala @@ -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 +class SameBytecode { + case class Foo(x: Int, y: String) + + def a = + Foo(1, "a") match { + case Foo(_: Int, y) => y + } + + // this method's body holds the tree that should be generated by the pattern matcher for method a (-Xprint:patmat) + // the test checks that bytecode for a and b is identical (modulo line numbers) + // we can't diff trees as they are quite different (patmat uses jumps to labels that cannot be expressed in source, for example) + // note that the actual tree is quite bad: we do an unnecessary null check, and local val (x3) + // some of these will be fixed soon (the initial null check is for the scrutinee, which is harder to fix in patmat) + def b: String = { + val x1 = Foo(1, "a") + if (x1.ne(null)) { + val x3 = x1.x + return x1.y + } + + throw new MatchError(x1) + } +} \ No newline at end of file diff --git a/test/files/jvm/patmat_opt_primitive_typetest/test.scala b/test/files/jvm/patmat_opt_primitive_typetest/test.scala new file mode 100644 index 0000000000..2927e763d5 --- /dev/null +++ b/test/files/jvm/patmat_opt_primitive_typetest/test.scala @@ -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")) + } +} -- cgit v1.2.3 From b92396b57912f040d8f536b8a60b844ff586ff0d Mon Sep 17 00:00:00 2001 From: Adriaan Moors Date: Tue, 29 Jan 2013 19:16:37 -0800 Subject: SI-6686 drop valdef unused in flatMapCond's block --- .../tools/nsc/typechecker/PatternMatching.scala | 16 ++- .../patmat_opt_ignore_underscore/Analyzed_1.scala | 1 - .../jvm/patmat_opt_no_nullcheck/Analyzed_1.scala | 1 - .../patmat_opt_primitive_typetest/Analyzed_1.scala | 1 - test/files/run/inline-ex-handlers.check | 136 ++++++++++----------- test/files/run/t6288b-jump-position.check | 6 +- 6 files changed, 80 insertions(+), 81 deletions(-) (limited to 'src') diff --git a/src/compiler/scala/tools/nsc/typechecker/PatternMatching.scala b/src/compiler/scala/tools/nsc/typechecker/PatternMatching.scala index c6f80c9b20..4b53802d95 100644 --- a/src/compiler/scala/tools/nsc/typechecker/PatternMatching.scala +++ b/src/compiler/scala/tools/nsc/typechecker/PatternMatching.scala @@ -3792,11 +3792,17 @@ trait PatternMatching extends Transform with TypingTransformers with ast.TreeDSL // nextBinder: T // next == MatchMonad[U] // returns MatchMonad[U] - def flatMapCond(cond: Tree, res: Tree, nextBinder: Symbol, next: Tree): Tree = - ifThenElseZero(cond, BLOCK( - VAL(nextBinder) === res, - next - )) + def flatMapCond(cond: Tree, res: Tree, nextBinder: Symbol, next: Tree): Tree = { + val rest = + // only emit a local val for `nextBinder` if it's actually referenced in `next` + if (next.exists(_.symbol eq nextBinder)) + BLOCK( + VAL(nextBinder) === res, + next + ) + else next + ifThenElseZero(cond, rest) + } // guardTree: Boolean // next: MatchMonad[T] diff --git a/test/files/jvm/patmat_opt_ignore_underscore/Analyzed_1.scala b/test/files/jvm/patmat_opt_ignore_underscore/Analyzed_1.scala index ac59e41ba7..fa3639380d 100644 --- a/test/files/jvm/patmat_opt_ignore_underscore/Analyzed_1.scala +++ b/test/files/jvm/patmat_opt_ignore_underscore/Analyzed_1.scala @@ -20,7 +20,6 @@ class SameBytecode { val x1 = Foo(1, "a") if (x1.ne(null)) { if (x1.x.isInstanceOf[String]) { - val x3 = x1.x.asInstanceOf[String] return x1.y } } diff --git a/test/files/jvm/patmat_opt_no_nullcheck/Analyzed_1.scala b/test/files/jvm/patmat_opt_no_nullcheck/Analyzed_1.scala index 1594eb523c..3a594c401e 100644 --- a/test/files/jvm/patmat_opt_no_nullcheck/Analyzed_1.scala +++ b/test/files/jvm/patmat_opt_no_nullcheck/Analyzed_1.scala @@ -14,7 +14,6 @@ class SameBytecode { if (x1.isInstanceOf[Foo]) { val x3 = x1.asInstanceOf[Foo] if (x3.x.isInstanceOf[String]) { - val x4 = x3.x.asInstanceOf[String] val x = () return } diff --git a/test/files/jvm/patmat_opt_primitive_typetest/Analyzed_1.scala b/test/files/jvm/patmat_opt_primitive_typetest/Analyzed_1.scala index c5b8811449..e5db6c4dd0 100644 --- a/test/files/jvm/patmat_opt_primitive_typetest/Analyzed_1.scala +++ b/test/files/jvm/patmat_opt_primitive_typetest/Analyzed_1.scala @@ -16,7 +16,6 @@ class SameBytecode { def b: String = { val x1 = Foo(1, "a") if (x1.ne(null)) { - val x3 = x1.x return x1.y } diff --git a/test/files/run/inline-ex-handlers.check b/test/files/run/inline-ex-handlers.check index 905dfc3ee7..f2f0b60687 100644 --- a/test/files/run/inline-ex-handlers.check +++ b/test/files/run/inline-ex-handlers.check @@ -21,15 +21,15 @@ < 92 JUMP 7 < < 7: -395c391 +391c387 < locals: value args, variable result, value ex6, value x4, value x5, value message, value x --- > locals: value args, variable result, value ex6, value x4, value x5, value x -397c393 +393c389 < blocks: [1,2,3,4,5,8,10,11,13] --- > blocks: [1,2,3,5,8,10,11,13,14] -421c417,426 +417c413,422 < 103 THROW(MyException) --- > ? STORE_LOCAL(value ex6) @@ -42,39 +42,39 @@ > 106 LOAD_LOCAL(value x4) > 106 IS_INSTANCE REF(class MyException) > 106 CZJUMP (BOOL)NE ? 5 : 8 -434,436d438 +430,432d434 < 101 JUMP 4 < < 4: -446,449d447 +442,445d443 < 106 LOAD_LOCAL(value x5) < 106 CALL_METHOD MyException.message (dynamic) < 106 STORE_LOCAL(value message) < 106 SCOPE_ENTER value message -451c449,450 +447c445,446 < 106 LOAD_LOCAL(value message) --- > ? LOAD_LOCAL(value x5) > 106 CALL_METHOD MyException.message (dynamic) -523c522 +519c518 < blocks: [1,2,3,4,6,7,8,9,10] --- > blocks: [1,2,3,4,6,7,8,9,10,11,12,13] -552c551 +548c547 < 306 THROW(MyException) --- > ? JUMP 11 -553a553,557 +549a549,553 > 11: > ? LOAD_LOCAL(variable monitor4) > 305 MONITOR_EXIT > ? JUMP 12 > -558c562 +554c558 < ? THROW(Throwable) --- > ? JUMP 12 -564c568,575 +560c564,571 < ? THROW(Throwable) --- > ? STORE_LOCAL(value t) @@ -85,7 +85,7 @@ > 304 MONITOR_EXIT > ? STORE_LOCAL(value t) > ? JUMP 13 -579a591,602 +575a587,598 > 13: > 310 LOAD_MODULE object Predef > 310 CALL_PRIMITIVE(StartConcat) @@ -98,35 +98,35 @@ > 310 CALL_METHOD scala.Predef.println (dynamic) > 310 JUMP 2 > -588c611 +584c607 < catch (Throwable) in ArrayBuffer(7, 8, 9, 10) starting at: 6 --- > catch (Throwable) in ArrayBuffer(7, 8, 9, 10, 11) starting at: 6 -591c614 +587c610 < catch (Throwable) in ArrayBuffer(4, 6, 7, 8, 9, 10) starting at: 3 --- > catch (Throwable) in ArrayBuffer(4, 6, 7, 8, 9, 10, 11, 12) starting at: 3 -623c646 +619c642 < blocks: [1,2,3,4,5,6,7,9,10] --- > blocks: [1,2,3,4,5,6,7,9,10,11,12] -647c670,671 +643c666,667 < 78 THROW(IllegalArgumentException) --- > ? STORE_LOCAL(value e) > ? JUMP 11 -648a673,677 +644a669,673 > 11: > 81 LOAD_LOCAL(value e) > ? STORE_LOCAL(variable exc1) > ? JUMP 12 > -676c705,706 +672c701,702 < 81 THROW(Exception) --- > ? STORE_LOCAL(variable exc1) > ? JUMP 12 -692a723,735 +688a719,731 > 12: > 83 LOAD_MODULE object Predef > 83 CONSTANT("finally") @@ -140,19 +140,19 @@ > 84 LOAD_LOCAL(variable exc1) > 84 THROW(Throwable) > -698c741 +694c737 < catch () in ArrayBuffer(4, 6, 7, 9) starting at: 3 --- > catch () in ArrayBuffer(4, 6, 7, 9, 11) starting at: 3 -722c765 +718c761 < locals: value args, variable result, value ex6, variable exc2, value x4, value x5, value message, value x, value ex6, value x4, value x5, value message, value x --- > locals: value args, variable result, value ex6, variable exc2, value x4, value x5, value x, value ex6, value x4, value x5, value x -724c767 +720c763 < blocks: [1,2,3,4,5,6,9,11,14,15,16,19,21,22,24,25] --- > blocks: [1,2,3,4,5,6,9,11,14,15,16,19,21,22,24,25,26,27,28] -748c791,798 +744c787,794 < 172 THROW(MyException) --- > ? STORE_LOCAL(value ex6) @@ -163,64 +163,64 @@ > 170 STORE_LOCAL(value x4) > 170 SCOPE_ENTER value x4 > 170 JUMP 15 -791,794d840 +787,790d836 < 175 LOAD_LOCAL(value x5) < 175 CALL_METHOD MyException.message (dynamic) < 175 STORE_LOCAL(value message) < 175 SCOPE_ENTER value message -796c842,843 +792c838,839 < 176 LOAD_LOCAL(value message) --- > ? LOAD_LOCAL(value x5) > 176 CALL_METHOD MyException.message (dynamic) -800c847,848 +796c843,844 < 177 LOAD_LOCAL(value message) --- > ? LOAD_LOCAL(value x5) > 177 CALL_METHOD MyException.message (dynamic) -802c850,851 +798c846,847 < 177 THROW(MyException) --- > ? STORE_LOCAL(value ex6) > ? JUMP 27 -806c855,856 +802c851,852 < 170 THROW(Throwable) --- > ? STORE_LOCAL(value ex6) > ? JUMP 27 -815a866,871 +811a862,867 > 27: > 169 LOAD_LOCAL(value ex6) > 169 STORE_LOCAL(value x4) > 169 SCOPE_ENTER value x4 > 169 JUMP 5 > -826,829d881 +822,825d877 < 180 LOAD_LOCAL(value x5) < 180 CALL_METHOD MyException.message (dynamic) < 180 STORE_LOCAL(value message) < 180 SCOPE_ENTER value message -831c883,884 +827c879,880 < 181 LOAD_LOCAL(value message) --- > ? LOAD_LOCAL(value x5) > 181 CALL_METHOD MyException.message (dynamic) -835c888,889 +831c884,885 < 182 LOAD_LOCAL(value message) --- > ? LOAD_LOCAL(value x5) > 182 CALL_METHOD MyException.message (dynamic) -837c891,892 +833c887,888 < 182 THROW(MyException) --- > ? STORE_LOCAL(variable exc2) > ? JUMP 28 -841c896,897 +837c892,893 < 169 THROW(Throwable) --- > ? STORE_LOCAL(variable exc2) > ? JUMP 28 -857a914,926 +853a910,922 > 28: > 184 LOAD_MODULE object Predef > 184 CONSTANT("finally") @@ -234,23 +234,23 @@ > 185 LOAD_LOCAL(variable exc2) > 185 THROW(Throwable) > -863c932 +859c928 < catch (Throwable) in ArrayBuffer(14, 15, 16, 19, 21, 22, 24) starting at: 4 --- > catch (Throwable) in ArrayBuffer(14, 15, 16, 19, 21, 22, 24, 26) starting at: 4 -866c935 +862c931 < catch () in ArrayBuffer(4, 5, 6, 9, 14, 15, 16, 19, 21, 22, 24) starting at: 3 --- > catch () in ArrayBuffer(4, 5, 6, 9, 14, 15, 16, 19, 21, 22, 24, 26, 27) starting at: 3 -890c959 +886c955 < locals: value args, variable result, value e, value ex6, value x4, value x5, value message, value x --- > locals: value args, variable result, value e, value ex6, value x4, value x5, value x -892c961 +888c957 < blocks: [1,2,3,6,7,8,11,13,14,16] --- > blocks: [1,2,3,6,7,8,11,13,14,16,17] -916c985,992 +912c981,988 < 124 THROW(MyException) --- > ? STORE_LOCAL(value ex6) @@ -261,29 +261,29 @@ > 122 STORE_LOCAL(value x4) > 122 SCOPE_ENTER value x4 > 122 JUMP 7 -941,944d1016 +937,940d1012 < 127 LOAD_LOCAL(value x5) < 127 CALL_METHOD MyException.message (dynamic) < 127 STORE_LOCAL(value message) < 127 SCOPE_ENTER value message -946c1018,1019 +942c1014,1015 < 127 LOAD_LOCAL(value message) --- > ? LOAD_LOCAL(value x5) > 127 CALL_METHOD MyException.message (dynamic) -975c1048 +971c1044 < catch (IllegalArgumentException) in ArrayBuffer(6, 7, 8, 11, 13, 14, 16) starting at: 3 --- > catch (IllegalArgumentException) in ArrayBuffer(6, 7, 8, 11, 13, 14, 16, 17) starting at: 3 -999c1072 +995c1068 < locals: value args, variable result, value ex6, value x4, value x5, value message, value x, value e --- > locals: value args, variable result, value ex6, value x4, value x5, value x, value e -1001c1074 +997c1070 < blocks: [1,2,3,4,5,8,12,13,14,16] --- > blocks: [1,2,3,5,8,12,13,14,16,17] -1025c1098,1107 +1021c1094,1103 < 148 THROW(MyException) --- > ? STORE_LOCAL(value ex6) @@ -296,25 +296,25 @@ > 154 LOAD_LOCAL(value x4) > 154 IS_INSTANCE REF(class MyException) > 154 CZJUMP (BOOL)NE ? 5 : 8 -1046,1048d1127 +1042,1044d1123 < 145 JUMP 4 < < 4: -1058,1061d1136 +1054,1057d1132 < 154 LOAD_LOCAL(value x5) < 154 CALL_METHOD MyException.message (dynamic) < 154 STORE_LOCAL(value message) < 154 SCOPE_ENTER value message -1063c1138,1139 +1059c1134,1135 < 154 LOAD_LOCAL(value message) --- > ? LOAD_LOCAL(value x5) > 154 CALL_METHOD MyException.message (dynamic) -1280c1356 +1276c1352 < blocks: [1,2,3,4,5,7] --- > blocks: [1,2,3,4,5,7,8] -1304c1380,1387 +1300c1376,1383 < 38 THROW(IllegalArgumentException) --- > ? STORE_LOCAL(value e) @@ -325,20 +325,20 @@ > 42 CONSTANT("IllegalArgumentException") > 42 CALL_METHOD scala.Predef.println (dynamic) > 42 JUMP 2 -1351c1434 +1347c1430 < locals: value args, variable result, value ex6, value x4, value x5, value message, value x --- > locals: value args, variable result, value ex6, value x4, value x5, value x -1353c1436 +1349c1432 < blocks: [1,2,3,4,5,8,10,11,13,14,16] --- > blocks: [1,2,3,5,8,10,11,13,14,16,17] -1377c1460,1461 +1373c1456,1457 < 203 THROW(MyException) --- > ? STORE_LOCAL(value ex6) > ? JUMP 17 -1397c1481,1490 +1393c1477,1486 < 209 THROW(MyException) --- > ? STORE_LOCAL(value ex6) @@ -351,41 +351,41 @@ > 212 LOAD_LOCAL(value x4) > 212 IS_INSTANCE REF(class MyException) > 212 CZJUMP (BOOL)NE ? 5 : 8 -1410,1412d1502 +1406,1408d1498 < 200 JUMP 4 < < 4: -1422,1425d1511 +1418,1421d1507 < 212 LOAD_LOCAL(value x5) < 212 CALL_METHOD MyException.message (dynamic) < 212 STORE_LOCAL(value message) < 212 SCOPE_ENTER value message -1427c1513,1514 +1423c1509,1510 < 213 LOAD_LOCAL(value message) --- > ? LOAD_LOCAL(value x5) > 213 CALL_METHOD MyException.message (dynamic) -1471c1558 +1467c1554 < blocks: [1,2,3,4,5,7] --- > blocks: [1,2,3,4,5,7,8] -1495c1582,1583 +1491c1578,1579 < 58 THROW(IllegalArgumentException) --- > ? STORE_LOCAL(value e) > ? JUMP 8 -1496a1585,1590 +1492a1581,1586 > 8: > 62 LOAD_MODULE object Predef > 62 CONSTANT("RuntimeException") > 62 CALL_METHOD scala.Predef.println (dynamic) > 62 JUMP 2 > -1544c1638 +1540c1634 < blocks: [1,2,3,4] --- > blocks: [1,2,3,4,5] -1564c1658,1663 +1560c1654,1659 < 229 THROW(MyException) --- > ? JUMP 5 @@ -394,19 +394,19 @@ > ? LOAD_LOCAL(variable monitor1) > 228 MONITOR_EXIT > 228 THROW(Throwable) -1570c1669 +1566c1665 < ? THROW(Throwable) --- > 228 THROW(Throwable) -1598c1697 +1594c1693 < locals: value args, variable result, variable monitor2, variable monitorResult1 --- > locals: value exception$1, value args, variable result, variable monitor2, variable monitorResult1 -1600c1699 +1596c1695 < blocks: [1,2,3,4] --- > blocks: [1,2,3,4,5] -1623c1722,1730 +1619c1718,1726 < 245 THROW(MyException) --- > ? STORE_LOCAL(value exception$1) @@ -418,7 +418,7 @@ > ? LOAD_LOCAL(variable monitor2) > 244 MONITOR_EXIT > 244 THROW(Throwable) -1629c1736 +1625c1732 < ? THROW(Throwable) --- > 244 THROW(Throwable) diff --git a/test/files/run/t6288b-jump-position.check b/test/files/run/t6288b-jump-position.check index 45ec31c308..ece88b18f0 100644 --- a/test/files/run/t6288b-jump-position.check +++ b/test/files/run/t6288b-jump-position.check @@ -19,7 +19,7 @@ object Case3 extends Object { Exception handlers: def main(args: Array[String] (ARRAY[REF(class String)])): Unit { - locals: value args, value x1, value x2, value x + locals: value args, value x1, value x startBlock: 1 blocks: [1,2,3,6,7] @@ -35,10 +35,6 @@ object Case3 extends Object { 5 CZJUMP (BOOL)NE ? 3 : 6 3: - 5 LOAD_LOCAL(value x1) - 5 CHECK_CAST REF(class String) - 5 STORE_LOCAL(value x2) - 5 SCOPE_ENTER value x2 6 LOAD_MODULE object Predef 6 CONSTANT("case 0") 6 CALL_METHOD scala.Predef.println (dynamic) -- cgit v1.2.3