Refix compact names w/o breaking local names #1259
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dwijnand
force-pushed
the
compatify2
branch
from
75acca7 to
85e0e2c
Compare
eed3si9n
reviewed
Sep 19, 2023
lrytz
reviewed
Sep 21, 2023
internal/compiler-bridge/src/main/scala/scala/reflect/ReflectAccess.scala
Outdated
Show resolved
Hide resolved
internal/compiler-bridge/src/main/scala/xsbt/CallbackGlobal.scala
Outdated
Show resolved
Hide resolved
internal/compiler-bridge/src/main/scala/xsbt/CallbackGlobal.scala
Outdated
Show resolved
Hide resolved
The previous fix phase-traveled to after flatten, which is the threshold for Symbol#name to start returning flattened names. Unfortunately there is a bug (one of a few in nsc) in that the owner of the local class symbols isn't changed as a part of a lambda lift info transform, it's changed as a side-effect of doing lambda lift tree transform. Additionally the flattened name is cached on first use, which in this case means using the wrong owner - that's why the compiler failed in pekko, because there's a check in the backend that the same classfile (by name) isn't emitted twice. Turns out, we don't need the full names of method local classes, so we can push that condition a bit higher in registerGeneratedClasses, avoiding useless work, useless phase travel, and erroneous caching. We also just straight up avoid phase traveling from xsbt-api (which runs after typer) all the way to flatten (which is almost at the end) for the flatten, compactified name, by reimplementing `flattenedName`, with the correct, recursive, flattened owner and name. By the by, the test case is pretty convoluted, despite trying to make it simpler - how many times have you defined and instantiated a method-local anonymous class, defining one of its methods using a case object?
dwijnand
force-pushed
the
compatify2
branch
from
85e0e2c to
a87fe0b
Compare
eed3si9n
approved these changes
Sep 26, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The previous fix phase-traveled to after flatten, which is the threshold
for Symbol#name to start returning flattened names. Unfortunately there
is a bug (one of a few in nsc) in that the owner of the local class
symbols isn't changed as a part of a lambda lift info transform, it's
changed as a side-effect of doing lambda lift tree transform.
Additionally the flattened name is cached on first use, which in this
case means using the wrong owner - that's why the compiler failed in
pekko, because there's a check in the backend that the same classfile
(by name) isn't emitted twice.
Turns out, we don't need the full names of method local classes, so we
can push that condition a bit higher in registerGeneratedClasses,
avoiding useless work, useless phase travel, and erroneous caching.
We also just straight up avoid phase traveling from xsbt-api (which runs
after typer) all the way to flatten (which is almost at the end) for the
flatten, compactified name, by reimplementing
flattenedName, with thecorrect, recursive, flattened owner and name.
By the by, the test case is pretty convoluted, despite trying to make it
simpler - how many times have you defined and instantiated a
method-local anonymous class, defining one of its methods using a case
object?