Simplify Source Positions #5713

odersky · 2019-01-15T12:57:20Z

Based on #5644, this PR simplifies and clarifies the handling of source positions.

The source file of a tree is always given explicitly when we construct the tree.
The initial span of the tree is computed from all children that have the
same source as the tree.
Computing the initial span of a tree or setting it using withSpan also fills in children's spans where possible. A filled-in child span is one of
- the end position of the sibling tree to its left, if one exists
- otherwise, the start position of the parent span, if it exists
- otherwise the start position of the sibling tree to its right, if it exists
- otherwise the span is left absent, to be filled in when parents further up are defined.

With these changes, there were now various trees that got rejected since they do not have a position (typically since all children come from different sources). This required various fixes, mostly in Inliner and Stager.

dottybot

Hello, and thank you for opening this PR! 🎉

All contributors have signed the CLA, thank you! ❤️

Commit Messages

We want to keep history, but for that to actually be useful we have
some rules on how to format our commit messages (relevant xkcd).

Please stick to these guidelines for commit messages:

Separate subject from body with a blank line

When fixing an issue, start your commit message with Fix #<ISSUE-NBR>:

Limit the subject line to 72 characters

Capitalize the subject line

Do not end the subject line with a period

Use the imperative mood in the subject line ("Add" instead of "Added")

Wrap the body at 80 characters

Use the body to explain what and why vs. how

adapted from https://chris.beams.io/posts/git-commit

Have an awesome day! ☀️

odersky · 2019-01-15T13:22:02Z

test performance please

dottybot · 2019-01-15T13:22:33Z

performance test scheduled: 3 job(s) in queue, 1 running.

odersky · 2019-01-15T13:28:59Z

Unfortunately, but not unexpectedly, there's again a bunch of REPL tests that break. REPL seems to build trees with multiple source files that are really all different objects standing for the same file. That's a bad idea now, since it will result in missing positions.

odersky · 2019-01-15T14:15:44Z

test performance please

dottybot · 2019-01-15T14:16:53Z

performance test scheduled: 3 job(s) in queue, 1 running.

dottybot · 2019-01-15T17:26:31Z

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/5713/ to see the changes.

Benchmarks is based on merging with master (615fdc9)

odersky · 2019-01-15T19:06:12Z

Performance looks OK. So maybe we finally reach the end of the tunnel?

odersky · 2019-01-15T20:09:05Z

@nicolasstucki Could you give this another round for getting the repl tests to pass? Thanks!

nicolasstucki · 2019-01-16T06:09:06Z

I will have a look at the repl failures.

nicolasstucki · 2019-01-16T10:34:40Z

Rebased

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

nicolasstucki · 2019-01-16T12:41:27Z

There is still an issue with SuperAccessors. It can be reproduced with
testOnly dotty.tools.repl.DocTests -- dotty.tools.repl.DocTests.docOfObject.

[error]     at dotty.tools.dotc.transform.SuperAccessors.wrapTemplate(SuperAccessors.scala:188)

is called twice with the same object.

nicolasstucki · 2019-01-16T13:50:28Z

Not anymore. Now it only happens when calling withSpan explicitly.

smarter · 2019-01-16T13:54:11Z

Are you sure ? The initial span in Positioned is computed using span = envelope(src) and envelope does look at the span of children to compute the span of the current element.

nicolasstucki · 2019-01-16T13:59:39Z

I missed that one.

nicolasstucki · 2019-01-16T14:43:39Z

The issue is that they come from two different files <console> and rs$line$1. That is why it is not assigned. Though in theory, those come from the same line.

allanrenucci · 2019-01-16T14:58:48Z

The issue is that they come from two different files <console> and rs$line$1. That is why it is not assigned. Though in theory, those come from the same line.

Then let's use the same filename 😄. I am wondering if we could use <console> everywhere. For instance, here: https://github.com/lampepfl/dotty/blob/4ecbb60119008febec99c153025ecd03da428c12/compiler/src/dotty/tools/repl/ReplCompiler.scala#L144
and here: https://github.com/lampepfl/dotty/blob/4ecbb60119008febec99c153025ecd03da428c12/compiler/src/dotty/tools/repl/ReplDriver.scala#L168
Or if the file name needs to match the name of the wrapper object for some reason

smarter · 2019-01-16T15:08:10Z

Then let's use the same filename

That used to work but one commit in this PR actually changed equality to not be based on filenames: 1a358c0 this is more correct in general but means we can't get away with just using the same name for the repl

nicolasstucki · 2019-01-16T16:04:44Z

I'm already working on that. The solution is to have the same SourceFile instance. This requires a bit of refactoring.

nicolasstucki · 2019-01-16T16:54:59Z

test performance please

dottybot · 2019-01-16T16:56:49Z

performance test scheduled: 1 job(s) in queue, 1 running.

dottybot · 2019-01-16T19:30:41Z

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/5713/ to see the changes.

Benchmarks is based on merging with master (5547a0b)

nicolasstucki

Commits for @odersky LGTM

odersky · 2019-01-18T12:54:15Z

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

-        if (f == null || f.isVirtual)
-          // don't trust virtual files for tree creation - replTest fails otherwise
-          ctx.source
+        if (f == null) ctx.source


Relieved that we could remove this!

odersky · 2019-01-18T13:00:48Z

compiler/src/dotty/tools/repl/ParseResult.scala


 import scala.annotation.internal.sharable

 /** A parsing result from string input */
 sealed trait ParseResult

 /** An error free parsing resulting in a list of untyped trees */
-case class Parsed(sourceCode: String, trees: List[untpd.Tree]) extends ParseResult


That looks much better.

odersky · 2019-01-18T13:03:10Z

Commits for @nicolasstucki LGTM

odersky · 2019-01-18T14:26:48Z

I ran some stats to find out how many SimpleIdentityMaps of size > 4 we create, and of what size. Test inputs is everything in dotc/typer (about 10000 loc).

Here's what we have:

SimpleIdentityMap/5 -> 205
SimpleIdentityMap/6 -> 78
SimpleIdentityMap/7 -> 28
SimpleIdentityMap/8 -> 10
SimpleIdentityMap/9 -> 2

Nothing beyond that.

smarter

Great stuff !

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

compiler/src/dotty/tools/dotc/util/SourceFile.scala

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

compiler/src/dotty/tools/dotc/parsing/Parsers.scala

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

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

library/src/scala/annotation/transientParam.scala

This is more efficient, especially for small sizes where setup cost for Array.copy is higher.

This makes the source change tracking in MacroTransform redundant.

odersky · 2019-01-18T17:14:38Z

@smarter All points addressed now, I think.

smarter

👍

smarter · 2019-01-18T17:32:38Z

compiler/src/dotty/tools/dotc/ast/Trees.scala


-      def Ident(tree: Tree)(name: Name): Ident = tree match {
+      protected def srcCtx(tree: Tree)(implicit ctx: Context) = ctx.withSource(tree.source)


This method is never used.

smarter · 2019-01-18T17:33:55Z

compiler/src/dotty/tools/dotc/typer/Inliner.scala


      /** Removes all Inlined trees, replacing them with blocks.
       *  Repositions all trees directly inside an inlined expansion of a non empty call to the position of the call.
       *  Any tree directly inside an empty call (inlined in the inlined code) retains their position.
       */
      class Reposition extends TreeMap {
        override def transform(tree: Tree)(implicit ctx: Context): Tree = {
-          tree match {
+          if (tree.source != ctx.source && tree.source.exists)


No longer needed since TreeMap does that now.

smarter · 2019-01-18T17:34:46Z

compiler/src/dotty/tools/dotc/parsing/Parsers.scala

@@ -71,11 +71,11 @@ object Parsers {
    /** Positions tree.
     *  If `t` does not have a position yet, set its position to the given one.


position -> span ?

smarter · 2019-01-18T17:36:05Z

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

                                            // Nodes which have the same positions as their parents are omitted.
                                            // offset_Deltas give difference of start/end offset wrt to the
                                            // same offset in the previously recorded node (or 0 for the first recorded node)
  Delta         = Int                       // Difference between consecutive offsets,
+  SOURCE        = 4                         // Impossible as header, since addr_Delta = 0 implies that we refer to the
+                                            // same treeas the previous one, but then hasStartDiff = 1 implies that


Suggested change

// same treeas the previous one, but then hasStartDiff = 1 implies that

// same tree as the previous one, but then hasStartDiff = 1 implies that

... and bump major Tasty version Also: drop unused method

dottybot reviewed Jan 15, 2019

View reviewed changes

odersky force-pushed the refactor-sourcepos branch from d5dfe16 to c816b6e Compare January 15, 2019 13:04

odersky force-pushed the refactor-sourcepos branch from 36c1314 to f494bd1 Compare January 15, 2019 19:20

odersky assigned nicolasstucki Jan 15, 2019

odersky requested a review from nicolasstucki January 15, 2019 20:09

This was referenced Jan 15, 2019

Improve Source Positions #5644

Closed

Finish Position-Related Renamings #5697

Closed

nicolasstucki force-pushed the refactor-sourcepos branch from c5635cb to 4f58ba4 Compare January 16, 2019 10:34

smarter reviewed Jan 16, 2019

View reviewed changes

compiler/src/dotty/tools/dotc/typer/Typer.scala Outdated Show resolved Hide resolved

nicolasstucki reviewed Jan 18, 2019

View reviewed changes

nicolasstucki requested review from smarter and nicolasstucki January 18, 2019 12:46

nicolasstucki assigned smarter and unassigned nicolasstucki Jan 18, 2019

odersky commented Jan 18, 2019

View reviewed changes

nicolasstucki mentioned this pull request Jan 18, 2019

Unused special case for AnnotatedType in RefinedPrinter #4657

Closed

smarter requested changes Jan 18, 2019

View reviewed changes

odersky added 5 commits January 18, 2019 17:39

Optimization: change Array.copy to System.arrayCopy

4ef3cea

This is more efficient, especially for small sizes where setup cost for Array.copy is higher.

Rename Parser.atPos -> atSpan

21b3904

Track source changes in TreeMap and TreeAccumulator

360510c

This makes the source change tracking in MacroTransform redundant.

Address other review comments

829acf0

Rename transientParam -> constructorOnly

9fa170c

Address overlooked comment

8d3b7ef

smarter approved these changes Jan 18, 2019

View reviewed changes

smarter reviewed Jan 18, 2019

View reviewed changes

Drop unnecessary compatibility hack

652b570

... and bump major Tasty version Also: drop unused method

odersky force-pushed the refactor-sourcepos branch from 005ff4e to 652b570 Compare January 18, 2019 17:38

odersky added 2 commits January 18, 2019 18:45

Talk consistently about spans in Parser

570b26f

Address more review comments

fc2e654

odersky merged commit e724058 into scala:master Jan 18, 2019

Blaisorblade deleted the refactor-sourcepos branch January 19, 2019 13:02

biboudis modified the milestones: 0.14 Tech Preview, 0.13 Tech Preview Jan 30, 2019


		def Ident(tree: Tree)(name: Name): Ident = tree match {
		protected def srcCtx(tree: Tree)(implicit ctx: Context) = ctx.withSource(tree.source)

		@@ -71,11 +71,11 @@ object Parsers {
		/** Positions tree.
		* If `t` does not have a position yet, set its position to the given one.

	// same treeas the previous one, but then hasStartDiff = 1 implies that
	// same tree as the previous one, but then hasStartDiff = 1 implies that

Simplify Source Positions #5713

Simplify Source Positions #5713

Uh oh!

Conversation

odersky commented Jan 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dottybot left a comment

Choose a reason for hiding this comment

Commit Messages

Uh oh!

odersky commented Jan 15, 2019

Uh oh!

dottybot commented Jan 15, 2019

Uh oh!

odersky commented Jan 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

odersky commented Jan 15, 2019

Uh oh!

dottybot commented Jan 15, 2019

Uh oh!

dottybot commented Jan 15, 2019

Uh oh!

odersky commented Jan 15, 2019

Uh oh!

odersky commented Jan 15, 2019

Uh oh!

nicolasstucki commented Jan 16, 2019

Uh oh!

nicolasstucki commented Jan 16, 2019

Uh oh!

Uh oh!

nicolasstucki commented Jan 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolasstucki commented Jan 16, 2019

Uh oh!

smarter commented Jan 16, 2019

Uh oh!

nicolasstucki commented Jan 16, 2019

Uh oh!

nicolasstucki commented Jan 16, 2019

Uh oh!

allanrenucci commented Jan 16, 2019

Uh oh!

smarter commented Jan 16, 2019

Uh oh!

nicolasstucki commented Jan 16, 2019

Uh oh!

nicolasstucki commented Jan 16, 2019

Uh oh!

dottybot commented Jan 16, 2019

Uh oh!

dottybot commented Jan 16, 2019

Uh oh!

nicolasstucki left a comment

Choose a reason for hiding this comment

Uh oh!

odersky Jan 18, 2019

Choose a reason for hiding this comment

Uh oh!

odersky Jan 18, 2019

Choose a reason for hiding this comment

Uh oh!

odersky commented Jan 18, 2019

Uh oh!

odersky commented Jan 18, 2019

Uh oh!

smarter left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

odersky commented Jan 15, 2019 •

edited

Loading

odersky commented Jan 15, 2019 •

edited

Loading

nicolasstucki commented Jan 16, 2019 •

edited

Loading