From 332cbfae1dfdfb67e2810dc705da9b60237086bd Mon Sep 17 00:00:00 2001 From: ghm Date: Mon, 18 Nov 2024 03:49:52 -0800 Subject: [PATCH] Support record destructuring in ArgumentSelectionDefectChecker. PiperOrigin-RevId: 697569466 --- .../ArgumentSelectionDefectChecker.java | 27 ++++------ .../argumentselectiondefects/Changes.java | 4 +- .../InvocationInfo.java | 3 +- .../argumentselectiondefects/Parameter.java | 51 +++++++++---------- .../ArgumentSelectionDefectCheckerTest.java | 22 -------- 5 files changed, 39 insertions(+), 68 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 ba4e79ef3c4..6c9e5861a70 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 @@ -84,8 +84,7 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState return Description.NO_MATCH; } - return visitNewClassOrMethodInvocation( - InvocationInfo.createFromMethodInvocation(tree, symbol, state)); + return visit(InvocationInfo.createFromMethodInvocation(tree, symbol, state)); } @Override @@ -97,28 +96,24 @@ public Description matchNewClass(NewClassTree tree, VisitorState state) { return Description.NO_MATCH; } - return visitNewClassOrMethodInvocation(InvocationInfo.createFromNewClass(tree, symbol, state)); + return visit(InvocationInfo.createFromNewClass(tree, symbol, state)); } - private Description visitNewClassOrMethodInvocation(InvocationInfo invocationInfo) { - + private Description visit(InvocationInfo invocationInfo) { Changes changes = argumentChangeFinder.findChanges(invocationInfo); if (changes.isEmpty()) { return Description.NO_MATCH; } - Description.Builder description = - buildDescription(invocationInfo.tree()).setMessage(changes.describe(invocationInfo)); - - // Fix 1 (semantics-preserving): apply comments with parameter names to potentially-swapped - // arguments of the method - description.addFix(changes.buildCommentArgumentsFix(invocationInfo)); - - // Fix 2: permute the arguments as required - description.addFix(changes.buildPermuteArgumentsFix(invocationInfo)); - - return description.build(); + return buildDescription(invocationInfo.tree()) + .setMessage(changes.describe(invocationInfo)) + // Fix 1 (semantics-preserving): apply comments with parameter names to potentially-swapped + // arguments of the method + .addFix(changes.buildCommentArgumentsFix(invocationInfo)) + // Fix 2: permute the arguments as required + .addFix(changes.buildPermuteArgumentsFix(invocationInfo)) + .build(); } /** diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/Changes.java b/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/Changes.java index 2ae08e34251..f26f58265d5 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/Changes.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/Changes.java @@ -21,7 +21,7 @@ import com.google.auto.value.AutoValue; import com.google.common.collect.ImmutableList; import com.google.errorprone.fixes.SuggestedFix; -import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.Tree; import java.util.stream.Collectors; /** @@ -65,7 +65,7 @@ SuggestedFix buildCommentArgumentsFix(InvocationInfo info) { SuggestedFix.Builder commentArgumentsFixBuilder = SuggestedFix.builder(); for (ParameterPair change : changedPairs()) { int index = change.formal().index(); - ExpressionTree actual = info.actualParameters().get(index); + Tree actual = info.actualParameters().get(index); int startPosition = getStartPosition(actual); String formal = info.formalParameters().get(index).getSimpleName().toString(); commentArgumentsFixBuilder.replace( diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/InvocationInfo.java b/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/InvocationInfo.java index 1c915419d42..85bc1775ee4 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/InvocationInfo.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/InvocationInfo.java @@ -19,7 +19,6 @@ import com.google.auto.value.AutoValue; import com.google.common.collect.ImmutableList; import com.google.errorprone.VisitorState; -import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.MethodInvocationTree; import com.sun.source.tree.NewClassTree; import com.sun.source.tree.Tree; @@ -39,7 +38,7 @@ abstract class InvocationInfo { abstract MethodSymbol symbol(); - abstract ImmutableList actualParameters(); + abstract ImmutableList actualParameters(); abstract ImmutableList formalParameters(); diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/Parameter.java b/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/Parameter.java index 7235a043588..12f771bf9f4 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/Parameter.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/Parameter.java @@ -32,6 +32,7 @@ import com.sun.source.tree.MemberSelectTree; import com.sun.source.tree.MethodInvocationTree; import com.sun.source.tree.NewClassTree; +import com.sun.source.tree.Tree; import com.sun.source.tree.Tree.Kind; import com.sun.source.tree.VariableTree; import com.sun.tools.javac.code.Symbol; @@ -88,14 +89,17 @@ static ImmutableList createListFromVarSymbols(List varSymb .collect(toImmutableList()); } - static ImmutableList createListFromExpressionTrees( - List trees) { + static ImmutableList createListFromExpressionTrees(List trees) { return Streams.mapWithIndex( trees.stream(), (t, i) -> new AutoValue_Parameter( getArgumentName(t), - Optional.ofNullable(ASTHelpers.getResultType(t)).orElse(Type.noType), + Optional.ofNullable( + t instanceof ExpressionTree + ? ASTHelpers.getResultType((ExpressionTree) t) + : ASTHelpers.getType(t)) + .orElse(Type.noType), (int) i, t.toString(), t.getKind(), @@ -162,28 +166,25 @@ private static String getClassName(ClassSymbol s) { * will return the marker for an unknown name. */ @VisibleForTesting - static String getArgumentName(ExpressionTree expressionTree) { - switch (expressionTree.getKind()) { - case MEMBER_SELECT -> { - return ((MemberSelectTree) expressionTree).getIdentifier().toString(); - } - case NULL_LITERAL -> { - // null could match anything pretty well - return NAME_NULL; - } + static String getArgumentName(Tree tree) { + return switch (tree.getKind()) { + case VARIABLE -> ((VariableTree) tree).getName().toString(); + case MEMBER_SELECT -> ((MemberSelectTree) tree).getIdentifier().toString(); + // null could match anything pretty well + case NULL_LITERAL -> NAME_NULL; case IDENTIFIER -> { - IdentifierTree idTree = (IdentifierTree) expressionTree; + IdentifierTree idTree = (IdentifierTree) tree; if (idTree.getName().contentEquals("this")) { // for the 'this' keyword the argument name is the name of the object's class Symbol sym = ASTHelpers.getSymbol(idTree); - return sym != null ? getClassName(ASTHelpers.enclosingClass(sym)) : NAME_NOT_PRESENT; + yield sym != null ? getClassName(ASTHelpers.enclosingClass(sym)) : NAME_NOT_PRESENT; } else { // if we have a variable, just extract its name - return idTree.getName().toString(); + yield idTree.getName().toString(); } } case METHOD_INVOCATION -> { - MethodInvocationTree methodInvocationTree = (MethodInvocationTree) expressionTree; + MethodInvocationTree methodInvocationTree = (MethodInvocationTree) tree; MethodSymbol methodSym = ASTHelpers.getSymbol(methodInvocationTree); String name = methodSym.getSimpleName().toString(); ImmutableList terms = NamingConventions.splitToLowercaseTerms(name); @@ -192,26 +193,24 @@ static String getArgumentName(ExpressionTree expressionTree) { if (terms.size() == 1) { ExpressionTree receiver = ASTHelpers.getReceiver(methodInvocationTree); if (receiver == null) { - return getClassName(ASTHelpers.enclosingClass(methodSym)); + yield getClassName(ASTHelpers.enclosingClass(methodSym)); } // recursively try to get a name from the receiver - return getArgumentName(receiver); + yield getArgumentName(receiver); } else { - return name.substring(firstTerm.length()); + yield name.substring(firstTerm.length()); } } else { - return name; + yield name; } } case NEW_CLASS -> { - MethodSymbol constructorSym = ASTHelpers.getSymbol((NewClassTree) expressionTree); - return constructorSym.owner != null + MethodSymbol constructorSym = ASTHelpers.getSymbol((NewClassTree) tree); + yield constructorSym.owner != null ? getClassName((ClassSymbol) constructorSym.owner) : NAME_NOT_PRESENT; } - default -> { - return NAME_NOT_PRESENT; - } - } + default -> NAME_NOT_PRESENT; + }; } } 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 6a650688d5c..5fcc032d1b7 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 @@ -399,28 +399,6 @@ Foo test(String first, String second) { } } -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();