Permalink
Browse files

SI-4827 Corrected positions assigned to constructor's default arg

* Default arguments are always retained on the <init> method (i.e.,
  the class' constructor). Therefore, when the <init> parameters are
  created, we need to use `duplicateAndKeepPositions` to make sure that
  if a default argument is present, its opaque position is retained as
  well. This is necessary because when parameter accessors (i.e.,
  `fieldDefs`) are created, all default arguments are discared ( as you
  can see in the code, the right-hand-side of a `field` is always an
  `EmptyTree`) - see changes in TreeGen.scala

* When constructing the `fieldDefs`, it is important to adapt their
  range position to avoid overlappings with the positions of default
  arguments. It is worth noting that updating the field's end position
  to `vd.rhs.pos.start` would be incorrect, because `askTypeAt(pos)`
  could return the incorrect tree when the position is equal to
  `vd.rhs.pos.start` (because two nodes including that point position
  would exist in the tree, and `CompilerControl.locateTree(pos)` would
  return the first tree that includes the passed `pos`). This is why
  `1` is subtracted to `vd.rhs.pos.start`. Alternatively, we could have
  used `vd.tpt.pos.end` with similar results. However the logic would
  have become slightly more complex as we would need to handle the case
  where `vd.tpt` doesn't have a range position (for instance, this can
  happen if `-Yinfer-argument-types` is enabled). Therefore, subtracting
  `1` from `vd.rhs.pos.start` seemed the cleanest solution at the
  moment. - see changes in TreeGen.scala.

* If the synthetic constructor contains trees with an opaque range
  position (see point above), it must have a transparent position.
  This can only happen if the constructor's parameters' positions are
  considered, which is why we are now passing `vparamss1` to
  `wrappingPos` - see changes in TreeGen.scala.

* The derived primary constructor should have a transparent position
  as it may contain trees with an opaque range position. Hence, the
  `primaryCtor` position is considered for computing the position of the
  derived constructor - see change in Typers.scala.

Finally, below follows the printing of the tree for test t4287, which
you should compare with the one attached with the previous commit
message:

```
[[syntax trees at end of typer]] // Foo.scala
[0:63]package [0:0]<empty> {
  [0:37]class Baz extends [9:37][39]scala.AnyRef {
    [10:20]<paramaccessor> private[this] val f: [14]Int = _;
    [14]<stable> <accessor> <paramaccessor> def f: [14]Int = [14][14]Baz.this.f;
    <10:31>def <init>(<10:31>f: [17]<type: [17]scala.Int> = [23:31]B.a): [9]Baz = <10:31>{
      <10:31><10:31><10:31>Baz.super.<init>();
      <10:31>()
    }
  };
  [6]<synthetic> object Baz extends [6][6]AnyRef {
    [6]def <init>(): [9]Baz.type = [6]{
      [6][6][6]Baz.super.<init>();
      [9]()
    };
    [14]<synthetic> def <init>$default$1: [14]Int = [30]B.a
  };
  [39:63]object B extends [48:63][63]scala.AnyRef {
    [63]def <init>(): [48]B.type = [63]{
      [63][63][63]B.super.<init>();
      [48]()
    };
    [52:61]private[this] val a: [56]Int = [60:61]2;
    [56]<stable> <accessor> def a: [56]Int = [56][56]B.this.a
  }
}
```

You should notice that the default arg of `Baz` constructor now has a
range position. And that explains why the associated test now returns
the right tree when asking hyperlinking at the location of the default
argument.
  • Loading branch information...
1 parent bdb0ac0 commit 4936c43c137e8e4d133dde397a121288490f78f9 @dotta dotta committed Dec 12, 2013
View
2 src/compiler/scala/tools/nsc/typechecker/Typers.scala
@@ -1918,7 +1918,7 @@ trait Typers extends Adaptations with Tags with TypersTracking with PatternTyper
val primaryCtor1 = primaryCtor match {
case DefDef(_, _, _, _, _, Block(earlyVals :+ global.pendingSuperCall, unit)) =>
val argss = superArgs(parents1.head) getOrElse Nil
- val pos = wrappingPos(parents1.head.pos, argss.flatten)
+ val pos = wrappingPos(parents1.head.pos, primaryCtor :: argss.flatten).makeTransparent
val superCall = atPos(pos)(PrimarySuperCall(argss))
deriveDefDef(primaryCtor)(block => Block(earlyVals :+ superCall, unit) setPos pos) setPos pos
case _ => primaryCtor
View
20 src/reflect/scala/reflect/internal/TreeGen.scala
@@ -341,11 +341,13 @@ abstract class TreeGen extends macros.TreeBuilder {
// create parameters for <init> as synthetic trees.
var vparamss1 = mmap(vparamss) { vd =>
- atPos(vd.pos.focus) {
+ val param = atPos(vd.pos.makeTransparent) {
val mods = Modifiers(vd.mods.flags & (IMPLICIT | DEFAULTPARAM | BYNAMEPARAM) | PARAM | PARAMACCESSOR)
- ValDef(mods withAnnotations vd.mods.annotations, vd.name, vd.tpt.duplicate, vd.rhs.duplicate)
+ ValDef(mods withAnnotations vd.mods.annotations, vd.name, vd.tpt.duplicate, duplicateAndKeepPositions(vd.rhs))
}
+ param
}
+
val (edefs, rest) = body span treeInfo.isEarlyDef
val (evdefs, etdefs) = edefs partition treeInfo.isEarlyValDef
val gvdefs = evdefs map {
@@ -378,15 +380,21 @@ abstract class TreeGen extends macros.TreeBuilder {
// this means that we don't know what will be the arguments of the super call
// therefore here we emit a dummy which gets populated when the template is named and typechecked
Some(
- // TODO: previously this was `wrappingPos(superPos, lvdefs ::: argss.flatten)`
- // is it going to be a problem that we can no longer include the `argss`?
- atPos(wrappingPos(superPos, lvdefs)) (
+ atPos(wrappingPos(superPos, lvdefs ::: vparamss1.flatten).makeTransparent) (
DefDef(constrMods, nme.CONSTRUCTOR, List(), vparamss1, TypeTree(), Block(lvdefs ::: List(superCall), Literal(Constant())))))
}
}
constr foreach (ensureNonOverlapping(_, parents ::: gvdefs, focus = false))
// Field definitions for the class - remove defaults.
- val fieldDefs = vparamss.flatten map (vd => copyValDef(vd)(mods = vd.mods &~ DEFAULTPARAM, rhs = EmptyTree))
+
+ val fieldDefs = vparamss.flatten map (vd => {
+ val field = copyValDef(vd)(mods = vd.mods &~ DEFAULTPARAM, rhs = EmptyTree)
+ // Prevent overlapping of `field` end's position with default argument's start position.
+ // This is needed for `Positions.Locator(pos).traverse` to return the correct tree when
+ // the `pos` is a point position with all its values equal to `vd.rhs.pos.start`.
+ if(field.pos.isRange && vd.rhs.pos.isRange) field.pos = field.pos.withEnd(vd.rhs.pos.start - 1)
+ field
+ })
global.Template(parents, self, gvdefs ::: fieldDefs ::: constr ++: etdefs ::: rest)
}
View
8 test/files/presentation/t4287.check
@@ -1,11 +1,11 @@
reload: Foo.scala
-askHyperlinkPos for `f ` at (1,24) Foo.scala
+askHyperlinkPos for `B` at (1,24) Foo.scala
================================================================================
-[response] found askHyperlinkPos for `f ` at (1,15) Foo.scala
+[response] found askHyperlinkPos for `B` at (3,8) Foo.scala
================================================================================
-askHyperlinkPos for `f ` at (1,31) Foo.scala
+askHyperlinkPos for `a` at (1,31) Foo.scala
================================================================================
-[response] found askHyperlinkPos for `f ` at (1,15) Foo.scala
+[response] found askHyperlinkPos for `a` at (4,7) Foo.scala
================================================================================
View
11 test/files/presentation/t4287c.check
@@ -0,0 +1,11 @@
+reload: Foo.scala
+
+askHyperlinkPos for `A` at (1,18) Foo.scala
+================================================================================
+[response] found askHyperlinkPos for `A` at (3,8) Foo.scala
+================================================================================
+
+askHyperlinkPos for `a` at (1,25) Foo.scala
+================================================================================
+[response] found askHyperlinkPos for `a` at (4,7) Foo.scala
+================================================================================
View
1 test/files/presentation/t4287c.flags
@@ -0,0 +1 @@
+-Yinfer-argument-types
View
3 test/files/presentation/t4287c/Test.scala
@@ -0,0 +1,3 @@
+import scala.tools.nsc.interactive.tests.InteractiveTest
+
+object Test extends InteractiveTest
View
9 test/files/presentation/t4287c/src/Foo.scala
@@ -0,0 +1,9 @@
+class A(a: Int = A/*#*/.a/*#*/)
+
+object A {
+ val a = 2
+}
+
+class B extends A {
+ def this(a) = this()
+}
View
30 test/files/run/t4287inferredMethodTypes.check
@@ -0,0 +1,30 @@
+[[syntax trees at end of typer]] // newSource1.scala
+[0:92]package [0:0]<empty> {
+ [0:21]class A extends [7:21][23]scala.AnyRef {
+ [8:16]<paramaccessor> private[this] val a: [8]Int = _;
+ <8:20>def <init>(<8:20>a: [11]<type: [11]scala.Int> = [17:20]A.a): [7]A = <8:20>{
+ <8:20><8:20><8:20>A.super.<init>();
+ <8:20>()
+ }
+ };
+ [23:47]object A extends [32:47][49]scala.AnyRef {
+ [49]def <init>(): [32]A.type = [49]{
+ [49][49][49]A.super.<init>();
+ [32]()
+ };
+ [36:45]private[this] val a: [40]Int = [44:45]2;
+ [40]<stable> <accessor> def a: [40]Int = [40][40]A.this.a;
+ [8]<synthetic> def <init>$default$1: [8]Int = [19]A.a
+ };
+ [49:92]class B extends [57:92][65:66]A {
+ [65]def <init>(): [57]B = [65]{
+ [65][65][65]B.super.<init>([65]A.<init>$default$1);
+ [57]()
+ };
+ [70:90]def <init>([79:80]a: [79]Int): [74]B = [84:90]{
+ [84:90][84:90][84]B.this.<init>();
+ [84]()
+ }
+ }
+}
+
View
25 test/files/run/t4287inferredMethodTypes.scala
@@ -0,0 +1,25 @@
+import scala.tools.partest.DirectTest
+
+object Test extends DirectTest {
+
+ override def extraSettings: String =
+ s"-usejavacp -Yinfer-argument-types -Xprint-pos -Xprint:typer -Yrangepos -Ystop-after:typer -d ${testOutput.path}"
+
+ override def code = """
+class A(a: Int = A.a)
+
+object A {
+ val a = 2
+}
+
+class B extends A {
+ def this(a) = this()
+}
+ """.trim
+
+ override def show(): Unit = {
+ Console.withErr(System.out) {
+ compile()
+ }
+ }
+}
View
4 test/files/run/t5603.check
@@ -10,10 +10,10 @@
[87:209]class C extends [94:209][151:159]Greeting {
[119:139]val nameElse = _;
[95:101]<paramaccessor> private[this] val i: [98:101]Int = _;
- <119:139>def <init>([95]i: [98]Int) = <119:139>{
+ <95:139>def <init>(<95:101>i: [98]Int) = <95:139>{
<119:139>val nameElse = <134:139>"Bob";
[NoPosition][NoPosition][NoPosition]super.<init>();
- [94]()
+ <95:139>()
};
[168:184]val name = [179:184]"avc";
[191:203][191:198]println([199:202]msg)

0 comments on commit 4936c43

Please sign in to comment.