Ignore checkcast in Java 21 type switch#2813
Conversation
| // 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) { |
There was a problem hiding this comment.
This is essentially "if not...else" which is a double negative. Perhaps refactor to "if null then return false".
There was a problem hiding this comment.
Yes, I have made the change
|
|
||
| private SwitchDetails findSwitchDetailsByPc(int pc) { | ||
| for (SwitchDetails switchDetails : switchOffsetStack) { | ||
| for (int i = 0; i < switchDetails.swOffsets.length; i++) { |
There was a problem hiding this comment.
Couldn't this be a second foreach loop?
There was a problem hiding this comment.
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
|
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. |
|
I've highligted the part where a JDK 21 is required: we're building bug samples using Java 21 language features. |
|
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) |
| uses: actions/setup-java@v4 | ||
| with: | ||
| java-version: '21' | ||
| java-version: '17' |
There was a problem hiding this comment.
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.
* Failing test case * fix: applied spotless * test: issue spotbugs#2782 reproducer * fix: look for java 21 type switches and ignore corresponding casts * test: enable the compilation of java 21 samples - Compile samples with JDK 21 - Added comments and fixed minor issues * ci: build on JDK 21 only now that there are Java 21 bug samples * doc: added changelog entries * fix: look for PC-1 (aload_<n>) or PC-2 (aload) 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 * ci: reverted to building on JDK 17 and 21 * test: added some tests checking that we can spot bad casts in a switch * doc: rephrased jdk requirements --------- Co-authored-by: Robin Jonsson Hector <rjonsson@righthub.com> Co-authored-by: Jeremy Landis <jeremylandis@hotmail.com>
Repurposed
SwitchHandlerto keep track of theInvokeDynamiccorresponding to a Java 21 type switch.We detect that the switch uses the object type by looking for a
typeSwitchinvocation.Then we ignore the
checkcastfor each of the switch's target PC + 1I 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 samplesThis PR enables the compilation of bug samples using Java 21 language features when running on JDK 21 or above