From 79a43d78b27232be005755eb206fd9e4ce9a0625 Mon Sep 17 00:00:00 2001 From: Jason Zaugg Date: Tue, 11 Dec 2012 12:26:52 +0100 Subject: [PATCH 1/4] SI-6288 Position argument of unapply `atPos(pos) { ... }` doesn't descend into children of already positioned trees, we need to manually set the position of `CODE.REF(binder)` to that of the stunt double `Ident(nme.SELECTOR_DUMMY)`. --- .../nsc/typechecker/PatternMatching.scala | 4 +-- test/files/run/t6288.check | 30 +++++++++++++++++++ test/files/run/t6288.scala | 25 ++++++++++++++++ 3 files changed, 57 insertions(+), 2 deletions(-) create mode 100644 test/files/run/t6288.check create mode 100644 test/files/run/t6288.scala diff --git a/src/compiler/scala/tools/nsc/typechecker/PatternMatching.scala b/src/compiler/scala/tools/nsc/typechecker/PatternMatching.scala index 834c64aaae..8582a773c3 100644 --- a/src/compiler/scala/tools/nsc/typechecker/PatternMatching.scala +++ b/src/compiler/scala/tools/nsc/typechecker/PatternMatching.scala @@ -801,8 +801,8 @@ trait PatternMatching extends Transform with TypingTransformers with ast.TreeDSL protected def spliceApply(binder: Symbol): Tree = { object splice extends Transformer { override def transform(t: Tree) = t match { - case Apply(x, List(Ident(nme.SELECTOR_DUMMY))) => - treeCopy.Apply(t, x, List(CODE.REF(binder))) + case Apply(x, List(i @ Ident(nme.SELECTOR_DUMMY))) => + treeCopy.Apply(t, x, List(CODE.REF(binder).setPos(i.pos))) case _ => super.transform(t) } } diff --git a/test/files/run/t6288.check b/test/files/run/t6288.check new file mode 100644 index 0000000000..3e65b8127c --- /dev/null +++ b/test/files/run/t6288.check @@ -0,0 +1,30 @@ +[[syntax trees at end of patmat]] // newSource1 +[7]package [7] { + [7]object Case3 extends [13][105]scala.AnyRef { + [105]def (): [13]Case3.type = [105]{ + [105][105][105]Case3.super.(); + [13]() + }; + [21]def unapply([29]z: [32]): [21]Option[Int] = [56][52][52]scala.Some.apply[[52]Int]([58]-1); + [64]{ + [64]case val x1: [64]Any = [64]""; + [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 = [64]o7.get; + [97][97]matchEnd4([97]()) + } + else + [84][84]case6() + }; + [64]case6(){ + [64][64]matchEnd4([64]throw [64][64][64]new [64]MatchError([64]x1)) + }; + [64]matchEnd4(x: [NoPosition]Unit){ + [64]x + } + } + } +} + diff --git a/test/files/run/t6288.scala b/test/files/run/t6288.scala new file mode 100644 index 0000000000..9d8fb990d7 --- /dev/null +++ b/test/files/run/t6288.scala @@ -0,0 +1,25 @@ +import scala.tools.partest._ +import java.io.{Console => _, _} + +object Test extends DirectTest { + + override def extraSettings: String = "-usejavacp -Xprint:patmat -Xprint-pos -d " + testOutput.path + + override def code = + """ + |object Case3 { + | def unapply(z: Any): Option[Int] = Some(-1) + | + | "" match { + | case Case3(nr) => () + | } + |}""".stripMargin.trim + + override def show(): Unit = { + // Now: [84][84]Case3.unapply([84]x1); + // Was: [84][84]Case3.unapply([64]x1); + Console.withErr(System.out) { + compile() + } + } +} From f69b8468b76edc9f25a4cb97022a136be988b236 Mon Sep 17 00:00:00 2001 From: Jason Zaugg Date: Tue, 11 Dec 2012 18:59:27 +0100 Subject: [PATCH 2/4] SI-6288 Fix positioning of label jumps ICode generation was assigning the position of the last label jump to all jumps to that particular label def. This problem is particularly annoying under the new pattern matcher: a breakpoint in the body of the final case will be triggered on the way out of the body of any other case. Thanks to @dragos for the expert guidance as we wended our way through GenICode to the troublesome code. Chalk up another bug for mutability. I believe that the ICode output should be stable enough to use a a .check file, if it proves otherwise we should make it so. --- .../tools/nsc/backend/icode/BasicBlocks.scala | 7 +- .../scala/tools/partest/DirectTest.scala | 4 + test/files/run/t6288b-jump-position.check | 80 +++++++++++++++++++ test/files/run/t6288b-jump-position.scala | 22 +++++ 4 files changed, 112 insertions(+), 1 deletion(-) create mode 100644 test/files/run/t6288b-jump-position.check create mode 100644 test/files/run/t6288b-jump-position.scala diff --git a/src/compiler/scala/tools/nsc/backend/icode/BasicBlocks.scala b/src/compiler/scala/tools/nsc/backend/icode/BasicBlocks.scala index 068836fe4f..d50d4cd125 100644 --- a/src/compiler/scala/tools/nsc/backend/icode/BasicBlocks.scala +++ b/src/compiler/scala/tools/nsc/backend/icode/BasicBlocks.scala @@ -320,7 +320,12 @@ trait BasicBlocks { else instrs.zipWithIndex collect { case (oldInstr, i) if map contains oldInstr => - code.touched |= replaceInstruction(i, map(oldInstr)) + // SI-6288 clone important here because `replaceInstruction` assigns + // a position to `newInstr`. Without this, a single instruction can + // be added twice, and the position last position assigned clobbers + // all previous positions in other usages. + val newInstr = map(oldInstr).clone() + code.touched |= replaceInstruction(i, newInstr) } ////////////////////// Emit ////////////////////// diff --git a/src/partest/scala/tools/partest/DirectTest.scala b/src/partest/scala/tools/partest/DirectTest.scala index 8c18809ad6..483cb491a1 100644 --- a/src/partest/scala/tools/partest/DirectTest.scala +++ b/src/partest/scala/tools/partest/DirectTest.scala @@ -39,6 +39,10 @@ abstract class DirectTest extends App { // new compiler def newCompiler(args: String*): Global = { val settings = newSettings((CommandLineParser tokenize ("-d \"" + testOutput.path + "\" " + extraSettings)) ++ args.toList) + newCompiler(settings) + } + + def newCompiler(settings: Settings): Global = { if (settings.Yrangepos.value) new Global(settings, reporter(settings)) with interactive.RangePositions else new Global(settings, reporter(settings)) } diff --git a/test/files/run/t6288b-jump-position.check b/test/files/run/t6288b-jump-position.check new file mode 100644 index 0000000000..45ec31c308 --- /dev/null +++ b/test/files/run/t6288b-jump-position.check @@ -0,0 +1,80 @@ +object Case3 extends Object { + // fields: + + // methods + def unapply(z: Object (REF(class Object))): Option { + locals: value z + startBlock: 1 + blocks: [1] + + 1: + 2 NEW REF(class Some) + 2 DUP(REF(class Some)) + 2 CONSTANT(-1) + 2 BOX INT + 2 CALL_METHOD scala.Some. (static-instance) + 2 RETURN(REF(class Option)) + + } + Exception handlers: + + def main(args: Array[String] (ARRAY[REF(class String)])): Unit { + locals: value args, value x1, value x2, value x + startBlock: 1 + blocks: [1,2,3,6,7] + + 1: + 4 CONSTANT("") + 4 STORE_LOCAL(value x1) + 4 SCOPE_ENTER value x1 + 4 JUMP 2 + + 2: + 5 LOAD_LOCAL(value x1) + 5 IS_INSTANCE REF(class String) + 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) + 6 LOAD_FIELD scala.runtime.BoxedUnit.UNIT + 6 STORE_LOCAL(value x) + 6 JUMP 7 + + 6: + 8 LOAD_MODULE object Predef + 8 CONSTANT("default") + 8 CALL_METHOD scala.Predef.println (dynamic) + 8 LOAD_FIELD scala.runtime.BoxedUnit.UNIT + 8 STORE_LOCAL(value x) + 8 JUMP 7 + + 7: + 10 LOAD_MODULE object Predef + 10 CONSTANT("done") + 10 CALL_METHOD scala.Predef.println (dynamic) + 10 RETURN(UNIT) + + } + Exception handlers: + + def (): Case3.type { + locals: + startBlock: 1 + blocks: [1] + + 1: + 12 THIS(Case3) + 12 CALL_METHOD java.lang.Object. (super()) + 12 RETURN(UNIT) + + } + Exception handlers: + + +} diff --git a/test/files/run/t6288b-jump-position.scala b/test/files/run/t6288b-jump-position.scala new file mode 100644 index 0000000000..e22a1ab120 --- /dev/null +++ b/test/files/run/t6288b-jump-position.scala @@ -0,0 +1,22 @@ +import scala.tools.partest.IcodeTest + +object Test extends IcodeTest { + override def code = + """object Case3 { // 01 + | def unapply(z: Any): Option[Int] = Some(-1) // 02 + | def main(args: Array[String]) { // 03 + | ("": Any) match { // 04 + | case x : String => // 05 Read: JUMP + | println("case 0") // 06 expecting "6 JUMP 7", was "8 JUMP 7" + | case _ => // 07 + | println("default") // 08 expecting "8 JUMP 7" + | } // 09 + | println("done") // 10 + | } + |}""".stripMargin + + override def show() { + val lines1 = collectIcode("") + println(lines1 mkString "\n") + } +} From 286dced26e0d12796ab183b273ce6f00da182709 Mon Sep 17 00:00:00 2001 From: Jason Zaugg Date: Tue, 11 Dec 2012 23:56:10 +0100 Subject: [PATCH 3/4] SI-6288 Remedy ill-positioned extractor binding. The call to `Option#get` on the result of the unapply method was unpositioned and ended up with the position of the `match`. --- .../nsc/typechecker/PatternMatching.scala | 2 +- test/files/run/inline-ex-handlers.check | 21 ++++++++++--------- test/files/run/t6288.check | 2 +- 3 files changed, 13 insertions(+), 12 deletions(-) diff --git a/src/compiler/scala/tools/nsc/typechecker/PatternMatching.scala b/src/compiler/scala/tools/nsc/typechecker/PatternMatching.scala index 8582a773c3..fa8aff5cdd 100644 --- a/src/compiler/scala/tools/nsc/typechecker/PatternMatching.scala +++ b/src/compiler/scala/tools/nsc/typechecker/PatternMatching.scala @@ -879,7 +879,7 @@ trait PatternMatching extends Transform with TypingTransformers with ast.TreeDSL override def transform(tree: Tree): Tree = { def subst(from: List[Symbol], to: List[Tree]): Tree = if (from.isEmpty) tree - else if (tree.symbol == from.head) typedIfOrigTyped(to.head.shallowDuplicate, tree.tpe) + else if (tree.symbol == from.head) typedIfOrigTyped(to.head.shallowDuplicate.setPos(tree.pos), tree.tpe) else subst(from.tail, to.tail) tree match { diff --git a/test/files/run/inline-ex-handlers.check b/test/files/run/inline-ex-handlers.check index e786c780d6..45db7c3a15 100644 --- a/test/files/run/inline-ex-handlers.check +++ b/test/files/run/inline-ex-handlers.check @@ -47,7 +47,7 @@ < 106 LOAD_LOCAL(value message) --- > ? LOAD_LOCAL(value x5) -> ? CALL_METHOD MyException.message (dynamic) +> 106 CALL_METHOD MyException.message (dynamic) 502c504 < blocks: [1,2,3,4,6,7,8,9,10] --- @@ -162,12 +162,12 @@ < 176 LOAD_LOCAL(value message) --- > ? LOAD_LOCAL(value x5) -> ? CALL_METHOD MyException.message (dynamic) +> 176 CALL_METHOD MyException.message (dynamic) 783c833,834 < 177 LOAD_LOCAL(value message) --- > ? LOAD_LOCAL(value x5) -> ? CALL_METHOD MyException.message (dynamic) +> 177 CALL_METHOD MyException.message (dynamic) 785c836,837 < 177 THROW(MyException) --- @@ -194,12 +194,12 @@ < 181 LOAD_LOCAL(value message) --- > ? LOAD_LOCAL(value x5) -> ? CALL_METHOD MyException.message (dynamic) +> 181 CALL_METHOD MyException.message (dynamic) 822c878,879 < 182 LOAD_LOCAL(value message) --- > ? LOAD_LOCAL(value x5) -> ? CALL_METHOD MyException.message (dynamic) +> 182 CALL_METHOD MyException.message (dynamic) 824c881,882 < 182 THROW(MyException) --- @@ -260,7 +260,7 @@ < 127 LOAD_LOCAL(value message) --- > ? LOAD_LOCAL(value x5) -> ? CALL_METHOD MyException.message (dynamic) +> 127 CALL_METHOD MyException.message (dynamic) 966c1042 < catch (IllegalArgumentException) in ArrayBuffer(6, 7, 8, 11, 14, 16, 17, 19) starting at: 3 --- @@ -299,7 +299,7 @@ < 154 LOAD_LOCAL(value message) --- > ? LOAD_LOCAL(value x5) -> ? CALL_METHOD MyException.message (dynamic) +> 154 CALL_METHOD MyException.message (dynamic) 1275c1354 < blocks: [1,2,3,4,5,7] --- @@ -354,22 +354,23 @@ < 213 LOAD_LOCAL(value message) --- > ? LOAD_LOCAL(value x5) -> ? CALL_METHOD MyException.message (dynamic) +> 213 CALL_METHOD MyException.message (dynamic) 1470c1560 < blocks: [1,2,3,4,5,7] --- > blocks: [1,2,3,4,5,7,8] -1494c1584,1591 +1494c1584,1585 < 58 THROW(IllegalArgumentException) --- > ? STORE_LOCAL(value e) > ? JUMP 8 -> +1495a1587,1592 > 8: > 62 LOAD_MODULE object Predef > 62 CONSTANT("RuntimeException") > 62 CALL_METHOD scala.Predef.println (dynamic) > 62 JUMP 2 +> 1543c1640 < blocks: [1,2,3,4] --- diff --git a/test/files/run/t6288.check b/test/files/run/t6288.check index 3e65b8127c..e1aa58ea82 100644 --- a/test/files/run/t6288.check +++ b/test/files/run/t6288.check @@ -12,7 +12,7 @@ [84] val o7: [84]Option[Int] = [84][84]Case3.unapply([84]x1); [84]if ([84]o7.isEmpty.unary_!) [84]{ - [90]val nr: [90]Int = [64]o7.get; + [90]val nr: [90]Int = [90]o7.get; [97][97]matchEnd4([97]()) } else From 601536136e6300cb8fef8f20b1f1e74cec033621 Mon Sep 17 00:00:00 2001 From: Jason Zaugg Date: Wed, 12 Dec 2012 00:01:58 +0100 Subject: [PATCH 4/4] Expand pattern match position tests. - Adds tests for unapplySeq and unapply: Boolean. Both seem to be well positioned after the previous changes. --- test/files/run/t6288.check | 61 ++++++++++++++++++++++++++++++++++++-- test/files/run/t6288.scala | 32 +++++++++++++++----- 2 files changed, 82 insertions(+), 11 deletions(-) diff --git a/test/files/run/t6288.check b/test/files/run/t6288.check index e1aa58ea82..af6bd5d269 100644 --- a/test/files/run/t6288.check +++ b/test/files/run/t6288.check @@ -1,8 +1,8 @@ [[syntax trees at end of patmat]] // newSource1 [7]package [7] { - [7]object Case3 extends [13][105]scala.AnyRef { - [105]def (): [13]Case3.type = [105]{ - [105][105][105]Case3.super.(); + [7]object Case3 extends [13][106]scala.AnyRef { + [106]def (): [13]Case3.type = [106]{ + [106][106][106]Case3.super.(); [13]() }; [21]def unapply([29]z: [32]): [21]Option[Int] = [56][52][52]scala.Some.apply[[52]Int]([58]-1); @@ -25,6 +25,61 @@ [64]x } } + }; + [113]object Case4 extends [119][217]scala.AnyRef { + [217]def (): [119]Case4.type = [217]{ + [217][217][217]Case4.super.(); + [119]() + }; + [127]def unapplySeq([138]z: [141]): [127]Option[List[Int]] = [167]scala.None; + [175]{ + [175]case val x1: [175]Any = [175]""; + [175]case5()[195]{ + [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]()) + } + else + [195][195]case6() + else + [195][195]case6() + }; + [175]case6(){ + [175][175]matchEnd4([175]throw [175][175][175]new [175]MatchError([175]x1)) + }; + [175]matchEnd4(x: [NoPosition]Unit){ + [175]x + } + } + }; + [224]object Case5 extends [230][312]scala.AnyRef { + [312]def (): [230]Case5.type = [312]{ + [312][312][312]Case5.super.(); + [230]() + }; + [238]def unapply([246]z: [249]): [238]Boolean = [265]true; + [273]{ + [273]case val x1: [273]Any = [273]""; + [273]case5()[293]{ + [293] val o7: [293]Option[List[Int]] = [293][293]Case4.unapplySeq([293]x1); + [293]if ([293]o7.isEmpty.unary_!) + [293]if ([293][293][293][293]o7.get.!=([293]null).&&([293][293][293][293]o7.get.lengthCompare([293]0).==([195]0))) + [304][304]matchEnd4([304]()) + else + [293][293]case6() + else + [293][293]case6() + }; + [273]case6(){ + [273][273]matchEnd4([273]throw [273][273][273]new [273]MatchError([273]x1)) + }; + [273]matchEnd4(x: [NoPosition]Unit){ + [273]x + } + } } } diff --git a/test/files/run/t6288.scala b/test/files/run/t6288.scala index 9d8fb990d7..cf5865e95a 100644 --- a/test/files/run/t6288.scala +++ b/test/files/run/t6288.scala @@ -6,14 +6,30 @@ object Test extends DirectTest { override def extraSettings: String = "-usejavacp -Xprint:patmat -Xprint-pos -d " + testOutput.path override def code = - """ - |object Case3 { - | def unapply(z: Any): Option[Int] = Some(-1) - | - | "" match { - | case Case3(nr) => () - | } - |}""".stripMargin.trim + """ + |object Case3 { + | def unapply(z: Any): Option[Int] = Some(-1) + | + | "" match { + | case Case3(nr) => () + | } + |} + |object Case4 { + | def unapplySeq(z: Any): Option[List[Int]] = None + | + | "" match { + | case Case4(nr) => () + | } + |} + |object Case5 { + | def unapply(z: Any): Boolean = true + | + | "" match { + | case Case4() => () + | } + |} + | + |""".stripMargin.trim override def show(): Unit = { // Now: [84][84]Case3.unapply([84]x1);