Moe Sync

annotations/src/main/java/com/google/errorprone/annotations/DoNotMock.java

@@ -0,0 +1,47 @@
+/*
+ * Copyright 2016 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.annotations;
+
+import java.lang.annotation.Documented;
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Inherited;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+
+/**
+ * Annotation representing a type that should not be mocked.
+ *
+ * <p>When marking a type {@code @DoNotMock}, you should always point to alternative testing
+ * solutions such as standard fakes or other testing utilities.
+ *
+ * <p>Mockito tests can enforce this annotation by using a custom MockMaker which intercepts
+ * creation of mocks.
+ *
+ */
+@Inherited
+@Documented
+@Retention(RetentionPolicy.RUNTIME)
+@Target({ElementType.TYPE, ElementType.ANNOTATION_TYPE})
+public @interface DoNotMock {
+  /**
+   * The reason why the annotated type should not be mocked.
+   *
+   * <p>This should suggest alternative APIs to use for testing objects of this type.
+   */
+  String value() default "Create a real instance instead";
+}

check_api/src/main/java/com/google/errorprone/VisitorState.java

@@ -72,7 +72,7 @@ public class VisitorState {
 
   // The default no-op implementation of DescriptionListener. We use this instead of null so callers
   // of getDescriptionListener() don't have to do null-checking.
-  private static final DescriptionListener NULL_LISTENER = description -> {};
+  private static void nullListener(Description description) {}
 
   /**
    * Return a VisitorState that has no Error Prone configuration, and can't report results.
@@ -82,7 +82,7 @@ public class VisitorState {
   public static VisitorState createForUtilityPurposes(Context context) {
     return new VisitorState(
         context,
-        NULL_LISTENER,
+        VisitorState::nullListener,
         ImmutableMap.of(),
         ErrorProneOptions.empty(),
         // Can't use this VisitorState to report results, so no-op collector.
@@ -138,7 +138,7 @@ public static VisitorState createConfiguredForCompilation(
   public VisitorState(Context context) {
     this(
         context,
-        NULL_LISTENER,
+        VisitorState::nullListener,
         ImmutableMap.of(),
         ErrorProneOptions.empty(),
         // Can't use this VisitorState to report results, so no-op collector.

check_api/src/main/java/com/google/errorprone/apply/ImportOrganizer.java

@@ -114,7 +114,7 @@ static Import importOf(String importString) {
     }
 
     @Override
-    public String toString() {
+    public final String toString() {
       return String.format("import%s %s", isStatic() ? " static" : "", getType());
     }
   }

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

@@ -113,6 +113,18 @@ protected Description describeMatch(Tree node, Fix fix) {
     return buildDescription(node).addFix(fix).build();
   }
 
+  /** Helper to create a Description for the common case where there is a fix. */
+  @CheckReturnValue
+  protected Description describeMatch(JCTree node, Fix fix) {
+    return describeMatch((Tree) node, fix);
+  }
+
+  /** Helper to create a Description for the common case where there is a fix. */
+  @CheckReturnValue
+  protected Description describeMatch(DiagnosticPosition position, Fix fix) {
+    return buildDescription(position).addFix(fix).build();
+  }
+
   /** Helper to create a Description for the common case where there is no fix. */
   @CheckReturnValue
   protected Description describeMatch(Tree node) {

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

@@ -160,8 +160,13 @@ public static Optional<SuggestedFix> addModifiers(
     if (originalModifiers == null) {
       return Optional.empty();
     }
-    Set<Modifier> toAdd =
-        Sets.difference(new TreeSet<>(Arrays.asList(modifiers)), originalModifiers.getFlags());
+    return addModifiers(tree, originalModifiers, state, new TreeSet<>(Arrays.asList(modifiers)));
+  }
+
+  /** Adds modifiers to the given declaration and corresponding modifiers tree. */
+  public static Optional<SuggestedFix> addModifiers(
+      Tree tree, ModifiersTree originalModifiers, VisitorState state, Set<Modifier> modifiers) {
+    Set<Modifier> toAdd = Sets.difference(modifiers, originalModifiers.getFlags());
     SuggestedFix.Builder fix = SuggestedFix.builder();
     List<Modifier> modifiersToWrite = new ArrayList<>();
     if (!originalModifiers.getFlags().isEmpty()) {
@@ -228,6 +233,12 @@ public static Optional<SuggestedFix> removeModifiers(
     if (originalModifiers == null) {
       return Optional.empty();
     }
+    return removeModifiers(originalModifiers, state, toRemove);
+  }
+
+  /** Adds modifiers to the given declaration and corresponding modifiers tree. */
+  public static Optional<SuggestedFix> removeModifiers(
+      ModifiersTree originalModifiers, VisitorState state, Set<Modifier> toRemove) {
     SuggestedFix.Builder fix = SuggestedFix.builder();
     List<ErrorProneToken> tokens = state.getTokensForNode(originalModifiers);
     int basePos = ((JCTree) originalModifiers).getStartPosition();
@@ -436,6 +447,17 @@ public void visitIdent(JCTree.JCIdent tree) {
                   fix.replace(tree, replacement);
                 }
               }
+
+              @Override
+              public void visitSelect(JCTree.JCFieldAccess tree) {
+                if (sym.equals(getSymbol(tree))) {
+                  fix.replace(
+                      state.getEndPosition(tree.getExpression()),
+                      state.getEndPosition(tree),
+                      "." + replacement);
+                }
+                super.visitSelect(tree);
+              }
             });
     return fix.build();
   }

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

@@ -52,7 +52,7 @@ public static enum Visibility {
     DEFAULT(null),
     PRIVATE(Modifier.PRIVATE);
 
-    private Modifier correspondingModifier;
+    private final Modifier correspondingModifier;
 
     Visibility(Modifier correspondingModifier) {
       this.correspondingModifier = correspondingModifier;

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

@@ -87,15 +87,23 @@ public String toString() {
    */
   public static String prettyMethodSignature(ClassSymbol origin, MethodSymbol m) {
     StringBuilder sb = new StringBuilder();
-    if (!m.owner.equals(origin)) {
-      sb.append(m.owner.getSimpleName()).append('.');
+    if (m.isConstructor()) {
+      Name name = m.owner.enclClass().getSimpleName();
+      if (name.isEmpty()) {
+        // use the superclass name of anonymous classes
+        name = m.owner.enclClass().getSuperclass().asElement().getSimpleName();
+      }
+      sb.append(name);
+    } else {
+      if (!m.owner.equals(origin)) {
+        sb.append(m.owner.getSimpleName()).append('.');
+      }
+      sb.append(m.getSimpleName());
     }
-    sb.append(m.isConstructor() ? origin.getSimpleName() : m.getSimpleName()).append('(');
     sb.append(
         m.getParameters().stream()
             .map(v -> v.type.accept(PRETTY_TYPE_VISITOR, null))
-            .collect(joining(", ")));
-    sb.append(')');
+            .collect(joining(", ", "(", ")")));
     return sb.toString();
   }
 

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

@@ -104,13 +104,12 @@ private Description checkClosed(ExpressionTree tree, VisitorState state) {
             }
             // The caller method is not annotated, so the closing of the returned resource is not
             // enforced. Suggest fixing this by annotating the caller method.
-            return buildDescription(tree)
-                .addFix(
-                    SuggestedFix.builder()
-                        .prefixWith(callerMethodTree, "@MustBeClosed\n")
-                        .addImport(MustBeClosed.class.getCanonicalName())
-                        .build())
-                .build();
+            return describeMatch(
+                tree,
+                SuggestedFix.builder()
+                    .prefixWith(callerMethodTree, "@MustBeClosed\n")
+                    .addImport(MustBeClosed.class.getCanonicalName())
+                    .build());
           }
           break;
         case CONDITIONAL_EXPRESSION:

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

@@ -52,7 +52,10 @@
     summary =
         "This method passes a pair of parameters through to String.format, but the enclosing "
             + "method wasn't annotated @FormatMethod. Doing so gives compile-time rather than "
-            + "run-time protection against malformed format strings.",
+            + "run-time protection against malformed format strings.\n\n"
+            + "WARNING: There's a very high chance that existing code will not be passing in "
+            + "well-formed format strings. Make sure you run tests including all users of "
+            + "this code before submitting.",
     severity = WARNING,
     tags = FRAGILE_CODE,
     providesFix = REQUIRES_HUMAN_ATTENTION)
@@ -99,15 +102,14 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState
     boolean fixable =
         formatParameter.get().equals(enclosingParameters.get(enclosingParameters.size() - 2));
     if (fixable) {
-      return buildDescription(enclosingMethod)
-          .addFix(
-              SuggestedFix.builder()
-                  .prefixWith(enclosingMethod, "@FormatMethod ")
-                  .prefixWith(formatParameter.get(), "@FormatString ")
-                  .addImport("com.google.errorprone.annotations.FormatMethod")
-                  .addImport("com.google.errorprone.annotations.FormatString")
-                  .build())
-          .build();
+      return describeMatch(
+          enclosingMethod,
+          SuggestedFix.builder()
+              .prefixWith(enclosingMethod, "@FormatMethod ")
+              .prefixWith(formatParameter.get(), "@FormatString ")
+              .addImport("com.google.errorprone.annotations.FormatMethod")
+              .addImport("com.google.errorprone.annotations.FormatString")
+              .build());
     }
     return buildDescription(enclosingMethod).setMessage(message() + REORDER).build();
   }

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

@@ -29,7 +29,6 @@
 import com.google.errorprone.fixes.SuggestedFixes;
 import com.google.errorprone.matchers.Description;
 import com.google.errorprone.matchers.MultiMatcher;
-import com.google.errorprone.matchers.MultiMatcher.MultiMatchResult;
 import com.google.errorprone.util.ASTHelpers;
 import com.sun.source.tree.AnnotationTree;
 import com.sun.source.tree.ClassTree;
@@ -193,19 +192,31 @@ private void moveTypeAnnotations(IdentifierTree node) {
               case METHOD:
               case VARIABLE:
               case ANNOTATED_TYPE:
-                MultiMatchResult<AnnotationTree> matchResult =
-                    HAS_TYPE_USE_ANNOTATION.multiMatchResult(parent, state);
-                if (matchResult.matches()) {
-                  for (AnnotationTree annotation : matchResult.matchingNodes()) {
-                    builder.delete(annotation);
-                    builder.prefixWith(node, state.getSourceForNode(annotation) + " ");
-                  }
+                moveTypeAnnotations(node, parent, state, builder);
+                break;
+              case PARAMETERIZED_TYPE:
+                Tree grandParent = getCurrentPath().getParentPath().getParentPath().getLeaf();
+                if (grandParent.getKind() == Kind.VARIABLE
+                    || grandParent.getKind() == Kind.METHOD) {
+                  moveTypeAnnotations(node, grandParent, state, builder);
                 }
                 break;
               default:
                 // Do nothing.
             }
           }
+
+          private void moveTypeAnnotations(
+              IdentifierTree node,
+              Tree annotationHolder,
+              VisitorState state,
+              SuggestedFix.Builder builder) {
+            for (AnnotationTree annotation :
+                HAS_TYPE_USE_ANNOTATION.multiMatchResult(annotationHolder, state).matchingNodes()) {
+              builder.delete(annotation);
+              builder.prefixWith(node, state.getSourceForNode(annotation) + " ");
+            }
+          }
         }.scan(path, null);
     if (firstFound == null) {
       // If no usage of the symbol was found, just leave the import to be cleaned up by the unused

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

@@ -61,13 +61,12 @@ public class BigDecimalLiteralDouble extends BugChecker implements NewClassTreeM
   // Matches literals and unary +/- followed by a literal, since most people conceptually think of
   // -1.0 as a literal. Doesn't handle nested unary operators as new BigDecimal(String) doesn't
   // accept multiple unary prefixes.
-  private static final Matcher<ExpressionTree> FLOATING_POINT_ARGUMENT =
-      (tree, state) -> {
-        if (tree.getKind() == Kind.UNARY_PLUS || tree.getKind() == Kind.UNARY_MINUS) {
-          tree = ((UnaryTree) tree).getExpression();
-        }
-        return tree.getKind() == Kind.DOUBLE_LITERAL || tree.getKind() == Kind.FLOAT_LITERAL;
-      };
+  private static boolean floatingPointArgument(ExpressionTree tree) {
+    if (tree.getKind() == Kind.UNARY_PLUS || tree.getKind() == Kind.UNARY_MINUS) {
+      tree = ((UnaryTree) tree).getExpression();
+    }
+    return tree.getKind() == Kind.DOUBLE_LITERAL || tree.getKind() == Kind.FLOAT_LITERAL;
+  }
 
   @Override
   public Description matchNewClass(NewClassTree tree, VisitorState state) {
@@ -76,7 +75,7 @@ public Description matchNewClass(NewClassTree tree, VisitorState state) {
     }
 
     ExpressionTree arg = getOnlyElement(tree.getArguments());
-    if (!FLOATING_POINT_ARGUMENT.matches(arg, state)) {
+    if (!floatingPointArgument(arg)) {
       return Description.NO_MATCH;
     }
 

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

@@ -0,0 +1,70 @@
+/*
+ * Copyright 2019 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.bugpatterns;
+
+import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
+
+import com.google.errorprone.BugPattern;
+import com.google.errorprone.VisitorState;
+import com.google.errorprone.util.ASTHelpers;
+import com.sun.source.tree.ExpressionTree;
+import com.sun.tools.javac.code.Flags;
+import com.sun.tools.javac.code.Symbol;
+import com.sun.tools.javac.code.Type;
+
+/** @author [email protected] (Liam Miller-Cushon) */
+@BugPattern(
+    name = "BoxedPrimitiveEquality",
+    summary =
+        "Comparison using reference equality instead of value equality. Reference equality of"
+            + " boxed primitive types is usually not useful, as they are value objects, and it is"
+            + " bug-prone, as instances are cached for some values but not others.",
+    severity = WARNING)
+public class BoxedPrimitiveEquality extends AbstractReferenceEquality {
+
+  @Override
+  protected boolean matchArgument(ExpressionTree tree, VisitorState state) {
+    Type type = ASTHelpers.getType(tree);
+    if (type == null) {
+      return false;
+    }
+    switch (state.getTypes().unboxedType(type).getTag()) {
+      case BYTE:
+      case CHAR:
+      case SHORT:
+      case INT:
+      case LONG:
+      case FLOAT:
+      case DOUBLE:
+      case BOOLEAN:
+        break;
+      default:
+        return false;
+    }
+    Symbol sym = ASTHelpers.getSymbol(tree);
+    if (sym instanceof Symbol.VarSymbol && isFinal(sym) && sym.isStatic()) {
+      // Using a static final field as a sentinel is OK
+      // TODO(cushon): revisit this assumption carried over from NumericEquality
+      return false;
+    }
+    return true;
+  }
+
+  public static boolean isFinal(Symbol s) {
+    return (s.flags() & Flags.FINAL) == Flags.FINAL;
+  }
+}