spotbugs#600: Fix false positves for RCN_REDUNDANT_NULLCHECK_OF_NONNU…

…LL_VALUE in Java 11

This should fix issue spotbugs#600 and spotbugs#1338. I also fixed the broken tests for spotbugs#259.

CHANGELOG.md

@@ -10,6 +10,9 @@ Currently the versioning policy of this project follows [Semantic Versioning v2.
 * Bump Saxon-HE from 10.3 to 10.5 ([#1513](https://github.com/spotbugs/spotbugs/pull/1513))
 * Function `mutableSignature()` improved and factored out from the `MutableStaticFields` detector
 
+### Fixed
+- Fixed False positives for RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE ([#600](https://github.com/spotbugs/spotbugs/issues/600) and [#1338](https://github.com/spotbugs/spotbugs/issues/1338))
+
 ## 4.2.3 - 2021-04-12
 
 ### Fixed

spotbugs-tests/src/test/java/edu/umd/cs/findbugs/nullness/Issue1338Test.java

@@ -0,0 +1,26 @@
+package edu.umd.cs.findbugs.nullness;
+
+import edu.umd.cs.findbugs.BugCollection;
+import edu.umd.cs.findbugs.test.SpotBugsRule;
+import org.junit.Rule;
+import org.junit.Test;
+
+import java.nio.file.Paths;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.emptyIterable;
+
+/**
+ * Unit test to reproduce <a href="https://github.com/spotbugs/spotbugs/issues/1338">#1338</a>.
+ */
+public class Issue1338Test {
+    @Rule
+    public SpotBugsRule spotbugs = new SpotBugsRule();
+
+    @Test
+    public void test() {
+        BugCollection bugCollection = spotbugs.performAnalysis(
+                Paths.get("../spotbugsTestCases/build/classes/java/main/ghIssues/Issue1338.class"));
+        assertThat(bugCollection, emptyIterable());
+    }
+}

spotbugs-tests/src/test/java/edu/umd/cs/findbugs/nullness/Issue259Test.java

@@ -1,7 +1,7 @@
 package edu.umd.cs.findbugs.nullness;
 
+import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.emptyIterable;
-import static org.junit.Assert.assertThat;
 
 import java.nio.file.Paths;
 
@@ -21,7 +21,9 @@ public class Issue259Test {
     @Test
     public void test() {
         BugCollection bugCollection = spotbugs.performAnalysis(
-                Paths.get("../spotbugsTestCases/build/classes/java/main/ghIssues/Issue259.class"));
+                Paths.get("../spotbugsTestCases/build/classes/java/main/ghIssues/Issue259.class"),
+                Paths.get("../spotbugsTestCases/build/classes/java/main/ghIssues/Issue259$1.class"),
+                Paths.get("../spotbugsTestCases/build/classes/java/main/ghIssues/Issue259$X.class"));
         assertThat(bugCollection, emptyIterable());
     }
 }

spotbugs-tests/src/test/java/edu/umd/cs/findbugs/nullness/Issue600Test.java

@@ -0,0 +1,50 @@
+package edu.umd.cs.findbugs.nullness;
+
+import edu.umd.cs.findbugs.BugCollection;
+import edu.umd.cs.findbugs.test.SpotBugsRule;
+import edu.umd.cs.findbugs.test.matcher.BugInstanceMatcher;
+import edu.umd.cs.findbugs.test.matcher.BugInstanceMatcherBuilder;
+import org.junit.Rule;
+import org.junit.Test;
+
+import java.nio.file.Paths;
+
+import static edu.umd.cs.findbugs.test.CountMatcher.containsExactly;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+/**
+ * Unit tests to reproduce <a href="https://github.com/spotbugs/spotbugs/issues/600">#600</a>.
+ */
+public class Issue600Test {
+    @Rule
+    public SpotBugsRule spotbugs = new SpotBugsRule();
+
+    @Test
+    public void testTryWithResourcesSimple() {
+        assertBugCount("TryWithResourcesSimple", 0);
+        assertBugCount("TryWithResourcesMultiple", 0);
+        assertBugCount("TryWithResourcesNested", 0);
+    }
+
+    @Test
+    public void testExplicitNulls() {
+        assertBugCount("ExplicitNullCheckSimple", 1);
+        assertBugCount("ExplicitNullCheckNested", 2);
+        assertBugCount("ExplicitNullCheckMultiple", 2);
+        assertBugCount("Mixed1", 1);
+        assertBugCount("Mixed2", 1);
+    }
+
+    private void assertBugCount(String clazz, int count) {
+        BugCollection bugCollection = analyse(clazz);
+        final BugInstanceMatcher bugTypeMatcher = new BugInstanceMatcherBuilder()
+                .bugType("RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE").build();
+        assertThat(bugCollection, containsExactly(count, bugTypeMatcher));
+    }
+
+    private BugCollection analyse(String clazz) {
+        return spotbugs.performAnalysis(
+                Paths.get("../spotbugsTestCases/build/classes/java/main/ghIssues/issue600/" + clazz + ".class"),
+                Paths.get("../spotbugsTestCases/build/classes/java/main/ghIssues/issue600/MyAutocloseable.class"));
+    }
+}

spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindNullDeref.java

@@ -30,6 +30,7 @@
 import java.util.Set;
 import java.util.SortedSet;
 import java.util.TreeSet;
+import java.util.stream.Collectors;
 
 import javax.annotation.CheckForNull;
 import javax.annotation.Nonnull;
@@ -48,6 +49,7 @@
 import org.apache.bcel.generic.IFNONNULL;
 import org.apache.bcel.generic.IFNULL;
 import org.apache.bcel.generic.INVOKEDYNAMIC;
+import org.apache.bcel.generic.INVOKEVIRTUAL;
 import org.apache.bcel.generic.Instruction;
 import org.apache.bcel.generic.InstructionHandle;
 import org.apache.bcel.generic.InstructionList;
@@ -1799,7 +1801,10 @@ boolean inIndirectCatchNullBlock(Location loc) {
      * @param cp the constant pool
      * @param pc the program counter
      * @return true if the pc specifies redundant null check generated by javac
-     * @see <a href="https://github.com/spotbugs/spotbugs/issues/259">false positive RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE on try-with-resources</a>
+     * @see
+     * <a href="https://github.com/spotbugs/spotbugs/issues/259">false positive RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE on try-with-resources</a>
+     * @see
+     * <a href="https://github.com/spotbugs/spotbugs/issues/600">false positive RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE on try-with-resources</a>
      */
     private boolean isGeneratedCodeInCatchBlock(@NonNull ConstantPool cp, int pc) {
         Code code = method.getCode();
@@ -1811,9 +1816,8 @@ private boolean isGeneratedCodeInCatchBlock(@NonNull ConstantPool cp, int pc) {
         // TODO: This instantiation could be high cost computation
         InstructionList list = new InstructionList(code.getCode());
 
-        return Arrays.stream(table)
+        List<CodeException> throwableList = Arrays.stream(table)
                 .filter(codeException -> {
-                    // assume that programmers rarely catch java.lang.Throwable instance explicitly
                     int catchType = codeException.getCatchType();
                     if (catchType == 0) {
                         // '0' means it catches any exceptions
@@ -1822,6 +1826,97 @@ private boolean isGeneratedCodeInCatchBlock(@NonNull ConstantPool cp, int pc) {
                     String exceptionName = cp.getConstantString(catchType, Const.CONSTANT_Class);
                     return "java/lang/Throwable".equals(exceptionName);
                 })
+                .collect(Collectors.toList());
+
+        ConstantPoolGen cpg = new ConstantPoolGen(cp);
+
+        LineNumberTable lineNumberTable = code.getLineNumberTable();
+        // The compiler generated code can also be written by the regular program. The only
+        // difference is that the line numbers for a lot of instructions are the same.
+        // This is what the code below relies on. Line numbers are optional, so it might
+        // not work in call cases.
+        //
+        // The following operations must have the same line numbers as the start or end of the original try catch block.
+        // - the reported code position (which is the null check)
+        // - at least one assignment
+        // - at least one call of addSuppressed
+        // - at least one throws statement
+        //
+        // The two calls to the close method need to have the same line number as the end of a throwables catch block.
+        // There must be also another catch of Throwable, but the line number does not need to match.
+        //
+        // To understand the code, please have a look at the test Issue600Test.
+        // TryWithResources* are the try-with-resources examples, ExplicitNullCheck* is TryWithResources* compiled and then decompiled.
+        if (lineNumberTable != null) {
+            // this line also marks the end of the try catch block
+            int line = lineNumberTable.getSourceLine(pc);
+            if (line > 0) {
+                // The two generated catch blocks might show different starting line numbers. Both are needed.
+                Set<Integer> relevantLineNumbers = throwableList.stream()
+                        .map(x -> lineNumberTable.getSourceLine(x.getStartPC()))
+                        .collect(Collectors.toSet());
+                relevantLineNumbers.add(line);
+
+                boolean assignmentPresent = false;
+                boolean addSuppressedPresent = false;
+                boolean throwsPresent = false;
+                int closeCounter = 0;
+                for (InstructionHandle handle : list.getInstructionHandles()) {
+                    int currentLine = lineNumberTable.getSourceLine(handle.getPosition());
+
+                    switch (handle.getInstruction().getOpcode()) {
+                    case Const.ASTORE:
+                    case Const.ASTORE_0:
+                    case Const.ASTORE_1:
+                    case Const.ASTORE_2:
+                    case Const.ASTORE_3:
+                        if (relevantLineNumbers.contains(currentLine)) {
+                            assignmentPresent = true;
+                        }
+                        break;
+                    case Const.INVOKEVIRTUAL:
+                        if (handle.getInstruction() instanceof INVOKEVIRTUAL) {
+                            String methodName = ((INVOKEVIRTUAL) handle.getInstruction()).getMethodName(cpg);
+                            if ("close".equals(methodName)) {
+                                // the close methods get the line number of the end of the try block assigned
+                                if (throwableList.stream().anyMatch(x -> lineNumberTable.getSourceLine(x.getEndPC()) == currentLine)) {
+                                    closeCounter++;
+                                }
+                            } else if ("addSuppressed".equals(methodName)) {
+                                if (relevantLineNumbers.contains(currentLine)) {
+                                    addSuppressedPresent = true;
+                                }
+                            }
+                        }
+                        break;
+                    case Const.ATHROW:
+                        if (relevantLineNumbers.contains(currentLine) && handle.getInstruction() instanceof ATHROW) {
+                            Class<?>[] exceptions = ((ATHROW) handle.getInstruction()).getExceptions();
+                            if (exceptions.length == 1 && Throwable.class.equals(exceptions[0])) {
+                                // even if try-with-resources catches exceptions, the compiler generates a nested try-catch with Throwable.
+                                throwsPresent = true;
+                            }
+                        }
+                        break;
+                    default:
+                        break;
+                    }
+                }
+                boolean matchingCatches = false;
+                if (throwableList.size() >= 2) {
+                    // make sure that the reported line matches the start or end line of the generated try catch blocks
+                    matchingCatches = throwableList.stream().anyMatch(
+                            x -> lineNumberTable.getSourceLine(x.getStartPC()) == line) || throwableList.stream().anyMatch(
+                                    x -> lineNumberTable.getSourceLine(x.getEndPC()) == line);
+                }
+                return matchingCatches && assignmentPresent && addSuppressedPresent && throwsPresent && closeCounter >= 2;
+            }
+        }
+
+
+        // assume that programmers rarely catch java.lang.Throwable instance explicitly
+        return throwableList
+                .stream()
                 .anyMatch(codeException -> {
                     InstructionHandle handle = list.findHandle(codeException.getEndPC());
                     int insnLength = handle.getInstruction().getLength();

spotbugsTestCases/src/java/ghIssues/Issue1338.java

@@ -0,0 +1,17 @@
+package ghIssues;
+
+import java.io.InputStream;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.security.KeyStore;
+
+public class Issue1338 {
+    public static KeyStore loadKeyStore(final Path path) throws Exception {
+        try (InputStream inputStream = Files.newInputStream(path)) {
+            // final KeyStore keyStore = KeyStore.getInstance("JKS");
+            // keyStore.load(inputStream, password.toCharArray());
+            // return keyStore;
+            return KeyStore.getInstance("JKS");
+        }
+    }
+}

spotbugsTestCases/src/java/ghIssues/issue600/ExplicitNullCheckMultiple.java

@@ -0,0 +1,48 @@
+package ghIssues.issue600;
+
+/**
+ * This class is the compiled and decompiled version of TryWithResourcesMultiple.
+ */
+public class ExplicitNullCheckMultiple {
+    public void test() throws Exception {
+        MyAutocloseable closeable1 = MyAutocloseable.create();
+
+        try {
+            MyAutocloseable closeable2 = MyAutocloseable.create();
+
+            try {
+                closeable1.exampleMethod();
+                closeable2.exampleMethod();
+            } catch (Throwable var7) {
+                if (closeable2 != null) {
+                    try {
+                        closeable2.close();
+                    } catch (Throwable var6) {
+                        var7.addSuppressed(var6);
+                    }
+                }
+
+                throw var7;
+            }
+
+            if (closeable2 != null) {
+                closeable2.close();
+            }
+        } catch (Throwable var8) {
+            if (closeable1 != null) {
+                try {
+                    closeable1.close();
+                } catch (Throwable var5) {
+                    var8.addSuppressed(var5);
+                }
+            }
+
+            throw var8;
+        }
+
+        if (closeable1 != null) {
+            closeable1.close();
+        }
+
+    }
+}

spotbugsTestCases/src/java/ghIssues/issue600/ExplicitNullCheckNested.java

@@ -0,0 +1,48 @@
+package ghIssues.issue600;
+
+/**
+ * This class is the compiled and decompiled version of TryWithResourcesNested.
+ */
+public class ExplicitNullCheckNested {
+    public void test() throws Exception {
+        MyAutocloseable closeable1 = MyAutocloseable.create();
+
+        try {
+            MyAutocloseable closeable2 = MyAutocloseable.create();
+
+            try {
+                closeable1.exampleMethod();
+                closeable2.exampleMethod();
+            } catch (Throwable var7) {
+                if (closeable2 != null) {
+                    try {
+                        closeable2.close();
+                    } catch (Throwable var6) {
+                        var7.addSuppressed(var6);
+                    }
+                }
+
+                throw var7;
+            }
+
+            if (closeable2 != null) {
+                closeable2.close();
+            }
+        } catch (Throwable var8) {
+            if (closeable1 != null) {
+                try {
+                    closeable1.close();
+                } catch (Throwable var5) {
+                    var8.addSuppressed(var5);
+                }
+            }
+
+            throw var8;
+        }
+
+        if (closeable1 != null) {
+            closeable1.close();
+        }
+
+    }
+}

spotbugsTestCases/src/java/ghIssues/issue600/ExplicitNullCheckSimple.java

@@ -0,0 +1,27 @@
+package ghIssues.issue600;
+
+/**
+ * This class is the compiled and decompiled version of TryWithResourcesSimple.
+ */
+public class ExplicitNullCheckSimple {
+    public void test() throws Exception {
+        MyAutocloseable closeable = MyAutocloseable.create();
+
+        try {
+            closeable.exampleMethod();
+        } catch (Throwable var5) {
+            if (closeable != null) {
+                try {
+                    closeable.close();
+                } catch (Throwable var4) {
+                    var5.addSuppressed(var4);
+                }
+            }
+            throw var5;
+        }
+
+        if (closeable != null) {
+            closeable.close();
+        }
+    }
+}