Revert to the Scala 2 name encoding scheme #7601
Merged
Conversation
This file contains hidden or 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
Member
|
You should probably get rid of https://github.com/lampepfl/dotty/blob/a356535118bdbf4dfbaa48b06a8c77380b3c3e9f/compiler/src/dotty/tools/backend/sjs/JSEncoding.scala#L232 in the Scala.js backend. Just preserve the handling of |
smarter
force-pushed
the
fix-encoding-2
branch
from
9bc4382 to
5b13635
Compare
odersky
requested changes
Nov 25, 2019
Contributor
odersky
left a comment
odersky
left a comment
There was a problem hiding this comment.
It's good that al aspects of encoding / decoding are now combined.
We should have a comment somewhere (maybe in NameTransformer) what the
precise name mangling scheme is now. The commit should also have a short message
indicating how this changed.
Otherwise LGTM
There were several issues with the scheme we used so far: - It's different from Scala 2, meaning that some Scala 2 methods could not be called from Dotty and vice-versa (see the added sbt-dotty/sbt-test/scala2-compat/akka/i3100.scala test for an example) - It can lead to invalid filenames on Windows (scala#7492) - The handling of backticks is broken: adding or removing backticks around a name changes how it's encoded. To maintain Scala 2 compat we don't have a lot of choices here, we must use the same scheme, so this commit aligns NameTransformer.scala with https://github.com/scala/scala/blob/2.13.x/src/library/scala/reflect/NameTransformer.scala Some examples: Method name | Old encoding | New encoding ----------------------------------------- a_+ | a_$plus | a_$plus `a_+` | a_+ | a_$plus `+_a` | +_a | $plus_a a_/ | a_$div | a_$div `a_/` | a_$u002F | a_$div If a Dotty method is called `def a_$plus` we won't misinterpret it as `a_+` because the method name comes from the tasty tree which stores unencoded names. On the other hand, names coming from Java / Scala 2 as well as top-level classnames might be misinterpreted as encoded names if they contain a user-written $, this is left unspecified. Fixes scala#3100. Fixes scala#5936. Fixes scala#7492.
smarter
force-pushed
the
fix-encoding-2
branch
from
5b13635 to
4bc3241
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
There were several issues with the scheme we used so far:
not be called from Dotty and vice-versa (see the added
sbt-dotty/sbt-test/scala2-compat/akka/i3100.scala test for an example)
around a name changes how it's encoded.
To maintain Scala 2 compat we don't have a lot of choices here, we must
use the same scheme, so this commit aligns NameTransformer.scala with
https://github.com/scala/scala/blob/2.13.x/src/library/scala/reflect/NameTransformer.scala
Fixes #3100. Fixes #5936. Fixes #7492.