Check a constructor's purity against its class's instance initializers

dataflow/src/main/java/org/checkerframework/dataflow/util/PurityChecker.java

@@ -8,13 +8,15 @@
 import com.sun.source.tree.ExpressionTree;
 import com.sun.source.tree.IdentifierTree;
 import com.sun.source.tree.MethodInvocationTree;
+import com.sun.source.tree.MethodTree;
 import com.sun.source.tree.NewClassTree;
 import com.sun.source.tree.ThrowTree;
 import com.sun.source.tree.Tree;
 import com.sun.source.tree.UnaryTree;
 import com.sun.source.util.TreePath;
 import com.sun.source.util.TreePathScanner;
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.EnumSet;
 import java.util.List;
 import javax.lang.model.element.ExecutableElement;
@@ -66,10 +68,43 @@ public static PurityResult checkPurity(
       boolean assumeSideEffectFree,
       boolean assumeDeterministic,
       boolean assumePureGetters) {
+    return checkPurity(
+        Collections.singletonList(statement),
+        annoProvider,
+        assumeSideEffectFree,
+        assumeDeterministic,
+        assumePureGetters);
+  }
+
+  /**
+   * Compute whether the given statements, taken together, are side-effect-free, deterministic, or
+   * both. Returns a result that can be queried.
+   *
+   * <p>Use this rather than calling {@link #checkPurity(TreePath, AnnotationProvider, boolean,
+   * boolean, boolean)} once per statement, for code that runs as a unit but is not contiguous in
+   * the source code: a constructor together with the instance initializers that run as part of it,
+   * for example.
+   *
+   * @param statements the statements to check
+   * @param annoProvider the annotation provider
+   * @param assumeSideEffectFree true if all methods should be assumed to be @SideEffectFree
+   * @param assumeDeterministic true if all methods should be assumed to be @Deterministic
+   * @param assumePureGetters true if all getter methods should be assumed to be @Pure
+   * @return information about whether the given statements are side-effect-free, deterministic, or
+   *     both
+   */
+  public static PurityResult checkPurity(
+      List<TreePath> statements,
+      AnnotationProvider annoProvider,
+      boolean assumeSideEffectFree,
+      boolean assumeDeterministic,
+      boolean assumePureGetters) {
     PurityCheckerHelper helper =
         new PurityCheckerHelper(
             annoProvider, assumeSideEffectFree, assumeDeterministic, assumePureGetters);
-    helper.scan(statement, null);
+    for (TreePath statement : statements) {
+      helper.scan(statement, null);
+    }
     return helper.purityResult;
   }
 
@@ -373,10 +408,8 @@ public Void visitUnary(UnaryTree tree, Void ignore) {
     protected void assignmentCheck(ExpressionTree variable) {
       variable = TreeUtils.withoutParens(variable);
       VariableElement fieldElt = TreeUtils.asFieldAccess(variable);
-      if (fieldElt != null
-          && isFieldInCurrentClass(fieldElt)
-          && TreePathUtil.inConstructor(getCurrentPath())) {
-        // assigning a field in a constructor
+      if (fieldElt != null && isFieldInCurrentClass(fieldElt) && inConstructorOrInitializer()) {
+        // assigning a field in a constructor or an initializer
         // TODO: add a check for ArrayAccessTree too.
         return;
       }
@@ -392,6 +425,33 @@ && isFieldInCurrentClass(fieldElt)
       }
     }
 
+    /**
+     * Returns true if the current path is in a constructor, or in an initializer of the class that
+     * immediately encloses it (an instance or static initializer block, or the initializer of a
+     * field). Such code runs while the object is being constructed, before it is visible to other
+     * code.
+     *
+     * <p>This differs from {@link TreePathUtil#inConstructor} for code in a local or anonymous
+     * class: an initializer of such a class runs when the class is instantiated, so what matters is
+     * the class member that encloses the code, not the method that encloses the class declaration.
+     *
+     * @return true if the current path is in a constructor or in an initializer
+     */
+    private boolean inConstructorOrInitializer() {
+      for (TreePath p = getCurrentPath(); p != null; p = p.getParentPath()) {
+        Tree leaf = p.getLeaf();
+        if (leaf instanceof MethodTree methodTree) {
+          return TreeUtils.isConstructor(methodTree);
+        }
+        if (leaf instanceof ClassTree) {
+          // No method intervenes between the class and the code, so the code is in an
+          // initializer of the class.
+          return true;
+        }
+      }
+      return false;
+    }
+
     /**
      * Returns true if the given field is defined by the current class.
      *

docs/CHANGELOG.md

@@ -7,6 +7,10 @@
 
 ### User-visible changes
 
+Under `-AcheckPurityAnnotations`, a constructor's purity annotation is now
+checked against its class's instance initializers, in addition to its body.
+This may cause new purity errors to be issued.
+
 ### Changes for type system implementers
 
 Renamed `AnnotatedTypes.innerMostType()` to `innermostComponentType()`.

framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java

@@ -1185,9 +1185,20 @@ protected void checkPurityAnnotations(MethodTree tree) {
     if (body == null) {
       r = new PurityResult();
     } else {
+      List<TreePath> toCheck = new ArrayList<>(2);
+      toCheck.add(body);
+      if (TreeUtils.isConstructor(tree)) {
+        MethodInvocationTree explicitCall = TreeUtils.getExplicitConstructorCall(tree);
+        if (explicitCall == null || !TreeUtils.isThisConstructorCall(explicitCall)) {
+          // The class's instance initializers run as part of this constructor.  If instead it
+          // delegates to another constructor of the same class, they run as part of that one, and
+          // this constructor is checked at its call to it, like any other method call.
+          toCheck.addAll(TreePathUtil.getInstanceInitializers(body));
+        }
+      }
       r =
           PurityChecker.checkPurity(
-              body, atypeFactory, assumeSideEffectFree, assumeDeterministic, assumePureGetters);
+              toCheck, atypeFactory, assumeSideEffectFree, assumeDeterministic, assumePureGetters);
     }
     if (!r.isPure(purityKinds)) {
       reportPurityErrors(r, tree, purityKinds);
@@ -1448,15 +1459,25 @@ private void reportPurityError(String msgKeyPrefix, IPair<Tree, String> r) {
     String reason = r.second;
     @SuppressWarnings("compilermessages")
     @CompilerMessageKey String msgKey = msgKeyPrefix + reason;
+    Object[] args;
     if (reason.equals("call")) {
       if (r.first instanceof MethodInvocationTree mitree) {
-        checker.reportError(r.first, msgKey, mitree.getMethodSelect());
+        args = new Object[] {mitree.getMethodSelect()};
       } else {
         NewClassTree nctree = (NewClassTree) r.first;
-        checker.reportError(r.first, msgKey, nctree.getIdentifier());
+        args = new Object[] {nctree.getIdentifier()};
       }
     } else {
-      checker.reportError(r.first, msgKey);
+      args = new Object[0];
+    }
+    // The same tree can be checked more than once:  an initializer is checked as part of every
+    // constructor that runs it, and every checker of a compound checker checks purity
+    // independently.  Report each message at each tree only once.
+    TreePath path = atypeFactory.getPath(r.first);
+    if (path == null) {
+      checker.reportError(r.first, msgKey, args);
+    } else {
+      checker.reportOnce(path, new DiagMessage(Diagnostic.Kind.ERROR, msgKey, args));
     }
   }
 

framework/tests/flow/PurityInitializers.java

@@ -0,0 +1,142 @@
+import org.checkerframework.dataflow.qual.SideEffectFree;
+
+// Tests that a constructor's purity is checked against the initializers that run as part of it:
+// instance initializer blocks and instance field initializers.
+public class PurityInitializers {
+
+  static int counter = 0;
+
+  static int bump() {
+    return counter++;
+  }
+
+  @SideEffectFree
+  static int pureValue() {
+    return 0;
+  }
+
+  // The effects of a field initializer are effects of the constructor.
+  static class FieldInitializer {
+    // :: error: [purity.not.sideeffectfree.call]
+    int x = bump();
+
+    @SideEffectFree
+    FieldInitializer() {}
+  }
+
+  // The effects of an instance initializer block are effects of the constructor.
+  static class InitializerBlock {
+    int x;
+
+    {
+      // :: error: [purity.not.sideeffectfree.call]
+      bump();
+    }
+
+    @SideEffectFree
+    InitializerBlock() {}
+  }
+
+  // The initializers run as part of each constructor, but each of their effects is one error, not
+  // one error per constructor.
+  static class TwoConstructors {
+    // :: error: [purity.not.sideeffectfree.call]
+    int x = bump();
+
+    @SideEffectFree
+    TwoConstructors() {}
+
+    @SideEffectFree
+    TwoConstructors(int i) {}
+  }
+
+  // A constructor may assign the fields of its own class, in an initializer as well as in the
+  // constructor's body.
+  static class AssignOwnField {
+    int x;
+    int y = 1;
+
+    {
+      x = 2;
+    }
+
+    @SideEffectFree
+    AssignOwnField() {
+      y = 3;
+    }
+  }
+
+  // Pure initializers do not make the constructor impure.
+  static class PureInitializer {
+    int x = pureValue();
+
+    {
+      x = pureValue();
+    }
+
+    @SideEffectFree
+    PureInitializer() {}
+  }
+
+  // Static initializers do not run as part of a constructor, so they are not its effects.
+  static class StaticInitializer {
+    static int x = bump();
+
+    static {
+      bump();
+    }
+
+    @SideEffectFree
+    StaticInitializer() {}
+  }
+
+  // A constructor that delegates via this(...) does not run the initializers a second time, so
+  // their effects are reported only once, for the constructor that does run them.
+  static class Delegating {
+    // :: error: [purity.not.sideeffectfree.call]
+    int x = bump();
+
+    @SideEffectFree
+    Delegating() {
+      this(0);
+    }
+
+    @SideEffectFree
+    Delegating(int i) {}
+  }
+
+  // An enum constant is a static field, so it is not an initializer of the enum's constructor.
+  enum SomeEnum {
+    A(bump()),
+    B(1);
+
+    final int x;
+
+    // The error is for the implicit call to the superclass constructor `Enum(String, int)`, which
+    // is not annotated; it is unrelated to the enum constants above.
+    @SideEffectFree
+    // :: error: [purity.not.sideeffectfree.call]
+    SomeEnum(int i) {
+      x = i;
+    }
+  }
+
+  // The same holds for a local class.  Its initializers run when it is instantiated, so what
+  // matters is the class member that encloses them, not the method that encloses the class.
+  Object localClass() {
+    class Local {
+      int x;
+
+      // :: error: [purity.not.sideeffectfree.call]
+      int y = bump();
+
+      {
+        x = 1;
+      }
+
+      @SideEffectFree
+      Local() {}
+    }
+    return new Local();
+  }
+}