Use symbolic names internally #2169

odersky · 2017-04-04T07:58:46Z

Change encoding to that symbolic operators are kept internally and are only emitted during the backend. The PR is based on #2128 of which it is the logical conclusion.

smarter · 2017-04-04T08:39:57Z

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

@@ -571,8 +571,8 @@ class Namer { typer: Typer =>

    /** Create links between companion object and companion class */
    def createLinks(classTree: TypeDef, moduleTree: TypeDef)(implicit ctx: Context) = {
-      val claz = ctx.effectiveScope.lookup(classTree.name.encode)
-      val modl = ctx.effectiveScope.lookup(moduleTree.name.encode)
+      val claz = ctx.denotNamed(classTree.name).symbol


Why does denotNamed makes sense here? I changed it to effectiveScope.lookup in b641181 assuming it was the right thing to do. Doesn't using denotNamed means that we could find a companion in an outer scope which would be very wrong?

Good point. I'll revert that change.

odersky · 2017-04-08T11:27:51Z

Status: I went over the whole PR documenting and polishing. Awaiting the backend to be made available so that we can apply the necessary changes to it.

smarter · 2017-04-10T16:23:14Z

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

+                  EXPANDEDPREFIX    Length qualified_NameRef selector_NameRef
+                  TRAITSETTER       Length qualified_NameRef selector_NameRef
+                  UNIQUE            Length separator_NameRef uniqid_Nat underlying_NameRef?
+									DEFAULTGETTER     Length underlying_NameRef index_Nat


Incorrect indentation here and below.

Indentation still broken.

DarkDimius · 2017-04-12T13:37:57Z

Awaiting the backend to be made available so that we can apply the necessary changes to it.

PRs by @smarter are all in and the backend has been moved to https://github.com/lampepfl/scala/.

odersky · 2017-04-28T13:43:48Z

The only remaining test failure is:

Failed tests:
[error] 	xsbt.ExtractUsedNamesSpecification

I have no idea what that is and can't find the string "ExtractUsedNamesSpecification" anywhere in the repo. @smarter or @felixmulder can you help?

smarter · 2017-04-28T13:56:58Z

@odersky It's here https://github.com/lampepfl/dotty/blob/master/sbt-bridge/test/xsbt/ExtractUsedNamesSpecification.scala

smarter · 2017-04-28T13:58:06Z

Some tests contain mangled name ($eq, $qmark, ...) which are now returned unmangled. It's safe to change the test to use the unmangled name.

odersky · 2017-04-28T14:04:27Z

@smarter Thanks! I have tried to change to symbolic names.

smarter · 2017-04-28T14:11:05Z

@odersky By the way, the report at the end of the CI output was not helpful here (since this is a regular JUnit test not part of the compiler test framework), but if you search for "ExtractUsedNamesSpecification" in the CI output, you'll find a much more detailed explanation of what failed.

odersky · 2017-04-28T14:28:38Z

Seems to all work out now. Ready for review.

smarter · 2017-04-28T14:33:19Z

Can you rebase now that #2322 has been merged?

odersky · 2017-04-28T15:55:40Z

Rebased

smarter · 2017-05-06T15:08:08Z

There's a commit called "Invalide fullName cache when name changes" (7593cf3) that doesn't seem to have anything do with cache invalidation. Did that change appear in another commit, was it lost, or intentionally dropped?

smarter · 2017-05-06T15:19:28Z

compiler/test/dotty/tools/dotc/CompilationTests.scala

@@ -244,7 +244,8 @@ class CompilationTests extends ParallelTesting {
    }.map(_.checkCompile()).foreach(_.delete())
  }

-  @Test def bytecodeIdempotency: Unit = {
+  /** Add a `z` so that hey run last. TODO: Only run them selectively? */


typo: hey -> they

smarter · 2017-05-06T15:20:10Z

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

      val thisKind = this.info.kind
      thisKind == kind ||
      !thisKind.definesNewName && thisKind.tag > kind.tag && underlying.is(kind)
    }
+
+    @sharable // because it's just a cache for performance
+    private[Names] var myMangledString: String = null


This should be private[this]

smarter · 2017-05-06T15:21:36Z

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

        apply(original, index)
      }
      else name
    }
  }

+  /** The kind of names that also encode a variance: 0 for contravariance, 1 for covariance. */


-1 for contravariance, not 0

No, it's 0 now.

smarter · 2017-05-06T15:22:20Z

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

+                  EXPANDEDPREFIX    Length qualified_NameRef selector_NameRef
+                  TRAITSETTER       Length qualified_NameRef selector_NameRef
+                  UNIQUE            Length separator_NameRef uniqid_Nat underlying_NameRef?
+									DEFAULTGETTER     Length underlying_NameRef index_Nat


Indentation still broken.

smarter · 2017-05-06T15:24:02Z

This isn't necessary for this PR to get in, but I think it would be great if we used different types for encoded and decoded names, to make sure we don't use one instead of the other. This should be possible now that we have a clearer picture of when things should be encoded/decoded.

odersky · 2017-05-06T16:23:40Z

There's a commit called "Invalide fullName cache when name changes" (7593cf3) that doesn't seem to have anything do with cache invalidation. Did that change appear in another commit, was it lost, or intentionally dropped?

That was intentionally dropped because it's now redundant. We found a way to do it without changing the name so no invalidation is necessary.

They take up time and should fail only rarely.

For many kinds this is needed because creation of a name kind involves a side effect, namely registering the name kind. If we delay this we might get a crash on unpickling because a tag is not set yet. This was observed when compiling the collection strawman test.

smarter · 2017-05-07T12:12:34Z

From http://dotty-ci.epfl.ch/lampepfl/dotty/2165/3:

[error] Test dotty.tools.dotc.CompilationTests.compilePos failed: java.lang.AssertionError: Expected no errors when compiling, but found: 0, took 54.642 sec

That's a weird error message, /cc @felixmulder

felixmulder · 2017-05-07T14:31:54Z

My guess is that an assertion fired, but unsure. I'd have to investigate

The previous change hid the refinements of `info` and apply/unapply. Turning SimpleNameKind and SignedNameKind back into objects fixes that. Neither name kind installs anything as a side effect so turning them into objects is OK.

odersky · 2017-05-07T15:12:19Z

Some interesting developments. Making all names strict uncovered a bug in avoid which is minimized in 6e3f325. It's fixed now. I experimented with making avoid an approximating typemap, but did not succeed. The current state is in staging/wip-avoid-as-approx-typemap.

smarter · 2017-05-07T15:13:38Z

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

@@ -61,8 +61,7 @@ object NameKinds {
    def infoString: String
  }

-  /** The kind of SimpleNames */
-  val SimpleNameKind = new NameKind(UTF8) { self =>
+  object SimpleNameKind extends NameKind(UTF8) { self =>


I think ultimately, we should just make the name registration thread-safe, it doesn't cost much and it's much easier to reason about than remembering which NameKinds have side-effects, etc.

But this is another issue. The problem was not threading but the fact that the registration was not performed at all, since objects are lazy.

Ah, I see, indeed that's different. If we had enums we could iterate over the namekinds to register them maybe :).

smarter reviewed Apr 4, 2017

View reviewed changes

felixmulder added this to the 0.1 Tech Preview milestone Apr 4, 2017

odersky force-pushed the change-late-op-encoding branch from 7c71b08 to 6868f97 Compare April 7, 2017 09:08

odersky force-pushed the change-late-op-encoding branch 3 times, most recently from 0dbb1de to 1a264e1 Compare April 10, 2017 14:36

smarter reviewed Apr 10, 2017

View reviewed changes

odersky force-pushed the change-late-op-encoding branch from 1a264e1 to 081a781 Compare April 11, 2017 12:08

odersky force-pushed the change-late-op-encoding branch from 081a781 to ac1415f Compare April 28, 2017 11:55

odersky force-pushed the change-late-op-encoding branch from 6005b22 to 49b8df2 Compare April 28, 2017 15:55

odersky changed the title ~~[WIP] Use symbolic names internally~~ Use symbolic names internally Apr 29, 2017

odersky added the stat:needs review label Apr 29, 2017

odersky force-pushed the change-late-op-encoding branch 3 times, most recently from e5f97e3 to 719e7c8 Compare May 6, 2017 13:36

smarter reviewed May 6, 2017

View reviewed changes

Drop $$local whan mangling fields

693e716

odersky added 2 commits May 7, 2017 11:37

Make idempotency tests run last

a6efb02

They take up time and should fail only rarely.

Adress reviewers comments

c6dafba

odersky force-pushed the change-late-op-encoding branch from cca78ff to c6dafba Compare May 7, 2017 09:37

odersky added 3 commits May 7, 2017 16:09

Fix bug in avoid

76e31aa

Fix checking condition

b10eece

Add test case

6e3f325

Fix NameKinds

d3501e1

The previous change hid the refinements of `info` and apply/unapply. Turning SimpleNameKind and SignedNameKind back into objects fixes that. Neither name kind installs anything as a side effect so turning them into objects is OK.

smarter reviewed May 7, 2017

View reviewed changes

smarter approved these changes May 7, 2017

View reviewed changes

smarter merged commit 1014af3 into scala:master May 7, 2017

allanrenucci deleted the change-late-op-encoding branch December 14, 2017 16:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use symbolic names internally #2169

Use symbolic names internally #2169

odersky commented Apr 4, 2017 •

edited

Loading

smarter Apr 4, 2017

odersky Apr 8, 2017

odersky commented Apr 8, 2017

smarter Apr 10, 2017

smarter May 6, 2017

DarkDimius commented Apr 12, 2017

odersky commented Apr 28, 2017

smarter commented Apr 28, 2017

smarter commented Apr 28, 2017

odersky commented Apr 28, 2017

smarter commented Apr 28, 2017

odersky commented Apr 28, 2017

smarter commented Apr 28, 2017

odersky commented Apr 28, 2017

smarter commented May 6, 2017

smarter May 6, 2017

smarter May 6, 2017

smarter May 6, 2017

odersky May 6, 2017

smarter May 6, 2017

smarter commented May 6, 2017

odersky commented May 6, 2017

smarter commented May 7, 2017

felixmulder commented May 7, 2017

odersky commented May 7, 2017

smarter May 7, 2017

odersky May 7, 2017

smarter May 7, 2017

Use symbolic names internally #2169

Use symbolic names internally #2169

Conversation

odersky commented Apr 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

odersky commented Apr 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DarkDimius commented Apr 12, 2017

odersky commented Apr 28, 2017

smarter commented Apr 28, 2017

smarter commented Apr 28, 2017

odersky commented Apr 28, 2017

smarter commented Apr 28, 2017

odersky commented Apr 28, 2017

smarter commented Apr 28, 2017

odersky commented Apr 28, 2017

smarter commented May 6, 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

Choose a reason for hiding this comment

smarter commented May 6, 2017

odersky commented May 6, 2017

smarter commented May 7, 2017

felixmulder commented May 7, 2017

odersky commented May 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

odersky commented Apr 4, 2017 •

edited

Loading