Suppresses RLC non-final field overwrite warning for safe constructor field initialization

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

@@ -4,12 +4,21 @@
 import com.google.common.collect.FluentIterable;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
+import com.sun.source.tree.AssignmentTree;
+import com.sun.source.tree.BlockTree;
+import com.sun.source.tree.ClassTree;
+import com.sun.source.tree.ExpressionStatementTree;
+import com.sun.source.tree.ExpressionTree;
+import com.sun.source.tree.IdentifierTree;
+import com.sun.source.tree.MemberSelectTree;
 import com.sun.source.tree.MethodInvocationTree;
 import com.sun.source.tree.MethodTree;
 import com.sun.source.tree.NewClassTree;
+import com.sun.source.tree.StatementTree;
 import com.sun.source.tree.Tree;
 import com.sun.source.tree.VariableTree;
 import com.sun.source.util.TreePath;
+import com.sun.source.util.TreeScanner;
 import java.util.ArrayDeque;
 import java.util.ArrayList;
 import java.util.Collection;
@@ -27,16 +36,20 @@
 import java.util.Objects;
 import java.util.Set;
 import java.util.StringJoiner;
+import java.util.concurrent.atomic.AtomicBoolean;
 import javax.lang.model.SourceVersion;
 import javax.lang.model.element.AnnotationMirror;
 import javax.lang.model.element.Element;
 import javax.lang.model.element.ElementKind;
 import javax.lang.model.element.ExecutableElement;
+import javax.lang.model.element.Modifier;
+import javax.lang.model.element.Name;
 import javax.lang.model.element.TypeElement;
 import javax.lang.model.element.VariableElement;
 import javax.lang.model.type.TypeKind;
 import javax.lang.model.type.TypeMirror;
 import org.checkerframework.checker.calledmethods.qual.CalledMethods;
+import org.checkerframework.checker.interning.qual.FindDistinct;
 import org.checkerframework.checker.mustcall.CreatesMustCallForToJavaExpression;
 import org.checkerframework.checker.mustcall.MustCallAnnotatedTypeFactory;
 import org.checkerframework.checker.mustcall.MustCallChecker;
@@ -1506,6 +1519,21 @@ private void checkReassignmentToField(Set<Obligation> obligations, AssignmentNod
           return;
         }
       }
+    } else if (TreeUtils.isConstructor(enclosingMethodTree)) {
+      // Suppress when this assignment is the first write to the private field in this constructor
+      // (later writes/calls are checked at their own CFG points).
+      Element enclosingClassElement =
+          TreeUtils.elementFromDeclaration(enclosingMethodTree).getEnclosingElement();
+      if (ElementUtils.isTypeElement(enclosingClassElement)) {
+        Element receiverElement = TypesUtils.getTypeElement(receiver.getType());
+        if (Objects.equals(enclosingClassElement, receiverElement)) {
+          VariableElement lhsElement = lhs.getElement();
+          if (lhsElement.getModifiers().contains(Modifier.PRIVATE)
+              && isFirstAssignmentToField(lhsElement, enclosingMethodTree, node.getTree())) {
+            return;
+          }
+        }
+      }
     }
 
     // Check that there is a corresponding CreatesMustCallFor annotation, unless this is
@@ -1625,6 +1653,146 @@ && varTrackedInObligations(obligations, (LocalVariableNode) receiver))
     }
   }
 
+  /**
+   * Determines whether the given assignment is the first write to a private field during object
+   * construction, in order to suppress potential false positive resource leak warnings.
+   *
+   * <p>This method takes a conservative approach: it returns {@code true} only if it can
+   * definitively prove that this is the first assignment to the field during construction. Later
+   * assignments, if any, are analyzed independently and will raise errors if they overwrite
+   * unclosed resources. Therefore, we only check statements before this assignment.
+   *
+   * <p>The result is {@code true} if all the following hold:
+   *
+   * <ul>
+   *   <li>(1) The field is private.
+   *   <li>(2) It has no non-null inline initializer at its declaration.
+   *   <li>(3) It is not assigned in any instance initializer block.
+   *   <li>(4) The constructor does not use constructor chaining via {@code this(...)}.
+   *   <li>(5) There are no earlier assignments to the same field before this one in the
+   *       constructor.
+   *   <li>(6) There are no method calls before this assignment that might have already set the
+   *       field.
+   * </ul>
+   *
+   * @param field the field being assigned
+   * @param constructor the constructor where the assignment appears
+   * @param currentAssignment the actual assignment tree being analyzed, which is a statement in
+   *     {@code constructor}
+   * @return true if this assignment can be safely considered the first write during construction
+   */
+  private boolean isFirstAssignmentToField(
+      VariableElement field, MethodTree constructor, @FindDistinct Tree currentAssignment) {
+    TreePath constructorPath = cmAtf.getPath(constructor);
+    ClassTree classTree = TreePathUtil.enclosingClass(constructorPath);
+    String fieldName = field.getSimpleName().toString();
+
+    // (2) Disallow non-null inline initializer
+    for (Tree member : classTree.getMembers()) {
+      if (member instanceof VariableTree) {
+        VariableTree decl = (VariableTree) member;
+        if (decl.getName().contentEquals(fieldName) && decl.getInitializer() != null) {
+          if (decl.getInitializer().getKind() != Tree.Kind.NULL_LITERAL) {
+            return false;
+          }
+        }
+      }
+    }
+
+    // (3) Disallow assignment in instance initializer blocks
+    for (Tree member : classTree.getMembers()) {
+      if (member instanceof BlockTree) {
+        BlockTree block = (BlockTree) member;
+        if (block.isStatic()) {
+          continue;
+        }
+        // The variables accessed from within the inner class need to be effectively final, so
+        // AtomicBoolean is used here.
+        AtomicBoolean found = new AtomicBoolean(false);
+        block.accept(
+            new TreeScanner<Void, Void>() {
+              @Override
+              public Void visitAssignment(AssignmentTree node, Void unused) {
+                ExpressionTree lhs = node.getVariable();
+                Name lhsName =
+                    lhs instanceof MemberSelectTree
+                        ? ((MemberSelectTree) lhs).getIdentifier()
+                        : lhs instanceof IdentifierTree ? ((IdentifierTree) lhs).getName() : null;
+                if (lhsName != null && lhsName.contentEquals(fieldName)) {
+                  found.set(true);
+                }
+                return super.visitAssignment(node, unused);
+              }
+            },
+            null);
+        if (found.get()) {
+          return false;
+        }
+      }
+    }
+
+    // (4) Check constructor initialization chaining
+    if (callsThisConstructor(constructor)) {
+      return false;
+    }
+
+    // (5) Check for earlier assignments to the same field, or
+    // (6) any method calls other than super.
+    List<? extends StatementTree> statements = constructor.getBody().getStatements();
+    for (StatementTree stmt : statements) {
+      if (!(stmt instanceof ExpressionStatementTree)) {
+        continue;
+      }
+      ExpressionTree expr = ((ExpressionStatementTree) stmt).getExpression();
+      // Stop when we reach the current assignment
+      if (expr == currentAssignment) {
+        break;
+      }
+      // (5) Prior assignment to the same field
+      if (expr instanceof AssignmentTree) {
+        ExpressionTree lhs = ((AssignmentTree) expr).getVariable();
+        Name lhsName = null;
+        if (lhs instanceof MemberSelectTree) {
+          lhsName = ((MemberSelectTree) lhs).getIdentifier();
+        } else if (lhs instanceof IdentifierTree) {
+          lhsName = ((IdentifierTree) lhs).getName();
+        }
+        if (lhsName != null && lhsName.contentEquals(fieldName)) {
+          return false; // Unsafe: field already assigned earlier
+        }
+      }
+      // (6) Any method call before assignment (except super constructor)
+      if (expr instanceof MethodInvocationTree
+          && !TreeUtils.isSuperConstructorCall((MethodInvocationTree) expr)) {
+        return false; // Unsafe: method may write to field internally
+      }
+    }
+    return true;
+  }
+
+  /**
+   * Returns true if the given constructor delegates to another constructor in the same class using
+   * a {@code this(...)} call as its first statement.
+   *
+   * @param constructor a constructor method
+   * @return {@code true} if the constructor starts with a {@code this(...)} call, {@code false}
+   *     otherwise
+   */
+  private boolean callsThisConstructor(MethodTree constructor) {
+    List<? extends StatementTree> statements = constructor.getBody().getStatements();
+    if (statements.isEmpty()) {
+      return false;
+    }
+    StatementTree firstStmt = statements.get(0);
+    if (firstStmt instanceof ExpressionStatementTree) {
+      ExpressionTree expr = ((ExpressionStatementTree) firstStmt).getExpression();
+      if (expr instanceof MethodInvocationTree) {
+        return TreeUtils.isThisConstructorCall((MethodInvocationTree) expr);
+      }
+    }
+    return false;
+  }
+
   /**
    * Checks that the method that encloses an assignment is marked with @CreatesMustCallFor
    * annotation whose target is the object whose field is being re-assigned.

checker/src/test/java/org/checkerframework/checker/test/junit/ResourceLeakFirstInitConstructorTest.java

@@ -0,0 +1,32 @@
+package org.checkerframework.checker.test.junit;
+
+import java.io.File;
+import java.util.List;
+import org.checkerframework.checker.resourceleak.ResourceLeakChecker;
+import org.checkerframework.framework.test.CheckerFrameworkPerDirectoryTest;
+import org.junit.runners.Parameterized.Parameters;
+
+/**
+ * Tests for validating safe suppression of resource leak warnings when a private field is
+ * initialized for the first time inside a constructor.
+ *
+ * <p>These tests check that the checker allows first-time constructor-based assignments (when safe)
+ * and continues to report reassignments or leaks in all other cases (e.g., after method calls,
+ * initializer blocks, etc.).
+ */
+public class ResourceLeakFirstInitConstructorTest extends CheckerFrameworkPerDirectoryTest {
+  public ResourceLeakFirstInitConstructorTest(List<File> testFiles) {
+    super(
+        testFiles,
+        ResourceLeakChecker.class,
+        "resourceleak-firstinitconstructor",
+        "-AwarnUnneededSuppressions",
+        "-encoding",
+        "UTF-8");
+  }
+
+  @Parameters
+  public static String[] getTestDirs() {
+    return new String[] {"resourceleak-firstinitconstructor"};
+  }
+}

checker/tests/resourceleak-firstinitconstructor/ConstructorChainingLeak.java

@@ -0,0 +1,38 @@
+// Test: Field is initialized in one constructor and reassigned in another via this() chaining.
+// Expected: Warning in constructor and open() due to reassignments.
+
+import java.io.FileInputStream;
+import org.checkerframework.checker.calledmethods.qual.*;
+import org.checkerframework.checker.mustcall.qual.*;
+
+@InheritableMustCall({"close"})
+class ConstructorChainingLeak {
+  private @Owning FileInputStream s;
+
+  public ConstructorChainingLeak() throws Exception {
+    this(42);
+    // :: error: (required.method.not.called)
+    s = new FileInputStream("test.txt");
+  }
+
+  private ConstructorChainingLeak(int x) throws Exception {
+    s = new FileInputStream("test.txt");
+  }
+
+  // :: error: (missing.creates.mustcall.for)
+  public void open() {
+    try {
+      // :: error: (required.method.not.called)
+      s = new FileInputStream("test.txt");
+    } catch (Exception e) {
+    }
+  }
+
+  @EnsuresCalledMethods(value = "this.s", methods = "close")
+  public void close() {
+    try {
+      s.close();
+    } catch (Exception e) {
+    }
+  }
+}

checker/tests/resourceleak-firstinitconstructor/ExplicitNullInitializer.java

@@ -0,0 +1,35 @@
+// Test: Field is explicitly initialized to null and assigned in constructor.
+// Expected: No warning in constructor, warning in open().
+
+import java.io.FileInputStream;
+import org.checkerframework.checker.calledmethods.qual.*;
+import org.checkerframework.checker.mustcall.qual.*;
+
+@InheritableMustCall({"close"})
+class ExplicitNullInitializer {
+  private @Owning FileInputStream s = null;
+
+  public ExplicitNullInitializer() {
+    try {
+      s = new FileInputStream("test.txt");
+    } catch (Exception e) {
+    }
+  }
+
+  // :: error: (missing.creates.mustcall.for)
+  public void open() {
+    try {
+      // :: error: (required.method.not.called)
+      s = new FileInputStream("test.txt");
+    } catch (Exception e) {
+    }
+  }
+
+  @EnsuresCalledMethods(value = "this.s", methods = "close")
+  public void close() {
+    try {
+      s.close();
+    } catch (Exception e) {
+    }
+  }
+}

checker/tests/resourceleak-firstinitconstructor/FirstAssignmentInConstructor.java

@@ -0,0 +1,35 @@
+// Test: Field has no initializer and is first assigned in constructor.
+// Expected: No warning in constructor, warning in open().
+
+import java.io.FileInputStream;
+import org.checkerframework.checker.calledmethods.qual.*;
+import org.checkerframework.checker.mustcall.qual.*;
+
+@InheritableMustCall({"close"})
+class FirstAssignmentInConstructor {
+  private @Owning FileInputStream s;
+
+  public FirstAssignmentInConstructor() {
+    try {
+      s = new FileInputStream("test.txt"); // no warning
+    } catch (Exception e) {
+    }
+  }
+
+  // :: error: (missing.creates.mustcall.for)
+  public void open() {
+    try {
+      // :: error: (required.method.not.called)
+      s = new FileInputStream("test.txt");
+    } catch (Exception e) {
+    }
+  }
+
+  @EnsuresCalledMethods(value = "this.s", methods = "close")
+  public void close() {
+    try {
+      s.close();
+    } catch (Exception e) {
+    }
+  }
+}