Add a check for parameter names of overriding methods.

PiperOrigin-RevId: 551444783

core/src/main/java/com/google/errorprone/bugpatterns/OverridingMethodInconsistentArgumentNamesChecker.java

@@ -0,0 +1,94 @@
+/*
+ * Copyright 2023 The Error Prone Authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.google.errorprone.bugpatterns;
+
+import static com.google.common.collect.ImmutableBiMap.toImmutableBiMap;
+import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
+import static com.google.errorprone.util.ASTHelpers.findSuperMethods;
+import static com.google.errorprone.util.ASTHelpers.getSymbol;
+import static java.util.stream.Collectors.joining;
+import static java.util.stream.IntStream.range;
+
+import com.google.common.collect.ImmutableBiMap;
+import com.google.errorprone.BugPattern;
+import com.google.errorprone.VisitorState;
+import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher;
+import com.google.errorprone.matchers.Description;
+import com.sun.source.tree.MethodTree;
+import com.sun.tools.javac.code.Symbol.MethodSymbol;
+import java.util.Optional;
+import javax.lang.model.element.Name;
+
+/** A {@link BugChecker}; see the associated {@link BugPattern} annotation for details. */
+@BugPattern(
+    summary = "Arguments of overriding method are inconsistent with overridden method.",
+    severity = WARNING)
+public class OverridingMethodInconsistentArgumentNamesChecker extends BugChecker
+    implements MethodTreeMatcher {
+
+  @Override
+  public Description matchMethod(MethodTree methodTree, VisitorState state) {
+    MethodSymbol methodSymbol = getSymbol(methodTree);
+    Optional<MethodSymbol> superMethod =
+        findSuperMethods(methodSymbol, state.getTypes()).stream().findFirst();
+    if (superMethod.isEmpty() || methodSymbol.isVarArgs() || superMethod.get().isVarArgs()) {
+      return Description.NO_MATCH;
+    }
+
+    ImmutableBiMap<Name, Integer> params = getParams(methodSymbol);
+    ImmutableBiMap<Name, Integer> superParams = getParams(superMethod.get());
+
+    for (Name param : params.keySet()) {
+      int position = params.get(param);
+      if (!superParams.containsKey(param) || position == superParams.get(param)) {
+        continue;
+      }
+      Name samePositionSuperParam = superParams.inverse().get(position);
+      if (params.containsKey(samePositionSuperParam)) {
+        return buildDescription(methodTree).setMessage(getDescription(superMethod.get())).build();
+      }
+    }
+
+    return Description.NO_MATCH;
+  }
+
+  private static ImmutableBiMap<Name, Integer> getParams(MethodSymbol methodSymbol) {
+    return range(0, methodSymbol.getParameters().size())
+        .boxed()
+        .collect(toImmutableBiMap(i -> methodSymbol.getParameters().get(i).name, i -> i));
+  }
+
+  /**
+   * Provides a violation description with suggested parameter ordering.
+   *
+   * <p>We're not returning a suggested fix as re-ordering is not safe and might break the code.
+   */
+  public String getDescription(MethodSymbol methodSymbol) {
+    return "The parameters of this method are inconsistent with the overridden method."
+        + " A consistent order would be: "
+        + getSuggestedSignature(methodSymbol);
+  }
+
+  private static String getSuggestedSignature(MethodSymbol methodSymbol) {
+    return String.format(
+        "%s(%s)", methodSymbol.getSimpleName(), getSuggestedParameters(methodSymbol));
+  }
+
+  private static String getSuggestedParameters(MethodSymbol methodSymbol) {
+    return methodSymbol.getParameters().stream().map(p -> p.name).collect(joining(", "));
+  }
+}

core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java

@@ -287,6 +287,7 @@
 import com.google.errorprone.bugpatterns.OutlineNone;
 import com.google.errorprone.bugpatterns.OverrideThrowableToString;
 import com.google.errorprone.bugpatterns.Overrides;
+import com.google.errorprone.bugpatterns.OverridingMethodInconsistentArgumentNamesChecker;
 import com.google.errorprone.bugpatterns.PackageInfo;
 import com.google.errorprone.bugpatterns.PackageLocation;
 import com.google.errorprone.bugpatterns.ParameterComment;
@@ -992,6 +993,7 @@ public static ScannerSupplier warningChecks() {
           OverrideThrowableToString.class,
           Overrides.class,
           OverridesGuiceInjectableMethod.class,
+          OverridingMethodInconsistentArgumentNamesChecker.class,
           ParameterName.class,
           PreconditionsCheckNotNullRepeated.class,
           PrimitiveAtomicReference.class,

core/src/test/java/com/google/errorprone/bugpatterns/OverridingMethodInconsistentArgumentNamesCheckerTest.java

@@ -0,0 +1,76 @@
+/*
+ * Copyright 2023 The Error Prone Authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.google.errorprone.bugpatterns;
+
+import com.google.errorprone.CompilationTestHelper;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+@RunWith(JUnit4.class)
+public class OverridingMethodInconsistentArgumentNamesCheckerTest {
+
+  private final CompilationTestHelper testHelper =
+      CompilationTestHelper.newInstance(
+          OverridingMethodInconsistentArgumentNamesChecker.class, getClass());
+
+  @Test
+  public void positiveSwap() {
+    testHelper
+        .addSourceLines("A.java", "class A {", "  void m(int p1, int p2) {}", "}")
+        .addSourceLines(
+            "B.java",
+            "class B extends A {",
+            "  @Override",
+            "  // BUG: Diagnostic contains: A consistent order would be: m(p1, p2)",
+            "  void m(int p2, int p1) {}",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  public void positivePermutation() {
+    testHelper
+        .addSourceLines("A.java", "class A {", "  void m(int p1, int p2, int p3) {}", "}")
+        .addSourceLines(
+            "B.java",
+            "class B extends A {",
+            "  @Override",
+            "  // BUG: Diagnostic contains: A consistent order would be: m(p1, p2, p3)",
+            "  void m(int p3, int p1, int p2) {}",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  public void negative() {
+    testHelper
+        .addSourceLines("A.java", "class A {", "  void m(int p1, int p2) {}", "}")
+        .addSourceLines(
+            "B.java", "class B extends A {", "  @Override", "  void m(int p1, int p2) {}", "}")
+        .doTest();
+  }
+
+  @Test
+  public void negative2() {
+    testHelper
+        .addSourceLines("A.java", "class A {", "  void m(int p1, int p2) {}", "}")
+        .addSourceLines(
+            "B.java", "class B extends A {", "  @Override", "  void m(int p1, int p3) {}", "}")
+        .doTest();
+  }
+}

docs/bugpattern/OverridingMethodInconsistentArgumentNamesChecker.md

@@ -0,0 +1,14 @@
+Inconsistently ordered parameters in method overrides mostly indicate an
+accidental bug in the overriding method. An example for an overriding method
+with inconsistent parameter names:
+
+```java
+class A {
+  public void foo(int foo, int baz) { ... }
+}
+
+class B extends A {
+  @Override
+  public void foo(int baz, int foo) { ... }
+}
+```