Support record destructuring in ArgumentSelectionDefectChecker.

PiperOrigin-RevId: 697569466
google · Nov 18, 2024 · 332cbfa · 332cbfa
1 parent 0db3360
commit 332cbfa
Show file tree

Hide file tree

Showing 5 changed files with 39 additions and 68 deletions.
diff --git a/...oogle/errorprone/bugpatterns/argumentselectiondefects/ArgumentSelectionDefectChecker.java b/...oogle/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
@@ -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/.../main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/InvocationInfo.java b/.../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<? extends ExpressionTree> actualParameters();
+  abstract ImmutableList<Tree> actualParameters();
 
   abstract ImmutableList<VarSymbol> 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
@@ -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<Parameter> createListFromVarSymbols(List<VarSymbol> varSymb
         .collect(toImmutableList());
   }
 
-  static ImmutableList<Parameter> createListFromExpressionTrees(
-      List<? extends ExpressionTree> trees) {
+  static ImmutableList<Parameter> createListFromExpressionTrees(List<? extends Tree> 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<String> 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/...e/errorprone/bugpatterns/argumentselectiondefects/ArgumentSelectionDefectCheckerTest.java b/...e/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();