Link from tasty #2910

nicolasstucki · 2017-07-24T20:38:30Z

No description provided.

smarter

This duplicates the already existing ClassSymbol#tree

nicolasstucki · 2017-07-25T07:20:35Z

@smarter deduplicated ClassSymbol#tree

nicolasstucki · 2017-07-28T14:34:31Z

The requested change has been made.

smarter · 2017-07-28T14:52:45Z

Thanks, not sure if I'll have time to review this today, this isn't urgent anyway is it?

nicolasstucki · 2017-07-28T15:02:53Z

It is not urgent.
It will help me expose some bug later on and make sure that the current state does not break.

nicolasstucki · 2017-08-04T12:54:11Z

Will need rebase on #2958

smarter · 2017-08-04T13:11:46Z

compiler/src/dotty/tools/dotc/core/Symbols.scala

+      * return the PackageDef tree for this class, otherwise EmptyTree.
+      * This will force the info of the class.
+      */
+    def unitTree(implicit ctx: Context): tpd.Tree /* tpd.PackageDef | tpd.TypeDef | tpd.EmptyTree */ = {


I'm unhappy about having both unitTree and tree. I think we should just have unitTree (but give it a different name like just tree because it's not the full compilation unit tree, it's only the current class tree wrapped with PackageDefs).

smarter · 2017-08-04T13:12:33Z

compiler/src/dotty/tools/dotc/core/Symbols.scala

+      else findTree(tree).get
+    }
+
+    /** If this is a top-level class, and if `-Yretain-trees` or `-YlinkOptimise` is set,


Can we please decide if options should be written -like-this or -likeThis and stick to it? Having both is really annoying.

Oh, it is actually -Xlink-optimise and this is a typo.

nicolasstucki · 2017-08-16T12:32:48Z

@smarter ping review

nicolasstucki · 2017-08-22T15:40:55Z

Rebased

nicolasstucki · 2017-08-31T14:19:01Z

Rebased

smarter · 2017-09-01T15:18:02Z

compiler/src/dotty/tools/dotc/CompilationUnit.scala

+  def mkCompilationUnit(clsd: ClassDenotation, unpickled: Tree)(implicit ctx: Context): CompilationUnit = {
+    val unit1 = new CompilationUnit(new SourceFile(clsd.symbol.sourceFile, Seq()))
+    unit1.tpdTree = unpickled
+    force.traverse(unit1.tpdTree)


Do you actually need to force the tree for the linker?

When we first tried it with @DarkDimius we needed it. But it looks like we do not need it anymore.

smarter · 2017-09-01T15:18:19Z

compiler/src/dotty/tools/dotc/Compiler.scala

@@ -47,6 +47,7 @@ class Compiler {
      List(new PostTyper),          // Additional checks and cleanups after type checking
      List(new sbt.ExtractAPI),     // Sends a representation of the API of classes to sbt via callbacks
      List(new Pickler),            // Generate TASTY info
+      List(new LinkAll),            // Link all


Not a very detailed description :)

smarter · 2017-09-01T15:19:36Z

compiler/src/dotty/tools/dotc/config/ScalaSettings.scala

@@ -102,6 +102,7 @@ class ScalaSettings extends Settings.SettingGroup {
  val YcheckAllPatmat = BooleanSetting("-Ycheck-all-patmat", "Check exhaustivity and redundancy of all pattern matching (used for testing the algorithm)")
  val YretainTrees = BooleanSetting("-Yretain-trees", "Retain trees for top-level classes, accessible from ClassSymbol#tree")
  val YshowTreeIds = BooleanSetting("-Yshow-tree-ids", "Uniquely tag all tree nodes in debugging output.")
+  val YRunClasspath = PathSetting("-YRunClasspath", "Specify where to find user class files while executing run tests.", defaultClasspath)


If this only for tests it shouldn't be a compiler option. You can use a property instead like the stuff in compiler/test/dotty/Properties.scala

smarter · 2017-09-01T15:20:01Z

compiler/src/dotty/tools/dotc/config/ScalaSettings.scala

@@ -113,6 +114,7 @@ class ScalaSettings extends Settings.SettingGroup {
  val YoptPhases = PhasesSetting("-Yopt-phases", "Restrict the optimisation phases to execute under -optimise.")
  val YoptFuel = IntSetting("-Yopt-fuel", "Maximum number of optimisations performed under -optimise.", -1)
  val optimise = BooleanSetting("-optimise", "Generates faster bytecode by applying local optimisations to the .program") withAbbreviation "-optimize"
+  val XlinkOptimise = BooleanSetting("-Xlink-optimise", "Link class files.").withAbbreviation("-Xlink-optimize")


Longer description?

smarter · 2017-09-01T15:22:32Z

compiler/src/dotty/tools/dotc/core/Symbols.scala

-     *  for this class, otherwise EmptyTree. This will force the info of the class.
-     */
-    def tree(implicit ctx: Context): tpd.Tree /* tpd.TypeDef | tpd.EmptyTree */ = {
+    /** If this is a top-level class, and if `-Yretain-trees` or `-Xlink-optimise` is set,


This comment is not 100% true since if this is a top-level class and -Xlink-optimise is set, but the class comes from the sources and not tasty, it will return EmptyTree.

smarter · 2017-09-01T15:26:06Z

compiler/src/dotty/tools/dotc/transform/FirstTransform.scala

@@ -154,6 +154,9 @@ class FirstTransform extends MiniPhaseTransform with InfoTransformer with Annota
    cpy.Template(impl)(self = EmptyValDef)
  }

+  override def transformPackageDef(tree: PackageDef)(implicit ctx: Context, info: TransformerInfo): Tree =


Add a comment explaining why this is needed?

smarter · 2017-09-01T15:26:57Z

compiler/src/dotty/tools/dotc/transform/FirstTransform.scala

@@ -154,6 +154,9 @@ class FirstTransform extends MiniPhaseTransform with InfoTransformer with Annota
    cpy.Template(impl)(self = EmptyValDef)
  }

+  override def transformPackageDef(tree: PackageDef)(implicit ctx: Context, info: TransformerInfo): Tree =
+    if (tree.stats.isEmpty) theEmptyTree else tree


Why theEmptyTree and not EmptyTree ?

smarter · 2017-09-01T15:42:06Z

compiler/src/dotty/tools/dotc/transform/LinkAll.scala

+
+  override def phaseName = "linkAll"
+
+  override def prepareForUnit(tree: tpd.Tree)(implicit ctx: Context): TreeTransform = NoTransform


see comment

smarter · 2017-09-01T15:44:14Z

compiler/src/dotty/tools/dotc/transform/LinkAll.scala

+    else super.runOn(units)
+
+  /** Loads and processes new compilation units, possibly loading more units. */
+  private def allUnits(processed: Set[CompilationUnit], unprocessed: Set[CompilationUnit], loadedClasses: Set[ClassDenotation])(implicit ctx: Context): List[CompilationUnit] = {


I would use an inner method for the recursive calls and not have the parameters processed and loadedClasses on the outer method.

smarter · 2017-09-01T15:50:23Z

compiler/src/dotty/tools/dotc/transform/LinkAll.scala

+        val acc1 = this(acc, constr)
+        inParents = true
+        val acc2 = this(acc1, parents)
+        inParents = false


Don't think this is correct in general, you assume that inParents will be false before you set it to true, but you can have a Template inside the parents of a Template, e.g:

class A(x: Int) class B extends A({ class Foo; 1 })

This works, I added a test for it.

nicolasstucki · 2017-09-05T08:26:47Z

@smarter, all requested changes have been done.

smarter requested changes Jul 24, 2017

View reviewed changes

nicolasstucki force-pushed the link-from-tasty branch from d544c1c to 3117430 Compare July 25, 2017 07:16

nicolasstucki added stat:wip stat:needs review and removed stat:wip labels Jul 25, 2017

nicolasstucki force-pushed the link-from-tasty branch 2 times, most recently from 08618cf to adab2a8 Compare July 25, 2017 14:36

nicolasstucki mentioned this pull request Aug 2, 2017

Issue with GADTs tasty types #2944

Closed

nicolasstucki added the stat:do not merge label Aug 4, 2017

nicolasstucki force-pushed the link-from-tasty branch from 30abe4f to 1a991ff Compare August 4, 2017 12:56

smarter reviewed Aug 4, 2017

View reviewed changes

nicolasstucki force-pushed the link-from-tasty branch from deb434c to caaeceb Compare August 8, 2017 13:47

nicolasstucki removed the stat:do not merge label Aug 8, 2017

nicolasstucki force-pushed the link-from-tasty branch from b8f8d36 to 9db592a Compare August 8, 2017 15:12

nicolasstucki assigned smarter Aug 15, 2017

nicolasstucki force-pushed the link-from-tasty branch from 9db592a to 2b2f22e Compare August 22, 2017 15:40

nicolasstucki force-pushed the link-from-tasty branch from 2b2f22e to 56987cb Compare August 31, 2017 14:18

smarter reviewed Sep 1, 2017

View reviewed changes

nicolasstucki force-pushed the link-from-tasty branch 3 times, most recently from ec50c12 to 1ce86d4 Compare September 5, 2017 08:03

nicolasstucki added 13 commits September 7, 2017 10:51

Load compilation units from tasty

7b0b245

Add link tests

96747c8

Unify tree loaders

da829a3

Move loadCompilationUnit out of FromTasty

7bbc52e

Remove empty packages loaded from tasty

23e2821

Update tests

f52541e

Remove unitTree and use tree instead

eb9afe2

Cleanup after check test failure

a2d0783

Extract -Xlink-optimise tests from vulpix

78548e0

Do not force trees when linking

7906928

Add documentation and improve code

5115c7f

Add example with Template inside parent

86e0e32

Add documentation

1f3c305

nicolasstucki force-pushed the link-from-tasty branch from 1ce86d4 to 8a4071a Compare September 7, 2017 08:56

Add clear separation between flags and classpath in tests

4f0b070

nicolasstucki force-pushed the link-from-tasty branch from 8a4071a to 4f0b070 Compare September 7, 2017 08:58

smarter approved these changes Sep 15, 2017

View reviewed changes

nicolasstucki merged commit d46d7a3 into scala:master Sep 15, 2017

allanrenucci deleted the link-from-tasty branch December 14, 2017 19:23


		override def phaseName = "linkAll"

		override def prepareForUnit(tree: tpd.Tree)(implicit ctx: Context): TreeTransform = NoTransform

Link from tasty #2910

Link from tasty #2910

Uh oh!

Conversation

nicolasstucki commented Jul 24, 2017

Uh oh!

smarter left a comment

Choose a reason for hiding this comment

Uh oh!

nicolasstucki commented Jul 25, 2017

Uh oh!

nicolasstucki commented Jul 28, 2017

Uh oh!

smarter commented Jul 28, 2017

Uh oh!

nicolasstucki commented Jul 28, 2017

Uh oh!

nicolasstucki commented Aug 4, 2017

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!

nicolasstucki commented Aug 16, 2017

Uh oh!

nicolasstucki commented Aug 22, 2017

Uh oh!

nicolasstucki commented Aug 31, 2017

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!

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!

nicolasstucki commented Sep 5, 2017

Uh oh!

Uh oh!