Skip to content
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.

Sign up for GitHub

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

Jump to bottom

classfile reader: handle JDK 9+ constant types in constant pool #19533

Merged
merged 2 commits into from
Jan 30, 2024
Merged

classfile reader: handle JDK 9+ constant types in constant pool #19533

merged 2 commits into from
Jan 30, 2024

Conversation

SethTisue
Copy link
Member

@SethTisue SethTisue commented Jan 26, 2024 •
edited

forward-port of scala/scala#10675 and scala/scala#8595
references scala/bug#12396 and scala/bug#11635
fixes #19527 ("bad constant pool tag 17")
also fixes unreported potential "bad constant pool tag 19" and "bad constant pool tag 20" errors

should be backported to 3.3.x, IMO

@SethTisue
Copy link
Member Author

SethTisue commented Jan 26, 2024 •
edited

as far as I know, this repo doesn't have a mechanism for restricting a test to run only on JDK 21+, so I put it in tests/disabled

I did verify locally, on JDK 21, that the test failed before the change and succeeded afterwards

marking as draft for now, pending review of the Scala 2 PR

@SethTisue SethTisue added the backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. label Jan 26, 2024
@SethTisue SethTisue marked this pull request as draft January 26, 2024 00:30
@som-snytt
Copy link
Contributor

@SethTisue there is // test: -jvm 11+

@SethTisue SethTisue marked this pull request as ready for review January 26, 2024 14:54
@SethTisue
Copy link
Member Author

SethTisue commented Jan 26, 2024 •
edited

thx @som-snytt — I've updated the PR to use that.

@sjrd sjrd enabled auto-merge January 26, 2024 15:11
@SethTisue
Copy link
Member Author

I've added a commit forward-porting @lrytz's ClassfileParserTest, which will let us know if ASM adds any further such constants

@SethTisue SethTisue marked this pull request as draft January 26, 2024 15:24
auto-merge was automatically disabled January 26, 2024 15:24

Pull request was converted to draft

@SethTisue
Copy link
Member Author

I've converted this back to a draft, because in porting Lukas's test, I noticed that Dotty's ClassfileConstants is lacking CONSTANT_MODULE and CONSTANT_PACKAGE. Need to look into why that is and whether it matters....

@SethTisue SethTisue changed the title classfile reader: allow CONSTANT_Dynamic in constant pool classfile reader: handle JDK 9+ constant types in constant pool Jan 26, 2024
SethTisue and others added 2 commits January 26, 2024 10:39
@SethTisue @lrytz
forward-port ClassfileParserTest from Scala 2
0c3933f 
Co-authored-by: Lukas Rytz <lukas.rytz@gmail.com>
@SethTisue
classfile reader: allow JDK 9+ constant types in constant pool
26852de 
forward-port of scala/scala#10675 and scala/scala#8595
references scala/bug#12396 and scala/bug#11635
fixes scala#19527 ("bad constant pool tag 17")
also fixes unreported potential "bad constant pool tag 19" and
"bad constant pool tag 20" errors
@SethTisue
Copy link
Member Author

SethTisue commented Jan 26, 2024 •
edited

Okay, I went back and added the additional constants from scala/bug#11635 . I did not forward-port the additional test from scala/scala#8595 . It is Scaladoc-based and did not trigger the bug on Scala 3, and I don't know how else to trigger it.

@SethTisue SethTisue marked this pull request as ready for review January 26, 2024 20:58
@SethTisue SethTisue merged commit 7a5cb6e into scala:main Jan 30, 2024
19 checks passed
@SethTisue SethTisue deleted the issue-19527 branch January 30, 2024 19:31
@SethTisue
Copy link
Member Author

@Kordyjan should I backport it myself, or just figure it will happen...?

@Kordyjan
Copy link
Contributor

I will handle that before 3.3.3.

@Kordyjan Kordyjan self-assigned this Jan 31, 2024
@Kordyjan Kordyjan added this to the 3.4.1 milestone Feb 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sjrd sjrd sjrd approved these changes

Assignees

@Kordyjan Kordyjan

Labels
backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it.
Projects
None yet
Milestone
3.4.1
Development

Successfully merging this pull request may close these issues.

"bad constant pool tag 17" when using a library compiled using Java 21
4 participants
@SethTisue @som-snytt @Kordyjan @sjrd