Compatibility with new Positions

[retronym]

We reverted for now, but we'd like to unrevert.

Here's my attempt at refining Paul's position's improvements to a point were scala-refactoring might be able to adapt to: https://github.com/retronym/scala/tree/topic/parser-revert

That failed with these errors:

Tests in error: 
  PrettyPrinterTest.multipleAssignmentWithTuple:350 » FailedInterrupt Compiler e...
  PrettyPrinterTest.multipleAssignmentWithPimpedTuple:361 » FailedInterrupt Comp...
  PrettyPrinterTest.multipleAssignmentWith4Tuple:372 » FailedInterrupt Compiler ...
  PrettyPrinterTest.multipleAssignmentFromMethodResult:393 » FailedInterrupt Com...
  IndividualSourceGenTest.manyParentheses:375 » FailedInterrupt Compiler excepti...
  IndividualSourceGenTest.newObject:655 » FailedInterrupt Compiler exception dur...
  IndividualSourceGenTest.newParameterizedObject:677 » FailedInterrupt Compiler ...
  IndividualSourceGenTest.newParameterizedObject2:703 » FailedInterrupt Compiler...
  IndividualSourceGenTest.annotation:723 » FailedInterrupt Compiler exception du...
  SourceGenTest.testNew:144 » FailedInterrupt Compiler exception during call to ...
  SourceGenTest.testThrow:166 » FailedInterrupt Compiler exception during call t...
  SourceGenTest.testAnnotation:194 » FailedInterrupt Compiler exception during c...
  SourceGenTest.testWhileLoop:430 » FailedInterrupt Compiler exception during ca...
  SourceGenTest.testDoWhileLoop:482 » FailedInterrupt Compiler exception during ...
  SourceGenTest.testTry:1195 » FailedInterrupt Compiler exception during call to...
  SourceGenTest.testImports:1420 » FailedInterrupt Compiler exception during cal...
  SourceGenTest.multipleAssignmentWithAnnotatedTree:252 » FailedInterrupt Compil...
  SourceGenTest.updateMethod:575 » FailedInterrupt Compiler exception during cal...
  SourceGenTest.testContextBounds:1514 » FailedInterrupt Compiler exception duri...
  RenameTest.renameTo:19 » Runtime Not implemented! Annotated
  RenameTest.renameTo:19 » Runtime Not implemented! Annotated
  RenameTest.renameTo:19 » Runtime Not implemented! Annotated

[misto]

Thanks! I'm on it. So far, what I have noticed is that before a multiple assignment val (a, b) = (1, 2) used to look like this:

List(<synthetic> <artifact> private[this] val x$1: (Int, Int) = (<empty>(1, 2): <empty>) match {
  case <empty>((a @ <empty>), (b @ <empty>)) => scala.Tuple2.apply[<empty>, <empty>](a, b)
}, private[this] val a: Int = _, private[this] val b: Int = _)

but now we lost the names of the private vals:

List(<synthetic> <artifact> private[this] val x$1: (Int, Int) = (<empty>(1, 2): <empty>) match {
  case <empty>((a @ <empty>), (b @ <empty>)) => scala.Tuple2.apply[<empty>, <empty>](a, b)
}, private val _ = _, private val _ = _)

[misto]

Next, the New tree positions doesn't enclose the new anymore. Couldn't we use the point to indicate where the tpt starts (if that's the reason why it was changed)?

[misto]

The failures that occur in the imports are caused by an incorrect position of the Import expr, the end is off by one, it points to the S instead of the .

import java.lang.String
                 ↑

[retronym]

/Cc @paulp

On Tuesday, September 24, 2013, Mirko Stocker wrote:

Next, the New tree positions doesn't enclose the new anymore. Couldn't we use the point to indicate where the tpt starts?

— Reply to this email directly or view it on GitHubhttps://github.com/scala-ide/scala-refactoring/issues/29#issuecomment-25027423 .

[paulp]

They're just bugs. The new positions are half the challenge. It wasn't "it doesn't enclose the new anymore", it was only "it now encloses the new for a different subset of situations." But it was all a consequence of trying to carve off a subset of the position code for an easier path through review. It's out of the frying pan, into the hard place.

[retronym]

Could you give an example of a New node that doesn't enclose the token? I can't reproduce:

scala> :paste
// Entering paste mode (ctrl-D to finish)

import scala.reflect.runtime.universe._
import scala.reflect.runtime.{universe => ru}
import scala.reflect.runtime.{currentMirror => cm}
import scala.tools.reflect.ToolBox
val toolbox = cm.mkToolBox(options = "-Yrangepos")
def test(expr: String) {
  val t = toolbox.parse(expr)
  println(expr)
  println(show(t, printPositions = true))
  println()
}

// Exiting paste mode, now interpreting.

import scala.reflect.runtime.universe._
import scala.reflect.runtime.{universe=>ru}
import scala.reflect.runtime.{currentMirror=>cm}
import scala.tools.reflect.ToolBox
toolbox: scala.tools.reflect.ToolBox[reflect.runtime.universe.type] = scala.tools.reflect.ToolBoxFactory$ToolBoxImpl@55ad5519
test: (expr: String)Unit

scala> test("new C")
new C
[4/0:5][4/0:5][4/0:5]new [4:5]C()

scala> test("new C(arg)")
new C(arg)
[4/0:10][4:5][4:5]new [4:5]C([6:9]arg)

scala> test("new C(arg) {}")
new C(arg) {}
[4/0:13]{
  <4:13>final class $anon extends [4:13][4:10][4:5]C([6:9]arg) {
    [4]def <init>() = [4]{
      [X][X]super.<init>();
      [4]()
    };
    [X]<empty>
  };
  [0:3][0:3][0:3]new [4]$anon()
}

(The new notation: [POINT/START:END], where POINT is omitted if it is the same as START.)

[retronym]

Oh, maybe its [0:3][0:3][0:3]new [4]$anon().

I notice that TreeGen.mkNew contains the following unused local method:

def enclosingPos = wrappingPos(cpos, parents ::: List(self) ::: stats)

[paulp]

You're only testing the parser that way. The major issues with positions are in the various synthetic trees created on the way from parser to typer: context and view bounds, closures, for comprehensions, case class supplements, moving constructors and early definitions into templates, assignment operators, etc. etc. etc.

[paulp]

The initial thing I was chasing with new is that the point falls outside the range sometimes, see SI-6768.

[retronym]

Many of those things come from the parser (context/view bounds, multi-assigns, for-comprehensions), so they are amenable to this style of testing, no?

The import bug is an easy one:

diff --git a/src/compiler/scala/tools/nsc/ast/parser/Parsers.scala b/src/compiler/scala/
index 1a897b6..7493b83 100644
--- a/src/compiler/scala/tools/nsc/ast/parser/Parsers.scala
+++ b/src/compiler/scala/tools/nsc/ast/parser/Parsers.scala
@@ -2312,10 +2312,11 @@ self =>
           case LBRACE   => importSelectors()      // import foo.bar.{ x, y, z }
           case _        =>
             val nameOffset = in.offset
-            val name = ident()
+            val name       = ident()
+            val end        = in.offset
             if (acceptIfPresent(DOT))
               // import foo.bar.ident.<unknown> and so create a select node and recurse
-              return loop(atPos(start, if (name == nme.ERROR) in.offset else nameOffset
+              return loop(atPos(start, if (name == nme.ERROR) in.offset else nameOffset
             // import foo.bar.Baz;
             else List(makeImportSelector(name, nameOffset))
         }

Anyway, I've got to take my leave from positions for today, but I retain my optimism!

[paulp]

I take it end is intended to be used somewhere, perhaps at the line which is diff-marked but mysteriously has no change.

[paulp]

Many of those things come from the parser (context/view bounds, multi-assigns, for-comprehensions), so they are amenable to this style of testing, no?

Lots of things are amenable to it, but lots aren't; you were investigating "new", which isn't - the source of the badly pointed new trees, or at least a source, was transformCaseApply

[retronym]

Ah, good old RefChecks.

[retronym]

The main reason I was testing in the manner was to make the offsets intelligible.

We can also do that by padding all the lines in a larger source file to a round number:

object Test {              //
val x = 0                  //
val y = 0                  //
var z = 0                  //
val a, b = 0               //
var c, d = 0               //
val Some(e) = Some(1)      //
val (f, g) = (1, 2)        //
def h = 0                  //
import java.lang           //
import java.{lang => lan}  //
import o.p, o.q            //
new C                      //
new C[Int] {}              //
new Some("")               //
Some("")                   //
def foo[A: C] = 0          //
class C[A <: Any]          //
}                          //

object o {
object p
object q
}

qbin/scalac -Xprint:parser sandbox/test.scala -Xprint-pos -Yrangepos
[[syntax trees at end of                    parser]] // test.scala
[0:601]package [0:0]<empty> {
  [7/0:541]object Test extends [12:541][571]scala.AnyRef {
    [12]def <init>() = [12]{
      [X][X][X]super.<init>();
      [12]()
    };
    [34/30:39]val x = [38:39]0;
    [64/60:69]val y = [68:69]0;
    [94/90:99]var z = [98:99]0;
    [124/120:125]val a = [131]0;
    [127:132]val b = [131:132]0;
    [154/150:155]var c = [161]0;
    [157:162]var d = [161:162]0;
    [188/180:201]val e = <184:201>[198/194:201][198/194:201][194:198]Some([199:200]1): @[198]scala.unchecked match {
      <184:191>case <184:191>[184:188]Some(<189:190>(e @ [189]_)) => <184:191>e
    };
    [214/210:229]<synthetic> <artifact> private[this] val x$1 = <214:229>[223:229][223:229][223]scala.Tuple2([224:225]1, [227:228]2): @[223]scala.unchecked match {
      <214:220>case <214:220>[214]scala.Tuple2(<215:216>(f @ [215]_), <218:219>(g @ [218]_)) => <214:220><214:220>scala.Tuple2(<214:220>f, <214:220>g)
    };
    [215]val f = [215]x$1._1;
    [218]val g = [218]x$1._2;
    [244/240:249]def h = [248:249]0;
    [277/270:286]import java.lang;
    [307/300:325]import java.{lang=>lan};
    [337/330:340]import o.p;
    [342:345]import o.q;
    [364/360:365][364/360:365][364/360:365]new [364:365]C();
    [394/390:403]{
      <394:403>final class $anon extends [394:403][394:400][394:395]C[[396:399]Int] {
        [394]def <init>() = [394]{
          [X][X][X]super.<init>();
          [394]()
        };
        [X]<empty>
      };
      [390:393][390:393][390:393]new [394]$anon()
    };
    [424/420:432][424:428][424:428]new [424:428]Some([429:431]"");
    [454/450:458][450:454]Some([455:457]"");
    [484/480:497]def foo[[488:489]A[489]](implicit [484/489:492]evidence$1: [491/489:492][491:492]C[[491]A]) = [496:497]0;
    [516/510:527]class C[[518:526]A[523:526] <: [523:526]Any] extends [527:527][540]scala.AnyRef {
      [540]def <init>() = [540]{
        [X][X][X]super.<init>();
        [540]()
      }
    }

[misto]

Here's my test case for the incorrect new position:

    class VBox[T](i: Int)
    class Test {
     var box = new VBox[Int](42)
    }

[retronym]

For posterity, here's how to run the test suite with a local build of Scala:

% git diff pom.xml
diff --git a/pom.xml b/pom.xml
index 41ebeec..94b79cf 100644
--- a/pom.xml
+++ b/pom.xml
@@ -34,7 +34,7 @@
     </developer>
   </developers>
   <properties>
-    <scala.version>2.9.2</scala.version>
+    <scala.version>2.11.0-SNAPSHOT</scala.version>
     <version.suffix>2_09</version.suffix>
     <tycho.version>0.15.0</tycho.version>
     <repo.scala-ide>http://download.scala-ide.org</repo.scala-ide>
@@ -49,7 +49,7 @@
     <dependency>
       <groupId>junit</groupId>
       <artifactId>junit</artifactId>
-      <version>4.7</version>
+      <version>4.8.1</version>
       <scope>test</scope>
     </dependency>
     <dependency>
@@ -61,6 +61,15 @@
       <groupId>org.scala-lang</groupId>
       <artifactId>scala-compiler</artifactId>
       <version>${scala.version}</version>
+      <scope>system</scope>
+      <systemPath>/code/scala/build/pack/lib/scala-compiler.jar</systemPath>
+    </dependency>
+    <dependency>
+      <groupId>org.scala-lang</groupId>
+      <artifactId>scala-reflect</artifactId>
+      <version>${scala.version}</version>
+      <scope>system</scope>
+      <systemPath>/code/scala/build/pack/lib/scala-reflect.jar</systemPath>
     </dependency>
   </dependencies>
   <build>

% mvn -Pscala-2.11.x -Dtest=SourceGenTest -DfailIfNoTests=false test

Remove -Dtest=SourceGenTest to run all tests. Add -o to stop Maven hitting the interwebs as much after the first build.

[kiritsuku]

I close this because the changes are so old, it is hard to get something useful out of it. Specific bug reports would be more useful. Please reopen if anyone disagrees.