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,19 @@
 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.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 +34,19 @@
 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.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;
@@ -1508,6 +1518,21 @@ private void checkReassignmentToField(Set<Obligation> obligations, AssignmentNod
           return;
         }
       }
+    } else if (TreeUtils.isConstructor(enclosingMethodTree)) {
+      // If this assignment the first write to the private field in this constructor,
+      // then do not throw non-final owning field reassignment error.
+      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 (isFirstWriteToFieldInConstructor(node.getTree(), lhsElement, enclosingMethodTree)) {
+            // Safe; first assignment in constructor.
+            return;
+          }
+        }
+      }
     }
 
     // Check that there is a corresponding CreatesMustCallFor annotation, unless this is
@@ -1627,6 +1652,158 @@ && varTrackedInObligations(obligations, (LocalVariableNode) receiver))
     }
   }
 
+  /**
+   * Returns true if the given assignment is the first write to a {@code private} field on its path
+   * in in the constructor. This method is conservative: it returns {@code false} unless it can
+   * prove that the write is the first.
+   *
+   * <p>The result is {@code true} only if all of the following hold:
+   *
+   * <ul>
+   *   <li>(1) The field is {@code private}.
+   *   <li>(2) The field has no non-null inline initializer at its declaration.
+   *   <li>(3) The field is not assigned in any instance initializer block.
+   *   <li>(4) The constructor does not delegate via {@code this(...)}.
+   *   <li>(5) No earlier assignment to the same field appears before the current statement.
+   *   <li>(6) No earlier method call appears before the current statement (except {@code
+   *       super(...)}).
+   * </ul>
+   *
+   * @param assignment the actual assignment tree being analyzed, which is a statement in
+   * @param field the field being assigned
+   * @param constructor the constructor where the assignment appears {@code constructor}
+   * @return true if this assignment is the first write during construction
+   */
+  private boolean isFirstWriteToFieldInConstructor(
+      @FindDistinct Tree assignment, VariableElement field, MethodTree constructor) {
+    // (1) The field must be private
+    if (!field.getModifiers().contains(Modifier.PRIVATE)) {
+      return false;
+    }
+
+    TreePath constructorPath = cmAtf.getPath(constructor);
+    ClassTree classTree = TreePathUtil.enclosingClass(constructorPath);
+
+    for (Tree member : classTree.getMembers()) {
+      // (2) Disallow non-null inline initializer on the same field declaration.
+      if (member instanceof VariableTree) {
+        VariableTree decl = (VariableTree) member;
+        VariableElement declElement = TreeUtils.elementFromDeclaration(decl);
+        if (field.equals(declElement)
+            && decl.getInitializer() != null
+            && decl.getInitializer().getKind() != Tree.Kind.NULL_LITERAL) {
+          return false;
+        }
+        continue;
+      }
+      // (3) Disallow assignment in any instance initializer block.
+      if (member instanceof BlockTree) {
+        BlockTree initBlock = (BlockTree) member;
+        if (initBlock.isStatic()) {
+          continue;
+        }
+        // The variables accessed from within the anonymous class need to be effectively final, so
+        // AtomicBoolean is used here.
+        AtomicBoolean isInitialized = new AtomicBoolean(false);
+        initBlock.accept(
+            new TreeScanner<Void, Void>() {
+              @Override
+              public Void visitAssignment(AssignmentTree assignmentTree, Void unused) {
+                ExpressionTree lhs = assignmentTree.getVariable();
+                Element lhsElement = TreeUtils.elementFromTree(lhs);
+                if (field.equals(lhsElement)) {
+                  isInitialized.set(true);
+                  return null;
+                }
+                return super.visitAssignment(assignmentTree, unused);
+              }
+            },
+            null);
+        if (isInitialized.get()) {
+          return false;
+        }
+      }
+    }
+
+    // (4) Reject constructor chaining via `this(...)`.
+    // If this constructor chains, the "first write" can occur in the callee constructor.
+    if (callsThisConstructor(constructor)) {
+      return false;
+    }
+
+    // (5) & (6): Single-pass scan in source order.
+    // For each top-level statement, descend into its subtree and:
+    //   - If we encounter an assignment to the same field:
+    //       * if it is the current assignment -> first write -> return true
+    //       * otherwise (earlier assignment) -> not first write -> return false
+    //   - If we encounter any method call before seeing the current assignment
+    //     (other than a super(...) ctor call) -> return false
+    List<? extends StatementTree> stmts = constructor.getBody().getStatements();
+    for (StatementTree st : stmts) {
+      Boolean scanResult =
+          new TreeScanner<Boolean, Void>() {
+            @Override
+            public Boolean visitAssignment(AssignmentTree node, Void p) {
+              Element lhsEl = TreeUtils.elementFromUse(node.getVariable());
+              if (field.equals(lhsEl)) {
+                return (node == assignment)
+                    ? Boolean.TRUE
+                    : Boolean.FALSE; // (5) Earlier assignment to same field
+              }
+              return super.visitAssignment(node, p);
+            }
+
+            @Override
+            public Boolean visitMethodInvocation(MethodInvocationTree node, Void p) {
+              // (6) Any earlier call might assign internally; allow only super(...).
+              return TreeUtils.isSuperConstructorCall(node)
+                  ? super.visitMethodInvocation(node, p)
+                  : Boolean.FALSE;
+            }
+
+            @Override
+            public Boolean reduce(Boolean r1, Boolean r2) {
+              // Return the first non-null result from child scans.
+              // This lets the traversal stop as soon as a matching condition is detected.
+              if (r1 != null) {
+                return r1;
+              }
+              return r2;
+            }
+          }.scan(st, null);
+
+      if (scanResult != null) {
+        return scanResult;
+      }
+    }
+    // The current assignment was not found in the constructor body, conservatively return false.
+    return false;
+  }
+
+  /**
+   * 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
+   */
+  private boolean callsThisConstructor(MethodTree constructor) {
+    List<? extends StatementTree> statements = constructor.getBody().getStatements();
+    if (statements.isEmpty()) {
+      return false;
+    }
+    // This code must be revisited when "JEP 482: Flexible Constructor Bodies" is finalized,
+    // because then a call to `this` need not be the very first statement in a constructor body.
+    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/FinalField.java

@@ -0,0 +1,27 @@
+import java.io.FileInputStream;
+import org.checkerframework.checker.calledmethods.qual.*;
+import org.checkerframework.checker.mustcall.qual.*;
+
+@InheritableMustCall({"close"})
+class FinalField {
+  private int i;
+
+  private final @Owning FileInputStream s;
+
+  public FinalField() throws Exception {
+    havoc();
+    s = new FileInputStream("test.txt");
+  }
+
+  void havoc() {
+    i++;
+  }
+
+  @EnsuresCalledMethods(value = "this.s", methods = "close")
+  public void close() {
+    try {
+      s.close();
+    } catch (Exception e) {
+    }
+  }
+}