From a57309b0183c0c70f6afe7bceea3678b62c4791b Mon Sep 17 00:00:00 2001 From: ghm Date: Fri, 6 Jan 2023 08:56:58 -0800 Subject: [PATCH] Add a check to reverse Yoda conditions. PiperOrigin-RevId: 500187575 --- .../errorprone/bugpatterns/YodaCondition.java | 112 +++++++++++++++ .../scanner/BuiltInCheckerSuppliers.java | 4 +- .../bugpatterns/YodaConditionTest.java | 136 ++++++++++++++++++ 3 files changed, 251 insertions(+), 1 deletion(-) create mode 100644 core/src/main/java/com/google/errorprone/bugpatterns/YodaCondition.java create mode 100644 core/src/test/java/com/google/errorprone/bugpatterns/YodaConditionTest.java diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/YodaCondition.java b/core/src/main/java/com/google/errorprone/bugpatterns/YodaCondition.java new file mode 100644 index 00000000000..f856d12d93b --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/YodaCondition.java @@ -0,0 +1,112 @@ +/* + * 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.errorprone.BugPattern.SeverityLevel.WARNING; +import static com.google.errorprone.fixes.SuggestedFixes.qualifyType; +import static com.google.errorprone.matchers.Description.NO_MATCH; +import static com.google.errorprone.matchers.Matchers.instanceEqualsInvocation; +import static com.google.errorprone.matchers.Matchers.staticEqualsInvocation; +import static com.google.errorprone.util.ASTHelpers.constValue; +import static com.google.errorprone.util.ASTHelpers.getReceiver; +import static com.google.errorprone.util.ASTHelpers.getSymbol; +import static java.lang.String.format; + +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker.BinaryTreeMatcher; +import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; +import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.matchers.Description; +import com.sun.source.tree.BinaryTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.source.tree.Tree; +import com.sun.tools.javac.code.Symbol.VarSymbol; +import java.util.Objects; + +/** See the summary. */ +@BugPattern( + summary = "The non-constant portion of an equals check generally comes first.", + severity = WARNING) +public final class YodaCondition extends BugChecker + implements BinaryTreeMatcher, MethodInvocationTreeMatcher { + @Override + public Description matchBinary(BinaryTree tree, VisitorState state) { + switch (tree.getKind()) { + case EQUAL_TO: + case NOT_EQUAL_TO: + return fix( + tree, + tree.getLeftOperand(), + tree.getRightOperand(), + /* provideNullSafeFix= */ false, + state); + default: + return NO_MATCH; + } + } + + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + if (staticEqualsInvocation().matches(tree, state)) { + return fix( + tree, + tree.getArguments().get(0), + tree.getArguments().get(1), + /* provideNullSafeFix= */ false, + state); + } + if (instanceEqualsInvocation().matches(tree, state)) { + return fix( + tree, + getReceiver(tree), + tree.getArguments().get(0), + /* provideNullSafeFix= */ true, + state); + } + return NO_MATCH; + } + + private Description fix( + Tree tree, Tree lhs, Tree rhs, boolean provideNullSafeFix, VisitorState state) { + if (seemsConstant(lhs) && !seemsConstant(rhs)) { + var description = buildDescription(lhs).addFix(SuggestedFix.swap(lhs, rhs)); + if (provideNullSafeFix) { + var fix = SuggestedFix.builder().setShortDescription("null-safe fix"); + description.addFix( + fix.replace( + tree, + format( + "%s.equals(%s, %s)", + qualifyType(state, fix, Objects.class.getName()), + state.getSourceForNode(rhs), + state.getSourceForNode(lhs))) + .build()); + } + return description.build(); + } + return NO_MATCH; + } + + private static boolean seemsConstant(Tree tree) { + if (constValue(tree) != null) { + return true; + } + var symbol = getSymbol(tree); + return symbol instanceof VarSymbol && symbol.isEnum(); + } +} 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 64e829aede3..8e5c44b0847 100644 --- a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java +++ b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java @@ -412,6 +412,7 @@ import com.google.errorprone.bugpatterns.WithSignatureDiscouraged; import com.google.errorprone.bugpatterns.WrongOneof; import com.google.errorprone.bugpatterns.XorPower; +import com.google.errorprone.bugpatterns.YodaCondition; import com.google.errorprone.bugpatterns.android.BinderIdentityRestoredDangerously; import com.google.errorprone.bugpatterns.android.BundleDeserializationCast; import com.google.errorprone.bugpatterns.android.FragmentInjection; @@ -1164,7 +1165,8 @@ public static ScannerSupplier errorChecks() { VarChecker.class, Varifier.class, VoidMissingNullable.class, - WildcardImport.class + WildcardImport.class, + YodaCondition.class // keep-sorted end ); diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/YodaConditionTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/YodaConditionTest.java new file mode 100644 index 00000000000..605790b462e --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/YodaConditionTest.java @@ -0,0 +1,136 @@ +/* + * 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.BugCheckerRefactoringTestHelper; +import com.google.errorprone.BugCheckerRefactoringTestHelper.FixChoosers; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public final class YodaConditionTest { + private final BugCheckerRefactoringTestHelper refactoring = + BugCheckerRefactoringTestHelper.newInstance(YodaCondition.class, getClass()); + + @Test + public void primitive() { + refactoring + .addInputLines( + "Test.java", + "class Test {", + " boolean yoda(int a) {", + " return 4 == a;", + " }", + " boolean notYoda(int a) {", + " return a == 4;", + " }", + "}") + .addOutputLines( + "Test.java", + "class Test {", + " boolean yoda(int a) {", + " return a == 4;", + " }", + " boolean notYoda(int a) {", + " return a == 4;", + " }", + "}") + .doTest(); + } + + @Test + public void enums() { + refactoring + .addInputLines( + "E.java", + "enum E {", + " A, B;", + " boolean foo(E e) {", + " return this == e;", + " }", + "}") + .expectUnchanged() + .addInputLines( + "Test.java", + "class Test {", + " boolean yoda(E a) {", + " return E.A == a;", + " }", + " boolean notYoda(E a) {", + " return a == E.A;", + " }", + "}") + .addOutputLines( + "Test.java", + "class Test {", + " boolean yoda(E a) {", + " return a == E.A;", + " }", + " boolean notYoda(E a) {", + " return a == E.A;", + " }", + "}") + .doTest(); + } + + @Test + public void nullUnsafeFix() { + refactoring + .addInputLines("E.java", "enum E {A, B}") + .expectUnchanged() + .addInputLines( + "Test.java", + "class Test {", + " boolean yoda(E a) {", + " return E.A.equals(a);", + " }", + "}") + .addOutputLines( + "Test.java", + "class Test {", + " boolean yoda(E a) {", + " return a.equals(E.A);", + " }", + "}") + .doTest(); + } + + @Test + public void nullSafeFix() { + refactoring + .addInputLines("E.java", "enum E {A, B}") + .expectUnchanged() + .addInputLines( + "Test.java", + "class Test {", + " boolean yoda(E a) {", + " return E.A.equals(a);", + " }", + "}") + .addOutputLines( + "Test.java", + "import java.util.Objects;", + "class Test {", + " boolean yoda(E a) {", + " return Objects.equals(a, E.A);", + " }", + "}") + .setFixChooser(FixChoosers.SECOND) + .doTest(); + } +}