Give position to symbols when unpickling #3525

smarter · 2017-11-21T22:10:28Z

I started working on this to fix a crash when compiling the collection-strawman contrib tests. I minimized it to anonBridge.scala (in the collection-strawman, unpickling happened because of inline def, here the test is in from-tasty so it will be compiled from an unpickled tree). Eventually, I would also like to use the unpickled symbol positions in the IDE, but this will require more work. (Right now, going from an arbitrary Position to a SourcePosition is annoying, it requires creating a SourceFile which requires converting the source file content to an array of bytes).

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 ("Added" instead of "Add")

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! ☀️

smarter · 2017-11-21T22:24:15Z

test performance please

dottybot · 2017-11-21T22:24:20Z

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

dottybot · 2017-11-21T23:47:25Z

Performance test finished successfully:

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

Benchmarks is based on merging with master (1d24b19)

odersky

Otherwise LGTM

odersky · 2017-11-27T16:03:15Z

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

@@ -93,6 +93,7 @@ class ScalaSettings extends Settings.SettingGroup {
  val YdetailedStats = BooleanSetting("-Ydetailed-stats", "show detailed internal compiler stats (needs Stats.enabled to be set to true).")
  val Yheartbeat = BooleanSetting("-Ydetailed-stats", "show heartbeat stack trace of compiler operations (needs Stats.enabled to be set to true).")
  val Yprintpos = BooleanSetting("-Yprintpos", "show tree positions.")
+  val YprintposSyms = BooleanSetting("-Yprintpos-syms", "show symbol definitions positions.")


Since -Yprintpos is about to change to Yprint-pos, I guess we also need an additional hypen here: -Yprint-pos-syms.

Sure, in which PR is Yprintpos changed to Yprint-pos ?

odersky · 2017-11-27T16:06:14Z

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

@@ -437,7 +437,7 @@ object Symbols {

    /** Does this symbol come from a currently compiled source file? */
    final def isDefinedInCurrentRun(implicit ctx: Context): Boolean = {
-      pos.exists && defRunId == ctx.runId
+      pos.exists && defRunId == ctx.runId && sourceFile == ctx.source.file


That's not the right condition anymore. See for instance how it is used in checkNoConflict. We want to flag top-level definitions in other source files if they clash with a definition in the current source file.

We could probably use as third criterion:

{ val file = assocatedFile file != null && !file.path.endsWith(".class") }

Hmm that's tricky because associatedFile forces things, I'll give it a try.

odersky · 2017-11-27T16:19:20Z

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

@@ -1155,7 +1155,8 @@ object Parsers {
        t
    }

-    def ascription(t: Tree, location: Location.Value) = atPos(startOffset(t), in.skipToken()) {
+    def ascription(t: Tree, location: Location.Value) = atPos(startOffset(t)) {
+      in.skipToken()


Why the change? The previous implementation would place the ^ under the : of an ascription, which seems to be what we want, no?

In all other cases, the point is under the first letter of the name of the definition (e.g. for def foo: Int = 1 the point is under f). This change means that I can always use the point of the definition tree to set the symbol position.

odersky · 2017-11-27T16:20:51Z

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

-             isSimilar(d.symbol.sourceFile.path, ctx.source.file.path),
-          s"private ${d.symbol.showLocated} in ${d.symbol.sourceFile} accessed from ${ctx.owner.showLocated} in ${ctx.source.file}")
+             isSimilar(d.symbol.sourceFile.path, ctx.owner.sourceFile.path),
+          s"private ${d.symbol.showLocated} in ${d.symbol.sourceFile} accessed from ${ctx.owner.showLocated} in ${ctx.owner.sourceFile}")


When would this make a difference?

When unpickling from tasty, ctx.source.file will be "Foo.class" whereas ctx.owner.sourceFile will be the source file name found in the SourceFile annotation of the tasty tree.

smarter · 2017-11-28T20:22:34Z

@odersky There's another difficulty with isDefinedInCurrentRun: If we're compiling from TASTY then associatedFile may very well end with .class but isDefinedInCurrentRun should return true. The only solution I see here is doing a check like ctx.runInfo.units.contains(associatedFile), but that requires adding units to RunInfo (currently it's only in Run which isn't accessible from Context), what do you think?

smarter · 2017-11-30T01:55:03Z

Alternative proposal to make isDefinedInCurrentRun easy to define: track whether a symbol comes from a compilation unit when the symbol is created. This could be done by giving a special meaning to one bit of the Coord class

smarter · 2017-12-01T14:43:35Z

test performance please

smarter · 2017-12-01T14:43:54Z

@odersky Could you review the last commit please?

dottybot · 2017-12-01T14:44:07Z

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

dottybot · 2017-12-01T16:09:49Z

Performance test finished successfully:

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

Benchmarks is based on merging with master (dc52e6f)

smarter · 2017-12-06T11:15:28Z

Ping for review of the last commit

odersky · 2017-12-06T12:24:20Z

I am worried that this seems to cause a slowdown of 6% for Dotty itself. What could be the cause of this?

odersky · 2017-12-06T12:29:05Z

Last commit LGTM, but it would be good to drill down on the performance issue.

smarter · 2017-12-06T12:46:27Z

test performance please

dottybot · 2017-12-06T12:47:22Z

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

odersky · 2017-12-06T12:49:28Z

I am still not convinced this is the right thing to do. We add a lot more positions to Tasty files which makes them larger. Symbols get larger too. It seems this has a significant performance implications. So what exactly are we trying to solve here? Can't this be obtained more cheaply?

If it's just the inlining, it would be easy to fix with an additional pass when we create the Inline Body annotation.

smarter · 2017-12-06T13:39:20Z

We add a lot more positions to Tasty files which makes them larger.

Not at all! The positions of symbols are derived from the position of trees, so we're not storing anything more.

Symbols get larger too.

I don't see how, symbols already had a coord field. It's just that for many of them it was set to NoCoord previously.

It seems this has a significant performance implications.

I think the benchmark result was a fluke, we'll see. If there are performance issues I'll fix them.

So what exactly are we trying to solve here?

Unpickled trees should be as close to their pre-pickling state as possible. This reduces the number of surprises when compiling from tasty since we're using the same code paths than when compiling from sources. Positions for symbols may also allow for better error messages, and could be used in the IDE to reduce the number of tree traversals done for operations such as jump-to-definition (the current PR doesn't do this because it's not trivial).

dottybot · 2017-12-06T14:11:25Z

Performance test finished successfully:

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

Benchmarks is based on merging with master (f92f278)

smarter · 2017-12-06T14:23:37Z

Hmm it looks like we have no new benchmarks recorded for master since 20/11 (see http://dotty-bench.epfl.ch), so it's difficult to measure the effect of this PR. Ping @liufengyun

dottybot · 2017-12-06T14:26:23Z

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

dottybot · 2017-12-06T15:48:50Z

Performance test finished successfully:

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

Benchmarks is based on merging with master (7848ef6)

And add it to the set of options passed to the pickling tests so that we can start checking that symbol positions are properly pickled and unpickled.

Previously, unpickled symbols always had position NoCoord, now their position is set based on the corresponding tree position. This does not always match the position used when they were pickled so `testPickling` currently fails, this is fixed in the subsequent commits of this PR. Symbol#isDefinedInCurrentRun is broken by this commit and fixed in a latter commit in this PR.

A symbol is created before the corresponding tree is unpickled, so we cannot rely on initialPos to set the symbol position. Instead, we always pickle the position of definitions. In practice, this means that we now always pickle the position of all `TypeDef` and `Bind` (the other `DefTree`s are all `WithLazyField` and thus already had their positions pickled).

The point of a definition should be the position of the first letter in the name of a definition. For a `Bind` node this is just the start (previously we were using the position of the `:`).

Various changes so that `testPickling` with `-Yprintpos-syms` now passes. It's not always obvious what position to use when new trees are created, here are the principles we used here: - When a definition tree is replaced by another definition tree (for example, in `ParamForwarding`), the new tree position is set to the old tree position. - When a reference tree is replaced by a definition tree representing an accessor and a new reference tree that calls the accessor (for example in `SuperAccessors`), the accessor position is set to the old tree *point*, and the new reference tree position is set to the old tree *position*. This makes sense because we already use zero-extent positions for compiler artifacts that the IDE should ignore and an accessor is a compilation artifact.

When compiling from TASTY, `ctx.source.file` will be the TASTY file corresponding to the current compilation unit, this will never match `d.symbol.sourceFile` whose value is set from the `SourceFile` annotation in the TASTY tree. Fixed by using `ctx.owner.sourceFile` instead. The added test, `anonBridge.scala` needs this fix to work. It also needs the previous work done in this PR to give positions to unpickled symbols, otherwise it crashes in `Typer#assertPositioned` because `Bridges#addBridge` creates a tree whose position comes from a symbol.

And add a test to make sure it's working. The test required a change to the testing infrastructure: files in compilation groups should be sorted alphabetically, otherwise we cannot know in advance if the error will happen in A1.scala or A2.scala

smarter · 2017-12-06T15:58:45Z

Oops, used the wrong syntax to test multiple commits, trying again...

smarter · 2017-12-06T16:00:51Z

test performance please 7848ef6 6b19932

dottybot · 2017-12-06T16:01:25Z

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

dottybot · 2017-12-06T17:24:25Z

Performance test finished successfully:

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

Benchmarks is based on merging with master (7848ef6)

smarter · 2017-12-06T18:43:17Z

@liufengyun Looks like testing multiple commits doesn't work anymore, or did I get the syntax wrong?

liufengyun · 2017-12-06T18:44:39Z

You need a colon after the please. updated: semicolon -> colon Le 6 déc. 2017 19:43, "Guillaume Martres" <[email protected]> a écrit :

…

@liufengyun <https://github.com/liufengyun> Looks like testing multiple commits doesn't work anymore, or did I get the syntax wrong? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3525 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAuDybN88AqGxz2TDWnbWBcYTcSthKwtks5s9uBHgaJpZM4Qmj8t> .

odersky · 2017-12-07T13:09:38Z

OK, I see the rationale now, and it seems performance implications are smaller than first feared. So I am OK to get this in. But maybe finish the performance evaluation first.

smarter · 2017-12-09T12:13:15Z

test performance please; 18767e0 6b19932

dottybot · 2017-12-09T12:13:23Z

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

liufengyun · 2017-12-09T12:28:01Z

@smarter Sorry, it's colon instead semicolon, I've a little problem with the punctuation names.

smarter · 2017-12-09T12:35:14Z

I tried using colon in #3525 (comment) but it didn't seem to work either ?

liufengyun · 2017-12-09T12:48:00Z

Let's try a new curve and see:

retest performance please: 18767e0 6b19932

liufengyun · 2017-12-09T13:12:43Z

retest performance please: 18767e0 6b19932

dottybot · 2017-12-09T13:12:59Z

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

dottybot · 2017-12-09T13:34:57Z

Performance test finished successfully:

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

Benchmarks is based on merging with master (18767e0)

dottybot · 2017-12-09T16:15:18Z

Performance test finished successfully:

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

Benchmarks is based on merging with master (18767e0)

dottybot reviewed Nov 21, 2017

View reviewed changes

smarter requested a review from odersky November 21, 2017 22:24

nicolasstucki added the stat:needs review label Nov 24, 2017

odersky self-assigned this Nov 27, 2017

odersky requested changes Nov 27, 2017

View reviewed changes

smarter force-pushed the tasty-symbols-pos branch from a059234 to 7a34e3b Compare November 28, 2017 20:20

This was referenced Nov 29, 2017

List pos and run tests that fail from tasty #3593

Merged

Compiling pos and run tests from TASTY #3594

Closed

smarter force-pushed the tasty-symbols-pos branch 3 times, most recently from 55506fc to 908c1c9 Compare December 1, 2017 14:23

smarter force-pushed the tasty-symbols-pos branch from 908c1c9 to a4618f4 Compare December 1, 2017 14:44

smarter added 8 commits December 6, 2017 16:55

Anonymous classes should not be called $anonfun

d2d2d8f

Add -Yprintpos-syms to print the position of symbols

21e90d8

And add it to the set of options passed to the pickling tests so that we can start checking that symbol positions are properly pickled and unpickled.

Fix the .pos.point of Bind nodes

1841b3d

The point of a definition should be the position of the first letter in the name of a definition. For a `Bind` node this is just the start (previously we were using the position of the `:`).

Fix Symbol#isDefinedInCurrentRun

6b19932

And add a test to make sure it's working. The test required a change to the testing infrastructure: files in compilation groups should be sorted alphabetically, otherwise we cannot know in advance if the error will happen in A1.scala or A2.scala

smarter force-pushed the tasty-symbols-pos branch from a4618f4 to 6b19932 Compare December 6, 2017 15:59

odersky approved these changes Dec 7, 2017

View reviewed changes

smarter merged commit fd861b0 into scala:master Dec 9, 2017

allanrenucci deleted the tasty-symbols-pos branch December 9, 2017 14:55

Give position to symbols when unpickling #3525

Give position to symbols when unpickling #3525

Uh oh!

Conversation

smarter commented Nov 21, 2017

Uh oh!

dottybot left a comment

Choose a reason for hiding this comment

Commit Messages

Uh oh!

smarter commented Nov 21, 2017

Uh oh!

dottybot commented Nov 21, 2017

Uh oh!

dottybot commented Nov 21, 2017

Uh oh!

odersky left a comment

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!

smarter commented Nov 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smarter commented Nov 30, 2017

Uh oh!

smarter commented Dec 1, 2017

Uh oh!

smarter commented Dec 1, 2017

Uh oh!

dottybot commented Dec 1, 2017

Uh oh!

dottybot commented Dec 1, 2017

Uh oh!

smarter commented Dec 6, 2017

Uh oh!

odersky commented Dec 6, 2017

Uh oh!

odersky commented Dec 6, 2017

Uh oh!

smarter commented Dec 6, 2017

Uh oh!

dottybot commented Dec 6, 2017

Uh oh!

odersky commented Dec 6, 2017

Uh oh!

smarter commented Dec 6, 2017

Uh oh!

dottybot commented Dec 6, 2017

Uh oh!

smarter commented Dec 6, 2017

Uh oh!

dottybot commented Dec 6, 2017

Uh oh!

dottybot commented Dec 6, 2017

Uh oh!

smarter commented Dec 6, 2017

Uh oh!

smarter commented Dec 6, 2017

Uh oh!

dottybot commented Dec 6, 2017

Uh oh!

dottybot commented Dec 6, 2017

Uh oh!

smarter commented Nov 28, 2017 •

edited

Loading

liufengyun commented Dec 6, 2017 via email •

edited

Loading