From b1e77bde7a8c01187e52d2f73ee9a8767f6f5b73 Mon Sep 17 00:00:00 2001 From: andrewrice Date: Wed, 10 May 2017 09:11:03 -0700 Subject: [PATCH] Checker for parameter order on autovalue constructor call The check works by looking at the arguments used to call the constructor of the AutoValue object. If a different permutation of the arguments would exactly match the parameters of the constructor then a warning is generated. Arguments for which a name cannot be extracted are left in their original position. Occasionally an author will deliberately swap the order of the constructor call e.g. if (first < last) { return new AutoValue_Foo(first, last); } else { return new AutoValue_Foo(last, first); } We avoid matching on these by looking to see if the permutation of the arguments exists in another call within the same method. The check is implemented as a special case of the ArgumentSelectionDefectChecker which in general looks to see if a permutation of the argument names is more suitable than the current one. RELNOTES: Checker for parameter order on autovalue constructor call ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=155635122 --- .../AutoValueConstructorOrderChecker.java | 126 ++++++++++++++++++ .../scanner/BuiltInCheckerSuppliers.java | 2 + .../AutoValueConstructorOrderCheckerTest.java | 103 ++++++++++++++ 3 files changed, 231 insertions(+) create mode 100644 core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/AutoValueConstructorOrderChecker.java create mode 100644 core/src/test/java/com/google/errorprone/bugpatterns/argumentselectiondefects/AutoValueConstructorOrderCheckerTest.java diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/AutoValueConstructorOrderChecker.java b/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/AutoValueConstructorOrderChecker.java new file mode 100644 index 00000000000..74d611565b8 --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/AutoValueConstructorOrderChecker.java @@ -0,0 +1,126 @@ +/* + * Copyright 2017 Google Inc. All Rights Reserved. + * + * 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.argumentselectiondefects; + +import static com.google.errorprone.BugPattern.Category.GUAVA; +import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; + +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.bugpatterns.BugChecker.NewClassTreeMatcher; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.NewClassTree; +import com.sun.tools.javac.code.Symbol; +import com.sun.tools.javac.code.Symbol.ClassSymbol; +import com.sun.tools.javac.code.Symbol.MethodSymbol; +import com.sun.tools.javac.code.Type; +import java.util.function.Function; + +/** + * Checker to make sure that constructors for AutoValue types are invoked with arguments in the + * correct order. + * + *

Warning: this check relies on recovering parameter names from library class files. These names + * are only included if you compile with debugging symbols (-g) or with -parameters. You also need + * to tell the compiler to read these names from the classfiles and so must compile your project + * with -parameters too. + * + * @author andrewrice@google.com (Andrew Rice) + */ +@BugPattern( + name = "AutoValueConstructorOrderChecker", + summary = "Arguments to AutoValue constructor are in the wrong order", + explanation = + "AutoValue constructors are synthesized with their parameters in the same order as the " + + "abstract accessor methods. Calls to the constructor need to match this ordering.", + category = GUAVA, + severity = WARNING +) +public class AutoValueConstructorOrderChecker extends BugChecker implements NewClassTreeMatcher { + + private ArgumentChangeFinder argumentChangeFinder = + ArgumentChangeFinder.builder() + .setDistanceFunction(buildDistanceFunction()) + .addHeuristic(allArgumentsMustMatch()) + .addHeuristic(new CreatesDuplicateCallHeuristic()) + .build(); + + @Override + public Description matchNewClass(NewClassTree tree, VisitorState state) { + MethodSymbol sym = ASTHelpers.getSymbol(tree); + if (sym == null) { + return Description.NO_MATCH; + } + + ClassSymbol owner = (ClassSymbol) sym.owner; + if (owner == null) { + return Description.NO_MATCH; + } + + Type superType = owner.getSuperclass(); + if (superType == null) { + return Description.NO_MATCH; + } + + Symbol superSymbol = superType.tsym; + if (superSymbol == null) { + return Description.NO_MATCH; + } + + if (!ASTHelpers.hasDirectAnnotationWithSimpleName(superSymbol, "AutoValue")) { + return Description.NO_MATCH; + } + + MethodSymbol symbol = ASTHelpers.getSymbol(tree); + if (symbol == null) { + return Description.NO_MATCH; + } + + InvocationInfo invocationInfo = InvocationInfo.createFromNewClass(tree, symbol, state); + + Changes changes = argumentChangeFinder.findChanges(invocationInfo); + + if (changes.isEmpty()) { + return Description.NO_MATCH; + } + + return buildDescription(invocationInfo.tree()) + .addFix(changes.buildPermuteArgumentsFix(invocationInfo)) + .build(); + } + + private static Function buildDistanceFunction() { + return new Function() { + @Override + public Double apply(ParameterPair parameterPair) { + Parameter formal = parameterPair.formal(); + Parameter actual = parameterPair.actual(); + if (formal.isUnknownName() || actual.isUnknownName()) { + return formal.index() == actual.index() ? 0.0 : 1.0; + } else { + return formal.name().equals(actual.name()) ? 0.0 : 1.0; + } + } + }; + } + + private static Heuristic allArgumentsMustMatch() { + return (changes, node, sym, state) -> changes.assignmentCost().stream().allMatch(c -> c < 1.0); + } +} 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 a6c9c7fb308..060a54b7f07 100644 --- a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java +++ b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java @@ -189,6 +189,7 @@ import com.google.errorprone.bugpatterns.android.StaticOrDefaultInterfaceMethod; import com.google.errorprone.bugpatterns.argumentselectiondefects.ArgumentSelectionDefectChecker; import com.google.errorprone.bugpatterns.argumentselectiondefects.AssertEqualsArgumentOrderChecker; +import com.google.errorprone.bugpatterns.argumentselectiondefects.AutoValueConstructorOrderChecker; import com.google.errorprone.bugpatterns.collectionincompatibletype.CollectionIncompatibleType; import com.google.errorprone.bugpatterns.collectionincompatibletype.CompatibleWithMisuse; import com.google.errorprone.bugpatterns.collectionincompatibletype.IncompatibleArgumentType; @@ -439,6 +440,7 @@ public static ScannerSupplier errorChecks() { AssertFalse.class, AssistedInjectAndInjectOnConstructors.class, AssistedInjectAndInjectOnSameConstructor.class, + AutoValueConstructorOrderChecker.class, BigDecimalLiteralDouble.class, BindingToUnqualifiedCommonType.class, ClassName.class, diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/argumentselectiondefects/AutoValueConstructorOrderCheckerTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/argumentselectiondefects/AutoValueConstructorOrderCheckerTest.java new file mode 100644 index 00000000000..d93a4617bee --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/argumentselectiondefects/AutoValueConstructorOrderCheckerTest.java @@ -0,0 +1,103 @@ +/* + * Copyright 2012 Google Inc. All Rights Reserved. + * + * 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.argumentselectiondefects; + +import com.google.errorprone.CompilationTestHelper; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** Tests for AutoValueConstructorOrderChecker */ +@RunWith(JUnit4.class) +public class AutoValueConstructorOrderCheckerTest { + + private CompilationTestHelper compilationHelper; + + @Before + public void setUp() { + compilationHelper = + CompilationTestHelper.newInstance(AutoValueConstructorOrderChecker.class, getClass()); + } + + @Test + public void autoValueChecker_detectsSwap_withExactNames() throws Exception { + compilationHelper + .addSourceLines( + "Test.java", + "import com.google.auto.value.AutoValue;", + "@AutoValue", + "abstract class Test {", + " abstract String valueOne();", + " abstract String valueTwo();", + " static Test create(String valueOne, String valueTwo) {", + " // BUG: Diagnostic contains: new AutoValue_Test(valueOne, valueTwo)", + " return new AutoValue_Test(valueTwo, valueOne);", + " }", + "}", + "class AutoValue_Test extends Test {", + " String valueOne() { return null; }", + " String valueTwo() { return null; }", + " AutoValue_Test(String valueOne, String valueTwo) {}", + "}") + .doTest(); + } + + @Test + public void autoValueChecker_ignoresSwap_withInexactNames() throws Exception { + compilationHelper + .addSourceLines( + "Test.java", + "import com.google.auto.value.AutoValue;", + "@AutoValue", + "abstract class Test {", + " abstract String valueOne();", + " abstract String valueTwo();", + " static Test create(String valueOneArg, String valueTwoArg) {", + " return new AutoValue_Test(valueTwoArg, valueOneArg);", + " }", + "}", + "class AutoValue_Test extends Test {", + " String valueOne() { return null; }", + " String valueTwo() { return null; }", + " AutoValue_Test(String valueOne, String valueTwo) {}", + "}") + .doTest(); + } + + @Test + public void autoValueChecker_makesNoSuggestion_withCorrectOrder() throws Exception { + compilationHelper + .addSourceLines( + "Test.java", + "import com.google.auto.value.AutoValue;", + "@AutoValue", + "abstract class Test {", + " abstract String valueOne();", + " abstract String valueTwo();", + " static Test create(String valueOne, String valueTwo) {", + " return new AutoValue_Test(valueOne, valueTwo);", + " }", + "}", + "class AutoValue_Test extends Test {", + " String valueOne() { return null; }", + " String valueTwo() { return null; }", + " AutoValue_Test(String valueOne, String valueTwo) {}", + "}") + .doTest(); + } +}