Fix a false positive in the Resource Leak Checker (#7060)

Author: Calvin-L

checker/src/main/java/org/checkerframework/checker/calledmethods/CalledMethodsTransfer.java

@@ -39,18 +39,6 @@
 /** A transfer function that accumulates the names of methods called. */
 public class CalledMethodsTransfer extends AccumulationTransfer {
 
-  /**
-   * {@link #makeExceptionalStores(MethodInvocationNode, TransferInput)} requires a TransferInput,
-   * but the actual exceptional stores need to be modified in {@link #accumulate(Node,
-   * TransferResult, String...)}, which only has access to a TransferResult. So this field is set to
-   * non-null in {@link #visitMethodInvocation(MethodInvocationNode, TransferInput)} via a call to
-   * {@link #makeExceptionalStores(MethodInvocationNode, TransferInput)} (which reads the CFStores
-   * from the TransferInput) before the call to accumulate(); accumulate() can then use this field
-   * to read the CFStores; and then finally this field is then reset to null afterwards to prevent
-   * it from being used somewhere it shouldn't be.
-   */
-  private @Nullable Map<TypeMirror, AccumulationStore> exceptionalStores;
-
   /**
    * The element for the CalledMethods annotation's value element. Stored in a field in this class
    * to prevent the need to cast to CalledMethods ATF every time it's used.
@@ -118,10 +106,22 @@ protected boolean shouldPerformWholeProgramInference(Tree expressionTree, Tree l
   @Override
   public TransferResult<AccumulationValue, AccumulationStore> visitMethodInvocation(
       MethodInvocationNode node, TransferInput<AccumulationValue, AccumulationStore> input) {
-    exceptionalStores = makeExceptionalStores(node, input);
+
+    // The call to `super.visitMethodInvocation()` modifies the input store in-place.  So if we end
+    // up needing to create the exceptional stores, then we'll need this copy taken beforehand.
+    AccumulationStore inputStore = input.getRegularStore().copy();
+
     TransferResult<AccumulationValue, AccumulationStore> superResult =
         super.visitMethodInvocation(node, input);
 
+    // Ensure that the result has a store for each possible exception.  This affects the behavior of
+    // accumulate(), which will accumulate values into the result's exceptional stores as well.
+    Map<TypeMirror, AccumulationStore> exceptionalStores = superResult.getExceptionalStores();
+    if (exceptionalStores == null) {
+      exceptionalStores = makeExceptionalStores(node, inputStore);
+      superResult = superResult.withExceptionalStores(exceptionalStores);
+    }
+
     ExecutableElement method = TreeUtils.elementFromUse(node.getTree());
     handleEnsuresCalledMethodsVarargs(node, method, superResult);
     handleEnsuresCalledMethodsOnException(node, method, exceptionalStores);
@@ -134,20 +134,19 @@ public TransferResult<AccumulationValue, AccumulationStore> visitMethodInvocatio
               .adjustMethodNameUsingValueChecker(methodName, node.getTree());
       accumulate(receiver, superResult, methodName);
     }
-    TransferResult<AccumulationValue, AccumulationStore> finalResult =
-        new ConditionalTransferResult<>(
-            superResult.getResultValue(),
-            superResult.getThenStore(),
-            superResult.getElseStore(),
-            exceptionalStores);
-    exceptionalStores = null;
-    return finalResult;
+    return new ConditionalTransferResult<>(
+        superResult.getResultValue(),
+        superResult.getThenStore(),
+        superResult.getElseStore(),
+        exceptionalStores);
   }
 
   @Override
   public void accumulate(
       Node node, TransferResult<AccumulationValue, AccumulationStore> result, String... values) {
     super.accumulate(node, result, values);
+
+    Map<TypeMirror, AccumulationStore> exceptionalStores = result.getExceptionalStores();
     if (exceptionalStores == null) {
       return;
     }
@@ -186,23 +185,21 @@ public void accumulate(
    * node} was definitely called.
    *
    * @param node a method invocation
-   * @param input the transfer input associated with the method invocation
+   * @param inputStore the transfer input associated with the method invocation
    * @return a map from types to stores. The keys are the same keys used by {@link
    *     ExceptionBlock#getExceptionalSuccessors()}. The values are copies of the regular store from
    *     {@code input}.
    */
   private Map<TypeMirror, AccumulationStore> makeExceptionalStores(
-      MethodInvocationNode node, TransferInput<AccumulationValue, AccumulationStore> input) {
+      MethodInvocationNode node, AccumulationStore inputStore) {
     if (!(node.getBlock() instanceof ExceptionBlock)) {
       // This can happen in some weird (buggy?) cases:
       // see https://github.com/typetools/checker-framework/issues/3585
       return Collections.emptyMap();
     }
     ExceptionBlock block = (ExceptionBlock) node.getBlock();
     Map<TypeMirror, AccumulationStore> result = new LinkedHashMap<>();
-    block
-        .getExceptionalSuccessors()
-        .forEach((tm, b) -> result.put(tm, input.getRegularStore().copy()));
+    block.getExceptionalSuccessors().forEach((tm, b) -> result.put(tm, inputStore.copy()));
     return result;
   }
 

checker/tests/resourceleak/TemporaryVariables.java

@@ -0,0 +1,30 @@
+// For some expressions like `alloc()`, the Resource Leak Checker creates anonymous "temporary
+// variables" to track must-call obligations.  These tests exercise some interesting interactions
+// involving temporary variables.
+
+import java.io.*;
+import org.checkerframework.checker.calledmethods.qual.*;
+import org.checkerframework.checker.mustcall.qual.*;
+
+abstract class TemporaryVariables {
+  public abstract Closeable alloc();
+
+  public void test1() throws IOException {
+    alloc().close();
+  }
+
+  // Identical to test1() but with explicit exception handling.
+  public void test2() throws IOException {
+    try {
+      alloc().close();
+    } catch (IOException e) {
+      throw e;
+    }
+  }
+
+  @EnsuresCalledMethods(value = "#1", methods = "close")
+  @EnsuresCalledMethodsOnException(value = "#1", methods = "close")
+  public void close(Closeable x) throws IOException {
+    x.close();
+  }
+}

dataflow/src/main/java/org/checkerframework/dataflow/analysis/ConditionalTransferResult.java

@@ -150,4 +150,11 @@ public String toString() {
   public boolean storeChanged() {
     return storeChanged;
   }
+
+  @Override
+  public ConditionalTransferResult<V, S> withExceptionalStores(
+      Map<TypeMirror, S> exceptionalStores) {
+    return new ConditionalTransferResult<>(
+        resultValue, thenStore, elseStore, exceptionalStores, storeChanged);
+  }
 }

dataflow/src/main/java/org/checkerframework/dataflow/analysis/RegularTransferResult.java

@@ -150,4 +150,9 @@ public String toString() {
   public boolean storeChanged() {
     return storeChanged;
   }
+
+  @Override
+  public RegularTransferResult<V, S> withExceptionalStores(Map<TypeMirror, S> exceptionalStores) {
+    return new RegularTransferResult<>(resultValue, store, exceptionalStores, storeChanged);
+  }
 }

dataflow/src/main/java/org/checkerframework/dataflow/analysis/TransferResult.java

@@ -141,4 +141,14 @@ public void setResultValue(V resultValue) {
    *     changed the regularStore, elseStore, or thenStore
    */
   public abstract boolean storeChanged();
+
+  /**
+   * Construct a shallow copy of this {@code TransferResult}, but with the given {@code
+   * exceptionalStores}.
+   *
+   * @param exceptionalStores the new exceptional stores to use
+   * @return a copy of this object modified to use the given exceptional stores
+   * @see #getExceptionalStores()
+   */
+  public abstract TransferResult<V, S> withExceptionalStores(Map<TypeMirror, S> exceptionalStores);
 }