Swap the order of ArgumentSelectionDefectChecker fixes, and add a mor…

…e detailed description RELNOTES: N/A ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=177675074
google · Dec 4, 2017 · 263c405 · 263c405
1 parent a1ef685
commit 263c405
Show file tree

Hide file tree

Showing 3 changed files with 47 additions and 10 deletions.
diff --git a/...oogle/errorprone/bugpatterns/argumentselectiondefects/ArgumentSelectionDefectChecker.java b/...oogle/errorprone/bugpatterns/argumentselectiondefects/ArgumentSelectionDefectChecker.java
@@ -21,11 +21,11 @@
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.errorprone.BugPattern;
+import com.google.errorprone.BugPattern.ProvidesFix;
 import com.google.errorprone.VisitorState;
 import com.google.errorprone.bugpatterns.BugChecker;
 import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher;
 import com.google.errorprone.bugpatterns.BugChecker.NewClassTreeMatcher;
-import com.google.errorprone.fixes.SuggestedFix;
 import com.google.errorprone.matchers.Description;
 import com.google.errorprone.names.NamingConventions;
 import com.google.errorprone.names.NeedlemanWunschEditDistance;
@@ -64,7 +64,8 @@
           + "swap clear to future readers of the code. Argument names annotated with a comment "
           + "containing the parameter name will not generate a warning.",
   category = JDK,
-  severity = WARNING
+  severity = WARNING,
+  providesFix = ProvidesFix.REQUIRES_HUMAN_ATTENTION
 )
 public class ArgumentSelectionDefectChecker extends BugChecker
     implements MethodInvocationTreeMatcher, NewClassTreeMatcher {
@@ -127,16 +128,17 @@ private Description visitNewClassOrMethodInvocation(InvocationInfo invocationInf
       return Description.NO_MATCH;
     }
 
-    // Fix1: permute the arguments as required
-    SuggestedFix permuteArgumentsFix = changes.buildPermuteArgumentsFix(invocationInfo);
+    Description.Builder description =
+        buildDescription(invocationInfo.tree()).setMessage(changes.describe(invocationInfo));
 
-    // Fix2: apply comments with parameter names to potentially-swapped arguments of the method
-    SuggestedFix commentArgumentsFix = changes.buildCommentArgumentsFix(invocationInfo);
+    // Fix 1 (semantics-preserving): apply comments with parameter names to potentially-swapped
+    // arguments of the method
+    description.addFix(changes.buildCommentArgumentsFix(invocationInfo));
 
-    return buildDescription(invocationInfo.tree())
-        .addFix(permuteArgumentsFix)
-        .addFix(commentArgumentsFix)
-        .build();
+    // Fix 2: permute the arguments as required
+    description.addFix(changes.buildPermuteArgumentsFix(invocationInfo));
+
+    return description.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,6 +21,7 @@
 import com.google.errorprone.fixes.SuggestedFix;
 import com.sun.source.tree.ExpressionTree;
 import com.sun.tools.javac.tree.JCTree;
+import java.util.stream.Collectors;
 
 /**
  * Value class for holding suggested changes to method call arguments.
@@ -83,4 +84,20 @@ SuggestedFix buildPermuteArgumentsFix(InvocationInfo info) {
     }
     return permuteArgumentsFixBuilder.build();
   }
+
+  public String describe(InvocationInfo info) {
+    return "The following arguments may have been swapped: "
+        + changedPairs()
+            .stream()
+            .map(
+                p ->
+                    String.format(
+                        "'%s' for formal parameter '%s'",
+                        info.state()
+                            .getSourceForNode(info.actualParameters().get(p.formal().index())),
+                        p.formal().name()))
+            .collect(Collectors.joining(", "))
+        + ". Either add clarifying `/* paramName= */` comments, or swap the arguments if that is"
+        + " what was intended";
+  }
 }
diff --git a/...e/errorprone/bugpatterns/argumentselectiondefects/ArgumentSelectionDefectCheckerTest.java b/...e/errorprone/bugpatterns/argumentselectiondefects/ArgumentSelectionDefectCheckerTest.java
@@ -340,4 +340,22 @@ public void parameterNamesAvailable_returnsTree_onMethodNotInCompilationUnit() {
             "}")
         .doTest();
   }
+
+  @Test
+  public void description() {
+    CompilationTestHelper.newInstance(ArgumentSelectionDefectWithStringEquality.class, getClass())
+        .addSourceLines(
+            "Test.java",
+            "abstract class Test {",
+            "  abstract void target(Object first, Object second);",
+            "  void test(Object first, Object second) {",
+            "     // BUG: Diagnostic contains: The following arguments may have been swapped:"
+                + " 'second' for formal parameter 'first', 'first' for formal parameter 'second'."
+                + " Either add clarifying `/* paramName= */` comments, or swap the arguments if"
+                + " that is what was intended",
+            "     target(second, first);",
+            "  }",
+            "}")
+        .doTest();
+  }
 }