From be9c329b4de91253bce21610cd995c7f4b41d25e Mon Sep 17 00:00:00 2001 From: Suzanne Millstein Date: Tue, 8 Sep 2026 10:44:25 -0700 Subject: [PATCH 1/4] Check a constructor's purity against its class's instance initializers Previously, `checkPurityAnnotations` scanned only `MethodTree.getBody()`, so a constructor's `@SideEffectFree`/`@Deterministic`/`@Pure` annotation was never checked against the instance initializer blocks and instance field initializers that the compiler runs as part of the constructor. This was accepted: class C { int x = sideEffectingMethod(); @SideEffectFree C() {} } The initializers are not attributed to a constructor that delegates via `this(...)`, since they run as part of the constructor it delegates to, which is checked at the delegating call like any other method call. Static initializers, including enum constants, are not attributed to any constructor. `PurityChecker.checkPurity()` gains an overload that checks several `TreePath`s against one `PurityResult`, for code that runs as a unit but is not contiguous. `PurityChecker.assignmentCheck` now decides whether it is in a constructor by walking up only as far as the enclosing class, rather than using `TreePathUtil.inConstructor`. Otherwise an initializer of a local or anonymous class would consult the method enclosing the class declaration, and assigning a field of the class in its own initializer would be reported as a side effect. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01A3mGe1a9p7MJ1jYyKLraQU --- .../dataflow/util/PurityChecker.java | 70 +++++++++- docs/CHANGELOG.md | 25 ++++ .../common/basetype/BaseTypeVisitor.java | 47 ++++++- framework/tests/flow/PurityInitializers.java | 129 ++++++++++++++++++ 4 files changed, 265 insertions(+), 6 deletions(-) create mode 100644 framework/tests/flow/PurityInitializers.java diff --git a/dataflow/src/main/java/org/checkerframework/dataflow/util/PurityChecker.java b/dataflow/src/main/java/org/checkerframework/dataflow/util/PurityChecker.java index 4831a9533c35..3a9196608d59 100644 --- a/dataflow/src/main/java/org/checkerframework/dataflow/util/PurityChecker.java +++ b/dataflow/src/main/java/org/checkerframework/dataflow/util/PurityChecker.java @@ -8,6 +8,7 @@ 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; @@ -15,6 +16,7 @@ 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. + * + *

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 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. + * + *

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. * diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 33434366b632..0074f137b285 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -7,8 +7,33 @@ ### User-visible changes +Under `-AcheckPurityAnnotations`, a constructor's purity annotation is now +checked against the instance initializers that run as part of the constructor: +the instance initializer blocks of its class and the initializers of its +instance fields. Previously only the constructor's body was checked, so the +following was accepted: + +```java +class C { + int x = sideEffectingMethod(); // now an error + @SideEffectFree C() {} +} +``` + +The initializers are not checked against a constructor that delegates to another +constructor of the same class via `this(...)`, because they do not run as part of +it. Static initializers are not checked against any constructor. + +Relatedly, an initializer of a local or anonymous class may now assign to a field +of that class without being reported as a side effect, as an initializer of any +other class already could. + ### Changes for type system implementers +`PurityChecker.checkPurity()` has a new overload that takes a list of +`TreePath`s and checks their purity together, for code that runs as a unit but is +not contiguous in the source code. + Renamed `AnnotatedTypes.innerMostType()` to `innermostComponentType()`. ### Closed issues diff --git a/framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java b/framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java index bdb7c5387bba..43d532362c8b 100644 --- a/framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java +++ b/framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java @@ -1185,9 +1185,14 @@ protected void checkPurityAnnotations(MethodTree tree) { if (body == null) { r = new PurityResult(); } else { + List toCheck = new ArrayList<>(2); + toCheck.add(body); + if (TreeUtils.isConstructor(tree)) { + toCheck.addAll(instanceInitializerPaths(tree, body)); + } r = PurityChecker.checkPurity( - body, atypeFactory, assumeSideEffectFree, assumeDeterministic, assumePureGetters); + toCheck, atypeFactory, assumeSideEffectFree, assumeDeterministic, assumePureGetters); } if (!r.isPure(purityKinds)) { reportPurityErrors(r, tree, purityKinds); @@ -1237,6 +1242,46 @@ protected void checkPurityAnnotations(MethodTree tree) { } } + /** + * Returns paths to the instance initializers that run as part of the given constructor: the + * instance initializer blocks of the constructor's class, and the initializers of its instance + * fields. The compiler runs these as part of the constructor, so the constructor's purity + * annotation applies to them, but they do not appear in the constructor's body. + * + *

Returns the empty list if the constructor delegates to another constructor of the same class + * via {@code this(...)}: then the initializers run as part of that constructor instead, and the + * delegating constructor is checked at its call to it, like any other method call. + * + * @param tree a constructor + * @param bodyPath the path to the constructor's body + * @return paths to the initializers that run as part of the given constructor + */ + private List instanceInitializerPaths(MethodTree tree, TreePath bodyPath) { + MethodInvocationTree explicitCall = TreeUtils.getExplicitConstructorCall(tree); + if (explicitCall != null && TreeUtils.isThisConstructorCall(explicitCall)) { + return Collections.emptyList(); + } + TreePath classPath = TreePathUtil.pathTillClass(bodyPath); + if (classPath == null) { + return Collections.emptyList(); + } + ClassTree classTree = (ClassTree) classPath.getLeaf(); + List result = new ArrayList<>(1); + for (Tree member : classTree.getMembers()) { + if (member instanceof BlockTree block) { + if (!block.isStatic()) { + result.add(new TreePath(classPath, block)); + } + } else if (member instanceof VariableTree variable) { + ExpressionTree initializer = variable.getInitializer(); + if (initializer != null && !variable.getModifiers().getFlags().contains(Modifier.STATIC)) { + result.add(new TreePath(new TreePath(classPath, variable), initializer)); + } + } + } + return result; + } + /** * Returns a diagnostic message for an annotation expression that cannot be parsed, describing * where the expression appears in addition to why it cannot be parsed. diff --git a/framework/tests/flow/PurityInitializers.java b/framework/tests/flow/PurityInitializers.java new file mode 100644 index 000000000000..1377d9532baa --- /dev/null +++ b/framework/tests/flow/PurityInitializers.java @@ -0,0 +1,129 @@ +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() {} + } + + // 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(); + } +} From e07272b7ac3860cac8c7f8d20fca99eb53d1004d Mon Sep 17 00:00:00 2001 From: Suzanne Millstein Date: Tue, 8 Sep 2026 12:36:28 -0700 Subject: [PATCH 2/4] Report each purity error only once `reportPurityError` used `reportError`, so the same error could be issued more than once at the same tree: an initializer is checked as part of every constructor that runs it, and every checker of a compound checker checks purity independently. A class with two `@SideEffectFree` constructors and an impure field initializer produced two identical errors, and the Nullness Checker produced two of every purity error. Use `reportOnce` instead. The per-directory test framework does not distinguish duplicate diagnostics, so `PurityInitializers.TwoConstructors` documents the intended behavior but cannot detect a regression; check that case with `javac` directly. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01A3mGe1a9p7MJ1jYyKLraQU --- .../common/basetype/BaseTypeVisitor.java | 16 +++++++++++++--- framework/tests/flow/PurityInitializers.java | 13 +++++++++++++ 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java b/framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java index 43d532362c8b..ea6538300e16 100644 --- a/framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java +++ b/framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java @@ -1493,15 +1493,25 @@ private void reportPurityError(String msgKeyPrefix, IPair 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)); } } diff --git a/framework/tests/flow/PurityInitializers.java b/framework/tests/flow/PurityInitializers.java index 1377d9532baa..2ed5f10a2b9e 100644 --- a/framework/tests/flow/PurityInitializers.java +++ b/framework/tests/flow/PurityInitializers.java @@ -37,6 +37,19 @@ static class InitializerBlock { 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 { From a31879d9e094fd37cc4e79ba60e12cb16e8fa5b6 Mon Sep 17 00:00:00 2001 From: Suzanne Millstein Date: Tue, 8 Sep 2026 12:41:16 -0700 Subject: [PATCH 3/4] Use `TreePathUtil.getInstanceInitializers` Commit b083d2603f added that method, along with `TreeUtils.getExplicitConstructorCall`, for exactly this purpose, but it had no callers. Use it instead of a private copy in `BaseTypeVisitor`. Besides avoiding two implementations of one rule, it guards against an interface, whose fields are implicitly static and whose tree modifiers therefore need not contain `static`. This is behavior-preserving -- interfaces have no constructors -- so it adds no test; the existing `PurityInitializers` tests cover the collection of initializers. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01A3mGe1a9p7MJ1jYyKLraQU --- .../common/basetype/BaseTypeVisitor.java | 48 +++---------------- 1 file changed, 7 insertions(+), 41 deletions(-) diff --git a/framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java b/framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java index ea6538300e16..8cb4f20b670e 100644 --- a/framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java +++ b/framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java @@ -1188,7 +1188,13 @@ protected void checkPurityAnnotations(MethodTree tree) { List toCheck = new ArrayList<>(2); toCheck.add(body); if (TreeUtils.isConstructor(tree)) { - toCheck.addAll(instanceInitializerPaths(tree, body)); + 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( @@ -1242,46 +1248,6 @@ protected void checkPurityAnnotations(MethodTree tree) { } } - /** - * Returns paths to the instance initializers that run as part of the given constructor: the - * instance initializer blocks of the constructor's class, and the initializers of its instance - * fields. The compiler runs these as part of the constructor, so the constructor's purity - * annotation applies to them, but they do not appear in the constructor's body. - * - *

Returns the empty list if the constructor delegates to another constructor of the same class - * via {@code this(...)}: then the initializers run as part of that constructor instead, and the - * delegating constructor is checked at its call to it, like any other method call. - * - * @param tree a constructor - * @param bodyPath the path to the constructor's body - * @return paths to the initializers that run as part of the given constructor - */ - private List instanceInitializerPaths(MethodTree tree, TreePath bodyPath) { - MethodInvocationTree explicitCall = TreeUtils.getExplicitConstructorCall(tree); - if (explicitCall != null && TreeUtils.isThisConstructorCall(explicitCall)) { - return Collections.emptyList(); - } - TreePath classPath = TreePathUtil.pathTillClass(bodyPath); - if (classPath == null) { - return Collections.emptyList(); - } - ClassTree classTree = (ClassTree) classPath.getLeaf(); - List result = new ArrayList<>(1); - for (Tree member : classTree.getMembers()) { - if (member instanceof BlockTree block) { - if (!block.isStatic()) { - result.add(new TreePath(classPath, block)); - } - } else if (member instanceof VariableTree variable) { - ExpressionTree initializer = variable.getInitializer(); - if (initializer != null && !variable.getModifiers().getFlags().contains(Modifier.STATIC)) { - result.add(new TreePath(new TreePath(classPath, variable), initializer)); - } - } - } - return result; - } - /** * Returns a diagnostic message for an annotation expression that cannot be parsed, describing * where the expression appears in addition to why it cannot be parsed. From b041795849cf1c08e853025f4c1042cdd5586ce0 Mon Sep 17 00:00:00 2001 From: Suzanne Millstein Date: Tue, 8 Sep 2026 14:20:54 -0700 Subject: [PATCH 4/4] Tweak changelog. --- docs/CHANGELOG.md | 25 ++----------------------- 1 file changed, 2 insertions(+), 23 deletions(-) diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 0074f137b285..3f36f9fd49bf 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -8,32 +8,11 @@ ### User-visible changes Under `-AcheckPurityAnnotations`, a constructor's purity annotation is now -checked against the instance initializers that run as part of the constructor: -the instance initializer blocks of its class and the initializers of its -instance fields. Previously only the constructor's body was checked, so the -following was accepted: - -```java -class C { - int x = sideEffectingMethod(); // now an error - @SideEffectFree C() {} -} -``` - -The initializers are not checked against a constructor that delegates to another -constructor of the same class via `this(...)`, because they do not run as part of -it. Static initializers are not checked against any constructor. - -Relatedly, an initializer of a local or anonymous class may now assign to a field -of that class without being reported as a side effect, as an initializer of any -other class already could. +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 -`PurityChecker.checkPurity()` has a new overload that takes a list of -`TreePath`s and checks their purity together, for code that runs as a unit but is -not contiguous in the source code. - Renamed `AnnotatedTypes.innerMostType()` to `innermostComponentType()`. ### Closed issues