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.

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

Ignore checkcast in Java 21 type switch #2813

Merged
merged 13 commits into from Jan 20, 2024
Merged

Conversation

gtoison
Copy link
Contributor

@gtoison gtoison commented Jan 14, 2024

Repurposed SwitchHandler to keep track of the InvokeDynamic corresponding to a Java 21 type switch.

We detect that the switch uses the object type by looking for a typeSwitch invocation.
Then we ignore the checkcast for each of the switch's target PC + 1

    11  invokedynamic 0 typeSwitch(java.lang.Object, int) : int [13]
    16  lookupswitch default: 72
          case 0: 44
          case 1: 58
    44  aload_2
    45  checkcast Issue2782$Impl1 [17]
    48  astore 4 [impl1]
    50  aload 4 [impl1]
    52  invokevirtual Issue2782$Impl1.helloFromImpl1() : Issue2782$MyInterface [19]
    55  goto 82
    58  aload_2
    59  checkcast Issue2782$Impl2 [23]
    62  astore 5 [impl2]
    64  aload 5 [impl2]
    66  invokevirtual Issue2782$Impl2.helloFromImpl2() : Issue2782$MyInterface [25]
    69  goto 82
    72  new java.lang.IllegalArgumentException [28]

I have removed the sample provided by @robinjhector in #2782 so we don't have to compile Java 21 code because it doesn't seem to work for now. Instead I'm using the compiled .class files directly.

Update: this PR effectively upgrades the build to JDK 21 (from 17 AND 21) since we now use Java 21 language features in bug samples

This PR enables the compilation of bug samples using Java 21 language features when running on JDK 21 or above

// Since we're on a checkcast look for a switch with a target on the previous PC
SwitchDetails switchDetails = findSwitchDetailsByPc(pc - 1);

if (switchDetails != null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is essentially "if not...else" which is a double negative. Perhaps refactor to "if null then return false".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I have made the change


private SwitchDetails findSwitchDetailsByPc(int pc) {
for (SwitchDetails switchDetails : switchOffsetStack) {
for (int i = 0; i < switchDetails.swOffsets.length; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't this be a second foreach loop?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have made the change

- Compile samples with JDK 21
- Added comments and fixed minor issues
Since the first instruction in a switch might be an aload and take an
additional operand we also need to check for PC-2 when checking if we're
at a case of the switch.
Added a test for nested switches
@hazendaz
Copy link
Member

Am I missing where jdk 21 only is required? I don't see any new imports and tests seem to all indicate only run with jdk 21 so I think jdk 17 is still allowed?

Also this is still in Draft so I presume you will remove it from draft once its ready to be merged.

@gtoison
Copy link
Contributor Author

gtoison commented Jan 16, 2024

I've highligted the part where a JDK 21 is required: we're building bug samples using Java 21 language features.
Ideally the samples would be compiled using various JDK, because the bytecode might be different.

@gtoison gtoison marked this pull request as ready for review January 16, 2024 06:07
@gtoison
Copy link
Contributor Author

gtoison commented Jan 18, 2024

I have reverted the change to continue building on JDK 17 and 21, the bug samples using java 21 langage features are not built when running on JDK 17 (and the correspoding tests are skipped)

README.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
spotbugsTestCases/src/java21/Issue2782.java Show resolved Hide resolved
uses: actions/setup-java@v4
with:
java-version: '21'
java-version: '17'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If Java 21 is needed for all the test to run, I think it's better if we stuck to this version for the release.

@hazendaz hazendaz merged commit d0a7219 into spotbugs:master Jan 20, 2024
6 of 9 checks passed
@hazendaz hazendaz added this to the SpotBugs 4.8.4 milestone Jan 27, 2024
@hazendaz hazendaz self-assigned this Jan 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants