Add original tree attachment #6013

jvican · 2017-07-29T11:20:50Z

Adds an original tree attachment that allows external tools (compiler
plugins like scalameta & zinc) to keep track of the previous, unadapted
tree. The reason why this is required is explained in SD-340: scala/scala-dev#340.

This change enables the incremental compiler to detect changes in final vals: sbt/zinc#227.

It also enables a fix for scala/bug#10426 by allowing the scaladoc
compiler to let the compiler adapt constants without losing the tree that
will be shown in the Scaladoc UI.

To maintainers: I was thinking of the best way to test this, but
couldn't come up with an elegant one. Do you suggest a way I could write
a test for this? Is there a precedent in testing information carried in
the trees?

I think @lrytz is the right person to review this, since he suggested
this fix in the first place.

Fixes scala/bug#7173

lrytz

I'd be interested to see how many such attachments are created, say when compiling the std lib. Could you do the experiment?

Also, should we eliminate the attachment at some phase? Maybe in erasure?

lrytz · 2017-08-02T09:59:38Z

src/compiler/scala/tools/nsc/typechecker/Typers.scala

@@ -27,7 +27,7 @@ import scala.reflect.macros.whitebox
 *  @author  Martin Odersky
 *  @version 1.0
 */
-trait Typers extends Adaptations with Tags with TypersTracking with PatternTypers {
+trait Typers extends Adaptations with Tags with TypersTracking with PatternTypers with StdAttachments {


I that necessary? The self type (Analyzer) includes StdAttachments.

lrytz · 2017-08-02T10:01:48Z

src/compiler/scala/tools/nsc/typechecker/StdAttachments.scala

+  case class OriginalAdaptedTreeAttachment(original: Tree)
+  def markOriginal(tree: Tree, original: Tree) = tree.updateAttachment(OriginalAdaptedTreeAttachment(original))
+  def unmarkOriginal(tree: Tree): Tree = tree.removeAttachment[OriginalAdaptedTreeAttachment]
+  def isAdaptedTree(tree: Tree): Boolean = tree.attachments.get[OriginalAdaptedTreeAttachment].isDefined


I personally prefer not to have those methods, they just make you go through an indirection when reading the code.

I've done them to follow the convention. Honestly, I like it. I was hoping that there was a place in the compiler where the utils and definitions of attachments in the compiler was defined.

If we remove the attachment after erasure, we'll use both unmarkOriginal and isAdaptedTree, right? In any case, I'm happy to remove them if you think they should go away.

I also prefer the more direct style (i.e., drop the additional methods). The method names are all pretty ambiguous. Also, why Original*Adapted* -- OriginalTreeAttachment captures the same info (the presence of the attachment implies the tree is no longer the original).

I'll use the more direct style, rename methods/names and update this PR.

Ok, as a reminder: please do not include the ticket in the commit title/body. It should be in the PR description. (Title should be short, referencing the issue in the commit message causes noise on every push.)

Our CONTRIBUTING.md says

At the end of the commit message, include "Fixes scala/bug#NNNN"

Should that be changed? I think it's good to have the reference in the commit message.

I'm fine with keeping it, I'll get over my OCD that hates seeing the stale commit references pile up until the git GC kicks in at github :-)

lrytz · 2017-08-02T10:13:44Z

Related to #4880, which WIPs the idea of keeping all trees (with ConstantType) until erasure.

jvican · 2017-08-02T11:04:36Z

Related to #4880, which WIPs the idea of keeping all trees (with ConstantType) until erasure.

I'm aware of this effort, hope it gets to the finish line at some point. Until that gets merged, I think this is the perfect solution for Zinc -- I would like to support incremental changes in constants for 2.12.4 codebases.

jvican · 2017-08-02T11:07:29Z

I'd be interested to see how many such attachments are created, say when compiling the std lib. Could you do the experiment?

How should I do this? Can you point me to some docs or previous examples?

Also, should we eliminate the attachment at some phase? Maybe in erasure?

Yes, this is indeed a good idea.

lrytz · 2017-08-02T11:17:33Z

How should I do this? Can you point me to some docs or previous examples?

For example just a HashSet in some top-level object, add to it in the constructor of OriginalAdaptedTreeAttachment, and print its content at the end of the run. Then build/quick/bin/scalac -d /tmp $(find src/library -name '*.scala')

Also, should we eliminate the attachment at some phase? Maybe in erasure?
Yes, this is indeed a good idea.

I'm not sure, I was just asking the question.

jvican · 2017-08-02T11:32:06Z

For example just a HashSet in some top-level object, add to it in the constructor of OriginalAdaptedTreeAttachment, and print its content at the end of the run. Then build/quick/bin/scalac -d /tmp $(find src/library -name '*.scala')

Thanks, will try that.

I'm not sure, I was just asking the question.

I think it's a good idea, neither Zinc, scaladoc or Scalameta traverse trees after typer, so it's a good tradeoff.

adriaanm · 2017-09-20T19:52:55Z

Ping! 👋 For this to ship as part of 2.12.4, it will have to be mergeable by Sep 27.

@lrytz

Adds an original tree attachment that allows external tools (compiler plugins like scalameta & zinc) to keep track of the previous, unadapted tree. The reason why this is required is explained in SD-340: scala/scala-dev#340. This change enables the incremental compiler to detect changes in `final val`s: sbt/zinc#227. It also enables a fix for scala/bug#10426 by allowing the scaladoc compiler to let the compiler adapt literals without losing the tree that will be shown in the Scaladoc UI. To maintainers: I was thinking of the best way to test this, but couldn't come up with an elegant one. Do you suggest a way I could write a test for this? Is there a precedent in testing information carried in the trees? I think @lrytz is the right person to review this, since he suggested this fix in the first place. Fixes scala/bug#7173.

jvican · 2017-09-25T12:52:07Z

@lrytz Our little experiment gives us the following data about adapted trees:

Phases	Number
typer	12718
patmat	1300
fields	479
specialize	469
total	15063

The total includes a few adapted trees that were done by other phases but were not important enough to add to the table.

jvican · 2017-09-25T12:54:10Z

I've added a second commit that cleans the original tree attachments in pre erasure so that we free up pointers to stale trees sooner in the pipeline. I think this hits the right trade-off, so thanks for the suggestion Lukas.

The PR is now updated and ready for another review.

jvican · 2017-09-25T14:04:07Z

/rebuild

adriaanm · 2017-09-25T18:11:31Z

Seems like a real (though strange) error.

adriaanm · 2017-09-25T21:51:57Z

src/compiler/scala/tools/nsc/transform/Erasure.scala

-            case tpe => specialScalaErasure(tpe)
+        case Literal(ct) =>
+          val cleanLiteral = {
+            if (tree.attachments.get[OriginalTreeAttachment].isEmpty) tree


Use contains instead?

Or better yet, just use remove directly. If this pattern of checking whether the attachment is there before removing is faster, we should implement it once in remove (one method call that requires a class tag is also better than two)

Done in last commit!

The original tree attachment is useful for compiler plugins and macros. However, this commit constraints compiler plugins to run before erasure if they want to inspect the original trees so that we free up memory as soon as possible. This commit removes the attachment in pre erasure, taking advantage that we modified literals before.

scala-jenkins added this to the 2.12.4 milestone Jul 29, 2017

jvican mentioned this pull request Jul 29, 2017

Zinc doesn't handle constants (final val) correctly sbt/zinc#227

Closed

jvican requested a review from lrytz July 29, 2017 11:41

lrytz reviewed Aug 2, 2017

View reviewed changes

jvican force-pushed the keep-track-of-constants branch from 88c4e79 to ef66011 Compare September 25, 2017 12:04

jvican force-pushed the keep-track-of-constants branch from ef66011 to 2e9a585 Compare September 25, 2017 12:05

jvican force-pushed the keep-track-of-constants branch from 30e021e to e547463 Compare September 25, 2017 19:07

adriaanm reviewed Sep 25, 2017

View reviewed changes

jvican force-pushed the keep-track-of-constants branch from e547463 to 92a1b91 Compare September 25, 2017 21:54

adriaanm changed the title ~~Fixes scala/bug#7173: Add original tree attachment~~ Add original tree attachment Sep 25, 2017

jvican force-pushed the keep-track-of-constants branch from 92a1b91 to 493ab52 Compare September 26, 2017 07:55

lrytz approved these changes Sep 26, 2017

View reviewed changes

lrytz merged commit e8cda7f into scala:2.12.x Sep 26, 2017

olafurpg mentioned this pull request Jan 17, 2018

Adapt scalameta custom analyzer to breaking changes in scala/scala#6087. scalacommunitybuild/scalameta#3

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add original tree attachment #6013

Add original tree attachment #6013

jvican commented Jul 29, 2017 •

edited by adriaanm

lrytz left a comment

lrytz Aug 2, 2017

lrytz Aug 2, 2017

jvican Aug 2, 2017

adriaanm Sep 20, 2017 •

edited

jvican Sep 20, 2017

adriaanm Sep 20, 2017 •

edited

lrytz Sep 21, 2017

adriaanm Sep 21, 2017

lrytz commented Aug 2, 2017

jvican commented Aug 2, 2017

jvican commented Aug 2, 2017

lrytz commented Aug 2, 2017

jvican commented Aug 2, 2017

adriaanm commented Sep 20, 2017

jvican commented Sep 25, 2017 •

edited

jvican commented Sep 25, 2017

jvican commented Sep 25, 2017

adriaanm commented Sep 25, 2017

adriaanm Sep 25, 2017

jvican Sep 25, 2017

adriaanm Sep 25, 2017

jvican Sep 26, 2017

Add original tree attachment #6013

Add original tree attachment #6013

Conversation

jvican commented Jul 29, 2017 • edited by adriaanm

lrytz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adriaanm Sep 20, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adriaanm Sep 20, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lrytz commented Aug 2, 2017

jvican commented Aug 2, 2017

jvican commented Aug 2, 2017

lrytz commented Aug 2, 2017

jvican commented Aug 2, 2017

adriaanm commented Sep 20, 2017

jvican commented Sep 25, 2017 • edited

jvican commented Sep 25, 2017

jvican commented Sep 25, 2017

adriaanm commented Sep 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvican commented Jul 29, 2017 •

edited by adriaanm

adriaanm Sep 20, 2017 •

edited

adriaanm Sep 20, 2017 •

edited

jvican commented Sep 25, 2017 •

edited