From 188b8b615fa6a4962aa5059efed40fcc01afb3e6 Mon Sep 17 00:00:00 2001 From: ghm Date: Wed, 30 Oct 2024 08:35:37 -0700 Subject: [PATCH] Add a test confirming that ArgumentSelectionDefectChecker works on record construction, but _not_ record deconstruction. PiperOrigin-RevId: 691430462 --- .../ArgumentSelectionDefectChecker.java | 48 +++----- .../AssertEqualsArgumentOrderChecker.java | 2 +- .../AutoValueConstructorOrderChecker.java | 5 +- .../argumentselectiondefects/Costs.java | 2 +- .../CreatesDuplicateCallHeuristic.java | 2 +- .../EnclosedByReverseHeuristic.java | 25 ++-- .../LowInformationNameHeuristic.java | 12 +- .../argumentselectiondefects/Matchers.java | 115 ++++++++---------- .../NameInCommentHeuristic.java | 2 +- .../PenaltyThresholdHeuristic.java | 2 +- .../ArgumentSelectionDefectCheckerTest.java | 60 ++++++++- 11 files changed, 151 insertions(+), 124 deletions(-) diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/ArgumentSelectionDefectChecker.java b/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/ArgumentSelectionDefectChecker.java index f535361a0562..ba4e79ef3c4d 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/ArgumentSelectionDefectChecker.java +++ b/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 buildDefaultDistanceFunction() { - return new Function() { - @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; } } diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/AssertEqualsArgumentOrderChecker.java b/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/AssertEqualsArgumentOrderChecker.java index d9c019b32f66..8ae69f606844 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/AssertEqualsArgumentOrderChecker.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/AssertEqualsArgumentOrderChecker.java @@ -43,7 +43,7 @@ * @author andrewrice@google.com (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 = diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/AutoValueConstructorOrderChecker.java b/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/AutoValueConstructorOrderChecker.java index f9e485375847..72d53b3a8d62 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/AutoValueConstructorOrderChecker.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/AutoValueConstructorOrderChecker.java @@ -41,7 +41,8 @@ * @author andrewrice@google.com (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; } diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/Costs.java b/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/Costs.java index 1ecfc39204d6..e7f2d24e937c 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/Costs.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/Costs.java @@ -30,7 +30,7 @@ * * @author andrewrice@google.com (Andrew Rice) */ -class Costs { +final class Costs { /** Formal parameters for the method being called. */ private final ImmutableList formals; diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/CreatesDuplicateCallHeuristic.java b/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/CreatesDuplicateCallHeuristic.java index 409c3dc55446..759f5c29f7ff 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/CreatesDuplicateCallHeuristic.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/CreatesDuplicateCallHeuristic.java @@ -35,7 +35,7 @@ * * @author andrewrice@google.com (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 diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/EnclosedByReverseHeuristic.java b/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/EnclosedByReverseHeuristic.java index 29d37637af18..99fb4f73909a 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/EnclosedByReverseHeuristic.java +++ b/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 andrewrice@google.com (Andrew Rice) */ -class EnclosedByReverseHeuristic implements Heuristic { +final class EnclosedByReverseHeuristic implements Heuristic { private static final ImmutableSet 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 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 getName(Tree tree) { diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/LowInformationNameHeuristic.java b/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/LowInformationNameHeuristic.java index e0ba0c8f6cff..715f9d9e4ec3 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/LowInformationNameHeuristic.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/LowInformationNameHeuristic.java @@ -29,7 +29,7 @@ * * @author andrewrice@google.com (Andrew Rice) */ -class LowInformationNameHeuristic implements Heuristic { +final class LowInformationNameHeuristic implements Heuristic { private static final ImmutableSet 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); } } diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/Matchers.java b/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/Matchers.java index bc472cd84ff3..0e99ee04545d 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/Matchers.java +++ b/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 AUTOVALUE_CONSTRUCTOR = - new Matcher() { - @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 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 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 METHOD_ANNOTATED_WITH_BEFORETEMPLATE = enclosingMethod(hasAnnotation("com.google.errorprone.refaster.annotation.BeforeTemplate")); - private static final Matcher TWO_PARAMETER_ASSERT = - new Matcher() { - @Override - public boolean matches(MethodInvocationTree tree, VisitorState state) { - List 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 THREE_PARAMETER_ASSERT = - new Matcher() { - @Override - public boolean matches(MethodInvocationTree tree, VisitorState state) { - List 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 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 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 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() {} diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/NameInCommentHeuristic.java b/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/NameInCommentHeuristic.java index 1490f6b65d8c..20cd8764afa1 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/NameInCommentHeuristic.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/NameInCommentHeuristic.java @@ -33,7 +33,7 @@ * * @author andrewrice@google.com (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 diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/PenaltyThresholdHeuristic.java b/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/PenaltyThresholdHeuristic.java index 95ead686c7f2..029cd27942d0 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/PenaltyThresholdHeuristic.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/PenaltyThresholdHeuristic.java @@ -28,7 +28,7 @@ * * @author andrewrice@google.com (Andrew Rice) */ -class PenaltyThresholdHeuristic implements Heuristic { +final class PenaltyThresholdHeuristic implements Heuristic { private final double threshold; diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/argumentselectiondefects/ArgumentSelectionDefectCheckerTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/argumentselectiondefects/ArgumentSelectionDefectCheckerTest.java index d505aa55c8e2..6a650688d5c1 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/argumentselectiondefects/ArgumentSelectionDefectCheckerTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/argumentselectiondefects/ArgumentSelectionDefectCheckerTest.java @@ -15,6 +15,8 @@ */ package com.google.errorprone.bugpatterns.argumentselectiondefects; +import static com.google.common.truth.TruthJUnit.assume; + import com.google.common.collect.ImmutableSet; import com.google.errorprone.BugPattern; import com.google.errorprone.BugPattern.SeverityLevel; @@ -38,6 +40,10 @@ @RunWith(JUnit4.class) public class ArgumentSelectionDefectCheckerTest { + private final CompilationTestHelper testHelper = + CompilationTestHelper.newInstance( + ArgumentSelectionDefectWithStringEquality.class, getClass()); + /** * A {@link BugChecker} which runs the ArgumentSelectionDefectChecker checker using string * equality for edit distance @@ -56,7 +62,7 @@ public ArgumentSelectionDefectWithStringEquality() { @Test public void argumentSelectionDefectChecker_findsSwap_withSwappedMatchingPair() { - CompilationTestHelper.newInstance(ArgumentSelectionDefectWithStringEquality.class, getClass()) + testHelper .addSourceLines( "Test.java", """ @@ -75,7 +81,7 @@ void test(Object first, Object second) { @Test public void argumentSelectionDefectChecker_findsSwap_withSwappedMatchingPairWithMethod() { - CompilationTestHelper.newInstance(ArgumentSelectionDefectWithStringEquality.class, getClass()) + testHelper .addSourceLines( "Test.java", """ @@ -96,7 +102,7 @@ void test(Object first) { @Test public void argumentSelectionDefectChecker_findsSwap_withOneNullArgument() { - CompilationTestHelper.newInstance(ArgumentSelectionDefectWithStringEquality.class, getClass()) + testHelper .addSourceLines( "Test.java", """ @@ -115,7 +121,7 @@ void test(Object second) { @Test public void argumentSelectionDefectChecker_rejectsSwap_withNoAssignableAlternatives() { - CompilationTestHelper.newInstance(ArgumentSelectionDefectWithStringEquality.class, getClass()) + testHelper .addSourceLines( "Test.java", """ @@ -132,7 +138,7 @@ void test(Integer first, String second) { @Test public void argumentSelectionDefectChecker_commentsOnlyOnSwappedPair_withThreeArguments() { - CompilationTestHelper.newInstance(ArgumentSelectionDefectWithStringEquality.class, getClass()) + testHelper .addSourceLines( "Test.java", """ @@ -360,7 +366,7 @@ public void parameterNamesAvailable_returnsTree_onMethodNotInCompilationUnit() { @Test public void description() { - CompilationTestHelper.newInstance(ArgumentSelectionDefectWithStringEquality.class, getClass()) + testHelper .addSourceLines( "Test.java", """ @@ -374,6 +380,48 @@ void test(Object first, Object second) { target(second, first); } } +""") + .doTest(); + } + + @Test + public void records() { + assume().that(Runtime.version().feature()).isAtLeast(16); + + testHelper + .addSourceLines( + "Test.java", + """ +class Test { + Foo test(String first, String second) { + // BUG: Diagnostic contains: may have been swapped + return new Foo(second, first); + } +} + +record Foo(String first, String second) {} +""") + .doTest(); + } + + @Test + public void recordDeconstruction() { + assume().that(Runtime.version().feature()).isAtLeast(21); + + testHelper + .addSourceLines( + "Test.java", + """ +class Test { + void test(Foo foo) { + switch (foo) { + // TODO(user): We should report a finding here! + case Foo(String second, String first) -> {} + } + } +} + +record Foo(String first, String second) {} """) .doTest(); }