From 4f98271820e6d993535f164d8fea966821190cda Mon Sep 17 00:00:00 2001 From: Error Prone Team Date: Mon, 24 Jul 2023 00:46:20 -0700 Subject: [PATCH] Add a check for parameter names of overriding methods. PiperOrigin-RevId: 550469200 --- ...ethodInconsistentArgumentNamesChecker.java | 94 +++++++++++++++++++ .../scanner/BuiltInCheckerSuppliers.java | 2 + ...dInconsistentArgumentNamesCheckerTest.java | 76 +++++++++++++++ ...gMethodInconsistentArgumentNamesChecker.md | 14 +++ 4 files changed, 186 insertions(+) create mode 100644 core/src/main/java/com/google/errorprone/bugpatterns/OverridingMethodInconsistentArgumentNamesChecker.java create mode 100644 core/src/test/java/com/google/errorprone/bugpatterns/OverridingMethodInconsistentArgumentNamesCheckerTest.java create mode 100644 docs/bugpattern/OverridingMethodInconsistentArgumentNamesChecker.md diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/OverridingMethodInconsistentArgumentNamesChecker.java b/core/src/main/java/com/google/errorprone/bugpatterns/OverridingMethodInconsistentArgumentNamesChecker.java new file mode 100644 index 000000000000..c275ef7695a0 --- /dev/null +++ b/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 superMethod = + findSuperMethods(methodSymbol, state.getTypes()).stream().findFirst(); + if (superMethod.isEmpty() || methodSymbol.isVarArgs() || superMethod.get().isVarArgs()) { + return Description.NO_MATCH; + } + + ImmutableBiMap params = getParams(methodSymbol); + ImmutableBiMap 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 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. + * + *

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(", ")); + } +} diff --git a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java index 9eff13a017ed..a4b4cba9b21c 100644 --- a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java +++ b/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, diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/OverridingMethodInconsistentArgumentNamesCheckerTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/OverridingMethodInconsistentArgumentNamesCheckerTest.java new file mode 100644 index 000000000000..ad4be765359b --- /dev/null +++ b/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(); + } +} diff --git a/docs/bugpattern/OverridingMethodInconsistentArgumentNamesChecker.md b/docs/bugpattern/OverridingMethodInconsistentArgumentNamesChecker.md new file mode 100644 index 000000000000..78012784b72f --- /dev/null +++ b/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) { ... } +} +```