KeyFor Checker: Convert annotations with parse errors to top for method invocations

Author: smillst

checker/src/main/java/org/checkerframework/checker/nullness/KeyForAnnotatedTypeFactory.java

@@ -1,6 +1,7 @@
 package org.checkerframework.checker.nullness;
 
 import com.sun.source.tree.ExpressionTree;
+import com.sun.source.tree.MethodInvocationTree;
 import com.sun.source.tree.NewClassTree;
 import com.sun.source.tree.Tree;
 import java.lang.annotation.Annotation;
@@ -10,6 +11,7 @@
 import java.util.LinkedHashSet;
 import java.util.Set;
 import javax.lang.model.element.AnnotationMirror;
+import javax.lang.model.element.Element;
 import javax.lang.model.element.ExecutableElement;
 import org.checkerframework.checker.nullness.qual.KeyFor;
 import org.checkerframework.checker.nullness.qual.KeyForBottom;
@@ -18,14 +20,21 @@
 import org.checkerframework.checker.signature.qual.CanonicalName;
 import org.checkerframework.common.basetype.BaseTypeChecker;
 import org.checkerframework.dataflow.cfg.node.Node;
+import org.checkerframework.dataflow.expression.JavaExpression;
+import org.checkerframework.dataflow.expression.Unknown;
 import org.checkerframework.dataflow.util.NodeUtils;
 import org.checkerframework.framework.flow.CFAbstractAnalysis;
+import org.checkerframework.framework.type.AnnotatedTypeFactory;
 import org.checkerframework.framework.type.AnnotatedTypeMirror;
+import org.checkerframework.framework.type.AnnotatedTypeMirror.AnnotatedExecutableType;
 import org.checkerframework.framework.type.GenericAnnotatedTypeFactory;
 import org.checkerframework.framework.type.QualifierHierarchy;
 import org.checkerframework.framework.type.SubtypeIsSupersetQualifierHierarchy;
 import org.checkerframework.framework.type.treeannotator.ListTreeAnnotator;
 import org.checkerframework.framework.type.treeannotator.TreeAnnotator;
+import org.checkerframework.framework.util.JavaExpressionParseUtil;
+import org.checkerframework.framework.util.StringToJavaExpression;
+import org.checkerframework.framework.util.dependenttypes.DependentTypesHelper;
 import org.checkerframework.javacutil.AnnotationBuilder;
 import org.checkerframework.javacutil.AnnotationUtils;
 import org.checkerframework.javacutil.TreeUtils;
@@ -221,4 +230,73 @@ boolean isMapPut(Node node) {
   public boolean shouldWarnIfStubRedundantWithBytecode() {
     return false;
   }
+
+  @Override
+  protected DependentTypesHelper createDependentTypesHelper() {
+    return new KeyForDependentTypesHelper(this);
+  }
+
+  /**
+   * Converts KeyFor annotations with errors into {@code @UnknownKeyFor} in the type of method
+   * invocations. This changes all qualifiers on the type of a method invocation expression, even
+   * qualifiers that are not primary annotations. This is unsound for qualifiers on type arguments
+   * or array elements on modifiable objects.
+   *
+   * <p>For example, if the type of some method called, {@code getList(...)}, is changed from {@code
+   * List<@KeyFor("a ? b : c") String>} to {@code List<@UnknownKeyFor String>}, then the returned
+   * list may have @UnknownKeyFor strings added.
+   *
+   * <pre>{@code
+   * List<String> l = getList(...);
+   * l.add(randoString);
+   * }</pre>
+   *
+   * This is probably ok for the KeyFor Checker because most of the collections of keys are
+   * unmodifiable.
+   */
+  static class KeyForDependentTypesHelper extends DependentTypesHelper {
+
+    /**
+     * Creates a {@code KeyForDependentTypesHelper}.
+     *
+     * @param factory annotated type factory
+     */
+    public KeyForDependentTypesHelper(AnnotatedTypeFactory factory) {
+      super(factory);
+    }
+
+    @Override
+    public void atMethodInvocation(
+        AnnotatedExecutableType methodType, MethodInvocationTree methodInvocationTree) {
+      if (!hasDependentAnnotations()) {
+        return;
+      }
+      Element methodElt = TreeUtils.elementFromUse(methodInvocationTree);
+
+      // The annotations on `declaredMethodType` will be copied to `methodType`.
+      AnnotatedExecutableType declaredMethodType =
+          (AnnotatedExecutableType) factory.getAnnotatedType(methodElt);
+      if (!hasDependentType(declaredMethodType)) {
+        return;
+      }
+
+      StringToJavaExpression stringToJavaExpr;
+      stringToJavaExpr =
+          stringExpr -> {
+            JavaExpression result =
+                StringToJavaExpression.atMethodInvocation(
+                    stringExpr, methodInvocationTree, factory.getChecker());
+            Unknown unknown = result.containedOfClass(Unknown.class);
+            if (unknown != null) {
+              throw JavaExpressionParseUtil.constructJavaExpressionParseError(
+                  result.toString(), "Expression " + unknown.toString() + " is unparsable.");
+            }
+            return result;
+          };
+
+      convertAnnotatedTypeMirror(stringToJavaExpr, declaredMethodType);
+      this.viewpointAdaptedCopier.visit(declaredMethodType, methodType);
+      this.errorAnnoReplacer.visit(methodType.getReturnType());
+    }
+  }
 }

checker/tests/nullness/Issue2470.java

@@ -43,7 +43,7 @@ static void ok() {
   static void buggy2() {
     new Example()
         .setS("test")
-        // :: error:(contracts.precondition)
+        // :: error: (contracts.precondition)
         .print();
   }
 

checker/tests/nullness/Issue6520.java

@@ -0,0 +1,14 @@
+package open.falsepos;
+
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+
+class Issue6520 {
+
+  private record Data(String value) {}
+
+  Issue6520(Stream<Data> data) {
+    data.collect(Collectors.groupingBy(Data::value)).entrySet().stream()
+        .filter(entry -> entry.getValue().size() > 1);
+  }
+}

checker/tests/nullness/Issue6750.java

@@ -0,0 +1,47 @@
+package open.falsepos;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.function.Function;
+import java.util.stream.Collectors;
+import org.checkerframework.checker.nullness.qual.KeyFor;
+
+public record Issue6750(String type) {
+
+  void needKeyFor(@KeyFor("#2") String s, Map<String, String> map) {
+    throw new RuntimeException();
+  }
+
+  @KeyFor("#1") String returnKeyFor(Map<String, String> map) {
+    throw new RuntimeException();
+  }
+
+  Map<String, String> getMap(Function<String, String> s) {
+    throw new RuntimeException();
+  }
+
+  void use() {
+    // :: error: (argument)
+    needKeyFor("", getMap(String::toString));
+    // :: error: (expression.unparsable) :: error: (assignment)
+    @KeyFor("getMap(String::toString)") String s = returnKeyFor(new HashMap<>(getMap(String::toString)));
+  }
+
+  void method(List<Issue6750> externals) {
+    externals.stream().collect(Collectors.groupingBy(Issue6750::type)).entrySet().stream()
+        .forEach(
+            values -> {
+              // :: error: (assignment)
+              @KeyFor({}) String b = values.getKey();
+            });
+  }
+
+  void test(List<Issue6750> externals) {
+    externals.stream().collect(Collectors.groupingBy(Issue6750::type)).entrySet().stream()
+        .forEach(
+            values -> {
+              throw new RuntimeException("");
+            });
+  }
+}

framework/src/main/java/org/checkerframework/framework/util/JavaExpressionParseUtil.java

@@ -1221,7 +1221,7 @@ public boolean isFlowParseError() {
    * @return a {@link JavaExpressionParseException} for the expression {@code expr} with explanation
    *     {@code explanation}.
    */
-  private static JavaExpressionParseException constructJavaExpressionParseError(
+  public static JavaExpressionParseException constructJavaExpressionParseError(
       String expr, String explanation) {
     if (expr == null) {
       throw new BugInCF("Must have an expression.");

framework/src/main/java/org/checkerframework/framework/util/dependenttypes/DependentTypesHelper.java

@@ -47,6 +47,7 @@
 import org.checkerframework.framework.type.AnnotatedTypeMirror.AnnotatedDeclaredType;
 import org.checkerframework.framework.type.AnnotatedTypeMirror.AnnotatedExecutableType;
 import org.checkerframework.framework.type.AnnotatedTypeParameterBounds;
+import org.checkerframework.framework.type.QualifierHierarchy;
 import org.checkerframework.framework.type.treeannotator.TreeAnnotator;
 import org.checkerframework.framework.type.visitor.AnnotatedTypeScanner;
 import org.checkerframework.framework.type.visitor.DoubleAnnotatedTypeScanner;
@@ -119,6 +120,12 @@ public class DependentTypesHelper {
   /** This scans an annotated type and returns a list of {@link DependentTypesError}. */
   private final ExpressionErrorCollector expressionErrorCollector = new ExpressionErrorCollector();
 
+  /**
+   * This scans the annotated type and replaces any dependent type annotation that has a parse error
+   * with the top annotation in the hierarchy.
+   */
+  protected final ErrorAnnoReplacer errorAnnoReplacer;
+
   /**
    * A scanner that applies a function to each {@link AnnotationMirror} and replaces it in the given
    * {@code AnnotatedTypeMirror}. (This side-effects the {@code AnnotatedTypeMirror}.)
@@ -129,7 +136,7 @@ public class DependentTypesHelper {
    * Copies annotations that might have been viewpoint adapted from the visited type (the first
    * formal parameter of {@code ViewpointAdaptedCopier#visit}) to the second formal parameter.
    */
-  private final ViewpointAdaptedCopier viewpointAdaptedCopier = new ViewpointAdaptedCopier();
+  protected final ViewpointAdaptedCopier viewpointAdaptedCopier = new ViewpointAdaptedCopier();
 
   /** The type mirror for java.lang.Object. */
   protected final TypeMirror objectTM;
@@ -141,7 +148,7 @@ public class DependentTypesHelper {
    */
   public DependentTypesHelper(AnnotatedTypeFactory factory) {
     this.factory = factory;
-
+    this.errorAnnoReplacer = new ErrorAnnoReplacer(factory.getQualifierHierarchy());
     this.annoToElements = new HashMap<>();
     for (Class<? extends Annotation> expressionAnno : factory.getSupportedTypeQualifiers()) {
       List<ExecutableElement> elementList =
@@ -1198,6 +1205,38 @@ private ExpressionErrorCollector() {
     }
   }
 
+  /**
+   * Replaces a dependent type annotation with a parser error with the top qualifier in the
+   * hierarchy.
+   */
+  protected class ErrorAnnoReplacer extends SimpleAnnotatedTypeScanner<Void, Void> {
+
+    /**
+     * Create an ErrorAnnoReplacer.
+     *
+     * @param qh the qualifier hierarchy
+     */
+    private ErrorAnnoReplacer(QualifierHierarchy qh) {
+      super(
+          (AnnotatedTypeMirror type, Void aVoid) -> {
+            AnnotationMirrorSet replacementAnnos = null;
+            for (AnnotationMirror am : type.getPrimaryAnnotations()) {
+              if (isExpressionAnno(am) && !errorElements(am).isEmpty()) {
+                if (replacementAnnos == null) {
+                  replacementAnnos = new AnnotationMirrorSet();
+                }
+                replacementAnnos.add(qh.getTopAnnotation(am));
+              }
+            }
+
+            if (replacementAnnos != null) {
+              type.replaceAnnotations(replacementAnnos);
+            }
+            return null;
+          });
+    }
+  }
+
   /**
    * Appends list2 to list1 in a new list. If either list is empty, returns the other. Thus, the
    * result may be aliased to one of the arguments and the client should only read, not write into,
@@ -1226,7 +1265,11 @@ private static List<DependentTypesError> concatenate(
    * visited type to the second formal parameter except for annotations on types that have been
    * substituted.
    */
-  private class ViewpointAdaptedCopier extends DoubleAnnotatedTypeScanner<Void> {
+  protected class ViewpointAdaptedCopier extends DoubleAnnotatedTypeScanner<Void> {
+
+    /** Create a ViewpointAdaptedCopier. */
+    private ViewpointAdaptedCopier() {}
+
     @Override
     protected Void scan(AnnotatedTypeMirror from, AnnotatedTypeMirror to) {
       if (from == null || to == null) {
@@ -1241,6 +1284,7 @@ protected Void scan(AnnotatedTypeMirror from, AnnotatedTypeMirror to) {
         }
       }
       to.replaceAnnotations(replacements);
+
       if (from.getKind() != to.getKind()
           || (from.getKind() == TypeKind.TYPEVAR
               && TypesUtils.isCapturedTypeVariable(to.getUnderlyingType()))) {
@@ -1277,7 +1321,7 @@ protected Void defaultAction(AnnotatedTypeMirror type1, AnnotatedTypeMirror type
    * @param atm a type
    * @return true if {@code atm} has any dependent type annotations
    */
-  private boolean hasDependentType(AnnotatedTypeMirror atm) {
+  protected boolean hasDependentType(AnnotatedTypeMirror atm) {
     if (atm == null) {
       return false;
     }