Change backend name handling #2322

odersky · 2017-04-27T17:18:57Z

Change backend name handling to support symbolic names. The idea is to
narrow the interface to names in the backend interface and avoid direct calls
to toString on names.

Needs updated backend # Conflicts: # compiler/src/dotty/tools/dotc/core/Names.scala

Also, fix a discrepancy in the signature of newWeakSet # Conflicts: # scala-backend

Avoid using toString directly.

odersky · 2017-04-27T21:18:55Z

How did I verify that this works? Starting with 2c4ac2c, I added an assertion that useMangled is false when taking the toString of a name. I had to temporarily reset useMangled in lots of front and middle end code called by the backend, so that in the end only real toString calls from the backend remained. I verified that all unit tests could be run without triggering the assertion.

It's tricky to make this a CI-suitable test though, because it is inherently sequential. Name#toString does not know about the context, so we need a global variable to control the assertion, which is not thread-safe.

So for the moment I submit as is. If toString calls should be introduced in the backend in the future they would likely produce wrong code for symbolic names (once these are in). I am aware that it's a weak barrier against regression. Once we get get parametricity in Any, we can enforce it in the types, so then we will be in the dry.

smarter · 2017-04-28T05:47:28Z

It's tricky to make this a CI-suitable test though, because it is inherently sequential. Name#toString does not know about the context, so we need a global variable to control the assertion, which is not thread-safe.

How about doing this instead:

def toString = {
  if (Config.checkBackendNames) {
    assert(!Thread.currentThread.getStackTrace.exists(_.toString.contains("dotty.tools.dotc.backend.jvm.GenBCode"),
      "Backend should not call Name#toString, Name#mangledString should be used instead."))
  }
  // ...
}

smarter · 2017-04-28T05:52:57Z

Better way to use getStackTrace:

Thread.currentThread.getStackTrace.exists(_.getClassName == classOf[GenBCode].getName)

smarter

Otherwise LGTM

smarter · 2017-04-28T05:32:51Z

compiler/src/dotty/tools/backend/jvm/DottyBackendInterface.scala

-    def javaBinaryName: Name = javaClassName.replace('.', '/').toTypeName // TODO: can we make this a string? addModuleSuffix(fullNameInternal('/'))
-    def javaClassName: String = toDenot(sym).fullName.toString// addModuleSuffix(fullNameInternal('.')).toString
+    def javaSimpleName: String = toDenot(sym).name.mangledString // addModuleSuffix(simpleName.dropLocal)
+    def javaBinaryName: String = javaClassName.replace('.', '/') // TODO: can we make this a string? addModuleSuffix(fullNameInternal('/'))


This TODO is no longer necessary.

smarter · 2017-04-28T05:40:50Z

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

@@ -67,6 +67,7 @@ object Names {
    def asSimpleName: SimpleTermName
    def toSimpleName: SimpleTermName
    def mangled: Name


Would be nice to add some documentation on the meaning/use of mangled.

odersky · 2017-04-28T06:43:47Z

But then we'd have to leave in the ~15 exceptions which allow toString when backend calls something else which requires it (such as .member). Possible but ugly.

These were caught when turning the backend check on.

smarter · 2017-04-28T08:10:24Z

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

+            // We print the stacktrace instead of doing an assert directly,
+            // because asserts are caught in exception handlers which might
+            // cause other failures. In that case the first, important failure
+            // is lost.
            println("Backend should not call Name#toString, Name#mangledString should be used instead.")


Use Console.err.println (or System.err.println) instead. printStackTrace will print on stderr, and if you use println right before it which prints on stdout, the output might get mixed.

smarter · 2017-04-28T08:11:03Z

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

+            // We print the stacktrace instead of doing an assert directly,
+            // because asserts are caught in exception handlers which might
+            // cause other failures. In that case the first, important failure
+            // is lost.
            println("Backend should not call Name#toString, Name#mangledString should be used instead.")
            new Error().printStackTrace()


Nicer way: Thread.dumpStack()

smarter · 2017-04-28T08:12:13Z

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

@@ -301,6 +305,9 @@ object Names {
        new String(chrs, start, length)
      }

+    /** It's OK to take a toString if the stacktrace does not occur a method


typo: "a method" -> "in a method"
And on the line below: "or it also" -> "or if it also"

smarter · 2017-04-28T08:14:44Z

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

+      !trace.exists(_.getClassName.endsWith("GenBCode")) ||
+      trace.exists(elem =>
+          List(
+              "mangledString",


An alternative to defining a whitelist in this way would be to define a new method named something like unmangledString that does what toString currently do, and have toString forward to it. Then we can just change all the code calling toString to use unmangledString instead, but keep the stacktrace check in toString

I think that would affect too many things. Note that most of the whitelist does not call toString directly, sometimes there are 10-20 calls in between.

DarkDimius

I've left small suggestion to make checkBackendNames into a flag, instead of compile-time constant. I don't think this code is performance critical, given that Dotty doesn't call it much at all and backend only calls it once for every name when generates its own representation.

Dotty-wise looks good enough to me.
Checking stack-trace seems to be robust enough, given that is passes in both boostrapped and non-bootstrapped mode.

DarkDimius · 2017-04-28T09:48:34Z

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

+   *  so not suitable for continuous testing. But it can be used to find a problem
+   *  when running a specific test.
+   */
+  final val checkBackendNames = false


I would propose to make this configurable as a flag. So that we can ask users to enable checked mode in case something looks very strange in their reports.

Maybe that's a bit paranoid? So far, errors resulting from producing symbolic strings were very easy to diagnose.

odersky added 5 commits April 27, 2017 19:15

Drop internalNameString and hava javaBinaryName return a string

8775dd1

Needs updated backend # Conflicts: # compiler/src/dotty/tools/dotc/core/Names.scala

Drop unused operations in NameHandler

ca46e49

Also, fix a discrepancy in the signature of newWeakSet # Conflicts: # scala-backend

Remove more name ops

4641bf2

[TO BE REVERTED] Gross hack to work around leaky NameHelper abstraction

2c4ac2c

Use mangled string in backend

0735a68

Avoid using toString directly.

odersky changed the title ~~[WIP] Change backend name handling~~ Change backend name handling Apr 27, 2017

odersky requested review from smarter and DarkDimius April 27, 2017 21:12

smarter approved these changes Apr 28, 2017

View reviewed changes

odersky added 3 commits April 28, 2017 09:51

Change some

b28f766

Change some toString occurrences on Name to mangledString

d7eea41

These were caught when turning the backend check on.

Document name checking

c3cc84f

smarter reviewed Apr 28, 2017

View reviewed changes

DarkDimius approved these changes Apr 28, 2017

View reviewed changes

odersky merged commit a447c5b into scala:master Apr 28, 2017

smarter mentioned this pull request Apr 28, 2017

Use symbolic names internally #2169

Merged

allanrenucci deleted the change-backend-names branch December 14, 2017 16:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Change backend name handling #2322

Change backend name handling #2322

Uh oh!

odersky commented Apr 27, 2017

Uh oh!

odersky commented Apr 27, 2017 •

edited

Loading

Uh oh!

smarter commented Apr 28, 2017

Uh oh!

smarter commented Apr 28, 2017

Uh oh!

smarter left a comment

Uh oh!

smarter Apr 28, 2017

Uh oh!

smarter Apr 28, 2017

Uh oh!

odersky commented Apr 28, 2017 •

edited

Loading

Uh oh!

smarter Apr 28, 2017 •

edited

Loading

Uh oh!

smarter Apr 28, 2017

Uh oh!

smarter Apr 28, 2017

Uh oh!

smarter Apr 28, 2017

Uh oh!

odersky Apr 28, 2017

Uh oh!

DarkDimius left a comment

Uh oh!

DarkDimius Apr 28, 2017

Uh oh!

odersky Apr 28, 2017 •

edited

Loading

Uh oh!

Uh oh!

Change backend name handling #2322

Change backend name handling #2322

Uh oh!

Conversation

odersky commented Apr 27, 2017

Uh oh!

odersky commented Apr 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smarter commented Apr 28, 2017

Uh oh!

smarter commented Apr 28, 2017

Uh oh!

smarter left a comment

Choose a reason for hiding this comment

Uh oh!

smarter Apr 28, 2017

Choose a reason for hiding this comment

Uh oh!

smarter Apr 28, 2017

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smarter Apr 28, 2017

Choose a reason for hiding this comment

Uh oh!

smarter Apr 28, 2017

Choose a reason for hiding this comment

Uh oh!

smarter Apr 28, 2017

Choose a reason for hiding this comment

Uh oh!

odersky Apr 28, 2017

Choose a reason for hiding this comment

Uh oh!

DarkDimius left a comment

Choose a reason for hiding this comment

Uh oh!

DarkDimius Apr 28, 2017

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

odersky commented Apr 27, 2017 •

edited

Loading

odersky commented Apr 28, 2017 •

edited

Loading

smarter Apr 28, 2017 •

edited

Loading

odersky Apr 28, 2017 •

edited

Loading