Fix ScalaSigReader to make it work with Scala 3 #1460
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
xuwei-k
reviewed
May 2, 2024
| given staging.Compiler = staging.Compiler.make(this.getClass.getClassLoader) | ||
|
|
||
| val nameWithSymbols = | ||
| name.replace("$minus", "-").replace("$hash", "#").replace("$bang", "!").replace("$plus", "+").replace("$percent", "%").replace("$at", "@") |
There was a problem hiding this comment.
I think we should use scala.reflect.NameTransformer
For scala 3, we replace the scalap dependency (which is not supported for scala 3) with multi-staging compilation, which allows us to read the `.tasty`` files on runtime. Most of the tests are reenabled for scala 3, as they should pass with the new implementation.
|
Hello! I added the changes to the build with different scala version as promised in the PR message. I tried to add a comment with the reasoning for it in the related code. The scalafmt check should also work now, so hopefully it will pass the CI this time |
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.
Hello!
This PR attempts to fix #1035 by replacing the unsupported scalap dependency with
with runtime multi-staging programming, which allows us to read the `.tasty`` files on runtime. Most of the tests are reenabled for scala 3, as they should pass with the new implementation.
Solution based on https://github.com/talenteca/olon-web-framework/blob/session-10/olon-json/src/main/scala/olon/json/Scala3Sig.scala
One caveat - I do not know if the performance of this solution will be satisfactory. We have to create a compiler instance for every ScalaSig function call, as the Compiler instance cannot work in a multithreaded context. This Compiler instance only does light operations like reading the
.tastyfiles, what I am worried about here is the continuous instancing.Another caveat - library users will have to add additional dependency manually onto the classpath (it will be set as
providedhere), allowing multi staging compilation. This is because the tasty format is not forward compatible, and what we need to read depends on which version of the compiler the user used. I understand that this would be a big change (possibly requiring some mention in the docs and a version bump?).Last caveat - older scala 3 versions (including LTS) have some problems with java reflection (scala/scala3#20263), which is used elsewhere (as in not in ScalaSigReader) in this project, which caused some issues with the tests. One solution is to upgrade the scala version used here, but a better one would be to keep the scala version as LTS, and only upgrade the scala version used to compile the tests. This will add some complexity to the build (and may need to also be mentioned in the docs).
Things left to do:
try to make tests with lazy vals pass- this is a bit difficult to do, so I will not be ableto get it to work in this PRPlease let me know what you think about this fix.