typetools/checker-framework 3.21.0 release (opprop#142)

Co-authored-by: Suzanne Millstein <[email protected]>
Co-authored-by: Michael Ernst <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Martin Kellogg <[email protected]>

build.gradle

@@ -3,7 +3,7 @@ import org.ajoberstar.grgit.Grgit
 
 plugins {
     // https://plugins.gradle.org/plugin/com.github.johnrengelman.shadow (v5 requires Gradle 5)
-    id 'com.github.johnrengelman.shadow' version '7.1.0'
+    id 'com.github.johnrengelman.shadow' version '7.1.1'
     // https://plugins.gradle.org/plugin/de.undercouch.download
     id 'de.undercouch.download' version '4.1.2'
     id 'java'
@@ -107,7 +107,7 @@ allprojects {
     // level (third number) if:
     //   * any new checkers have been added, or
     //   * backward-incompatible changes have been made to APIs or elsewhere.
-    version '3.20.0'
+    version '3.21.0'
 
     repositories {
         mavenCentral()
@@ -723,15 +723,11 @@ subprojects {
         //  * Temporarily comment out "-Werror" elsewhere in this file
         //  * Repeatedly run `./gradlew clean compileJava` and fix all errors
         //  * Uncomment "-Werror"
-        //  * Don't edit framework/build.gradle to use the same version number
-        //    (it is updated when the annotated version of Guava is updated).
-        errorprone group: 'com.google.errorprone', name: 'error_prone_core', version: '2.10.0'
+        ext.errorproneVersion = '2.10.0'
+        errorprone group: 'com.google.errorprone', name: 'error_prone_core', version: errorproneVersion
 
         // TODO: it's a bug that annotatedlib:guava requires the error_prone_annotations dependency.
-        // Update the next two version numbers in tandem.  Get the Error Prone version from the "compile
-        // dependencies" section of https://mvnrepository.com/artifact/com.google.guava/guava/30.1.1-jre .
-        // (It isn't at https://mvnrepository.com/artifact/org.checkerframework.annotatedlib/guava/30.1.1-jre, which is the bug.)
-        annotatedGuava 'com.google.errorprone:error_prone_annotations:2.10.0'
+        annotatedGuava "com.google.errorprone:error_prone_annotations:${errorproneVersion}"
         annotatedGuava ('org.checkerframework.annotatedlib:guava:30.1.1-jre') {
             // So long as Guava only uses annotations from checker-qual, excluding it should not cause problems.
             exclude group: 'org.checkerframework'

checker-qual/src/main/java/org/checkerframework/checker/mustcall/qual/NotOwning.java

@@ -7,7 +7,8 @@
 
 /**
  * Annotation indicating that ownership should not be transferred to the annotated parameter, field,
- * or (when this is written on a method) return type, for the purposes of Must Call checking.
+ * or method's call sites, for the purposes of Must Call checking. For a full description of the
+ * semantics, see the documentation of {@link Owning}.
  *
  * <p>Parameters and fields are treated as if they have this annotation by default unless they have
  * {@link Owning}.

checker-qual/src/main/java/org/checkerframework/checker/mustcall/qual/Owning.java

@@ -6,12 +6,26 @@
 import java.lang.annotation.Target;
 
 /**
- * Annotation indicating that ownership should be transferred to the annotated parameter, field, or
- * (when written on a method) return type, for the purposes of Must Call checking. Static fields
- * cannot be owning.
+ * Annotation indicating that ownership should be transferred to the annotated element for the
+ * purposes of Must Call checking. When written on a parameter, the annotation indicates that Must
+ * Call checking should be performed in the body of the method, not at call sites. When written on a
+ * method, the annotation indicates that return expressions do not need to be checked in the method
+ * body, but at call sites. When written on a field, the annotation indicates that fulfilling the
+ * must-call obligations of an instance of the class in which the field is declared also results in
+ * the annotated field's must-call obligations being satisfied. Static fields cannot be owning.
  *
- * <p>Method return types are treated as if they have this annotation by default unless their method
- * is annotated as {@link NotOwning}.
+ * <p>This annotation is a declaration annotation rather than a type annotation, because it does not
+ * logically belong to any type hierarchy. Logically, it is a directive to the Resource Leak Checker
+ * that informs it whether it is necessary to check that a value's must-call obligations have been
+ * satisfied. In that way, it can be viewed as an annotation expressing an aliasing relationship:
+ * passing a object with a non-empty must-call obligation to a method with an owning parameter
+ * resolves that object's must-call obligation, because the ownership annotation expresses that the
+ * object at the call site and the parameter in the method's body are aliases, and so checking only
+ * one of the two is required.
+ *
+ * <p>Methods are treated as if they have this annotation by default unless their method is
+ * annotated as {@link NotOwning}. Parameters and fields are treated as {@link NotOwning} by
+ * default.
  *
  * <p>When the -AnoLightweightOwnership command-line argument is passed to the checker, this
  * annotation and {@link NotOwning} are ignored.

checker/src/main/java/org/checkerframework/checker/index/upperbound/UpperBoundTransfer.java

@@ -860,7 +860,7 @@ public TransferResult<CFValue, CFStore> visitCase(
         // TODO: This cannot be done in strengthenAnnotationOfEqualTo, because that does not provide
         // transfer input.
         List<Node> caseNodes = n.getCaseOperands();
-        AssignmentNode assign = (AssignmentNode) n.getSwitchOperand();
+        AssignmentNode assign = n.getSwitchOperand();
         Node switchNode = assign.getExpression();
         for (Node caseNode : caseNodes) {
             refineSubtrahendWithOffset(switchNode, caseNode, false, in, result.getThenStore());

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

@@ -17,6 +17,7 @@
 import org.checkerframework.checker.mustcall.MustCallAnnotatedTypeFactory;
 import org.checkerframework.checker.mustcall.MustCallChecker;
 import org.checkerframework.checker.mustcall.qual.MustCall;
+import org.checkerframework.checker.mustcall.qual.MustCallAlias;
 import org.checkerframework.checker.mustcall.qual.NotOwning;
 import org.checkerframework.checker.mustcall.qual.Owning;
 import org.checkerframework.checker.nullness.qual.Nullable;
@@ -246,6 +247,22 @@ private boolean canBeSatisfiedThrough(LocalVariableNode localVariableNode) {
             return getResourceAlias(localVariableNode) != null;
         }
 
+        /**
+         * Does this Obligation contain any resource aliases that were derived from {@link
+         * MustCallAlias} parameters?
+         *
+         * @return the logical or of the {@link ResourceAlias#derivedFromMustCallAliasParam} fields
+         *     of this Obligation's resource aliases
+         */
+        public boolean derivedFromMustCallAlias() {
+            for (ResourceAlias ra : resourceAliases) {
+                if (ra.derivedFromMustCallAliasParam) {
+                    return true;
+                }
+            }
+            return false;
+        }
+
         /**
          * Gets the must-call methods (i.e. the list of methods that must be called to satisfy the
          * must-call obligation) of the resource represented by this Obligation.
@@ -384,14 +401,43 @@ public int hashCode() {
         public final Tree tree;
 
         /**
-         * Create a new resource alias.
+         * Was this ResourceAlias derived from a parameter to a method that was annotated as {@link
+         * MustCallAlias}? If so, the obligation containing this resource alias must be discharged
+         * only in one of the following ways:
+         *
+         * <ul>
+         *   <li>it is passed to another method or constructor in an @MustCallAlias position, and
+         *       then the containing method returns that method’s result, or the call is a super()
+         *       constructor call annotated with {@link MustCallAlias}, or
+         *   <li>it is stored in an owning field of the class under analysis
+         * </ul>
+         */
+        public final boolean derivedFromMustCallAliasParam;
+
+        /**
+         * Create a new resource alias. This constructor should only be used if the resource alias
+         * was not derived from a method parameter annotated as {@link MustCallAlias}.
          *
          * @param reference the local variable
          * @param tree the tree
          */
         public ResourceAlias(LocalVariable reference, Tree tree) {
+            this(reference, tree, false);
+        }
+
+        /**
+         * Create a new resource alias.
+         *
+         * @param reference the local variable
+         * @param tree the tree
+         * @param derivedFromMustCallAliasParam true iff this resource alias was created because of
+         *     an {@link MustCallAlias} parameter
+         */
+        public ResourceAlias(
+                LocalVariable reference, Tree tree, boolean derivedFromMustCallAliasParam) {
             this.reference = reference;
             this.tree = tree;
+            this.derivedFromMustCallAliasParam = derivedFromMustCallAliasParam;
         }
 
         @Override
@@ -722,9 +768,10 @@ private void updateObligationsWithInvocationResult(Set<Obligation> obligations,
                             ((MethodInvocationNode) node).getTarget().getReceiver()));
         }
 
-        ResourceAlias tmpVarAsResourceAlias = new ResourceAlias(new LocalVariable(tmpVar), tree);
         if (mustCallAliases.isEmpty()) {
             // If mustCallAliases is an empty List, add tmpVarAsResourceAlias to a new set.
+            ResourceAlias tmpVarAsResourceAlias =
+                    new ResourceAlias(new LocalVariable(tmpVar), tree);
             obligations.add(new Obligation(ImmutableSet.of(tmpVarAsResourceAlias)));
         } else {
             for (Node mustCallAlias : mustCallAliases) {
@@ -743,6 +790,12 @@ private void updateObligationsWithInvocationResult(Set<Obligation> obligations,
                     Obligation obligationContainingMustCallAlias =
                             getObligationForVar(obligations, (LocalVariableNode) mustCallAlias);
                     if (obligationContainingMustCallAlias != null) {
+                        ResourceAlias tmpVarAsResourceAlias =
+                                new ResourceAlias(
+                                        new LocalVariable(tmpVar),
+                                        tree,
+                                        obligationContainingMustCallAlias
+                                                .derivedFromMustCallAlias());
                         Set<ResourceAlias> newResourceAliasSet =
                                 FluentIterable.from(
                                                 obligationContainingMustCallAlias.resourceAliases)
@@ -929,9 +982,16 @@ private void removeObligationsAtOwnershipTransferToParameters(
                     for (AnnotationMirror anno : annotationMirrors) {
                         if (AnnotationUtils.areSameByName(
                                 anno, "org.checkerframework.checker.mustcall.qual.Owning")) {
-                            // transfer ownership!
-                            obligations.remove(getObligationForVar(obligations, local));
-                            break;
+                            Obligation localObligation = getObligationForVar(obligations, local);
+                            // Passing to an owning parameter is not sufficient to resolve the
+                            // obligation created from a MustCallAlias parameter, because the
+                            // containing
+                            // method must actually return the value.
+                            if (!localObligation.derivedFromMustCallAlias()) {
+                                // Transfer ownership!
+                                obligations.remove(localObligation);
+                                break;
+                            }
                         }
                     }
                 }
@@ -1034,10 +1094,20 @@ private void updateObligationsForAssignment(
             if (isOwningField
                     && rhs instanceof LocalVariableNode
                     && (typeFactory.canCreateObligations() || ElementUtils.isFinal(lhsElement))) {
-                removeObligationsContainingVar(obligations, (LocalVariableNode) rhs);
+                // Assigning to an owning field is sufficient to clear a must-call alias obligation
+                // in
+                // a constructor.
+                Element enclosingCtr = lhsElement.getEnclosingElement();
+                if (enclosingCtr != null && enclosingCtr.getKind() != ElementKind.CONSTRUCTOR) {
+                    removeObligationsContainingVar(obligations, (LocalVariableNode) rhs);
+                } else {
+                    removeObligationsContainingVarIfNotDerivedFromMustCallAlias(
+                            obligations, (LocalVariableNode) rhs);
+                }
             }
         } else if (lhsElement.getKind() == ElementKind.RESOURCE_VARIABLE && isMustCallClose(rhs)) {
-            removeObligationsContainingVar(obligations, (LocalVariableNode) rhs);
+            removeObligationsContainingVarIfNotDerivedFromMustCallAlias(
+                    obligations, (LocalVariableNode) rhs);
         } else if (lhs instanceof LocalVariableNode) {
             LocalVariableNode lhsVar = (LocalVariableNode) lhs;
             updateObligationsForPseudoAssignment(obligations, assignmentNode, lhsVar, rhs);
@@ -1074,6 +1144,22 @@ private void removeObligationsContainingVar(
         }
     }
 
+    /**
+     * Remove any Obligations that contain {@code var} in their resource-alias set, if those
+     * resources were not derived from an {@link MustCallAlias} parameter.
+     *
+     * @param obligations the set of Obligations
+     * @param var a variable
+     */
+    private void removeObligationsContainingVarIfNotDerivedFromMustCallAlias(
+            Set<Obligation> obligations, LocalVariableNode var) {
+        Obligation obligationForVar = getObligationForVar(obligations, var);
+        while (obligationForVar != null && !obligationForVar.derivedFromMustCallAlias()) {
+            obligations.remove(obligationForVar);
+            obligationForVar = getObligationForVar(obligations, var);
+        }
+    }
+
     /**
      * Update a set of tracked Obligations to account for a (pseudo-)assignment to some variable, as
      * in a gen-kill dataflow analysis problem. That is, add ("gen") and remove ("kill") resource
@@ -1670,7 +1756,21 @@ && inCastOrTernary(currentBlockNodes.get(0))) {
                         continue;
                     }
 
-                    // At this point, a consistency check will definitely occur.
+                    // At this point, a consistency check will definitely occur, unless the
+                    // obligation
+                    // was derived from a MustCallAlias parameter. If it was, an error is
+                    // immediately
+                    // issued, because such a parameter should not go out of scope without its
+                    // obligation
+                    // being resolved some other way.
+                    if (obligation.derivedFromMustCallAlias()) {
+                        checker.reportError(
+                                obligation.resourceAliases.asList().get(0).tree,
+                                "mustcallalias.out.of.scope",
+                                exitReasonForErrorMessage);
+                        continue;
+                    }
+
                     // Which stores from the called-methods and must-call checkers are used in
                     // the consistency check varies depending on the context. The rules are:
                     // 1. if the current block has no nodes (and therefore the store must come from
@@ -1839,15 +1939,18 @@ private Set<Obligation> computeOwningParameters(ControlFlowGraph cfg) {
             Set<Obligation> result = new LinkedHashSet<>(1);
             for (VariableTree param : method.getParameters()) {
                 Element paramElement = TreeUtils.elementFromDeclaration(param);
-                if (typeFactory.hasMustCallAlias(paramElement)
+                boolean hasMustCallAlias = typeFactory.hasMustCallAlias(paramElement);
+                if (hasMustCallAlias
                         || (typeFactory.declaredTypeHasMustCall(param)
                                 && !checker.hasOption(MustCallChecker.NO_LIGHTWEIGHT_OWNERSHIP)
                                 && paramElement.getAnnotation(Owning.class) != null)) {
                     result.add(
                             new Obligation(
                                     ImmutableSet.of(
                                             new ResourceAlias(
-                                                    new LocalVariable(paramElement), param))));
+                                                    new LocalVariable(paramElement),
+                                                    param,
+                                                    hasMustCallAlias))));
                     // Increment numMustCall for each @Owning parameter tracked by the enclosing
                     // method.
                     incrementNumMustCall(paramElement);

checker/src/main/java/org/checkerframework/checker/resourceleak/messages.properties

@@ -4,3 +4,4 @@ incompatible.creates.mustcall.for=Method %s re-assigns the non-final, owning fie
 reset.not.owning=Calling method %s resets the must-call obligations of the expression %s, which is non-owning. Either annotate its declaration with an @Owning annotation, extract it into a local variable, or write a corresponding @CreatesMustCallFor annotation on the method that encloses this statement.
 creates.mustcall.for.override.invalid=Method %s cannot override method %s, which defines fewer @CreatesMustCallFor targets.\nfound:    %s\nrequired: %s
 destructor.exceptional.postcondition=Method %s must resolve the must-call obligations of the owning field %s along all paths, including exceptional paths. On an exceptional path, the @EnsuresCalledMethods annotation was not satisfied.\nFound:    %s\nRequired: %s
+mustcallalias.out.of.scope=This @MustCallAlias parameter might go out of scope without being assigned into an owning field of this object (if this is a constructor) or returned.\nReason for going out of scope: %s

checker/tests/mustcall/OwningParams.java

@@ -1,23 +1,13 @@
-// @above-java11-jdk-skip-test until this is fixed: https://tinyurl.com/cfissue/4934
-// Use of @Owning on receiver parameter doesn't work in Java 17+.
-// The TODO comment below might indicate that it also doesn't work before.
-
-// Tests that parameters (including receiver parameters) marked as @Owning are still checked.
+// Tests that parameters marked as @Owning are still checked.
 
 import org.checkerframework.checker.mustcall.qual.*;
 
 class OwningParams {
     static void o1(@Owning OwningParams o) {}
 
-    void o2(@Owning OwningParams this) {}
-
     void test(@Owning @MustCall({"a"}) OwningParams o, @Owning OwningParams p) {
         // :: error: argument.type.incompatible
         o1(o);
-        // TODO: this error doesn't show up! See MustCallVisitor#skipReceiverSubtypeCheck
-        //  error: method.invocation
-        o.o2();
         o1(p);
-        p.o2();
     }
 }

checker/tests/nullness/java17/NullnessSwitchArrows.java

@@ -36,8 +36,6 @@ void method2() {
             default -> throw new IllegalStateException("Invalid day: " + day);
         }
 
-        // TODO: this is a false positive.  It works for case: statements; see below.
-        // :: error: (dereference.of.nullable)
         o.toString();
     }
 

checker/tests/nullness/java17/NullnessSwitchExpressions.java

@@ -36,8 +36,6 @@ void method2() {
                     default -> throw new IllegalStateException("Invalid day: " + day);
                 };
 
-        // TODO: This is a false positive.
-        // :: error: (dereference.of.nullable)
         o.toString();
     }
 
@@ -51,8 +49,6 @@ void method3() {
                         String s = null;
                         if (day == Day.THURSDAY) {
                             s = "hello";
-                            // TODO: This is a false positive.
-                            // :: error: (dereference.of.nullable)
                             s.toString();
                         }
                         yield s;