Delay name mangling #2128

odersky · 2017-03-21T10:52:29Z

This PR aims to delay name mangling until the point where we emit code. Before, names should be "semantic", i.e. the kind of a name is represented by its structure instead of its characters.

smarter · 2017-03-21T17:23:10Z

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

+  /** A term name that's derived from an `underlying` name and that
+   *  adds `info` to it.
+   */
+  class DerivedTermName(override val underlying: TermName, override val info: NameInfo)


Instead of having a NameInfo object as parameter, what about a NameFlagSet ?

smarter · 2017-03-23T19:47:26Z

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

@@ -163,7 +163,7 @@ object Names {
    def info = NameInfo.TermName
    def underlying: TermName = unsupported("underlying")

-    private var derivedNames: AnyRef /* SimpleMap | j.u.HashMap */ =
+    @sharable private var derivedNames: AnyRef /* SimpleMap | j.u.HashMap */ =


Why is this safe? synchronized is not used when modifying derivedNames currently.

odersky · 2017-03-28T21:54:12Z

The build failures are due to ContextEscapeDetection not compiling. I see:

/drone/src/github.com/lampepfl/dotty/compiler/test/dotty/tools/ContextEscapeDetection.java:23: cannot find symbol
232s
865
[error] symbol: class Context
232s
866
[error] location: class dotty.tools.dotc.core.Contexts
232s
867
[error] public abstract Contexts.Context getCtx();

Locally sbt test succeeds for me. Who can help debug this?

odersky · 2017-03-28T21:54:44Z

Rebased to master

smarter · 2017-03-29T19:27:49Z

Locally sbt test succeeds for me. Who can help debug this?

The issue only happen with the bootstrapped tests, to run them you need to first publish the non-bootstrapped compiler:

publishLocal

Then run the tests in the dotty-compiler-bootstrapped project:

dotty-compiler-bootstrapped/test

The test failure is very reminiscent of a problem I fixed in the backend some times ago: lampepfl/scala#4

The problem was that the inner class table in the classfile had incorrect outer class names, and indeed if we do javap -v on the compiled-by-dotty Contexts.scala we see:

public static abstract #10= #7 of #9; //Context=class dotty/tools/dotc/core/Contexts$Context of class dotty/tools/dotc/core/Contexts$

Instead of (from the compiled-by-scalac Contexts.scala):

public static abstract #25= #24 of #2; //Context=class dotty/tools/dotc/core/Contexts$Context of class dotty/tools/dotc/core/Contexts

The crucial difference is the last thing in the line: Contexts$ instead of Contexts. The problem comes from DottyBackendInterface#nameHelper#dropModule:

def dropModule: Name = n.stripModuleClassSuffix

If I print n.stripModuleClassSuffix I get a name which ends with a $ which is clearly wrong. Further debugging shows that the names are TypeName which wrap SimpleTermName so they have no semantic information, I have no idea why.

smarter · 2017-03-29T20:04:42Z

Ah, I see now, the full relevant code in the backend is:

          val outerName = innerClassSym.rawowner.javaBinaryName
          // Java compatibility. See the big comment in BTypes that summarizes the InnerClass spec.
          val outerNameModule = if (innerClassSym.rawowner.isTopLevelModuleClass) outerName.dropModule

Where javaBinaryName is defined in the backend interface:

def javaBinaryName: Name = toDenot(sym).fullNameSeparated("/")

And fullNameSeparated is defined to return a non-semantic name. This can be fixed using .unmangleClassName, I've pushed a commit to do so in javaBinaryName, though I'm not sure if this is the best place for it.

smarter · 2017-03-29T21:22:17Z

The current failures are due to:

assert(isValidJVMName(sym.name), s"${sym.fullName} name is invalid on jvm")

Because in some cases we get sym.name.toString like: scala.collection$TraversableOnce$collectFirst$$B which is technically incorrect since it contains a .. I don't know why this happens or why it's only happening in the bootstrapped tests.

smarter · 2017-03-30T12:27:32Z

I have two features request to aid debugging, let me know what you think:

A rawString or equivalent method to get the full structure of the name, like printing a raw type or a raw tree, debugString is not enough since it doesn't tell if you're dealing with a PrefixNamedKind, etc.
An (optional?) id for names to figure out where they were constructed by doing assert(id == 42), like with symbols. (Names don't have stable hashcodes so that can't be used).

smarter · 2017-04-01T17:38:40Z

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

@@ -339,7 +339,12 @@ object Names {

    def isSimple = false
    def asSimpleName = throw new UnsupportedOperationException(s"$debugString is not a simple name")
-    def toSimpleName = termName(toString)
+
+    private var simpleName: SimpleTermName = null


I think you want private[this] var here to avoid creating a setter

smarter · 2017-04-01T21:47:22Z

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

+    def name = myName
+
+    /** Update the name; only called when unpickling top-level classes */
+    def name_=(n: Name) = myName = n


Should this setter be protected[dotc] like def info_= ?

smarter · 2017-04-02T17:32:54Z

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

      override def newScopeLikeThis() = new PackageScope
    }

    private[core] val currentDecls: MutableScope = new PackageScope()

+    def isFlatName(name: SimpleTermName) = name.lastIndexOf('$', name.length - 2) >= 0


Could you add a comment explaining the name.length - 2 here? Also why is this logic different from the one for the other overload for isFlatName ?

smarter · 2017-04-02T17:47:11Z

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

          newScopeEntry(defn.newFunctionNTrait(name.asTypeName))
+        else if (isFlatName(mangled.toSimpleName) && enterFlatClasses.isDefined) {
+          Stats.record("package scopes with flatnames entered")
+          enterFlatClasses.get(ctx)


Why not use ensureComplete here?

odersky · 2017-04-06T13:00:55Z

Rebased to master

odersky · 2017-04-10T08:09:36Z

This commit is too big to leave in the queue. There's constant breakage wrt other commits. So I want to get this in by the end of today at the latest.

smarter · 2017-04-10T16:26:00Z

This PR LGTM, except that I don't see how derivedNames can be made @sharable without using synchronized blocks (also there's a test failure but I assume you noticed that).

smarter · 2017-04-10T18:02:10Z

The use of @sharable for:

  @sharable private val simpleNameKinds = new mutable.HashMap[Int, ClassifiedNameKind]
  @sharable private val qualifiedNameKinds = new mutable.HashMap[Int, QualifiedNameKind]
  @sharable private val uniqueNameKinds = new mutable.HashMap[String, UniqueNameKind]

is also suspicious. Right now it should be fine since all the NameKinds are instantiated in object NameKinds, but nothing enforces that, it would help if the constructors for NameKinds were all private.

Remove unused functionality

It fails without any test output in partest

Drop Seq implementation of name. This implementation was always problematic because it entailed potentially very costly conversions to toSimpleName. We now have better control over when we convert a name to a simple name.

toSimpleName is called a lot from the backend, so it makes sense to memoize it. It would be even better to communicate with the backend using strings, because then we would not have to enter all these simple names in the name table.

Classfile parsing is about JVM-level names, not Scala level ones. So it is more consistent to use mangled names throughout.

Simplifies the test that the class represented by a classfile is the class logically referenced by the file. The simplification is needed if we want to populate package scopes with unmangled classnames.

Once we start using unencoded operators internally, we will face the problem that one cannot decode realiably a class file filename. We therefore turn things around, keeping members of package scopes in mangled and encoded form. This is compensated by (1) mangling names for lookup of such members and (2) when unpickling from Scala 2 info or Tasty, comparing mangled names when matching a read class or module object against a root.

Mangled is like toSimpleName, except that it keeps the term/type distinction.

Prevviously they were actually unused.

and fix a bug in TreeUnpickler

Since we now have separate package-scopes, it's easier to have them take into account the special role played by the scala package. So we can drop the funky logic of `makeScalaSpecial`.

Names with internal $'s are entered in package scopes only if - we look for a name with internal $'s. - we want to know all the members of a package scope This optimization seems to be fairly effective. The typical range of package scopes that need $-names is between 0 and 20%. The optimization seems to improve execution time of all unit tests by about 3%. Also. drop the inheritance from Iterable to Scope. The reason is that we now need a context parameter for toList and other Iterable operations which makes them impossible to fit into the Iterable framework.

Running the test suite with the pickling printer on showed up two more problems which are fixed in this commit.

Used a hardcoded string before, which caused test failures.

odersky · 2017-04-11T09:49:23Z

I'll roll the private constructor changes into the next PR.

smarter reviewed Mar 21, 2017

View reviewed changes

smarter reviewed Mar 23, 2017

View reviewed changes

odersky force-pushed the add-semantic-names branch from e295718 to 9981ae1 Compare March 27, 2017 08:09

odersky force-pushed the add-semantic-names branch from 06316e6 to ae6bd27 Compare March 28, 2017 21:54

odersky force-pushed the add-semantic-names branch 3 times, most recently from e52ad28 to 29b51e8 Compare March 31, 2017 14:06

smarter reviewed Apr 1, 2017

View reviewed changes

smarter reviewed Apr 2, 2017

View reviewed changes

odersky force-pushed the add-semantic-names branch from 94e70aa to c6b6b30 Compare April 3, 2017 15:43

odersky mentioned this pull request Apr 4, 2017

Use symbolic names internally #2169

Merged

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

odersky force-pushed the add-semantic-names branch from c6b6b30 to 96304b4 Compare April 6, 2017 12:57

odersky changed the title ~~[WIP] Delay name mangling~~ Delay name mangling Apr 7, 2017

odersky force-pushed the add-semantic-names branch from d14baae to 5bada41 Compare April 8, 2017 11:30

odersky mentioned this pull request Apr 10, 2017

Fix #2152: Instantiate dependent result type parameters #2209

Merged

odersky force-pushed the add-semantic-names branch from 77992c9 to 1058278 Compare April 10, 2017 14:30

odersky force-pushed the add-semantic-names branch from 21bc82f to 628a967 Compare April 10, 2017 20:28

odersky added 24 commits April 11, 2017 09:33

Avoid duplicate hashCode/equals

e3e5e79

Make module var names semantic

a3f6ca5

Make outer select names semantic

24f40bc

Cleanups of NameOps

130069a

Remove unused functionality

Run compileStdLib only as junit test

7a927ce

It fails without any test output in partest

Names are no longer Seqs

b4f21c6

Drop Seq implementation of name. This implementation was always problematic because it entailed potentially very costly conversions to toSimpleName. We now have better control over when we convert a name to a simple name.

Merge likeTyped and likeKinded into likeSpaced

4a54b2c

Further simplification for Name

a574ba6

Bugfix in stripModuleClassSuffix

c646883

Memoize toSimpleName

9f7012e

toSimpleName is called a lot from the backend, so it makes sense to memoize it. It would be even better to communicate with the backend using strings, because then we would not have to enter all these simple names in the name table.

Don't unmangle names in ClassfileParser

b2bd0bf

Classfile parsing is about JVM-level names, not Scala level ones. So it is more consistent to use mangled names throughout.

Simplify classfile parser "own name" checking

e2fb134

Simplifies the test that the class represented by a classfile is the class logically referenced by the file. The simplification is needed if we want to populate package scopes with unmangled classnames.

Introduce mangled method

2bfb2ca

Mangled is like toSimpleName, except that it keeps the term/type distinction.

Fix and activate package scopes

bd992dd

Prevviously they were actually unused.

Avoid setter in Name

a7ee8dc

and fix a bug in TreeUnpickler

Scope refactoring

264211f

Since we now have separate package-scopes, it's easier to have them take into account the special role played by the scala package. So we can drop the funky logic of `makeScalaSpecial`.

Make extension method names semantic

868a6a1

Fix rebase breakage

b17af4b

Fix pickling/unpickling of names

800b1ad

Running the test suite with the pickling printer on showed up two more problems which are fixed in this commit.

Adapt isPackageObject to semantic naming

a5b6bd6

Used a hardcoded string before, which caused test failures.

Better documentation of sharable structures in Names, NameKinds

bec4f9d

Fix type error

87608bd

odersky merged commit 579571e into scala:master Apr 11, 2017

OlivierBlanvillain mentioned this pull request Apr 12, 2017

Regression: exception while transforming attempt of class class dotty.tools.dotc.ast.Trees$Ident #2233

Closed

nicolasstucki mentioned this pull request Jul 31, 2017

Symbol encoding should be done late #30

Closed

allanrenucci deleted the add-semantic-names branch December 14, 2017 16:57

smarter mentioned this pull request Jan 10, 2018

unexpandedName needs hardening. #765

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delay name mangling #2128

Delay name mangling #2128

odersky commented Mar 21, 2017 •

edited by SethTisue

Loading

smarter Mar 21, 2017

smarter Mar 23, 2017

odersky commented Mar 28, 2017

odersky commented Mar 28, 2017

smarter commented Mar 29, 2017

smarter commented Mar 29, 2017

smarter commented Mar 29, 2017

smarter commented Mar 30, 2017

smarter Apr 1, 2017

smarter Apr 1, 2017

smarter Apr 2, 2017

smarter Apr 2, 2017

odersky commented Apr 6, 2017

odersky commented Apr 10, 2017

smarter commented Apr 10, 2017

smarter commented Apr 10, 2017

odersky commented Apr 11, 2017

Delay name mangling #2128

Delay name mangling #2128

Conversation

odersky commented Mar 21, 2017 • edited by SethTisue Loading

smarter Mar 21, 2017

Choose a reason for hiding this comment

smarter Mar 23, 2017

Choose a reason for hiding this comment

odersky commented Mar 28, 2017

odersky commented Mar 28, 2017

smarter commented Mar 29, 2017

smarter commented Mar 29, 2017

smarter commented Mar 29, 2017

smarter commented Mar 30, 2017

smarter Apr 1, 2017

Choose a reason for hiding this comment

smarter Apr 1, 2017

Choose a reason for hiding this comment

smarter Apr 2, 2017

Choose a reason for hiding this comment

smarter Apr 2, 2017

Choose a reason for hiding this comment

odersky commented Apr 6, 2017

odersky commented Apr 10, 2017

smarter commented Apr 10, 2017

smarter commented Apr 10, 2017

odersky commented Apr 11, 2017

odersky commented Mar 21, 2017 •

edited by SethTisue

Loading