fix: redundant RCN false positive to build SpotBugs with Java 11 (#1248)

* ci: build with Java 11

* build: configure Gradle to use --release option

As a workaround for #996, skip the compilation for Bug1169.java in
soitbugsTestCases project.

* build: fix a typo in the build script

* fix: ignore RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE right after try-block

close #259

* docs: add CHANGELOG entry to describe the fix

.github/workflows/build.yml

@@ -12,10 +12,10 @@ jobs:
       - uses: actions/checkout@722adc6
         with:
           fetch-depth: 0
-      - name: Set up JDK 8
+      - name: Set up JDK 11
         uses: actions/setup-java@081536e
         with:
-          java-version: '8.0.x'
+          java-version: '11.0.x'
       - uses: actions/cache@cffae95
         with:
           path: ~/.gradle/caches

.github/workflows/release.yml

@@ -11,10 +11,10 @@ jobs:
 
     steps:
       - uses: actions/checkout@722adc6
-      - name: Set up JDK 8
+      - name: Set up JDK 11
         uses: actions/setup-java@081536e
         with:
-          java-version: '8.0.x'
+          java-version: '11.0.x'
       - uses: actions/cache@cffae95
         with:
           path: ~/.gradle/caches

CHANGELOG.md

@@ -5,6 +5,8 @@ This is the changelog for SpotBugs. This follows [Keep a Changelog v1.0.0](http:
 Currently the versioning policy of this project follows [Semantic Versioning v2.0.0](http://semver.org/spec/v2.0.0.html).
 
 ## Unreleased - 2020-??-??
+### Fixed
+* False positive `RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE` on try-with-resources ([#259](https://github.com/spotbugs/spotbugs/issues/259))
 
 ## 4.1.2 - 2020-08-18
 ### Fixed

build.gradle

@@ -22,10 +22,6 @@ subprojects {
   group = 'com.github.spotbugs'
   version = rootProject.version
 
-  // Java versions
-  project.sourceCompatibility = 1.8
-  project.targetCompatibility = 1.8
-
   repositories {
     jcenter()
   }

gradle/java.gradle

@@ -7,6 +7,9 @@ tasks.withType(JavaCompile) {
 tasks.withType(Javadoc) {
     options.encoding = 'UTF-8'
 }
+tasks.compileJava {
+    options.compilerArgs.addAll(['--release', '8'])
+}
 
 // make the build reproducible https://reproducible-builds.org/
 tasks.withType(AbstractArchiveTask) {

spotbugs/build.gradle

@@ -109,6 +109,10 @@ clean {
   delete ".libs"
 }
 
+tasks.compileGuiJava {
+  options.compilerArgs.addAll(['--release', '8'])
+}
+
 tasks.withType(Jar).all {
   includeEmptyDirs = false
   // FIXME: this is ugly, but is what ant is currently doing... our own jars are included as dependencies :S

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

@@ -35,6 +35,7 @@
 import javax.annotation.Nonnull;
 import javax.annotation.meta.When;
 
+import edu.umd.cs.findbugs.annotations.NonNull;
 import org.apache.bcel.Const;
 import org.apache.bcel.classfile.Code;
 import org.apache.bcel.classfile.CodeException;
@@ -49,6 +50,7 @@
 import org.apache.bcel.generic.INVOKEDYNAMIC;
 import org.apache.bcel.generic.Instruction;
 import org.apache.bcel.generic.InstructionHandle;
+import org.apache.bcel.generic.InstructionList;
 import org.apache.bcel.generic.InstructionTargeter;
 import org.apache.bcel.generic.InvokeInstruction;
 import org.apache.bcel.generic.MethodGen;
@@ -126,6 +128,8 @@
 import edu.umd.cs.findbugs.props.WarningPropertyUtil;
 import edu.umd.cs.findbugs.util.Values;
 import edu.umd.cs.findbugs.visitclass.Util;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 /**
  * A Detector to find instructions where a NullPointerException might be raised.
@@ -137,6 +141,7 @@
  * @see edu.umd.cs.findbugs.ba.npe.IsNullValueAnalysis
  */
 public class FindNullDeref implements Detector, UseAnnotationDatabase, NullDerefAndRedundantComparisonCollector {
+    private final Logger log = LoggerFactory.getLogger(getClass());
 
     public static final boolean DEBUG = SystemProperties.getBoolean("fnd.debug");
 
@@ -922,7 +927,6 @@ public boolean skipIfInsideCatchNull() {
      *             instead
      */
     @Override
-    @Deprecated
     public void foundNullDeref(Location location, ValueNumber valueNumber, IsNullValue refValue, ValueNumberFrame vnaFrame) {
         foundNullDeref(location, valueNumber, refValue, vnaFrame, true);
     }
@@ -1153,6 +1157,9 @@ public void foundRedundantNullCheck(Location location, RedundantBranch redundant
             if (redundantBranch.firstValue.isDefinitelyNull()) {
                 warning = "RCN_REDUNDANT_NULLCHECK_OF_NULL_VALUE";
                 priority = NORMAL_PRIORITY;
+            } else if (isGeneratedCodeInCatchBlock(method.getConstantPool(), pc)) {
+                log.debug("skip RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE found in the generated code at {}", location);
+                return;
             } else {
                 warning = "RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE";
                 valueIsNull = false;
@@ -1783,4 +1790,42 @@ boolean inIndirectCatchNullBlock(Location loc) {
                 "java/lang/Throwable", pc);
         return catchSize < 5;
     }
+
+    /**
+     * Java 11+ compiler generates redundant null checks for try-with-resources.
+     * This methods detects the {@code ifnull} bytecode generated by javac,
+     * to help detector to avoid to report needless {@code RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE} bug.
+     *
+     * @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>
+     */
+    private boolean isGeneratedCodeInCatchBlock(@NonNull ConstantPool cp, int pc) {
+        Code code = method.getCode();
+        if (code == null) {
+            return false;
+        }
+        CodeException[] table = code.getExceptionTable();
+
+        // TODO: This instantiation could be high cost computation
+        InstructionList list = new InstructionList(code.getCode());
+
+        return 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
+                        return true;
+                    }
+                    String exceptionName = cp.getConstantString(catchType, Const.CONSTANT_Class);
+                    return "java/lang/Throwable".equals(exceptionName);
+                })
+                .anyMatch(codeException -> {
+                    InstructionHandle handle = list.findHandle(codeException.getEndPC());
+                    int insnLength = handle.getInstruction().getLength();
+                    return codeException.getEndPC() + insnLength == pc;
+                });
+    }
 }

spotbugsTestCases/build.gradle

@@ -32,6 +32,11 @@ dependencies {
 tasks.withType(JavaCompile).all {
   options.compilerArgs << '-Xlint:none'
   options.encoding = 'UTF-8'
+  if (it.name == 'classesJava11') {
+    options.compilerArgs.addAll(['--release', '11'])
+  } else if (it.name != 'compileJava') {
+    options.compilerArgs.addAll(['--release', '8'])
+  }
 }
 
 // This disables hundreds of javadoc warnings on missing tags etc, see #340
@@ -50,28 +55,19 @@ javadoc {
 }
 
 task classesJava8(type:JavaCompile) {
-  sourceCompatibility = '1.8'
-  targetCompatibility = '1.8'
   destinationDir = file("$buildDir/classes/java/java8")
   classpath = sourceSets.main.compileClasspath
   source = file('src/java8')
 }
 
 task classesJava11(type:JavaCompile) {
-  sourceCompatibility = '11'
-  targetCompatibility = '11'
   destinationDir = file("$buildDir/classes/java/java11")
   classpath = sourceSets.main.compileClasspath
   source = file('src/java11')
 }
 
-if (System.getProperty("java.specification.version") == "1.8") {
-  // JDK11 removed several deprecated methods like Thread#destroy() and System.runFinalizersOnExit().
-  // so code that depends on them should be built only on JDK8.
-  tasks['classes'].dependsOn classesJava8
-} else {
-  tasks['classes'].dependsOn classesJava11
-}
+tasks['classes'].dependsOn classesJava8
+tasks['classes'].dependsOn classesJava11
 
 sonarqube {
   // this project should not be analyzed with sonarqube