Moe Sync

check_api/src/main/java/com/google/errorprone/dataflow/nullnesspropagation/NullnessPropagationTransfer.java

@@ -16,6 +16,7 @@
 
 package com.google.errorprone.dataflow.nullnesspropagation;
 
+import static com.google.common.collect.ImmutableList.toImmutableList;
 import static com.google.errorprone.dataflow.nullnesspropagation.Nullness.NONNULL;
 import static com.google.errorprone.dataflow.nullnesspropagation.Nullness.NULL;
 import static com.google.errorprone.dataflow.nullnesspropagation.Nullness.NULLABLE;
@@ -38,6 +39,7 @@
 import com.google.common.primitives.UnsignedLong;
 import com.google.errorprone.dataflow.LocalStore;
 import com.google.errorprone.dataflow.LocalVariableValues;
+import com.google.errorprone.util.MoreAnnotations;
 import com.sun.source.tree.ClassTree;
 import com.sun.source.tree.CompilationUnitTree;
 import com.sun.source.tree.ExpressionTree;
@@ -72,7 +74,6 @@
 import java.util.Objects;
 import java.util.Set;
 import javax.annotation.Nullable;
-import javax.lang.model.element.AnnotationMirror;
 import javax.lang.model.element.VariableElement;
 import org.checkerframework.dataflow.analysis.Analysis;
 import org.checkerframework.dataflow.cfg.CFGBuilder;
@@ -852,20 +853,16 @@ private ClassAndMethod(
     static ClassAndMethod make(MethodSymbol methodSymbol, @Nullable Types types) {
       // TODO(b/71812955): consider just wrapping methodSymbol instead of copying everything out.
       // TODO(b/71812955): for type variables, check for type annotations on the referenced variable
-      ImmutableList.Builder<String> annotations = ImmutableList.builder();
-      for (AnnotationMirror annotationMirror : methodSymbol.getAnnotationMirrors()) {
-        annotations.add(annotationMirror.getAnnotationType().toString());
-      }
-      for (AnnotationMirror annotationMirror :
-          methodSymbol.getReturnType().getAnnotationMirrors()) {
-        annotations.add(annotationMirror.getAnnotationType().toString());
-      }
+      ImmutableList<String> annotations =
+          MoreAnnotations.getDeclarationAndTypeAttributes(methodSymbol)
+              .map(c -> c.getAnnotationType().asElement().toString())
+              .collect(toImmutableList());
 
       ClassSymbol clazzSymbol = (ClassSymbol) methodSymbol.owner;
       return new ClassAndMethod(
           clazzSymbol.getQualifiedName().toString(),
           methodSymbol.getSimpleName().toString(),
-          annotations.build(),
+          annotations,
           methodSymbol.isStatic(),
           methodSymbol.getReturnType().isPrimitive(),
           methodSymbol.getReturnType().getTag() == BOOLEAN,

check_api/src/main/java/com/google/errorprone/dataflow/nullnesspropagation/TrustingNullnessPropagation.java

@@ -17,13 +17,12 @@
 package com.google.errorprone.dataflow.nullnesspropagation;
 
 import com.google.common.base.Predicate;
+import com.google.common.collect.ImmutableSet;
 import com.google.errorprone.dataflow.LocalStore;
+import com.google.errorprone.util.MoreAnnotations;
 import com.sun.tools.javac.code.Symbol;
-import com.sun.tools.javac.code.Symbol.MethodSymbol;
 import java.util.List;
 import javax.annotation.Nullable;
-import javax.lang.model.AnnotatedConstruct;
-import javax.lang.model.element.AnnotationMirror;
 import javax.lang.model.element.Element;
 import org.checkerframework.dataflow.cfg.UnderlyingAST;
 import org.checkerframework.dataflow.cfg.node.LocalVariableNode;
@@ -71,7 +70,7 @@ public LocalStore<Nullness> initialStore(
     LocalStore.Builder<Nullness> result = LocalStore.<Nullness>empty().toBuilder();
     for (LocalVariableNode param : parameters) {
       Element element = param.getElement();
-      Nullness assumed = nullnessFromAnnotations(element, param.getType());
+      Nullness assumed = nullnessFromAnnotations((Symbol) element);
       result.setInformation(element, assumed);
     }
     return result.build();
@@ -87,23 +86,18 @@ Nullness fieldNullness(@Nullable ClassAndField accessed) {
     return nullnessFromAnnotations(accessed.symbol);
   }
 
+  // TODO(b/79270313): consolidate hard-coded names of @Nullable annotations
+  private static final ImmutableSet<String> NULLABLE_ANNOTATION_SIMPLE_NAMES =
+      ImmutableSet.of("Nullable", "NullableDecl");
+
   /** Returns nullability based on the presence of a {@code Nullable} annotation. */
   static Nullness nullnessFromAnnotations(Symbol symbol) {
-    return nullnessFromAnnotations(
-        symbol,
-        symbol instanceof MethodSymbol ? ((MethodSymbol) symbol).getReturnType() : symbol.type);
-  }
-
-  // TODO(b/71812955): for type variables, check for type annotations on the referenced variable
-  private static Nullness nullnessFromAnnotations(AnnotatedConstruct... elements) {
-    for (AnnotatedConstruct element : elements) {
-      for (AnnotationMirror anno : element.getAnnotationMirrors()) {
-        // Check for Nullable like ReturnValueIsNonNull
-        if (anno.getAnnotationType().toString().endsWith(".Nullable")
-            || anno.getAnnotationType().toString().endsWith(".NullableDecl")) {
-          return Nullness.NULLABLE;
-        }
-      }
+    if (MoreAnnotations.getDeclarationAndTypeAttributes(symbol)
+        .anyMatch(
+            c ->
+                NULLABLE_ANNOTATION_SIMPLE_NAMES.contains(
+                    c.getAnnotationType().asElement().getSimpleName().toString()))) {
+      return Nullness.NULLABLE;
     }
     return Nullness.NONNULL;
   }

check_api/src/main/java/com/google/errorprone/fixes/Replacements.java

@@ -29,6 +29,7 @@
 import java.util.LinkedHashSet;
 import java.util.Set;
 import java.util.TreeMap;
+import java.util.TreeSet;
 
 /** A collection of {@link Replacement}s to be made to a source file. */
 public class Replacements {
@@ -50,6 +51,7 @@ public int compare(Range<Integer> o1, Range<Integer> o2) {
 
   private final TreeMap<Range<Integer>, Replacement> replacements = new TreeMap<>(DESCENDING);
   private final RangeMap<Integer, Replacement> overlaps = TreeRangeMap.create();
+  private final TreeSet<Integer> zeroLengthRanges = new TreeSet<>();
 
   public Replacements add(Replacement replacement) {
     if (replacements.containsKey(replacement.range())) {
@@ -76,14 +78,29 @@ public Replacements add(Replacement replacement) {
   }
 
   private void checkOverlaps(Replacement replacement) {
+    Range<Integer> replacementRange = replacement.range();
     Collection<Replacement> overlap =
-        overlaps.subRangeMap(replacement.range()).asMapOfRanges().values();
+        overlaps.subRangeMap(replacementRange).asMapOfRanges().values();
     checkArgument(
         overlap.isEmpty(),
         "%s overlaps with existing replacements: %s",
         replacement,
         Joiner.on(", ").join(overlap));
-    overlaps.put(replacement.range(), replacement);
+    Set<Integer> containedZeroLengthRangeStarts =
+        zeroLengthRanges.subSet(
+            replacementRange.lowerEndpoint(),
+            /* fromInclusive= */ false,
+            replacementRange.upperEndpoint(),
+            /* toInclusive= */ false);
+    checkArgument(
+        containedZeroLengthRangeStarts.isEmpty(),
+        "%s overlaps with existing zero-length replacements: %s",
+        replacement,
+        Joiner.on(", ").join(containedZeroLengthRangeStarts));
+    overlaps.put(replacementRange, replacement);
+    if (replacementRange.isEmpty()) {
+      zeroLengthRanges.add(replacementRange.lowerEndpoint());
+    }
   }
 
   /** Non-overlapping replacements, sorted in descending order by position. */

check_api/src/main/java/com/google/errorprone/fixes/SuggestedFixes.java

@@ -149,56 +149,66 @@ public static Optional<SuggestedFix> addModifiers(
     }
     Set<Modifier> toAdd =
         Sets.difference(new TreeSet<>(Arrays.asList(modifiers)), originalModifiers.getFlags());
-    if (originalModifiers.getFlags().isEmpty()) {
-      int pos =
-          state.getEndPosition(originalModifiers) != Position.NOPOS
-              ? state.getEndPosition(originalModifiers) + 1
-              : ((JCTree) tree).getStartPosition();
-      int base = ((JCTree) tree).getStartPosition();
-      Optional<Integer> insert =
-          state
-              .getTokensForNode(tree)
-              .stream()
-              .map(token -> token.pos() + base)
-              .filter(thisPos -> thisPos >= pos)
-              .findFirst();
-      int insertPos = insert.orElse(pos); // shouldn't ever be able to get to the else
-      return Optional.of(
-          SuggestedFix.replace(insertPos, insertPos, Joiner.on(' ').join(toAdd) + " "));
-    }
-    // a map from modifiers to modifier position (or -1 if the modifier is being added)
-    // modifiers are sorted in Google Java Style order
-    Map<Modifier, Integer> modifierPositions = new TreeMap<>();
-    for (Modifier mod : toAdd) {
-      modifierPositions.put(mod, -1);
-    }
-    List<ErrorProneToken> tokens = state.getTokensForNode(originalModifiers);
-    int base = ((JCTree) originalModifiers).getStartPosition();
-    for (ErrorProneToken tok : tokens) {
-      Modifier mod = getTokModifierKind(tok);
-      if (mod != null) {
-        modifierPositions.put(mod, base + tok.pos());
-      }
-    }
     SuggestedFix.Builder fix = SuggestedFix.builder();
-    // walk the map of all modifiers, and accumulate a list of new modifiers to insert
-    // beside an existing modifier
     List<Modifier> modifiersToWrite = new ArrayList<>();
-    for (Modifier mod : modifierPositions.keySet()) {
-      int p = modifierPositions.get(mod);
-      if (p == -1) {
-        modifiersToWrite.add(mod);
-      } else if (!modifiersToWrite.isEmpty()) {
-        fix.replace(p, p, Joiner.on(' ').join(modifiersToWrite) + " ");
-        modifiersToWrite.clear();
+    if (!originalModifiers.getFlags().isEmpty()) {
+      // a map from modifiers to modifier position (or -1 if the modifier is being added)
+      // modifiers are sorted in Google Java Style order
+      Map<Modifier, Integer> modifierPositions = new TreeMap<>();
+      for (Modifier mod : toAdd) {
+        modifierPositions.put(mod, -1);
       }
+      List<ErrorProneToken> tokens = state.getTokensForNode(originalModifiers);
+      int base = ((JCTree) originalModifiers).getStartPosition();
+      for (ErrorProneToken tok : tokens) {
+        Modifier mod = getTokModifierKind(tok);
+        if (mod != null) {
+          modifierPositions.put(mod, base + tok.pos());
+        }
+      }
+      // walk the map of all modifiers, and accumulate a list of new modifiers to insert
+      // beside an existing modifier
+      for (Modifier mod : modifierPositions.keySet()) {
+        int p = modifierPositions.get(mod);
+        if (p == -1) {
+          modifiersToWrite.add(mod);
+        } else if (!modifiersToWrite.isEmpty()) {
+          fix.replace(p, p, Joiner.on(' ').join(modifiersToWrite) + " ");
+          modifiersToWrite.clear();
+        }
+      }
+    } else {
+      modifiersToWrite.addAll(toAdd);
     }
-    if (!modifiersToWrite.isEmpty()) {
-      fix.postfixWith(originalModifiers, " " + Joiner.on(' ').join(modifiersToWrite));
-    }
+    addRemainingModifiers(tree, state, originalModifiers, modifiersToWrite, fix);
     return Optional.of(fix.build());
   }
 
+  private static void addRemainingModifiers(
+      Tree tree,
+      VisitorState state,
+      ModifiersTree originalModifiers,
+      Collection<Modifier> toAdd,
+      SuggestedFix.Builder fix) {
+    if (toAdd.isEmpty()) {
+      return;
+    }
+    int pos =
+        state.getEndPosition(originalModifiers) != Position.NOPOS
+            ? state.getEndPosition(originalModifiers) + 1
+            : ((JCTree) tree).getStartPosition();
+    int base = ((JCTree) tree).getStartPosition();
+    Optional<Integer> insert =
+        state
+            .getTokensForNode(tree)
+            .stream()
+            .map(token -> token.pos() + base)
+            .filter(thisPos -> thisPos >= pos)
+            .findFirst();
+    int insertPos = insert.orElse(pos); // shouldn't ever be able to get to the else
+    fix.replace(insertPos, insertPos, Joiner.on(' ').join(toAdd) + " ");
+  }
+
   /** Remove modifiers from the given class, method, or field declaration. */
   public static Optional<SuggestedFix> removeModifiers(
       Tree tree, VisitorState state, Modifier... modifiers) {
@@ -216,7 +226,6 @@ public static Optional<SuggestedFix> removeModifiers(
       if (toRemove.contains(mod)) {
         empty = false;
         fix.replace(basePos + tok.pos(), basePos + tok.endPos() + 1, "");
-        break;
       }
     }
     if (empty) {
@@ -734,7 +743,7 @@ public CharSequence getCharContent(boolean ignoreEncodingErrors) throws IOExcept
     try {
       newTask.analyze();
     } catch (Throwable e) {
-      // ignored
+      return false; // ¯\_(ツ)_/¯
     }
     return countErrors(diagnosticListener) == 0;
   }

check_api/src/main/java/com/google/errorprone/util/ASTHelpers.java

@@ -1398,4 +1398,22 @@ private Type getConditionType(Tree condition) {
       return null;
     }
   }
+
+  /**
+   * Returns declaration annotations of the given symbol, as well as 'top-level' type annotations,
+   * including :
+   *
+   * <ul>
+   *   <li>Type annotations of the return type of a method.
+   *   <li>Type annotations on the type of a formal parameter or field.
+   * </ul>
+   *
+   * <p>One might expect this to be equivalent to information returned by {@link
+   * Type#getAnnotationMirrors}, but javac doesn't associate type annotation information with types
+   * for symbols completed from class files, so that approach doesn't work across compilation
+   * boundaries.
+   */
+  public static Stream<Attribute.Compound> getDeclarationAndTypeAttributes(Symbol sym) {
+    return MoreAnnotations.getDeclarationAndTypeAttributes(sym);
+  }
 }

check_api/src/main/java/com/google/errorprone/util/MoreAnnotations.java

@@ -0,0 +1,86 @@
+/*
+ * Copyright 2012 The Error Prone Authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.google.errorprone.util;
+
+import com.google.common.collect.Streams;
+import com.sun.tools.javac.code.Attribute;
+import com.sun.tools.javac.code.Symbol;
+import com.sun.tools.javac.code.Symbol.MethodSymbol;
+import com.sun.tools.javac.code.TargetType;
+import com.sun.tools.javac.code.TypeAnnotationPosition;
+import java.util.stream.Stream;
+
+/** Annotation-related utilities. */
+public final class MoreAnnotations {
+
+  /**
+   * Returns declaration annotations of the given symbol, as well as 'top-level' type annotations,
+   * including :
+   *
+   * <ul>
+   *   <li>Type annotations of the return type of a method.
+   *   <li>Type annotations on the type of a formal parameter or field.
+   * </ul>
+   *
+   * <p>One might expect this to be equivalent to information returned by {@link
+   * com.sun.tools.javac.code.Type#getAnnotationMirrors}, but javac doesn't associate type
+   * annotation information with types for symbols completed from class files, so that approach
+   * doesn't work across compilation boundaries.
+   */
+  public static Stream<Attribute.Compound> getDeclarationAndTypeAttributes(Symbol sym) {
+    Symbol typeAnnotationOwner;
+    switch (sym.getKind()) {
+      case PARAMETER:
+        typeAnnotationOwner = sym.owner;
+        break;
+      default:
+        typeAnnotationOwner = sym;
+    }
+    return Streams.concat(
+        sym.getRawAttributes().stream(),
+        typeAnnotationOwner
+            .getRawTypeAttributes()
+            .stream()
+            .filter(anno -> isAnnotationOnType(sym, anno.position)));
+  }
+
+  private static boolean isAnnotationOnType(Symbol sym, TypeAnnotationPosition position) {
+    if (!position.location.isEmpty()) {
+      return false;
+    }
+    switch (sym.getKind()) {
+      case LOCAL_VARIABLE:
+        return position.type == TargetType.LOCAL_VARIABLE;
+      case FIELD:
+        return position.type == TargetType.FIELD;
+      case METHOD:
+        return position.type == TargetType.METHOD_RETURN;
+      case PARAMETER:
+        switch (position.type) {
+          case METHOD_FORMAL_PARAMETER:
+            return ((MethodSymbol) sym.owner).getParameters().indexOf(sym)
+                == position.parameter_index;
+          default:
+            return false;
+        }
+      default:
+        throw new AssertionError(sym.getKind());
+    }
+  }
+
+  private MoreAnnotations() {}
+}