Add a test confirming that ArgumentSelectionDefectChecker works on re…

…cord construction, but _not_ record deconstruction.

PiperOrigin-RevId: 691430462

core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/ArgumentSelectionDefectChecker.java

@@ -31,7 +31,6 @@
 import com.sun.source.tree.MethodInvocationTree;
 import com.sun.source.tree.NewClassTree;
 import com.sun.tools.javac.code.Symbol.MethodSymbol;
-import java.util.function.Function;
 
 /**
  * Checks the lexical distance between method parameter names and the argument names at call sites.
@@ -62,7 +61,7 @@ public class ArgumentSelectionDefectChecker extends BugChecker
   public ArgumentSelectionDefectChecker() {
     this(
         ArgumentChangeFinder.builder()
-            .setDistanceFunction(buildDefaultDistanceFunction())
+            .setDistanceFunction(ArgumentSelectionDefectChecker::defaultDistanceFunction)
             .addHeuristic(new LowInformationNameHeuristic())
             .addHeuristic(new PenaltyThresholdHeuristic())
             .addHeuristic(new EnclosedByReverseHeuristic())
@@ -94,7 +93,7 @@ public Description matchNewClass(NewClassTree tree, VisitorState state) {
     MethodSymbol symbol = ASTHelpers.getSymbol(tree);
 
     // Don't return a match if the AutoValueConstructorOrderChecker would match it too
-    if (Matchers.AUTOVALUE_CONSTRUCTOR.matches(tree, state)) {
+    if (Matchers.isAutoValueConstructor(tree)) {
       return Description.NO_MATCH;
     }
 
@@ -128,30 +127,23 @@ private Description visitNewClassOrMethodInvocation(InvocationInfo invocationInf
    * normalised NeedlemanWunschEditDistance. Otherwise, one of the names is unknown and so we return
    * 0 distance between it and its original parameter and infinite distance between all others.
    */
-  private static Function<ParameterPair, Double> buildDefaultDistanceFunction() {
-    return new Function<ParameterPair, Double>() {
-      @Override
-      public Double apply(ParameterPair pair) {
-        if (pair.formal().isNullLiteral() || pair.actual().isNullLiteral()) {
-          return 0.0;
-        }
-
-        if (!pair.formal().isUnknownName() && !pair.actual().isUnknownName()) {
-          String normalizedSource =
-              NamingConventions.convertToLowerUnderscore(pair.formal().name());
-          String normalizedTarget =
-              NamingConventions.convertToLowerUnderscore(pair.actual().name());
-          return NeedlemanWunschEditDistance.getNormalizedEditDistance(
-              /* source= */ normalizedSource,
-              /* target= */ normalizedTarget,
-              /* caseSensitive= */ false,
-              /* changeCost= */ 8,
-              /* openGapCost= */ 8,
-              /* continueGapCost= */ 1);
-        }
-
-        return pair.formal().index() == pair.actual().index() ? 0.0 : Double.POSITIVE_INFINITY;
-      }
-    };
+  private static double defaultDistanceFunction(ParameterPair pair) {
+    if (pair.formal().isNullLiteral() || pair.actual().isNullLiteral()) {
+      return 0.0;
+    }
+
+    if (!pair.formal().isUnknownName() && !pair.actual().isUnknownName()) {
+      String normalizedSource = NamingConventions.convertToLowerUnderscore(pair.formal().name());
+      String normalizedTarget = NamingConventions.convertToLowerUnderscore(pair.actual().name());
+      return NeedlemanWunschEditDistance.getNormalizedEditDistance(
+          /* source= */ normalizedSource,
+          /* target= */ normalizedTarget,
+          /* caseSensitive= */ false,
+          /* changeCost= */ 8,
+          /* openGapCost= */ 8,
+          /* continueGapCost= */ 1);
+    }
+
+    return pair.formal().index() == pair.actual().index() ? 0.0 : Double.POSITIVE_INFINITY;
   }
 }

core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/AssertEqualsArgumentOrderChecker.java

@@ -43,7 +43,7 @@
  * @author [email protected] (Andrew Rice)
  */
 @BugPattern(summary = "Arguments are swapped in assertEquals-like call", severity = WARNING)
-public class AssertEqualsArgumentOrderChecker extends BugChecker
+public final class AssertEqualsArgumentOrderChecker extends BugChecker
     implements MethodInvocationTreeMatcher {
 
   private final ArgumentChangeFinder argumentchangeFinder =

core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/AutoValueConstructorOrderChecker.java

@@ -41,7 +41,8 @@
  * @author [email protected] (Andrew Rice)
  */
 @BugPattern(summary = "Arguments to AutoValue constructor are in the wrong order", severity = ERROR)
-public class AutoValueConstructorOrderChecker extends BugChecker implements NewClassTreeMatcher {
+public final class AutoValueConstructorOrderChecker extends BugChecker
+    implements NewClassTreeMatcher {
 
   private final ArgumentChangeFinder argumentChangeFinder =
       ArgumentChangeFinder.builder()
@@ -52,7 +53,7 @@ public class AutoValueConstructorOrderChecker extends BugChecker implements NewC
 
   @Override
   public Description matchNewClass(NewClassTree tree, VisitorState state) {
-    if (!Matchers.AUTOVALUE_CONSTRUCTOR.matches(tree, state)) {
+    if (!Matchers.isAutoValueConstructor(tree)) {
       return Description.NO_MATCH;
     }
 

core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/Costs.java

@@ -30,7 +30,7 @@
  *
  * @author [email protected] (Andrew Rice)
  */
-class Costs {
+final class Costs {
 
   /** Formal parameters for the method being called. */
   private final ImmutableList<Parameter> formals;

core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/CreatesDuplicateCallHeuristic.java

@@ -35,7 +35,7 @@
  *
  * @author [email protected] (Andrew Rice)
  */
-class CreatesDuplicateCallHeuristic implements Heuristic {
+final class CreatesDuplicateCallHeuristic implements Heuristic {
 
   /**
    * Returns true if there are no other calls to this method which already have an actual parameter

core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/EnclosedByReverseHeuristic.java

@@ -16,9 +16,12 @@
 
 package com.google.errorprone.bugpatterns.argumentselectiondefects;
 
+import static com.google.common.collect.Streams.stream;
+import static com.google.errorprone.names.NamingConventions.splitToLowercaseTerms;
+import static java.util.Collections.disjoint;
+
 import com.google.common.collect.ImmutableSet;
 import com.google.errorprone.VisitorState;
-import com.google.errorprone.names.NamingConventions;
 import com.sun.source.tree.ClassTree;
 import com.sun.source.tree.MethodTree;
 import com.sun.source.tree.Tree;
@@ -32,7 +35,7 @@
  *
  * @author [email protected] (Andrew Rice)
  */
-class EnclosedByReverseHeuristic implements Heuristic {
+final class EnclosedByReverseHeuristic implements Heuristic {
 
   private static final ImmutableSet<String> DEFAULT_REVERSE_WORDS_TERMS =
       ImmutableSet.of(
@@ -75,18 +78,12 @@ public boolean isAcceptableChange(
     return findReverseWordsMatchInParentNodes(state) == null;
   }
 
-  protected @Nullable String findReverseWordsMatchInParentNodes(VisitorState state) {
-    for (Tree tree : state.getPath()) {
-      Optional<String> name = getName(tree);
-      if (name.isPresent()) {
-        for (String term : NamingConventions.splitToLowercaseTerms(name.get())) {
-          if (reverseWordsTerms.contains(term)) {
-            return term;
-          }
-        }
-      }
-    }
-    return null;
+  private @Nullable String findReverseWordsMatchInParentNodes(VisitorState state) {
+    return stream(state.getPath())
+        .flatMap(t -> getName(t).stream())
+        .filter(n -> !disjoint(splitToLowercaseTerms(n), reverseWordsTerms))
+        .findFirst()
+        .orElse(null);
   }
 
   private static Optional<String> getName(Tree tree) {

core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/LowInformationNameHeuristic.java

@@ -29,7 +29,7 @@
  *
  * @author [email protected] (Andrew Rice)
  */
-class LowInformationNameHeuristic implements Heuristic {
+final class LowInformationNameHeuristic implements Heuristic {
 
   private static final ImmutableSet<String> DEFAULT_FORMAL_PARAMETER_EXCLUSION_REGEXS =
       ImmutableSet.of(
@@ -60,11 +60,9 @@ public boolean isAcceptableChange(
    * parameter name.
    */
   protected @Nullable String findMatch(Parameter parameter) {
-    for (String regex : overloadedNamesRegexs) {
-      if (parameter.name().matches(regex)) {
-        return regex;
-      }
-    }
-    return null;
+    return overloadedNamesRegexs.stream()
+        .filter(r -> parameter.name().matches(r))
+        .findFirst()
+        .orElse(null);
   }
 }

core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/Matchers.java

@@ -24,6 +24,8 @@
 import static com.google.errorprone.matchers.Matchers.isSubtypeOf;
 import static com.google.errorprone.matchers.Matchers.not;
 import static com.google.errorprone.matchers.Matchers.staticMethod;
+import static com.google.errorprone.util.ASTHelpers.getSymbol;
+import static com.google.errorprone.util.ASTHelpers.hasDirectAnnotationWithSimpleName;
 
 import com.google.errorprone.VisitorState;
 import com.google.errorprone.matchers.ChildMultiMatcher.MatchType;
@@ -49,71 +51,60 @@
 final class Matchers {
 
   /** Matches if the tree is a constructor for an AutoValue class. */
-  static final Matcher<NewClassTree> AUTOVALUE_CONSTRUCTOR =
-      new Matcher<NewClassTree>() {
-        @Override
-        public boolean matches(NewClassTree tree, VisitorState state) {
-          MethodSymbol sym = ASTHelpers.getSymbol(tree);
-
-          ClassSymbol owner = (ClassSymbol) sym.owner;
-          if (owner == null) {
-            return false;
-          }
-
-          Type superType = owner.getSuperclass();
-          if (superType == null) {
-            return false;
-          }
-
-          Symbol superSymbol = superType.tsym;
-          if (superSymbol == null) {
-            return false;
-          }
-
-          if (!ASTHelpers.hasDirectAnnotationWithSimpleName(superSymbol, "AutoValue")) {
-            return false;
-          }
-
-          return true;
-        }
-      };
-
-  // if any of the arguments are instances of throwable then abort - people like to use
-  // 'expected' as the name of the exception they are expecting
-  private static final Matcher<MethodInvocationTree> ARGUMENT_EXTENDS_TRHOWABLE =
+  static boolean isAutoValueConstructor(NewClassTree tree) {
+    MethodSymbol sym = getSymbol(tree);
+
+    ClassSymbol owner = (ClassSymbol) sym.owner;
+    if (owner == null) {
+      return false;
+    }
+
+    Type superType = owner.getSuperclass();
+    if (superType == null) {
+      return false;
+    }
+
+    Symbol superSymbol = superType.tsym;
+    if (superSymbol == null) {
+      return false;
+    }
+
+    if (!hasDirectAnnotationWithSimpleName(superSymbol, "AutoValue")) {
+      return false;
+    }
+
+    return true;
+  }
+
+  /**
+   * If any of the arguments are instances of throwable then abort - people like to use 'expected'
+   * as the name of the exception they are expecting.
+   */
+  private static final Matcher<MethodInvocationTree> ARGUMENT_EXTENDS_THROWABLE =
       hasArguments(MatchType.AT_LEAST_ONE, isSubtypeOf(Throwable.class));
 
-  // if the method is a refaster-before template then it might be explicitly matching bad behaviour
+  /**
+   * If the method is a refaster-before template then it might be explicitly matching bad behaviour.
+   */
   private static final Matcher<MethodInvocationTree> METHOD_ANNOTATED_WITH_BEFORETEMPLATE =
       enclosingMethod(hasAnnotation("com.google.errorprone.refaster.annotation.BeforeTemplate"));
 
-  private static final Matcher<MethodInvocationTree> TWO_PARAMETER_ASSERT =
-      new Matcher<MethodInvocationTree>() {
-        @Override
-        public boolean matches(MethodInvocationTree tree, VisitorState state) {
-          List<VarSymbol> parameters = ASTHelpers.getSymbol(tree).getParameters();
-          if (parameters.size() != 2) {
-            return false;
-          }
-          return ASTHelpers.isSameType(
-              parameters.get(0).asType(), parameters.get(1).asType(), state);
-        }
-      };
-
-  private static final Matcher<MethodInvocationTree> THREE_PARAMETER_ASSERT =
-      new Matcher<MethodInvocationTree>() {
-        @Override
-        public boolean matches(MethodInvocationTree tree, VisitorState state) {
-          List<VarSymbol> parameters = ASTHelpers.getSymbol(tree).getParameters();
-          if (parameters.size() != 3) {
-            return false;
-          }
-          return ASTHelpers.isSameType(
-                  parameters.get(0).asType(), state.getSymtab().stringType, state)
-              && ASTHelpers.isSameType(
-                  parameters.get(1).asType(), parameters.get(2).asType(), state);
-        }
-      };
+  private static boolean isTwoParameterAssert(MethodInvocationTree tree, VisitorState state) {
+    List<VarSymbol> parameters = getSymbol(tree).getParameters();
+    if (parameters.size() != 2) {
+      return false;
+    }
+    return ASTHelpers.isSameType(parameters.get(0).asType(), parameters.get(1).asType(), state);
+  }
+
+  private static boolean isThreeParameterAssert(MethodInvocationTree tree, VisitorState state) {
+    List<VarSymbol> parameters = getSymbol(tree).getParameters();
+    if (parameters.size() != 3) {
+      return false;
+    }
+    return ASTHelpers.isSameType(parameters.get(0).asType(), state.getSymtab().stringType, state)
+        && ASTHelpers.isSameType(parameters.get(1).asType(), parameters.get(2).asType(), state);
+  }
 
   /** Matches if the tree corresponds to an assertEquals-style method */
   static final Matcher<MethodInvocationTree> ASSERT_METHOD =
@@ -128,8 +119,8 @@ public boolean matches(MethodInvocationTree tree, VisitorState state) {
                   information which would cause the tests to fail.*/
                   "ErrorProneTest")
               .withNameMatching(Pattern.compile("assert.*")),
-          anyOf(TWO_PARAMETER_ASSERT, THREE_PARAMETER_ASSERT),
-          not(ARGUMENT_EXTENDS_TRHOWABLE),
+          anyOf(Matchers::isTwoParameterAssert, Matchers::isThreeParameterAssert),
+          not(ARGUMENT_EXTENDS_THROWABLE),
           not(METHOD_ANNOTATED_WITH_BEFORETEMPLATE));
 
   private Matchers() {}

core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/NameInCommentHeuristic.java

@@ -33,7 +33,7 @@
  *
  * @author [email protected] (Andrew Rice)
  */
-class NameInCommentHeuristic implements Heuristic {
+final class NameInCommentHeuristic implements Heuristic {
 
   /**
    * Return true if there are no comments on the original actual parameter of a change which match

core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/PenaltyThresholdHeuristic.java

@@ -28,7 +28,7 @@
  *
  * @author [email protected] (Andrew Rice)
  */
-class PenaltyThresholdHeuristic implements Heuristic {
+final class PenaltyThresholdHeuristic implements Heuristic {
 
   private final double threshold;