Skip to content

Change backend name handling #2322

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Apr 28, 2017
Merged

Change backend name handling #2322

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
8775dd1
Drop internalNameString and hava javaBinaryName return a string
odersky Apr 27, 2017
ca46e49
Drop unused operations in NameHandler
odersky Apr 27, 2017
4641bf2
Remove more name ops
odersky Apr 27, 2017
2c4ac2c
[TO BE REVERTED] Gross hack to work around leaky NameHelper abstraction
odersky Apr 4, 2017
0735a68
Use mangled string in backend
odersky Apr 27, 2017
b28f766
Change some
odersky Apr 28, 2017
d7eea41
Change some toString occurrences on Name to mangledString
odersky Apr 28, 2017
c3cc84f
Document name checking
odersky Apr 28, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions compiler/src/dotty/tools/dotc/config/Config.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,13 @@ object Config {
*/
final val checkNoSkolemsInInfo = false

/** Check that Name#toString is not called directly from backend by analyzing
* the stack trace of each toString call on names. This is very expensive,
* so not suitable for continuous testing. But it can be used to find a problem
* when running a specific test.
*/
final val checkBackendNames = false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@odersky odersky Apr 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/** Type comparer will fail with an assert if the upper bound
* of a constrained parameter becomes Nothing. This should be turned
* on only for specific debugging as normally instantiation to Nothing
Expand Down
28 changes: 27 additions & 1 deletion compiler/src/dotty/tools/dotc/core/Names.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import collection.mutable.{ Builder, StringBuilder, AnyRefMap }
import collection.immutable.WrappedString
import collection.generic.CanBuildFrom
import util.{DotClass, SimpleMap}
import config.Config
import java.util.HashMap

//import annotation.volatile
Expand Down Expand Up @@ -289,7 +290,32 @@ object Names {

override def toString =
if (length == 0) ""
else new String(chrs, start, length)
else {
if (Config.checkBackendNames) {
if (!toStringOK) {
println("Backend should not call Name#toString, Name#mangledString should be used instead.")
Copy link
Member

@smarter smarter Apr 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

new Error().printStackTrace()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nicer way: Thread.dumpStack()

assert(false)
}
}
new String(chrs, start, length)
}

private def toStringOK = {
val trace = Thread.currentThread.getStackTrace
!trace.exists(_.getClassName.endsWith("GenBCode")) ||
trace.exists(elem =>
List(
"mangledString",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"toSimpleName",
"decode",
"unmangle",
"dotty$tools$dotc$core$NameOps$NameDecorator$$functionArityFor$extension",
"dotty$tools$dotc$typer$Checking$CheckNonCyclicMap$$apply",
"$plus$plus",
"readConstant")
.contains(elem.getMethodName))
}

def debugString: String = toString
}
Expand Down