Towards correct positions in TASTY types #1634

odersky · 2016-10-27T12:17:34Z

No description provided.

smarter · 2016-10-27T13:10:52Z

src/dotty/tools/dotc/core/tasty/TreeBuffer.scala

+  /** A map from `Key`s to lists of `Value`s. Optimized for the case
+   *  where only a single value is associated with a key.
+   */
+  class MultiMap[Key <: AnyRef, Value] {


I would call it IdentityMultiMap to make the semantics obvious.

smarter · 2016-10-28T00:27:10Z

I've started experimenting with renaming refactoring support (I have type at point, find all refs and jump to definition working) and I think we need to keep the point in a Position much more often to get this to work, consider:

class Foo {
  val bar: Int = 1
}
object Test {
  val foo = new Foo

  foo.bar
}

foo.bar has position [70..74..77], if I want to rename bar to something else I need to replace the range 74..77, but I don't have the point information after unpickling.

odersky · 2016-10-28T10:20:50Z

@smarter I don't think you need the point since you have the name. So you can just go back from the end position, no?

smarter · 2016-10-28T14:51:55Z

Good point, this almost work but I'm not sure what to do about backquoted names:

class Foo {
  val bar: Int = 1
}
object Test {
  val foo = new Foo

  foo.`bar`
}

I just need to go back two more characters from the end position if the name is backquoted, but how do I know this? It's not even recorded in the untyped tree.

odersky · 2016-10-29T08:37:58Z

@smarter I think you have to look at the source to find out whether it's
backquoted. In a nutshell:

If it's a definition the point will give you the beginning of the name.
If it's a selection, the end pos will give you the end of the name
Only if it's an ident you you get the range that spans the full name.

In the first two cases, you have to look at the source to make sense of it.
That includes backquotes, but currently also encodings, i.e. instead of +
you will see $plus as a name. A possible way to deal with it is to write
a simple pairs of regex parsers that can read a name backwards and forwards.

Martin

On Fri, Oct 28, 2016 at 4:51 PM, Guillaume Martres <[email protected]

wrote:

Good point, this almost work but I'm not sure what to do about backquoted
names:

class Foo {
val bar: Int = 1
}object Test {
val foo = new Foo

foo.bar
}

I just need to go back two more characters from the end position if the
name is backquoted, but how do I know this? It's not even recorded in the
untyped tree.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#1634 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAwlVleokJv_HryaV8DrpWe5nFBuYRyyks5q4gwNgaJpZM4KiR0Y
.
{"api_version":"1.0","publisher":{"api_key":"
05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":
{"external_key":"github/lampepfl/dotty","title":"
lampepfl/dotty","subtitle":"GitHub repository","main_image_url":"
https://cloud.githubusercontent.com/assets/143418/17495839/a5054eac-5d88-
11e6-95fc-7290892c7bb5.png","avatar_image_url":"https://
cloud.githubusercontent.com/assets/143418/15842166/
7c72db34-2c0b-11e6-9aed-b52498112777.png","action":{"name":"Open in
GitHub","url":"https://github.com/lampepfl/dotty"}},"
updates":{"snippets":[{"icon":"PERSON","message":"@smarter in #1634: Good
point, this almost work but I'm not sure what to do about backquoted
names:\r\nscala\r\nclass Foo {\r\n val bar: Int = 1\r\n}\r\nobject Test {\r\n val foo = new Foo\r\n\r\n foo.bar\r\n}\r\n\r\nI just need to go
back two more characters from the end position if the name is backquoted,
but how do I know this? It's not even recorded in the untyped
tree."}],"action":{"name":"View Pull Request","url":"https://
github.com//pull/1634#issuecomment-256941614"}}}

Prof. Martin Odersky
LAMP/IC, EPFL

smarter · 2016-10-29T08:59:26Z

In the first two cases, you have to look at the source to make sense of it.

Is namePos wrong then? It tries to set a position for a name as pos.point, pos.point + name.length: https://github.com/lampepfl/dotty/blob/master/src/dotty/tools/dotc/ast/Trees.scala#L309

If we want to pickle type trees, we need a type assigner for RefinedTypeTree.

Pick a less common name for the missing identifier. Depending on my classpath I sometimes got `x is not a package` as an additional error for this one.

As a side effect, avoid creating synthetic parameters in lambda abstract.

It seems like overengineering to use different names for poly methods in definitions and synthetic lambdas.

Express them in terms PolyTypeTrees rather than having an irregular, untyped only tparams field. This is necessary if we want to pickle type trees instead of types, because now the rhs of a typedef tells the whole story, so we are not required any longer to use the info of the symbol.

Lots of other changes to make positions work out everywhere. One important change is that now trees can be shared, just as types can. This change improves memory requirements (a bit) and also makes positions in shared trees more robust.

smarter · 2016-11-17T10:30:48Z

src/dotty/tools/dotc/core/tasty/TastyFormat.scala

+                  IDENTtpt              NameRef Type     // used when type ident's type is not a TypeRef
+                  SELECTtpt             NameRef qual_Term
+                  SINGLETONtpt          Path
+                  REFINDtpt      Length underlying_Term refinement_Stat*


typo: REFINEDtpt

smarter · 2016-11-17T10:32:25Z

src/dotty/tools/dotc/core/tasty/TastyPickler.scala

@@ -61,7 +61,7 @@ class TastyPickler {
   * Note that a tree can have several addresses, if it is shared,


I think that this comment needs to be updated now that we only store one address for a tree

smarter · 2016-11-17T10:32:41Z

src/dotty/tools/dotc/core/tasty/TreeBuffer.scala

@@ -17,26 +17,20 @@ class TreeBuffer extends TastyBuffer(50000) {
  private var delta: Array[Int] = _
  private var numOffsets = 0

-  private type TreeAddrs = Any // really: Addr | List[Addr]
-
  /** A map from trees to the address(es) at which a tree is pickled. There may be several
   *  such addresses if the tree is shared. To keep the map compact, the value type is a


Same for this comment.

smarter · 2016-11-17T10:47:24Z

src/dotty/tools/dotc/core/tasty/TreePickler.scala

+          }
+        case AppliedTypeTree(tycon, args) =>
+          writeByte(APPLIEDtpt)
+          withLength { pickleTree(tycon); args.foreach(pickleTree(_)) }


could be written args.foreach(pickleTree) to be consistent with other cases.

smarter · 2016-11-17T16:31:05Z

src/dotty/tools/dotc/printing/RefinedPrinter.scala

@@ -134,6 +147,8 @@ class RefinedPrinter(_ctx: Context) extends PlainPrinter(_ctx) {
        return toTextParents(tp.parentsWithArgs) ~ "{...}"
      case JavaArrayType(elemtp) =>
        return toText(elemtp) ~ "[]"
+      case tp: AnnotatedType if homogenizedView =>
+        withoutPos(super.toText(tp))


Could you add a comment explaining why this is needed?

smarter · 2016-11-17T16:33:58Z

src/dotty/tools/dotc/core/tasty/TreePickler.scala

-            val SuperType(_, mixinType) = tree.tpe
-            pickleType(mixinType)
+        case This(_) =>
+          pickleType(tree.tpe)


The qualifier of a This tree might be something we would need (for example when renaming a class), should we pickle the complete tree instead of just pickling its type?

Right now, This takes a name, not an ident, so there is no position kept anyway. This is something that should be fixed (in a separate PR?) Same for mixin qualifiers of Super trees.

smarter · 2016-11-17T19:26:58Z

src/dotty/tools/dotc/printing/RefinedPrinter.scala

@@ -148,6 +148,10 @@ class RefinedPrinter(_ctx: Context) extends PlainPrinter(_ctx) {
      case JavaArrayType(elemtp) =>
        return toText(elemtp) ~ "[]"
      case tp: AnnotatedType if homogenizedView =>
+        // Positions of annotations in types are not serialized
+        // (they don;t need to because we keep the original type tree with


Typo: don;t -> don't

smarter · 2016-11-17T19:27:45Z

src/dotty/tools/dotc/core/tasty/TastyPickler.scala

-   * Note that a tree can have several addresses, if it is shared,
-   * i.e. accessible from different paths. Any such sharing is undone by pickling.
+  /** The address in the TASTY file of a given tree, or None if unknown.
+   *  Note that trees are looked up by for reference equality,


typo: "by for reference equality" -> "by reference equality"

The qualifier of a This and the mixin of a Super were names, which meant that their positions were lost. Now they are untyped idents.

smarter · 2016-11-21T17:34:28Z

LGTM!

smarter · 2016-11-21T20:27:41Z

I had to add two commits to get the tests to pass on top of the restructuring PR (#1636):

smarter · 2016-11-22T10:26:10Z

src/dotty/tools/dotc/typer/Typer.scala

-    assignType(cpy.TypeDef(tdef)(name, typedType(rhs), Nil), sym)
+    val rhs1 = tdef.rhs match {
+      case rhs @ PolyTypeTree(tparams, body) =>
+        val tparams1 = tparams.map(typed(_)).asInstanceOf[List[TypeDef]]


A bit late, but any reason to not use typedPolyTypeTree here?

odersky added the stat:in progress label Oct 27, 2016

smarter reviewed Oct 28, 2016

View reviewed changes

odersky force-pushed the change-tasty-pos-ctd branch from 941b7f4 to 64ba394 Compare October 28, 2016 07:39

odersky added 5 commits November 11, 2016 14:53

Add assignType for RefinedTypeTrees

987c590

If we want to pickle type trees, we need a type assigner for RefinedTypeTree.

Make test more robust

4902271

Pick a less common name for the missing identifier. Depending on my classpath I sometimes got `x is not a package` as an additional error for this one.

Replace PolyType.fromSymbols with LambdaAbstract

49cef12

As a side effect, avoid creating synthetic parameters in lambda abstract.

Merge syntheticTypeParamNames and syntheticLambdaParamNames

f3c8fdd

It seems like overengineering to use different names for poly methods in definitions and synthetic lambdas.

odersky force-pushed the change-tasty-pos-ctd branch from d84f342 to 97b6985 Compare November 11, 2016 21:20

Pickle and unpickle type trees

fe20b90

Lots of other changes to make positions work out everywhere. One important change is that now trees can be shared, just as types can. This change improves memory requirements (a bit) and also makes positions in shared trees more robust.

smarter reviewed Nov 17, 2016

View reviewed changes

Address reviewer's comments.

5637720

smarter approved these changes Nov 17, 2016

View reviewed changes

odersky added 3 commits November 20, 2016 15:50

Fix typos

5c7617b

Make This and Super take idents as qualifier/mixin

7f0637c

The qualifier of a This and the mixin of a Super were names, which meant that their positions were lost. Now they are untyped idents.

Make inliner not rely on ambiguous map key

a2b1e60

dottybot added the reviewed label Nov 21, 2016

odersky merged commit 5b40951 into scala:master Nov 21, 2016

odersky removed reviewed labels Nov 21, 2016

smarter reviewed Nov 22, 2016

View reviewed changes

allanrenucci deleted the change-tasty-pos-ctd branch December 14, 2017 16:58

		@@ -61,7 +61,7 @@ class TastyPickler {
		* Note that a tree can have several addresses, if it is shared,

Towards correct positions in TASTY types #1634

Towards correct positions in TASTY types #1634

Uh oh!

Conversation

odersky commented Oct 27, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smarter commented Oct 28, 2016

Uh oh!

odersky commented Oct 28, 2016

Uh oh!

smarter commented Oct 28, 2016

Uh oh!

odersky commented Oct 29, 2016

Uh oh!

smarter commented Oct 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smarter commented Nov 21, 2016

Uh oh!

smarter commented Nov 21, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!