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
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
package edu.umd.cs.findbugs.detect;

import edu.umd.cs.findbugs.AbstractIntegrationTest;
import edu.umd.cs.findbugs.BugCollection;
import edu.umd.cs.findbugs.test.matcher.BugInstanceMatcher;
import edu.umd.cs.findbugs.test.matcher.BugInstanceMatcherBuilder;
import org.junit.jupiter.api.Test;

import static edu.umd.cs.findbugs.test.CountMatcher.containsExactly;
import static org.hamcrest.MatcherAssert.*;

import edu.umd.cs.findbugs.test.SpotBugsRunner;
import edu.umd.cs.findbugs.test.SpotBugsExtension;

import org.junit.jupiter.api.extension.ExtendWith;

import java.nio.file.Paths;

@ExtendWith(SpotBugsExtension.class)
class Issue2782Test extends AbstractIntegrationTest {

@Test
void testIssue(SpotBugsRunner spotbugs) {
BugCollection bugCollection = spotbugs.performAnalysis(
Paths.get("../spotbugsTestCases/src/classSamples/java21typeSwitchSample/Issue2782.class"),
Paths.get("../spotbugsTestCases/src/classSamples/java21typeSwitchSample/Issue2782$MyInterface.class"),
Paths.get("../spotbugsTestCases/src/classSamples/java21typeSwitchSample/Issue2782$Impl1.class"),
Paths.get("../spotbugsTestCases/src/classSamples/java21typeSwitchSample/Issue2782$Impl2.class"));

BugInstanceMatcher matcher = new BugInstanceMatcherBuilder()
.bugType("BC_UNCONFIRMED_CAST")
.build();

assertThat(bugCollection, containsExactly(0, matcher));
}
}
53 changes: 49 additions & 4 deletions spotbugs/src/main/java/edu/umd/cs/findbugs/SwitchHandler.java
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,9 @@
package edu.umd.cs.findbugs;

import java.util.ArrayList;
import java.util.HashSet;
import java.util.List;
import java.util.Set;

import javax.annotation.CheckForNull;

Expand All @@ -32,9 +34,11 @@

public class SwitchHandler {
private final List<SwitchDetails> switchOffsetStack;
private final Set<Integer> typeSwitchPcs;

public SwitchHandler() {
switchOffsetStack = new ArrayList<>();
typeSwitchPcs = new HashSet<>();
}

public int stackSize() {
Expand All @@ -55,11 +59,13 @@ int numEnumValues(@CheckForNull XClass c) {
}

public void enterSwitch(DismantleBytecode dbc, @CheckForNull XClass enumType) {

assert dbc.getOpcode() == Const.TABLESWITCH || dbc.getOpcode() == Const.LOOKUPSWITCH;
int[] switchOffsets = dbc.getSwitchOffsets();
SwitchDetails details = new SwitchDetails(dbc.getPC(), switchOffsets, dbc.getDefaultSwitchOffset(), switchOffsets.length == numEnumValues(
enumType));
enterSwitch(dbc.getOpcode(), dbc.getPC(), switchOffsets, dbc.getDefaultSwitchOffset(), switchOffsets.length == numEnumValues(enumType));
}

public void enterSwitch(int opCode, int pc, int[] switchOffsets, int defaultSwitchOffset, @CheckForNull boolean exhaustive) {
assert opCode == Const.TABLESWITCH || opCode == Const.LOOKUPSWITCH;
SwitchDetails details = new SwitchDetails(pc, switchOffsets, defaultSwitchOffset, exhaustive);


int size = switchOffsetStack.size();
Expand Down Expand Up @@ -120,6 +126,45 @@ public SourceLineAnnotation getCurrentSwitchStatement(BytecodeScanningDetector d
detector.getClassContext(), detector, details.switchPC, details.switchPC + details.maxOffset - 1);
}

public void sawInvokeDynamic(int pc, String methodName) {
if ("typeSwitch".equals(methodName)) {
typeSwitchPcs.add(pc + 5);
}
}

/**
* @param opCode The operation code
* @param pc The program counter
* @return <code>true</code>If this instruction is a cast for a type switch
*/
public boolean isTypeSwitchCaseCheckCast(int opCode, int pc) {
if (opCode != Const.CHECKCAST) {
return false;
}

// For a type switch each case starts with an aload_2 followed by a checkcast
// 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

return typeSwitchPcs.contains(switchDetails.switchPC);
}

return false;
}

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

if (pc == switchDetails.swOffsets[i] + switchDetails.switchPC) {
return switchDetails;
}
}
}

return null;
}

public static class SwitchDetails {
final int switchPC;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import org.apache.bcel.generic.LDC;
import org.apache.bcel.generic.MethodGen;
import org.apache.bcel.generic.ReferenceType;
import org.apache.bcel.generic.LOOKUPSWITCH;
import org.apache.bcel.generic.Type;
import org.apache.bcel.generic.TypedInstruction;

Expand All @@ -38,6 +39,7 @@
import edu.umd.cs.findbugs.MethodAnnotation;
import edu.umd.cs.findbugs.Priorities;
import edu.umd.cs.findbugs.SourceLineAnnotation;
import edu.umd.cs.findbugs.SwitchHandler;
import edu.umd.cs.findbugs.SystemProperties;
import edu.umd.cs.findbugs.ba.CFG;
import edu.umd.cs.findbugs.ba.CFGBuilderException;
Expand Down Expand Up @@ -196,6 +198,7 @@ private void analyzeMethod(ClassContext classContext, Method method) throws CFGB
Map<BugAnnotation, String> instanceOfChecks = new HashMap<>();
String constantClass = null;
boolean methodInvocationWasGeneric = false;
SwitchHandler switchHandler = new SwitchHandler();

int pcForConstantClass = -1;
for (Iterator<Location> i = cfg.locationIterator(); i.hasNext();) {
Expand All @@ -207,8 +210,24 @@ private void analyzeMethod(ClassContext classContext, Method method) throws CFGB

boolean wasMethodInvocationWasGeneric = methodInvocationWasGeneric;
methodInvocationWasGeneric = false;

if (ins instanceof LOOKUPSWITCH) {
LOOKUPSWITCH switchInstruction = (LOOKUPSWITCH) ins;
int[] indices = switchInstruction.getIndices();

switchHandler.enterSwitch(ins.getOpcode(),
pc,
indices,
0, // Not sure how to get the default offset from BCEL but it doesn't matter here
false); // It shouldn't matter here if the switch was exhaustive or not
}

if (ins instanceof InvokeInstruction) {
if (ins instanceof INVOKEDYNAMIC) {
INVOKEDYNAMIC invokeDynamicInstruction = (INVOKEDYNAMIC) ins;
String invokeMethodName = invokeDynamicInstruction.getMethodName(cpg);
switchHandler.sawInvokeDynamic(pc, invokeMethodName);

continue;
}
InvokeInstruction iinv = (InvokeInstruction) ins;
Expand Down Expand Up @@ -325,6 +344,11 @@ private void analyzeMethod(ClassContext classContext, Method method) throws CFGB
continue;
}

if (switchHandler.isTypeSwitchCaseCheckCast(ins.getOpcode(), pc)) {
// javac adds a checkcast for each case in a type switch, ignore them
continue;
}

String refSig = refType.getSignature();
String castSig2 = castSig;
String refSig2 = refSig;
Expand Down
7 changes: 7 additions & 0 deletions spotbugsTestCases/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -81,10 +81,17 @@ def classesJava17 = tasks.register('classesJava17', JavaCompile) {
source = file('src/java17')
}

def classesJava21 = tasks.register('classesJava21', JavaCompile) {
destinationDirectory.set(layout.buildDirectory.dir("classes/java/java21"))
classpath = sourceSets.main.compileClasspath
source = file('src/java21')
}

tasks.named('classes').configure {
dependsOn classesJava8
dependsOn classesJava11
dependsOn classesJava17
dependsOn classesJava21
}

sonar {
Expand Down
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.