-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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.
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
Changes from all commits
8775dd1
ca46e49
4641bf2
2c4ac2c
0735a68
b28f766
d7eea41
c3cc84f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -67,6 +68,7 @@ object Names { | |
def asSimpleName: SimpleTermName | ||
def toSimpleName: SimpleTermName | ||
def mangled: Name | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would be nice to add some documentation on the meaning/use of |
||
def mangledString: String = mangled.toString | ||
|
||
def rewrite(f: PartialFunction[Name, Name]): ThisName | ||
def collect[T](f: PartialFunction[Name, T]): Option[T] | ||
|
@@ -287,7 +289,40 @@ object Names { | |
override def hashCode: Int = start | ||
|
||
override def toString = | ||
if (length == 0) "" else new String(chrs, start, length) | ||
if (length == 0) "" | ||
else { | ||
if (Config.checkBackendNames) { | ||
if (!toStringOK) { | ||
// 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() | ||
assert(false) | ||
} | ||
} | ||
new String(chrs, start, length) | ||
} | ||
|
||
/** It's OK to take a toString if the stacktrace does not occur a method | ||
* in GenBCode or it also contains one of the whitelisted methods below. | ||
*/ | ||
private def toStringOK = { | ||
val trace = Thread.currentThread.getStackTrace | ||
!trace.exists(_.getClassName.endsWith("GenBCode")) || | ||
trace.exists(elem => | ||
List( | ||
"mangledString", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This TODO is no longer necessary.