Do not report DLS_DEAD_LOCAL_STORE for java 21 type switch variables (

#2828) * test: reproducer for issue #2736 * fix: detect astore instructions corresponding to java 21 type switches * doc: added changelog entry * fix: reverted start import change * fix: ordered imports and removed duplicate
spotbugs · Feb 3, 2024 · d4380b3 · d4380b3
1 parent 0487564
commit d4380b3
Show file tree

Hide file tree

Showing 5 changed files with 88 additions and 5 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -18,7 +18,7 @@ Currently the versioning policy of this project follows [Semantic Versioning v2.
 - Remove AppleExtension library (note: menus slightly changed) ([#2823](https://github.com/spotbugs/spotbugs/pull/2823))
 - Fixed error preventing SpotBugs from reporting FE_FLOATING_POINT_EQUALITY ([#2843](https://github.com/spotbugs/spotbugs/pull/2843))
 - Fixed NP_LOAD_OF_KNOWN_NULL_VALUE and RCN_REDUNDANT_NULLCHECK_OF_NULL_VALUE false positives in try-with-resources generated finally blocks ([#2844](https://github.com/spotbugs/spotbugs/pull/2844))
-
+- Do not report DLS_DEAD_LOCAL_STORE for Java 21's type switches ([#2828](https://github.com/spotbugs/spotbugs/pull/2828))
 
 ### Added
 - New detector `MultipleInstantiationsOfSingletons` and introduced new bug types:

diff --git a/spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/Issue2782Test.java b/spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/Issue2782Test.java
@@ -21,11 +21,17 @@ void testIssue() {
                 "../java21/Issue2782$Impl1.class",
                 "../java21/Issue2782$Impl2.class");
 
+        // Checks for issue 2782
         assertBugCount("BC_UNCONFIRMED_CAST", 1);
         assertBugCount("BC_UNCONFIRMED_CAST_OF_RETURN_VALUE", 1);
 
         assertBugAtLine("BC_UNCONFIRMED_CAST", 50);
         assertBugAtLine("BC_UNCONFIRMED_CAST_OF_RETURN_VALUE", 49);
+
+        // Checks for issue 2736
+        assertBugCount("DLS_DEAD_LOCAL_STORE", 1);
+
+        assertBugAtLine("DLS_DEAD_LOCAL_STORE", 59);
     }
 
     private void assertBugCount(String type, int expectedCount) {

diff --git a/spotbugs/src/main/java/edu/umd/cs/findbugs/SwitchHandler.java b/spotbugs/src/main/java/edu/umd/cs/findbugs/SwitchHandler.java
@@ -27,7 +27,13 @@
 import javax.annotation.CheckForNull;
 
 import org.apache.bcel.Const;
+import org.apache.bcel.generic.ASTORE;
+import org.apache.bcel.generic.CHECKCAST;
+import org.apache.bcel.generic.Instruction;
+import org.apache.bcel.generic.InstructionHandle;
 
+import edu.umd.cs.findbugs.ba.BasicBlock;
+import edu.umd.cs.findbugs.ba.Location;
 import edu.umd.cs.findbugs.ba.XClass;
 import edu.umd.cs.findbugs.ba.XField;
 import edu.umd.cs.findbugs.visitclass.DismantleBytecode;
@@ -73,7 +79,7 @@ public void enterSwitch(DismantleBytecode dbc, @CheckForNull XClass enumType) {
      * @param defaultSwitchOffset The PC of the default case
      * @param exhaustive <code>true</code> if the switch is exhaustive
      */
-    public void enterSwitch(int opCode, int pc, int[] switchOffsets, int defaultSwitchOffset, @CheckForNull boolean exhaustive) {
+    public void enterSwitch(int opCode, int pc, int[] switchOffsets, int defaultSwitchOffset, boolean exhaustive) {
         assert opCode == Const.TABLESWITCH || opCode == Const.LOOKUPSWITCH;
         SwitchDetails details = new SwitchDetails(pc, switchOffsets, defaultSwitchOffset, exhaustive);
 
@@ -171,6 +177,36 @@ public boolean isTypeSwitchCaseCheckCast(int opCode, int pc) {
         return switchDetails != null && typeSwitchPC.contains(switchDetails.switchPC);
     }
 
+    /**
+     * In type switches an <code>ASTORE</code> is inserted by the compiler for each case. This method checks if the
+     * instruction is one of these loads and then checks if the corresponding switch is a type switch.
+     * We're looking for: an ASTORE preceded by a CHECKCAST preceded by an instruction at the offset of a switch case
+     *
+     * @param location The Location
+     * @return <code>true</code>If this instruction is a load for a type switch
+     */
+    public boolean isTypeSwitchCaseLoad(Location location) {
+        Instruction ins = location.getHandle().getInstruction();
+        if (!(ins instanceof ASTORE)) {
+            return false;
+        }
+
+        BasicBlock block = location.getBasicBlock();
+        InstructionHandle prev = block.getPredecessorOf(location.getHandle());
+
+        if (prev == null) {
+            return false;
+        }
+
+        if (!(prev.getInstruction() instanceof CHECKCAST)) {
+            return false;
+        }
+
+        SwitchDetails switchDetails = findSwitchDetailsByPc(prev.getPosition() - 1, prev.getPosition() - 2);
+
+        return switchDetails != null && typeSwitchPC.contains(switchDetails.switchPC);
+    }
+
     /**
      * Finds a switch from the first PC of a case
      *

diff --git a/spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindDeadLocalStores.java b/spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindDeadLocalStores.java
@@ -47,17 +47,20 @@
 import org.apache.bcel.generic.Instruction;
 import org.apache.bcel.generic.InstructionHandle;
 import org.apache.bcel.generic.InvokeInstruction;
+import org.apache.bcel.generic.INVOKEDYNAMIC;
 import org.apache.bcel.generic.LDC;
 import org.apache.bcel.generic.LRETURN;
 import org.apache.bcel.generic.LSTORE;
 import org.apache.bcel.generic.LoadInstruction;
+import org.apache.bcel.generic.LOOKUPSWITCH;
 import org.apache.bcel.generic.MULTIANEWARRAY;
 import org.apache.bcel.generic.MethodGen;
 import org.apache.bcel.generic.NEWARRAY;
 import org.apache.bcel.generic.ObjectType;
 import org.apache.bcel.generic.StoreInstruction;
 import org.apache.bcel.generic.Type;
 
+
 import edu.umd.cs.findbugs.BugAccumulator;
 import edu.umd.cs.findbugs.BugInstance;
 import edu.umd.cs.findbugs.BugReporter;
@@ -67,6 +70,7 @@
 import edu.umd.cs.findbugs.LocalVariableAnnotation;
 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.AnalysisContext;
 import edu.umd.cs.findbugs.ba.CFG;
@@ -219,6 +223,7 @@ private void analyzeMethod(ClassContext classContext, Method method) throws Data
         int[] localLoadCount = new int[numLocals];
         int[] localIncrementCount = new int[numLocals];
         MethodGen methodGen = classContext.getMethodGen(method);
+        ConstantPoolGen cpg = classContext.getConstantPoolGen();
         CFG cfg = classContext.getCFG(method);
         if (cfg.isFlagSet(CFG.FOUND_INEXACT_UNCONDITIONAL_THROWERS)) {
             return;
@@ -245,6 +250,9 @@ private void analyzeMethod(ClassContext classContext, Method method) throws Data
         // due to inlining of finally blocks.
         BitSet liveStoreSourceLineSet = new BitSet();
 
+        // Keep track of the switches so we can ignore the dead stores corresponding to type switches
+        SwitchHandler switchHandler = new SwitchHandler();
+
         // Scan method for
         // - dead stores
         // - stores to parameters that are dead upon entry to the method
@@ -254,18 +262,41 @@ private void analyzeMethod(ClassContext classContext, Method method) throws Data
             BugInstance pendingBugReportAboutOverwrittenParameter = null;
             try {
                 WarningPropertySet<WarningProperty> propertySet = new WarningPropertySet<>();
+
+                InstructionHandle handle = location.getHandle();
+                int pc = handle.getPosition();
+
+                if (handle.getInstruction() instanceof INVOKEDYNAMIC) {
+                    INVOKEDYNAMIC invokeDynamicInstruction = (INVOKEDYNAMIC) handle.getInstruction();
+                    String invokeMethodName = invokeDynamicInstruction.getMethodName(cpg);
+                    switchHandler.sawInvokeDynamic(pc, invokeMethodName);
+
+                    continue;
+                } else if (handle.getInstruction() instanceof LOOKUPSWITCH) {
+                    LOOKUPSWITCH switchInstruction = (LOOKUPSWITCH) handle.getInstruction();
+                    int[] indices = switchInstruction.getIndices();
+
+                    switchHandler.enterSwitch(switchInstruction.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
+                }
+
                 // Skip any instruction which is not a store
                 if (!isStore(location)) {
                     continue;
                 }
 
+                if (switchHandler.isTypeSwitchCaseLoad(location)) {
+                    continue;
+                }
+
                 // Heuristic: exception handler blocks often contain
                 // dead stores generated by the compiler.
                 if (location.getBasicBlock().isExceptionHandler()) {
                     propertySet.addProperty(DeadLocalStoreProperty.EXCEPTION_HANDLER);
                 }
-                InstructionHandle handle = location.getHandle();
-                int pc = handle.getPosition();
                 IndexedInstruction ins = (IndexedInstruction) location.getHandle().getInstruction();
 
                 int local = ins.getIndex();
@@ -369,7 +400,6 @@ private void analyzeMethod(ClassContext classContext, Method method) throws Data
                         continue; // not an interesting dead store
                     } else if (prevIns instanceof GETSTATIC) {
                         GETSTATIC getStatic = (GETSTATIC) prevIns;
-                        ConstantPoolGen cpg = methodGen.getConstantPool();
                         foundDeadClassInitialization = getStatic.getFieldName(cpg).startsWith("class$")
                                 && "Ljava/lang/Class;".equals(getStatic.getSignature(cpg));
                         for (Iterator<Location> j = cfg.locationIterator(); j.hasNext();) {

diff --git a/spotbugsTestCases/src/java21/Issue2782.java b/spotbugsTestCases/src/java21/Issue2782.java
@@ -51,4 +51,15 @@ public static int badCast(Object value) {
             default -> 42;
         };
     }
+
+    public static int deadStore(int value) {
+        switch (value) {
+            case 0:
+            	int x = 0;
+                --x; // DLS_DEAD_LOCAL_STORE
+            	return value;
+            default:
+            	return 42;
+        }
+    }
 }