Fix a resource leak false positive due to a cast (#6821)

Co-authored-by: Manu Sridharan <[email protected]>

Author: Calvin-L

checker/src/main/java/org/checkerframework/checker/resourceleak/MustCallConsistencyAnalyzer.java

@@ -54,7 +54,6 @@
 import org.checkerframework.dataflow.cfg.block.Block.BlockType;
 import org.checkerframework.dataflow.cfg.block.ConditionalBlock;
 import org.checkerframework.dataflow.cfg.block.ExceptionBlock;
-import org.checkerframework.dataflow.cfg.block.SingleSuccessorBlock;
 import org.checkerframework.dataflow.cfg.node.AssignmentNode;
 import org.checkerframework.dataflow.cfg.node.ClassNameNode;
 import org.checkerframework.dataflow.cfg.node.FieldAccessNode;
@@ -66,7 +65,6 @@
 import org.checkerframework.dataflow.cfg.node.ReturnNode;
 import org.checkerframework.dataflow.cfg.node.SuperNode;
 import org.checkerframework.dataflow.cfg.node.ThisNode;
-import org.checkerframework.dataflow.cfg.node.TypeCastNode;
 import org.checkerframework.dataflow.expression.FieldAccess;
 import org.checkerframework.dataflow.expression.JavaExpression;
 import org.checkerframework.dataflow.expression.LocalVariable;
@@ -986,36 +984,6 @@ private boolean returnTypeIsMustCallAliasWithUntrackable(MethodInvocationNode no
     return !mustCallAliasArguments.isEmpty();
   }
 
-  /**
-   * Checks if {@code node} is either directly enclosed by a {@link TypeCastNode}, by looking at the
-   * successor block in the CFG. In this case the enclosing operator is a "no-op" that evaluates to
-   * the same value as {@code node}. This method is only used within {@link
-   * #propagateObligationsToSuccessorBlocks(ControlFlowGraph, Set, Block, Set, Deque)} to ensure
-   * Obligations are propagated to cast nodes properly. It relies on the assumption that a {@link
-   * TypeCastNode} will only appear in a CFG as the first node in a block.
-   *
-   * @param node the CFG node
-   * @return {@code true} if {@code node} is in a {@link SingleSuccessorBlock} {@code b}, the first
-   *     {@link Node} in {@code b}'s successor block is a {@link TypeCastNode}, and {@code node} is
-   *     an operand of the successor node; {@code false} otherwise
-   */
-  private boolean inCast(Node node) {
-    if (!(node.getBlock() instanceof SingleSuccessorBlock)) {
-      return false;
-    }
-    Block successorBlock = ((SingleSuccessorBlock) node.getBlock()).getSuccessor();
-    if (successorBlock != null) {
-      List<Node> succNodes = successorBlock.getNodes();
-      if (succNodes.size() > 0) {
-        Node succNode = succNodes.get(0);
-        if (succNode instanceof TypeCastNode) {
-          return ((TypeCastNode) succNode).getOperand().equals(node);
-        }
-      }
-    }
-    return false;
-  }
-
   /**
    * Transfer ownership of any locals passed as arguments to {@code @Owning} parameters at a method
    * or constructor call by removing the Obligations corresponding to those locals.
@@ -2073,17 +2041,6 @@ private void propagateObligationsToSuccessorBlock(
           }
         }
 
-        // Always propagate the Obligation to the successor if current block represents
-        // code nested in a cast.  Without this logic, the analysis may report a false
-        // positive when the Obligation represents a temporary variable for a nested
-        // expression, as the temporary may not appear in the successor store and hence
-        // seems to be going out of scope.  The temporary will be handled with special
-        // logic; casts are unwrapped at various points in the analysis.
-        if (currentBlockNodes.size() == 1 && inCast(currentBlockNodes.get(0))) {
-          successorObligations.add(obligation);
-          continue;
-        }
-
         // At this point, a consistency check will definitely occur, unless the
         // obligation was derived from a MustCallAlias parameter. If it was, an error is
         // immediately issued, because such a parameter should not go out of scope

checker/tests/resourceleak/CastBeforeFinallyBlock.java

@@ -0,0 +1,24 @@
+import java.io.Closeable;
+
+public abstract class CastBeforeFinallyBlock {
+
+  protected abstract Closeable alloc() throws Exception;
+
+  protected abstract Object getInt() throws Exception;
+
+  public void f() throws Exception {
+    // Previous versions of the resource leak checker reported a false positive
+    // for this code ("close may not have been invoked on y").  However, this
+    // code is correct.
+
+    Integer x = null;
+    try {
+      try (Closeable y = alloc()) {
+        System.out.println(y);
+      }
+      x = (Integer) getInt();
+    } finally {
+      System.out.println(x);
+    }
+  }
+}

checker/tests/resourceleak/SafeCast.java

@@ -0,0 +1,13 @@
+// simple test that the Resource Leak Checker can track data flow through a downcast
+import java.io.Closeable;
+import java.io.InputStream;
+
+public abstract class SafeCast {
+
+  protected abstract Closeable alloc() throws Exception;
+
+  public void f() throws Exception {
+    InputStream s = (InputStream) alloc();
+    s.close();
+  }
+}