Resource Leak Checker (#4687)

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

@@ -49,7 +49,7 @@
  * When the -AnoResourceAliases command-line argument is passed to the checker, this annotation is
  * treated identically to {@link PolyMustCall}.
  *
- * @checker_framework.manual #must-call-checker Must Call Checker
+ * @checker_framework.manual #resource-leak-checker Resource Leak Checker
  * @checker_framework.manual #qualifier-polymorphism Qualifier polymorphism
  */
 @Documented

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

@@ -15,7 +15,7 @@
  * <p>When the -AnoLightweightOwnership command-line argument is passed to the checker, this
  * annotation and {@link Owning} are ignored.
  *
- * @checker_framework.manual #must-call-checker Must Call Checker
+ * @checker_framework.manual #resource-leak-checker Resource Leak Checker
  */
 @Retention(RetentionPolicy.RUNTIME)
 @Target({ElementType.METHOD, ElementType.PARAMETER, ElementType.FIELD})

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

@@ -7,15 +7,16 @@
 
 /**
  * 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.
+ * (when written on a method) return type, for the purposes of Must Call checking. 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>When the -AnoLightweightOwnership command-line argument is passed to the checker, this
  * annotation and {@link NotOwning} are ignored.
  *
- * @checker_framework.manual #must-call-checker Must Call Checker
+ * @checker_framework.manual #resource-leak-checker Resource Leak Checker
  */
 @Retention(RetentionPolicy.RUNTIME)
 @Target({ElementType.METHOD, ElementType.PARAMETER, ElementType.FIELD})

checker/bin-devel/count-suppressions

@@ -54,6 +54,9 @@ fi
 if [ "$regex" = "index" ]; then
     regex="\(index\|lessthan\|lowerbound\|samelen\|searchindex\|substringindex\|upperbound\)"
 fi
+if [ "$regex" = "resourceleak" ]; then
+  regex="\(mustcall\|objectconstruction\|resourceleak\)"
+fi
 
 # Diagnostics
 # echo "regex=${regex}"
@@ -67,10 +70,10 @@ countedreasons=$(mktemp /tmp/count-suppressions."$(date +%Y%m%d-%H%M%S)"-XXX)
 #    include "@SuppressWarnings" because it might appear on the previous line.
 #  * @AssumeAssertion(checkername)
 # This grep command captures a few stray lines; users should ignore them.
-# This grep command assumes that tests are not annotated, and it hard-codes ignoring "annotated-jdk", "jdk", and "true positive".
+# This grep command assumes that tests are not annotated, and it hard-codes ignoring "annotated-jdk", "jdk", "true positive", "// TP" (as an alias for "true positive"), and "count-suppressions-ignore".
 ${GREP} -n --recursive --include='*.java' "\"${regex}[:\"]\(.*[^;]\)\?\(\$\|//\)\|@AssumeAssertion(${regex})" \
     | grep -v "@AnnotatedFor" | grep -v "/tests/" \
-    | grep -v "/annotated-jdk/" | grep -v "/jdk/" | grep -v "^jdk/" | grep -v "true positive" > "${greplines}"
+    | grep -v "/annotated-jdk/" | grep -v "/jdk/" | grep -v "^jdk/" | grep -v "true positive" | grep -v "// TP" | grep -v "count-suppressions-ignore" > "${greplines}"
 
 total=$(wc -l < "${greplines}")
 ## Don't output a total, to avoid people using this approximate count.

checker/build.gradle

@@ -40,6 +40,16 @@ dependencies {
     implementation 'org.plumelib:reflection-util:1.0.3'
     implementation 'org.plumelib:plume-util:1.5.5'
 
+    // 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/28.2-jre .
+    // (It isn't at https://mvnrepository.com/artifact/org.checkerframework.annotatedlib/guava/28.2-jre, which is the bug.)
+    implementation 'com.google.errorprone:error_prone_annotations:2.7.1'
+    implementation ('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'
+    }
+
     // Dependencies added to "shadow" appear as dependencies in Maven Central.
     shadow project(':checker-qual')
     shadow project(':checker-util')

checker/src/main/java/org/checkerframework/checker/calledmethods/CalledMethodsAnnotatedTypeFactory.java

@@ -91,7 +91,11 @@ public CalledMethodsAnnotatedTypeFactory(BaseTypeChecker checker) {
     // therefore treat it as top.
     addAliasedTypeAnnotation(
         "org.checkerframework.checker.builder.qual.NotCalledMethods", this.top);
-    this.postInit();
+
+    // Don't call postInit() for subclasses.
+    if (this.getClass() == CalledMethodsAnnotatedTypeFactory.class) {
+      this.postInit();
+    }
   }
 
   /**

checker/src/main/java/org/checkerframework/checker/calledmethods/CalledMethodsChecker.java

@@ -60,7 +60,7 @@ public class CalledMethodsChecker extends AccumulationChecker {
 
   /**
    * The number of calls to build frameworks supported by this invocation. Incremented only if the
-   * {@link #COUNT_FRAMEWORK_BUILD_CALLS} option was supplied.
+   * {@link #COUNT_FRAMEWORK_BUILD_CALLS} command-line option was supplied.
    */
   int numBuildCalls = 0;
 

checker/src/main/java/org/checkerframework/checker/mustcall/MustCallAnnotatedTypeFactory.java

@@ -184,7 +184,7 @@ protected TypeAnnotator createTypeAnnotator() {
    * @param elt an element; may be null, in which case this method always returns false
    * @return true iff the given element represents a resource variable
    */
-  private boolean isResourceVariable(@Nullable Element elt) {
+  /* package-private*/ boolean isResourceVariable(@Nullable Element elt) {
     return elt != null && elt.getKind() == ElementKind.RESOURCE_VARIABLE;
   }
 
@@ -248,6 +248,16 @@ protected DefaultQualifierForUseTypeAnnotator createDefaultForUseTypeAnnotator()
     return new MustCallDefaultQualifierForUseTypeAnnotator();
   }
 
+  /**
+   * Returns the {@link MustCall#value} element. For use with {@link
+   * AnnotationUtils#getElementValueArray}.
+   *
+   * @return the {@link MustCall#value} element
+   */
+  public ExecutableElement getMustCallValueElement() {
+    return mustCallValueElement;
+  }
+
   /** Support @InheritableMustCall meaning @MustCall on all subtype elements. */
   class MustCallDefaultQualifierForUseTypeAnnotator extends DefaultQualifierForUseTypeAnnotator {
 

checker/src/main/java/org/checkerframework/checker/mustcall/MustCallChecker.java

@@ -7,7 +7,7 @@
 
 /**
  * This typechecker ensures that {@code @}{@link MustCall} annotations are consistent with one
- * another. The Object Construction Checker verifies that the given methods are actually called.
+ * another. The Resource Leak Checker verifies that the given methods are actually called.
  */
 @StubFiles({
   "JavaEE.astub",

checker/src/main/java/org/checkerframework/checker/mustcall/MustCallTransfer.java

@@ -10,10 +10,12 @@
 import javax.lang.model.element.AnnotationMirror;
 import javax.lang.model.element.Element;
 import javax.lang.model.type.TypeMirror;
+import org.checkerframework.checker.mustcall.qual.MustCall;
 import org.checkerframework.checker.nullness.qual.MonotonicNonNull;
 import org.checkerframework.checker.nullness.qual.Nullable;
 import org.checkerframework.dataflow.analysis.TransferInput;
 import org.checkerframework.dataflow.analysis.TransferResult;
+import org.checkerframework.dataflow.cfg.node.AssignmentNode;
 import org.checkerframework.dataflow.cfg.node.LocalVariableNode;
 import org.checkerframework.dataflow.cfg.node.MethodInvocationNode;
 import org.checkerframework.dataflow.cfg.node.Node;
@@ -94,6 +96,28 @@ private AnnotationMirror getDefaultStringType(StringConversionNode n) {
     return this.defaultStringType;
   }
 
+  @Override
+  public TransferResult<CFValue, CFStore> visitAssignment(
+      AssignmentNode n, TransferInput<CFValue, CFStore> in) {
+    TransferResult<CFValue, CFStore> result = super.visitAssignment(n, in);
+    // Remove "close" from the type in the store for resource variables.
+    // The Resource Leak Checker relies on this code to avoid checking that
+    // resource variables are closed.
+    if (atypeFactory.isResourceVariable(TreeUtils.elementFromTree(n.getTarget().getTree()))) {
+      CFStore store = result.getRegularStore();
+      JavaExpression expr = JavaExpression.fromNode(n.getTarget());
+      CFValue value = store.getValue(expr);
+      AnnotationMirror withClose =
+          atypeFactory.getAnnotationByClass(value.getAnnotations(), MustCall.class);
+      if (withClose == null) {
+        return result;
+      }
+      AnnotationMirror withoutClose = atypeFactory.withoutClose(withClose);
+      insertIntoStores(result, expr, withoutClose);
+    }
+    return result;
+  }
+
   @Override
   public TransferResult<CFValue, CFStore> visitMethodInvocation(
       MethodInvocationNode n, TransferInput<CFValue, CFStore> in) {
@@ -160,7 +184,7 @@ public TransferResult<CFValue, CFStore> visitTernaryExpression(
 
   /**
    * This method either creates or looks up the temp var t for node, and then updates the store to
-   * give t the same type as node.
+   * give t the same type as {@code node}.
    *
    * @param node the node to be assigned to a temporary variable
    * @param result the transfer result containing the store to be modified