-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add :javap
and :asmp
to the REPL
#12210
base: main
Are you sure you want to change the base?
Conversation
d9e689d
to
4c57f07
Compare
Can't have too many |
4c57f07
to
e0a0341
Compare
Now with experimental |
@griggt is this still in draft or is it ready for review? |
This comment has been minimized.
This comment has been minimized.
This comment was marked as outdated.
This comment was marked as outdated.
914fe48
to
ea1ce6d
Compare
@griggt where does this stand? I'm interested in seeing this land and might be interested in helping. (or reviewing it, if you finish it yourself.) |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
3ccaacb
to
a2c3874
Compare
It seems that our current version of https://github.com/lampepfl/dotty/runs/5608623717?check_suite_focus=true#step:10:122
By contrast the -- Edit: we're on ASM 9.1, scala/scala is on 9.2 |
I've needed this so badly recently, I meant to come back and whatever the phrase is, put my shoulder to the whatever you put your shoulder to. I need to see what stuff compiles to. Edit: I already forgot I made this comment two days ago. Actually, what I needed today is |
We discussed this some at the Scala 3 issue spree today. One of the remaining issues is the interaction with |
// borrowed from ExtractDependencies#recordDependency and adapted to handle @targetName | ||
private def binaryClassName: String = |
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.
To expand on Seth's comment, I believe this PR correctly deals with classes with the @targetName
annotation because I borrowed and adapted this binaryClassName
code from ExtractDependencies
. But I suspect there may be incremental compilation issues related to @targetName
, and ultimately I think this binaryClassName
code should be shared rather than duplicated.
I'm going to try to find some test cases and open a separate ticket for any incremental compilation issues.
a2c3874
to
0b6af1d
Compare
This commit only provides a framework to support bytecode disassembly from within the REPL, it does not supply any concrete implementations using any particular disassembler -- those will follow in subsequent commits. Adapted from the Scala 2 :javap implementation, which was written by Paul Phillips and Som Snytt / A. P. Marki
Provides bytecode disassembly using the `javap` tool/interface supplied by the user's JDK. Adapted from the Scala 2 implementation, which was written by Paul Phillips and Som Snytt / A. P. Marki
Provides bytecode disassembly using the ASM library bundled with the Scala compiler.
It's been quite some time here, so I wanted to circle back around. Is there any plans to return to this or bring it over the finish line? |
Is there a volunteer who would like to take this over and try to get it across the finish line? |
I have been following it for a long time when I was still using Scala2, but I don't know why it didn't move forward. |
PSA there is no finish line. It is an oval track of endless laps. Edit: I think there is also a PR to move repl to a subproject. The comments made me revisit this and the scala 2 PRs. I wonder why it invites so many LOC. The scala 2 effort got pushback from retronym on complexity, and he was partially appeased. He comments sagely that it would be nice to support multi-release jars, which would hide some multiplexing/multicomplexity at the expense of a (one-time) hit to build complexity. Alternatively, support only javap tool interface (latest) and asm is whatever the asm jar version is. |
I believe this draft to be fully functional, please give it a try if interested.
Unlike the Scala 2 implementation, this one has accomodations for working with JDK 16+. However, in light of the comments in scala/bug#12378, perhaps we should be looking to support an
:asmp
instead?Closes lampepfl/dotty-feature-requests#80
[test_windows_full]
[test_java8]
[test_java11]
[test_java15]
[test_java17]
[test_java18]
[test_java19]