Change SuggestedFixes.{addModifiers,removeModifiers} to return an Opt…

…ional.

RELNOTES: Changed SuggestedFixes.{addModifiers,removeModifiers} to return
an Optional.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=173177755

check_api/src/main/java/com/google/errorprone/bugpatterns/BugChecker.java

@@ -86,6 +86,7 @@
 import java.util.Collections;
 import java.util.Map;
 import java.util.Objects;
+import java.util.Optional;
 import java.util.Set;
 import javax.annotation.CheckReturnValue;
 
@@ -117,6 +118,12 @@ protected Description describeMatch(Tree node) {
     return buildDescription(node).build();
   }
 
+  /** Helper to create a Description for the common case where there is an {@link Optional} fix. */
+  @CheckReturnValue
+  protected Description describeMatch(Tree node, Optional<? extends Fix> fix) {
+    return buildDescription(node).addFix(fix).build();
+  }
+
   /**
    * Returns a Description builder, which allows you to customize the diagnostic with a custom
    * message or multiple fixes.

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

@@ -29,7 +29,6 @@
 
 import com.google.common.base.Function;
 import com.google.common.base.Joiner;
-import com.google.common.base.Optional;
 import com.google.common.base.Predicates;
 import com.google.common.collect.FluentIterable;
 import com.google.common.collect.ImmutableList;
@@ -85,6 +84,7 @@
 import java.util.Deque;
 import java.util.List;
 import java.util.Map;
+import java.util.Optional;
 import java.util.Set;
 import java.util.TreeMap;
 import java.util.TreeSet;
@@ -139,12 +139,12 @@ private static Modifier getTokModifierKind(ErrorProneToken tok) {
     }
   }
 
-  /** Add modifiers to the given class, method, or field declaration. */
-  @Nullable
-  public static SuggestedFix addModifiers(Tree tree, VisitorState state, Modifier... modifiers) {
+  /** Adds modifiers to the given class, method, or field declaration. */
+  public static Optional<SuggestedFix> addModifiers(
+      Tree tree, VisitorState state, Modifier... modifiers) {
     ModifiersTree originalModifiers = getModifiers(tree);
     if (originalModifiers == null) {
-      return null;
+      return Optional.empty();
     }
     Set<Modifier> toAdd =
         Sets.difference(new TreeSet<>(Arrays.asList(modifiers)), originalModifiers.getFlags());
@@ -154,15 +154,16 @@ public static SuggestedFix addModifiers(Tree tree, VisitorState state, Modifier.
               ? state.getEndPosition(originalModifiers) + 1
               : ((JCTree) tree).getStartPosition();
       int base = ((JCTree) tree).getStartPosition();
-      java.util.Optional<Integer> insert =
+      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 SuggestedFix.replace(insertPos, insertPos, Joiner.on(' ').join(toAdd) + " ");
+      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
@@ -194,16 +195,16 @@ public static SuggestedFix addModifiers(Tree tree, VisitorState state, Modifier.
     if (!modifiersToWrite.isEmpty()) {
       fix.postfixWith(originalModifiers, " " + Joiner.on(' ').join(modifiersToWrite));
     }
-    return fix.build();
+    return Optional.of(fix.build());
   }
 
   /** Remove modifiers from the given class, method, or field declaration. */
-  @Nullable
-  public static SuggestedFix removeModifiers(Tree tree, VisitorState state, Modifier... modifiers) {
+  public static Optional<SuggestedFix> removeModifiers(
+      Tree tree, VisitorState state, Modifier... modifiers) {
     Set<Modifier> toRemove = ImmutableSet.copyOf(modifiers);
     ModifiersTree originalModifiers = getModifiers(tree);
     if (originalModifiers == null) {
-      return null;
+      return Optional.empty();
     }
     SuggestedFix.Builder fix = SuggestedFix.builder();
     List<ErrorProneToken> tokens = state.getTokensForNode(originalModifiers);
@@ -218,9 +219,9 @@ public static SuggestedFix removeModifiers(Tree tree, VisitorState state, Modifi
       }
     }
     if (empty) {
-      return null;
+      return Optional.empty();
     }
-    return fix.build();
+    return Optional.of(fix.build());
   }
 
   /**
@@ -607,7 +608,7 @@ private static Optional<ExpressionTree> findArgument(
         }
       }
     }
-    return Optional.absent();
+    return Optional.empty();
   }
 
   /**

check_api/src/main/java/com/google/errorprone/matchers/Description.java

@@ -16,7 +16,7 @@
 
 package com.google.errorprone.matchers;
 
-import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.base.Strings.isNullOrEmpty;
 import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION;
 
@@ -29,6 +29,7 @@
 import com.google.errorprone.fixes.Fix;
 import com.sun.source.tree.Tree;
 import java.util.List;
+import java.util.Optional;
 import javax.annotation.CheckReturnValue;
 import javax.annotation.Nullable;
 
@@ -178,21 +179,34 @@ private Builder(
      * @throws IllegalArgumentException if {@code fix} is {@code null}
      */
     public Builder addFix(Fix fix) {
-      checkArgument(fix != null, "fix must not be null");
+      checkNotNull(fix, "fix must not be null");
       if (!fix.isEmpty()) {
         fixListBuilder.add(fix);
       }
       return this;
     }
 
+    /**
+     * Adds a suggested fix for this {@code Description} if {@code fix} is present. Fixes should be
+     * added in order of decreasing preference. Adding an empty fix is a no-op.
+     *
+     * @param fix a suggested fix for this problem
+     * @throws IllegalArgumentException if {@code fix} is {@code null}
+     */
+    public Builder addFix(Optional<? extends Fix> fix) {
+      checkNotNull(fix, "fix must not be null");
+      fix.ifPresent(this::addFix);
+      return this;
+    }
+
     /**
      * Add each fix in order.
      *
      * @param fixes a list of suggested fixes for this problem
      * @throws IllegalArgumentException if {@code fixes} or any of its elements are {@code null}
      */
     public Builder addAllFixes(List<? extends Fix> fixes) {
-      checkArgument(fixes != null, "fixes must not be null");
+      checkNotNull(fixes, "fixes must not be null");
       for (Fix fix : fixes) {
         addFix(fix);
       }

core/src/main/java/com/google/errorprone/bugpatterns/JUnit3TestNotRun.java

@@ -119,12 +119,12 @@ public Description matchMethod(MethodTree methodTree, VisitorState state) {
     }
 
     // Make method public (if not already public).
-    fixes.add(addModifiers(methodTree, state, Modifier.PUBLIC));
+    addModifiers(methodTree, state, Modifier.PUBLIC).ifPresent(fixes::add);
     // Remove any other visibility modifiers (if present).
-    fixes.add(removeModifiers(methodTree, state, Modifier.PRIVATE, Modifier.PROTECTED));
+    removeModifiers(methodTree, state, Modifier.PRIVATE, Modifier.PROTECTED).ifPresent(fixes::add);
     // Remove static modifier (if present).
     // N.B. must occur in separate step because removeModifiers only removes one modifier at a time.
-    fixes.add(removeModifiers(methodTree, state, Modifier.STATIC));
+    removeModifiers(methodTree, state, Modifier.STATIC).ifPresent(fixes::add);
 
     return describeMatch(methodTree, mergeFixes(fixes));
   }

core/src/main/java/com/google/errorprone/bugpatterns/JUnit4TestNotRun.java

@@ -48,6 +48,7 @@
 import com.sun.source.util.TreeScanner;
 import com.sun.tools.javac.code.Symbol.MethodSymbol;
 import java.util.List;
+import java.util.Optional;
 import javax.lang.model.element.Modifier;
 
 /** @author eaftan@google.com (Eddie Aftandilian) */
@@ -178,10 +179,11 @@ public Boolean reduce(Boolean r1, Boolean r2) {
    * </ol>
    */
   private Description describeFixes(MethodTree methodTree, VisitorState state) {
-    SuggestedFix removeStatic = SuggestedFixes.removeModifiers(methodTree, state, Modifier.STATIC);
+    Optional<SuggestedFix> removeStatic =
+        SuggestedFixes.removeModifiers(methodTree, state, Modifier.STATIC);
     SuggestedFix testFix =
         SuggestedFix.builder()
-            .merge(removeStatic)
+            .merge(removeStatic.orElse(null))
             .addImport(TEST_CLASS)
             .prefixWith(methodTree, TEST_ANNOTATION)
             .build();
@@ -194,8 +196,8 @@ private Description describeFixes(MethodTree methodTree, VisitorState state) {
 
     SuggestedFix visibilityFix =
         SuggestedFix.builder()
-            .merge(SuggestedFixes.removeModifiers(methodTree, state, Modifier.PUBLIC))
-            .merge(SuggestedFixes.addModifiers(methodTree, state, Modifier.PRIVATE))
+            .merge(SuggestedFixes.removeModifiers(methodTree, state, Modifier.PUBLIC).orElse(null))
+            .merge(SuggestedFixes.addModifiers(methodTree, state, Modifier.PRIVATE).orElse(null))
             .build();
 
     // Suggest @Ignore first if test method is named like a purposely disabled test.

core/src/main/java/com/google/errorprone/bugpatterns/VarChecker.java

@@ -38,6 +38,7 @@
 import com.sun.tools.javac.code.Symbol;
 import java.util.Collections;
 import java.util.EnumSet;
+import java.util.Optional;
 import javax.lang.model.element.Modifier;
 
 /** @author cushon@google.com (Liam Miller-Cushon) */
@@ -91,10 +92,10 @@ private Description handleLocalOrParam(VariableTree tree, VisitorState state, Sy
       if (Source.instance(state.context).allowEffectivelyFinalInInnerClasses()) {
         // In Java 8, the final modifier is never necessary on locals/parameters because
         // effectively final variables can be used anywhere a final variable is required.
-        Fix fix = SuggestedFixes.removeModifiers(tree, state, Modifier.FINAL);
-        // The fix may be null for TWR variables that were not explicitly final
-        if (fix != null) {
-          return buildDescription(tree).setMessage(UNNECESSARY_FINAL).addFix(fix).build();
+        Optional<SuggestedFix> fix = SuggestedFixes.removeModifiers(tree, state, Modifier.FINAL);
+        // The fix may not be present for TWR variables that were not explicitly final
+        if (fix.isPresent()) {
+          return buildDescription(tree).setMessage(UNNECESSARY_FINAL).addFix(fix.get()).build();
         }
       }
       return Description.NO_MATCH;

core/src/main/java/com/google/errorprone/bugpatterns/threadsafety/DoubleCheckedLocking.java

@@ -28,7 +28,6 @@
 import com.google.errorprone.VisitorState;
 import com.google.errorprone.bugpatterns.BugChecker;
 import com.google.errorprone.bugpatterns.BugChecker.IfTreeMatcher;
-import com.google.errorprone.fixes.Fix;
 import com.google.errorprone.fixes.SuggestedFixes;
 import com.google.errorprone.matchers.Description;
 import com.google.errorprone.util.ASTHelpers;
@@ -98,10 +97,7 @@ private Description handleField(IfTree outerIf, VarSymbol sym, VisitorState stat
     Description.Builder builder = buildDescription(outerIf);
     JCTree fieldDecl = findFieldDeclaration(state.getPath(), sym);
     if (fieldDecl != null) {
-      Fix fix = SuggestedFixes.addModifiers(fieldDecl, state, Modifier.VOLATILE);
-      if (fix != null) {
-        builder.addFix(fix);
-      }
+      builder.addFix(SuggestedFixes.addModifiers(fieldDecl, state, Modifier.VOLATILE));
     }
     return builder.build();
   }

core/src/test/java/com/google/errorprone/fixes/SuggestedFixesTest.java

@@ -29,6 +29,7 @@
 import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode;
 import com.google.errorprone.BugPattern;
 import com.google.errorprone.BugPattern.Category;
+import com.google.errorprone.BugPattern.ProvidesFix;
 import com.google.errorprone.BugPattern.SeverityLevel;
 import com.google.errorprone.CompilationTestHelper;
 import com.google.errorprone.VisitorState;
@@ -55,6 +56,7 @@
 import com.sun.tools.javac.tree.JCTree;
 import java.io.IOException;
 import java.lang.annotation.Retention;
+import java.util.Optional;
 import javax.lang.model.element.Modifier;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -111,7 +113,7 @@ private Description editModifiers(Tree tree, VisitorState state) {
               ASTHelpers.findEnclosingNode(state.getPath(), ClassTree.class), EditModifiers.class);
       Modifier mod = MODIFIERS_BY_NAME.get(editModifiers.value());
       Verify.verifyNotNull(mod, editModifiers.value());
-      Fix fix;
+      Optional<SuggestedFix> fix;
       switch (editModifiers.kind()) {
         case ADD:
           fix = SuggestedFixes.addModifiers(tree, state, mod);
@@ -234,7 +236,8 @@ public void removeModifiers() {
     category = Category.ONE_OFF,
     name = "CastReturn",
     severity = SeverityLevel.ERROR,
-    summary = "Adds casts to returned expressions"
+    summary = "Adds casts to returned expressions",
+    providesFix = ProvidesFix.REQUIRES_HUMAN_ATTENTION
   )
   public static class CastReturn extends BugChecker implements ReturnTreeMatcher {
 
@@ -257,7 +260,8 @@ public Description matchReturn(ReturnTree tree, VisitorState state) {
     category = Category.ONE_OFF,
     name = "CastReturn",
     severity = SeverityLevel.ERROR,
-    summary = "Adds casts to returned expressions"
+    summary = "Adds casts to returned expressions",
+    providesFix = ProvidesFix.REQUIRES_HUMAN_ATTENTION
   )
   public static class CastReturnFullType extends BugChecker implements ReturnTreeMatcher {
 
@@ -395,7 +399,8 @@ public void fullQualifiedName_typeVariable() {
     name = "JavadocQualifier",
     category = BugPattern.Category.JDK,
     summary = "all javadoc links should be qualified",
-    severity = ERROR
+    severity = ERROR,
+    providesFix = ProvidesFix.REQUIRES_HUMAN_ATTENTION
   )
   public static class JavadocQualifier extends BugChecker implements BugChecker.ClassTreeMatcher {
     @Override
@@ -445,7 +450,13 @@ public void qualifyJavadocTest() throws Exception {
         .doTest(TEXT_MATCH);
   }
 
-  @BugPattern(name = "SuppressMe", category = Category.ONE_OFF, summary = "", severity = ERROR)
+  @BugPattern(
+    name = "SuppressMe",
+    category = Category.ONE_OFF,
+    summary = "",
+    severity = ERROR,
+    providesFix = ProvidesFix.REQUIRES_HUMAN_ATTENTION
+  )
   static final class SuppressMe extends BugChecker implements LiteralTreeMatcher {
     @Override
     public Description matchLiteral(LiteralTree tree, VisitorState state) {
@@ -497,7 +508,13 @@ public void testSuppressWarningsFix() throws IOException {
   }
 
   /** A test bugchecker that deletes any field whose removal doesn't break the compilation. */
-  @BugPattern(name = "CompilesWithFixChecker", category = JDK, summary = "", severity = ERROR)
+  @BugPattern(
+    name = "CompilesWithFixChecker",
+    category = JDK,
+    summary = "",
+    severity = ERROR,
+    providesFix = ProvidesFix.REQUIRES_HUMAN_ATTENTION
+  )
   public static class CompilesWithFixChecker extends BugChecker implements VariableTreeMatcher {
     @Override
     public Description matchVariable(VariableTree tree, VisitorState state) {
@@ -532,7 +549,13 @@ public void compilesWithFixTest() throws IOException {
   }
 
   /** A test bugchecker that deletes an exception from throws. */
-  @BugPattern(name = "RemovesExceptionChecker", category = JDK, summary = "", severity = ERROR)
+  @BugPattern(
+    name = "RemovesExceptionChecker",
+    category = JDK,
+    summary = "",
+    severity = ERROR,
+    providesFix = ProvidesFix.REQUIRES_HUMAN_ATTENTION
+  )
   public static class RemovesExceptionsChecker extends BugChecker implements MethodTreeMatcher {
 
     private final int index;