Fix spotbugs#79, solving false positive OBL_UNSATIFIED_OBLIGATION wit…

…h try with resources (spotbugs#1479) * Fix issue spotbugs#79 * modification according to reviews * Modify based on requested changes * Fix | Log.debug format, LoggerFactory usage
gamesh411 · Apr 15, 2021 · 9624de3 · 9624de3
1 parent 85f2763
commit 9624de3
Show file tree

Hide file tree

Showing 4 changed files with 75 additions and 1 deletion.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -7,9 +7,9 @@ Currently the versioning policy of this project follows [Semantic Versioning v2.
 ## Unreleased - 2021-??-??
 
 ### Fixed
-
 - Inconsistency in the description of `DLS_DEAD_LOCAL_INCREMENT_IN_RETURN`, `VO_VOLATILE_INCREMENT` and `QF_QUESTIONABLE_FOR_LOOP` ([#1470](https://github.com/spotbugs/spotbugs/issues/1470))
 - Should issue warning for SecureRandom object created and used only once ([#1464](https://github.com/spotbugs/spotbugs/issues/1464))
+- False positive OBL_UNSATIFIED_OBLIGATION with try with resources ([#79](https://github.com/spotbugs/spotbugs/issues/79))
 
 ## 4.2.2 - 2021-03-03
 

diff --git a/spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/Issue79Test.java b/spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/Issue79Test.java
@@ -0,0 +1,27 @@
+package edu.umd.cs.findbugs.detect;
+
+import edu.umd.cs.findbugs.AbstractIntegrationTest;
+import edu.umd.cs.findbugs.test.matcher.BugInstanceMatcher;
+import edu.umd.cs.findbugs.test.matcher.BugInstanceMatcherBuilder;
+import org.junit.Test;
+
+import static edu.umd.cs.findbugs.test.CountMatcher.containsExactly;
+import static org.junit.Assert.assertThat;
+
+/**
+ * SpotBugs should remove the ResultSet obligation from all set
+ * when one occurrence of that type of obligation is Statement
+ * from all states.
+ *
+ * @see <a href="https://github.com/spotbugs/spotbugs/issues/79">GitHub issue</a>
+ * @since 4.1.3
+ */
+public class Issue79Test extends AbstractIntegrationTest {
+
+    @Test
+    public void test() {
+        performAnalysis("ghIssues/Issue79.class");
+        BugInstanceMatcher bugMatcher = new BugInstanceMatcherBuilder().build();
+        assertThat(getBugCollection(), containsExactly(0, bugMatcher));
+    }
+}
diff --git a/spotbugs/src/main/java/edu/umd/cs/findbugs/ba/obl/ObligationAnalysis.java b/spotbugs/src/main/java/edu/umd/cs/findbugs/ba/obl/ObligationAnalysis.java
@@ -19,6 +19,7 @@
 
 package edu.umd.cs.findbugs.ba.obl;
 
+import java.lang.invoke.MethodHandles;
 import java.util.Collection;
 import java.util.HashMap;
 import java.util.HashSet;
@@ -53,6 +54,7 @@
 import edu.umd.cs.findbugs.classfile.ClassDescriptor;
 import edu.umd.cs.findbugs.classfile.DescriptorFactory;
 import edu.umd.cs.findbugs.classfile.IErrorLogger;
+import org.slf4j.LoggerFactory;
 
 /**
  * Dataflow analysis to track obligations (i/o streams and other resources which
@@ -89,6 +91,8 @@ public class ObligationAnalysis extends ForwardDataflowAnalysis<StateSet> {
 
     static final ClassDescriptor willClose = DescriptorFactory.createClassDescriptor(WillClose.class);
 
+    private static final org.slf4j.Logger LOG = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
     /**
      * Constructor.
      *
@@ -215,6 +219,16 @@ public void edgeTransfer(Edge edge, StateSet fact) throws DataflowAnalysisExcept
                             + edge.getSource().getLastInstruction());
                 }
                 fact.deleteObligation(comparedObligation, edge.getTarget().getLabel());
+
+                // closing a Statement closes the ResultSet
+                Obligation statement = database.getFactory().getObligationByName("java.sql.Statement");
+                if (comparedObligation.equals(statement)) {
+                    Obligation resultSet = database.getFactory().getObligationByName("java.sql.ResultSet");
+                    fact.deleteObligation(resultSet, edge.getTarget().getLabel());
+                    if (DEBUG_NULL_CHECK) {
+                        LOG.debug("Deleting {} on edge from comparison {}", resultSet, edge.getSource().getLastInstruction());
+                    }
+                }
             }
         }
     }

diff --git a/spotbugsTestCases/src/java/ghIssues/Issue79.java b/spotbugsTestCases/src/java/ghIssues/Issue79.java
@@ -0,0 +1,33 @@
+package ghIssues;
+
+import org.slf4j.LoggerFactory;
+import java.lang.invoke.MethodHandles;
+import java.sql.Connection;
+import java.sql.PreparedStatement;
+import java.sql.ResultSet;
+import java.sql.SQLException;
+
+public class Issue79 {
+    private static final String QUERY = "";
+    private static final org.slf4j.Logger LOG = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+    public void f(Connection cnx) throws SQLException {
+        PreparedStatement st = null;
+        ResultSet rs = null;
+        try {
+            st = cnx.prepareStatement(QUERY);
+            rs = st.executeQuery();
+            while (rs.next()) {
+                LOG.info(rs.getString("ID"));
+            }
+        } finally {
+            /*
+             * The statement closes the result set, and there is no scenario where st may be null
+             * but not the resultset, however an unsatisfied obligation is reported on the resultset
+             */
+            if (st != null) {
+                st.close();
+            }
+        }
+    }
+}