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..3f36f9fd49bf 100644 --- a/docs/CHANGELOG.md +++ b/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()`. 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..8cb4f20b670e 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,20 @@ protected void checkPurityAnnotations(MethodTree tree) { if (body == null) { r = new PurityResult(); } else { + List 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 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 new file mode 100644 index 000000000000..2ed5f10a2b9e --- /dev/null +++ b/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(); + } +}