scala / scala-dev

Scala 2 team issues. Not for user-facing bugs or directly actionable user-facing improvements. For build/test/infra and for longer-term planning and idea tracking. Our bug tracker is at https://github.com/scala/bug/issues
Apache License 2.0
130 stars 15 forks source link

positions incorrect for `this` references in traits #186

Closed retronym closed 8 years ago

retronym commented 8 years ago
package p1
trait C {
  def test = {
    val c = 1
    try {
      toString
    } finally {
      toString
    }
  }
}
 public static java.lang.String test$(p1.C);
    descriptor: (Lp1/C;)Ljava/lang/String;
    flags: ACC_PUBLIC, ACC_STATIC
    Code:
      stack=2, locals=3, args_size=1
         0: iconst_1
         1: istore_1
         2: aload_0
         3: invokevirtual #15                 // Method java/lang/Object.toString:()Ljava/lang/String;
         6: goto          17
         9: astore_2
        10: aload_0
        11: invokevirtual #15                 // Method java/lang/Object.toString:()Ljava/lang/String;
        14: pop
        15: aload_2
        16: athrow
        17: aload_0
        18: invokevirtual #15                 // Method java/lang/Object.toString:()Ljava/lang/String;
        21: pop
        22: areturn
      Exception table:
         from    to  target type
             2     9     9   any
      LocalVariableTable:
        Start  Length  Slot  Name   Signature
            1      21     1     c   I
            0      23     0 $this   Lp1/C;
      LineNumberTable:
        line 4: 0
        line 3: 2
        line 3: 10
        line 8: 15
        line 3: 17
        line 8: 22

Looks good if we make it private:

  private java.lang.String test();
    descriptor: ()Ljava/lang/String;
    flags: ACC_PRIVATE
    Code:
      stack=2, locals=3, args_size=1
         0: iconst_1
         1: istore_1
         2: aload_0
         3: invokevirtual #13                 // Method java/lang/Object.toString:()Ljava/lang/String;
         6: goto          17
         9: astore_2
        10: aload_0
        11: invokevirtual #13                 // Method java/lang/Object.toString:()Ljava/lang/String;
        14: pop
        15: aload_2
        16: athrow
        17: aload_0
        18: invokevirtual #13                 // Method java/lang/Object.toString:()Ljava/lang/String;
        21: pop
        22: areturn
      Exception table:
         from    to  target type
             2     9     9   any
      LocalVariableTable:
        Start  Length  Slot  Name   Signature
            1      21     1     c   I
            0      23     0  this   Lp1/C;
      LineNumberTable:
        line 4: 0
        line 6: 2
        line 8: 10

so I suspect the problem stems from some detail about positions when we split the method into the test/test$ pair in the backend.

retronym commented 8 years ago

This seems to be one way to fix it:

diff --git a/src/compiler/scala/tools/nsc/ast/TreeGen.scala b/src/compiler/scala/tools/nsc/ast/TreeGen.scala
index bc89609..07bb129 100644
--- a/src/compiler/scala/tools/nsc/ast/TreeGen.scala
+++ b/src/compiler/scala/tools/nsc/ast/TreeGen.scala
@@ -350,7 +350,7 @@ abstract class TreeGen extends scala.reflect.internal.TreeGen with TreeDSL {
       case mt @ MethodType(params, res) => copyMethodType(mt, selfParamSym :: params, res)
     })
     val selfParam = ValDef(selfParamSym)
-    val rhs = orig.rhs.substituteThis(newSym.owner, atPos(newSym.pos)(gen.mkAttributedIdent(selfParamSym)))
+    val rhs = orig.rhs.substituteThis(newSym.owner, gen.mkAttributedIdent(selfParamSym))
       .substituteSymbols(origParams, newSym.info.params.drop(1)).changeOwner(origSym -> newSym)
     treeCopy.DefDef(orig, orig.mods, orig.name, orig.tparams, (selfParam :: orig.vparamss.head) :: Nil, orig.tpt, rhs).setSymbol(newSym)
   }

An alternative will be to change ThisSubstitutor to copy the position of the original This node onto the instance Ident($this) that we are replacing it with.

retronym commented 8 years ago

Surveying only other usages of substituteThis I could find:

extmethods

Uses an unpositioned reference to $this (a single tree is shared which isn't a great practice, if we position it at the This we'll need to avoid tree sharing).

image

retronym commented 8 years ago

https://github.com/scala/scala/pull/5296