Improve documentation of commonAssignmentCheck and correct overrides

checker/src/main/java/org/checkerframework/checker/formatter/FormatterVisitor.java

@@ -260,11 +260,11 @@ public static int formatStringIndex(ExecutableElement m) {
   protected boolean commonAssignmentCheck(
       AnnotatedTypeMirror varType,
       AnnotatedTypeMirror valueType,
-      Tree valueTree,
+      Tree errorLocation,
       @CompilerMessageKey String errorKey,
       Object... extraArgs) {
     boolean result =
-        super.commonAssignmentCheck(varType, valueType, valueTree, errorKey, extraArgs);
+        super.commonAssignmentCheck(varType, valueType, errorLocation, errorKey, extraArgs);
 
     AnnotationMirror rhs = valueType.getPrimaryAnnotationInHierarchy(atypeFactory.UNKNOWNFORMAT);
     AnnotationMirror lhs = varType.getPrimaryAnnotationInHierarchy(atypeFactory.UNKNOWNFORMAT);
@@ -282,7 +282,7 @@ protected boolean commonAssignmentCheck(
 
       if (rhsArgTypes.length < lhsArgTypes.length) {
         checker.reportWarning(
-            valueTree, "format.missing.arguments", varType.toString(), valueType.toString());
+            errorLocation, "format.missing.arguments", varType.toString(), valueType.toString());
         result = false;
       }
     }

checker/src/main/java/org/checkerframework/checker/i18nformatter/I18nFormatterVisitor.java

@@ -122,7 +122,7 @@ private void checkInvocationFormatFor(I18nFormatCall fc) {
   protected boolean commonAssignmentCheck(
       AnnotatedTypeMirror varType,
       AnnotatedTypeMirror valueType,
-      Tree valueTree,
+      Tree errorLocation,
       @CompilerMessageKey String errorKey,
       Object... extraArgs) {
     boolean result = true;
@@ -148,12 +148,15 @@ protected boolean commonAssignmentCheck(
         // It is legal to use a format string with fewer format specifiers
         // than required, but a warning is issued.
         checker.reportWarning(
-            valueTree, "i18nformat.missing.arguments", varType.toString(), valueType.toString());
+            errorLocation,
+            "i18nformat.missing.arguments",
+            varType.toString(),
+            valueType.toString());
       } else if (rhsArgTypes.length > lhsArgTypes.length) {
         // Since it is known that too many conversion categories were provided, issue a more
         // specific error message to that effect than "assignment".
         checker.reportError(
-            valueTree, "i18nformat.excess.arguments", varType.toString(), valueType.toString());
+            errorLocation, "i18nformat.excess.arguments", varType.toString(), valueType.toString());
         result = false;
       }
     }
@@ -165,7 +168,8 @@ protected boolean commonAssignmentCheck(
     // message issued for a given line of code will take precedence over the "assignment"
     // issued by super.commonAssignmentCheck().
     result =
-        super.commonAssignmentCheck(varType, valueType, valueTree, errorKey, extraArgs) && result;
+        super.commonAssignmentCheck(varType, valueType, errorLocation, errorKey, extraArgs)
+            && result;
     return result;
   }
 }

checker/src/main/java/org/checkerframework/checker/index/inequality/LessThanVisitor.java

@@ -69,10 +69,13 @@ protected boolean commonAssignmentCheck(
   @Override
   protected boolean commonAssignmentCheck(
       AnnotatedTypeMirror varType,
-      AnnotatedTypeMirror valueType,
-      Tree valueTree,
+      ExpressionTree valueTree,
       @CompilerMessageKey String errorKey,
       Object... extraArgs) {
+
+    if (shouldSkipUses(valueTree)) {
+      return true;
+    }
     // If value is less than all expressions in the annotation in varType,
     // using the Value Checker, then skip the common assignment check.
     // Also skip the check if the only expression is "a + 1" and the valueTree is "a".
@@ -101,14 +104,11 @@ protected boolean commonAssignmentCheck(
       }
 
       if (isLessThan) {
-        // Print the messages because super isn't called.
-        commonAssignmentCheckStartDiagnostic(varType, valueType, valueTree);
-        commonAssignmentCheckEndDiagnostic(true, "isLessThan", varType, valueType, valueTree);
         // skip call to super, everything is OK.
         return true;
       }
     }
-    return super.commonAssignmentCheck(varType, valueType, valueTree, errorKey, extraArgs);
+    return super.commonAssignmentCheck(varType, valueTree, errorKey, extraArgs);
   }
 
   @Override

checker/src/main/java/org/checkerframework/checker/index/samelen/SameLenVisitor.java

@@ -1,11 +1,11 @@
 package org.checkerframework.checker.index.samelen;
 
 import com.sun.source.tree.ExpressionTree;
-import com.sun.source.tree.Tree;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.TreeSet;
 import javax.lang.model.element.AnnotationMirror;
+import javax.lang.model.type.TypeMirror;
 import org.checkerframework.checker.compilermsgs.qual.CompilerMessageKey;
 import org.checkerframework.checker.index.IndexUtil;
 import org.checkerframework.checker.index.qual.PolySameLen;
@@ -30,34 +30,39 @@ public SameLenVisitor(BaseTypeChecker checker) {
   @Override
   protected boolean commonAssignmentCheck(
       AnnotatedTypeMirror varType,
-      AnnotatedTypeMirror valueType,
-      Tree valueTree,
+      ExpressionTree valueTree,
       @CompilerMessageKey String errorKey,
       Object... extraArgs) {
-    if (IndexUtil.isSequenceType(valueType.getUnderlyingType())
-        && TreeUtils.isExpressionTree(valueTree)
-        // if both annotations are @PolySameLen, there is nothing to do
-        && !(valueType.hasPrimaryAnnotation(PolySameLen.class)
-            && varType.hasPrimaryAnnotation(PolySameLen.class))) {
 
-      JavaExpression rhs = JavaExpression.fromTree((ExpressionTree) valueTree);
-      if (rhs != null && SameLenAnnotatedTypeFactory.mayAppearInSameLen(rhs)) {
-        String rhsExpr = rhs.toString();
-        AnnotationMirror sameLenAnno = valueType.getPrimaryAnnotation(SameLen.class);
-        Collection<String> exprs;
-        if (sameLenAnno == null) {
-          exprs = Collections.singletonList(rhsExpr);
-        } else {
-          exprs =
-              new TreeSet<>(
-                  AnnotationUtils.getElementValueArray(
-                      sameLenAnno, atypeFactory.sameLenValueElement, String.class));
-          exprs.add(rhsExpr);
+    if (shouldSkipUses(valueTree)) {
+      return true;
+    }
+    TypeMirror valueTypeMirror = TreeUtils.typeOf(valueTree);
+    if (IndexUtil.isSequenceType(valueTypeMirror) && TreeUtils.isExpressionTree(valueTree)) {
+      AnnotatedTypeMirror valueType = atypeFactory.getAnnotatedType(valueTree);
+      // if both annotations are @PolySameLen, there is nothing to do
+      if (!(valueType.hasPrimaryAnnotation(PolySameLen.class)
+          && varType.hasPrimaryAnnotation(PolySameLen.class))) {
+        JavaExpression rhs = JavaExpression.fromTree(valueTree);
+        if (rhs != null && SameLenAnnotatedTypeFactory.mayAppearInSameLen(rhs)) {
+          String rhsExpr = rhs.toString();
+          AnnotationMirror sameLenAnno = valueType.getPrimaryAnnotation(SameLen.class);
+          Collection<String> exprs;
+          if (sameLenAnno == null) {
+            exprs = Collections.singletonList(rhsExpr);
+          } else {
+            exprs =
+                new TreeSet<>(
+                    AnnotationUtils.getElementValueArray(
+                        sameLenAnno, atypeFactory.sameLenValueElement, String.class));
+            exprs.add(rhsExpr);
+          }
+          AnnotationMirror newSameLen = atypeFactory.createSameLen(exprs);
+          valueType.replaceAnnotation(newSameLen);
+          return super.commonAssignmentCheck(varType, valueType, valueTree, errorKey, extraArgs);
         }
-        AnnotationMirror newSameLen = atypeFactory.createSameLen(exprs);
-        valueType.replaceAnnotation(newSameLen);
       }
     }
-    return super.commonAssignmentCheck(varType, valueType, valueTree, errorKey, extraArgs);
+    return super.commonAssignmentCheck(varType, valueTree, errorKey, extraArgs);
   }
 }

checker/src/main/java/org/checkerframework/checker/lock/LockVisitor.java

@@ -341,7 +341,7 @@ protected void checkConstructorResult(
   protected boolean commonAssignmentCheck(
       AnnotatedTypeMirror varType,
       AnnotatedTypeMirror valueType,
-      Tree valueTree,
+      Tree errorLocation,
       @CompilerMessageKey String errorKey,
       Object... extraArgs) {
 
@@ -355,7 +355,7 @@ protected boolean commonAssignmentCheck(
     boolean result = true;
     if (varType.hasPrimaryAnnotation(GuardSatisfied.class)) {
       if (valueType.hasPrimaryAnnotation(GuardedBy.class)) {
-        return checkLock(valueTree, valueType.getPrimaryAnnotation(GuardedBy.class));
+        return checkLock(errorLocation, valueType.getPrimaryAnnotation(GuardedBy.class));
       } else if (valueType.hasPrimaryAnnotation(GuardSatisfied.class)) {
         // TODO: Find a cleaner, non-abstraction-breaking way to know whether method actual
         // parameters are being assigned to formal parameters.
@@ -371,7 +371,7 @@ protected boolean commonAssignmentCheck(
 
           if (varTypeGuardSatisfiedIndex == -1 && valueTypeGuardSatisfiedIndex == -1) {
             checker.reportError(
-                valueTree, "guardsatisfied.assignment.disallowed", varType, valueType);
+                errorLocation, "guardsatisfied.assignment.disallowed", varType, valueType);
             result = false;
           }
         } else {
@@ -400,7 +400,8 @@ protected boolean commonAssignmentCheck(
     }
 
     result =
-        super.commonAssignmentCheck(varType, valueType, valueTree, errorKey, extraArgs) && result;
+        super.commonAssignmentCheck(varType, valueType, errorLocation, errorKey, extraArgs)
+            && result;
     return result;
   }
 

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

@@ -236,18 +236,18 @@ protected boolean commonAssignmentCheck(
   protected boolean commonAssignmentCheck(
       AnnotatedTypeMirror varType,
       AnnotatedTypeMirror valueType,
-      Tree valueTree,
+      Tree errorLocation,
       @CompilerMessageKey String errorKey,
       Object... extraArgs) {
     if (TypesUtils.isPrimitive(varType.getUnderlyingType())
         && !TypesUtils.isPrimitive(valueType.getUnderlyingType())) {
-      boolean succeed = checkForNullability(valueType, valueTree, UNBOXING_OF_NULLABLE);
+      boolean succeed = checkForNullability(valueType, errorLocation, UNBOXING_OF_NULLABLE);
       if (!succeed) {
         // Only issue the unboxing of nullable error.
         return false;
       }
     }
-    return super.commonAssignmentCheck(varType, valueType, valueTree, errorKey, extraArgs);
+    return super.commonAssignmentCheck(varType, valueType, errorLocation, errorKey, extraArgs);
   }
 
   /** Case 1: Check for null dereferencing. */

framework/src/main/java/org/checkerframework/common/aliasing/AliasingVisitor.java

@@ -13,7 +13,6 @@
 import javax.lang.model.element.ExecutableElement;
 import javax.lang.model.element.VariableElement;
 import org.checkerframework.checker.compilermsgs.qual.CompilerMessageKey;
-import org.checkerframework.checker.formatter.qual.FormatMethod;
 import org.checkerframework.common.aliasing.qual.LeakedToResult;
 import org.checkerframework.common.aliasing.qual.NonLeaked;
 import org.checkerframework.common.aliasing.qual.Unique;
@@ -172,15 +171,16 @@ protected boolean commonAssignmentCheck(
   }
 
   @Override
-  @FormatMethod
   protected boolean commonAssignmentCheck(
       AnnotatedTypeMirror varType,
-      AnnotatedTypeMirror valueType,
-      Tree valueTree,
+      ExpressionTree valueTree,
       @CompilerMessageKey String errorKey,
       Object... extraArgs) {
-    boolean result =
-        super.commonAssignmentCheck(varType, valueType, valueTree, errorKey, extraArgs);
+
+    if (shouldSkipUses(valueTree)) {
+      return true;
+    }
+    boolean result = super.commonAssignmentCheck(varType, valueTree, errorKey, extraArgs);
 
     // If we are visiting a pseudo-assignment, visitorLeafKind is either
     // Tree.Kind.NEW_CLASS or Tree.Kind.METHOD_INVOCATION.

framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java

@@ -2923,6 +2923,12 @@ protected AnnotationMirrorSet getThrowUpperBoundAnnotations() {
    * Checks the validity of an assignment (or pseudo-assignment) from a value to a variable and
    * emits an error message (through the compiler's messaging interface) if it is not valid.
    *
+   * <p>Because this method is not called for all pseudo-assignments, subclasses should only
+   * override this class if {@code varTree} is needed. Otherwise, {@link
+   * #commonAssignmentCheck(AnnotatedTypeMirror, ExpressionTree, String, Object...)} or {@link
+   * #commonAssignmentCheck(AnnotatedTypeMirror, AnnotatedTypeMirror, Tree, String, Object...)}
+   * should be overridden instead.
+   *
    * @param varTree the AST node for the lvalue (usually a variable)
    * @param valueExpTree the AST node for the rvalue (the new value)
    * @param errorKey the error message key to use if the check fails
@@ -2957,8 +2963,15 @@ protected boolean commonAssignmentCheck(
   }
 
   /**
-   * Checks the validity of an assignment (or pseudo-assignment) from a value to a variable and
-   * emits an error message (through the compiler's messaging interface) if it is not valid.
+   * Checks the validity of an assignment (or pseudo-assignment) from {@code valueExpTree} to a
+   * variable with type {@code varType} and emits an error message (through the compiler's messaging
+   * interface) if it is not valid.
+   *
+   * <p>Because this method is not called for all pseudo-assignments, subclasses should only
+   * override this, if the tree of the value expression is required; otherwise, override {@link
+   * #commonAssignmentCheck(AnnotatedTypeMirror, AnnotatedTypeMirror, Tree, String, Object...)}. If
+   * this method is overridden, then the first then should call {@link
+   * #shouldSkipUses(ExpressionTree)} and return if that method returns true.
    *
    * @param varType the annotated type for the lvalue (usually a variable)
    * @param valueExpTree the AST node for the rvalue (the new value)
@@ -2972,18 +2985,6 @@ protected boolean commonAssignmentCheck(
       @CompilerMessageKey String errorKey,
       Object... extraArgs) {
     if (shouldSkipUses(valueExpTree)) {
-      if (showchecks) {
-        System.out.printf(
-            "%s %s (at %s): actual tree = %s %s%n   expected: %s %s%n",
-            this.getClass().getSimpleName(),
-            "skipping test whether actual is a subtype of expected"
-                + " because shouldSkipUses() returned true",
-            fileAndLineNumber(valueExpTree),
-            valueExpTree.getKind(),
-            valueExpTree,
-            varType.getKind(),
-            varType.toString());
-      }
       return true;
     }
     if (valueExpTree.getKind() == Tree.Kind.MEMBER_REFERENCE
@@ -3039,24 +3040,29 @@ protected boolean commonAssignmentCheck(
   }
 
   /**
-   * Checks the validity of an assignment (or pseudo-assignment) from a value to a variable and
-   * emits an error message (through the compiler's messaging interface) if it is not valid.
+   * Checks the validity of an assignment (or pseudo-assignment) from {@code valueType} to {@code
+   * variableType} and emits an error message (through the compiler's messaging interface) if it is
+   * not valid.
+   *
+   * <p>Subclasses should override this method if in all cases unless the tree for the {@code
+   * varType} or {@code valueType} are needed.
    *
    * @param varType the annotated type of the variable
    * @param valueType the annotated type of the value
-   * @param valueExpTree the location to use when reporting the error message
+   * @param errorLocation the location to use when reporting the error message; this is NOT the tree
+   *     for {@code valueType} in all cases
    * @param errorKey the error message key to use if the check fails
    * @param extraArgs arguments to the error message key, before "found" and "expected" types
    * @return true if the check succeeds, false if an error message was issued
    */
   protected boolean commonAssignmentCheck(
       AnnotatedTypeMirror varType,
       AnnotatedTypeMirror valueType,
-      Tree valueExpTree,
+      Tree errorLocation,
       @CompilerMessageKey String errorKey,
       Object... extraArgs) {
 
-    commonAssignmentCheckStartDiagnostic(varType, valueType, valueExpTree);
+    commonAssignmentCheckStartDiagnostic(varType, valueType, errorLocation);
 
     AnnotatedTypeMirror widenedValueType = atypeFactory.getWidenedType(valueType, varType);
     boolean result = typeHierarchy.isSubtype(widenedValueType, varType);
@@ -3066,7 +3072,7 @@ protected boolean commonAssignmentCheck(
       for (Class<? extends Annotation> mono : atypeFactory.getSupportedMonotonicTypeQualifiers()) {
         if (valueType.hasPrimaryAnnotation(mono) && varType.hasPrimaryAnnotation(mono)) {
           checker.reportError(
-              valueExpTree,
+              errorLocation,
               "monotonic",
               mono.getSimpleName(),
               mono.getSimpleName(),
@@ -3081,12 +3087,12 @@ protected boolean commonAssignmentCheck(
       String valueTypeString = pair.found;
       String varTypeString = pair.required;
       checker.reportError(
-          valueExpTree,
+          errorLocation,
           errorKey,
           ArraysPlume.concatenate(extraArgs, valueTypeString, varTypeString));
     }
 
-    commonAssignmentCheckEndDiagnostic(result, null, varType, valueType, valueExpTree);
+    commonAssignmentCheckEndDiagnostic(result, null, varType, valueType, errorLocation);
 
     return result;
   }
@@ -4838,7 +4844,21 @@ protected final boolean shouldSkipUses(ExpressionTree exprTree) {
       return true;
     }
     Element elm = TreeUtils.elementFromTree(exprTree);
-    return checker.shouldSkipUses(elm);
+    boolean shouldSkipUses = checker.shouldSkipUses(elm);
+    if (showchecks) {
+      TypeMirror exprTypeMirror = TreeUtils.typeOf(exprTree);
+      System.out.printf(
+          "%s %s (at %s): actual tree = %s %s%n   expected: %s %s%n",
+          this.getClass().getSimpleName(),
+          "skipping test whether actual is a subtype of expected"
+              + " because shouldSkipUses() returned true",
+          fileAndLineNumber(exprTree),
+          exprTree.getKind(),
+          exprTree,
+          exprTypeMirror.getKind(),
+          exprTypeMirror.toString());
+    }
+    return shouldSkipUses;
   }
 
   // **********************************************************************