From 3d065dedd372426aa392f5379f66d508f8306a19 Mon Sep 17 00:00:00 2001 From: ghm Date: Thu, 2 May 2024 06:10:59 -0700 Subject: [PATCH] Add an initial check to simplify pattern matching instanceofs. This only handles the simplest case of, ``` if (x instanceof Foo) { var foo = (Foo) x; ... } ``` While we should also be catching: ``` if (!(x instanceof Foo)) { ... } ... var foo = (Foo) x; ``` That's certainly a bit harder to do, though! PiperOrigin-RevId: 630041067 --- .../google/errorprone/util/SourceVersion.java | 5 + .../PatternMatchingInstanceof.java | 121 ++++++++++++++ .../scanner/BuiltInCheckerSuppliers.java | 2 + .../PatternMatchingInstanceofTest.java | 151 ++++++++++++++++++ 4 files changed, 279 insertions(+) create mode 100644 core/src/main/java/com/google/errorprone/bugpatterns/PatternMatchingInstanceof.java create mode 100644 core/src/test/java/com/google/errorprone/bugpatterns/PatternMatchingInstanceofTest.java diff --git a/check_api/src/main/java/com/google/errorprone/util/SourceVersion.java b/check_api/src/main/java/com/google/errorprone/util/SourceVersion.java index 39fdc7d6947..bceb86a51e4 100644 --- a/check_api/src/main/java/com/google/errorprone/util/SourceVersion.java +++ b/check_api/src/main/java/com/google/errorprone/util/SourceVersion.java @@ -40,6 +40,11 @@ public static boolean supportsEffectivelyFinal(Context context) { return sourceIsAtLeast(context, 8); } + /** Returns whether the compiler supports pattern-matching instanceofs. */ + public static boolean supportsPatternMatchingInstanceof(Context context) { + return sourceIsAtLeast(context, 21); + } + private static boolean sourceIsAtLeast(Context context, int version) { Source lowerBound = Source.lookup(Integer.toString(version)); return lowerBound != null && Source.instance(context).compareTo(lowerBound) >= 0; diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/PatternMatchingInstanceof.java b/core/src/main/java/com/google/errorprone/bugpatterns/PatternMatchingInstanceof.java new file mode 100644 index 00000000000..9842d71e613 --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/PatternMatchingInstanceof.java @@ -0,0 +1,121 @@ +/* + * Copyright 2024 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.matchers.Description.NO_MATCH; +import static com.google.errorprone.util.ASTHelpers.getSymbol; +import static com.google.errorprone.util.ASTHelpers.getType; +import static com.google.errorprone.util.ASTHelpers.isSameType; +import static com.google.errorprone.util.SourceVersion.supportsPatternMatchingInstanceof; + +import com.google.common.collect.ImmutableSet; +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker.IfTreeMatcher; +import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.matchers.Description; +import com.sun.source.tree.BinaryTree; +import com.sun.source.tree.BlockTree; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.IfTree; +import com.sun.source.tree.InstanceOfTree; +import com.sun.source.tree.ParenthesizedTree; +import com.sun.source.tree.Tree.Kind; +import com.sun.source.tree.TypeCastTree; +import com.sun.source.tree.VariableTree; +import com.sun.source.util.SimpleTreeVisitor; +import com.sun.tools.javac.code.Symbol.VarSymbol; + +/** A BugPattern; see the summary. */ +@BugPattern( + severity = WARNING, + summary = "This code can be simplified to use a pattern-matching instanceof.") +public final class PatternMatchingInstanceof extends BugChecker implements IfTreeMatcher { + @Override + public Description matchIf(IfTree tree, VisitorState state) { + if (!supportsPatternMatchingInstanceof(state.context)) { + return NO_MATCH; + } + ImmutableSet instanceofChecks = scanForInstanceOf(tree.getCondition()); + if (instanceofChecks.isEmpty()) { + return NO_MATCH; + } + var body = tree.getThenStatement(); + if (!(body instanceof BlockTree)) { + return NO_MATCH; + } + var block = (BlockTree) body; + if (block.getStatements().isEmpty()) { + return NO_MATCH; + } + var firstStatement = block.getStatements().get(0); + if (!(firstStatement instanceof VariableTree)) { + return NO_MATCH; + } + var variableTree = (VariableTree) firstStatement; + if (!(variableTree.getInitializer() instanceof TypeCastTree)) { + return NO_MATCH; + } + var typeCast = (TypeCastTree) variableTree.getInitializer(); + var matchingInstanceof = + instanceofChecks.stream() + .filter( + i -> + isSameType(getType(i.getType()), getType(typeCast.getType()), state) + && getSymbol(i.getExpression()) instanceof VarSymbol + && getSymbol(i.getExpression()).equals(getSymbol(typeCast.getExpression()))) + .findFirst() + .orElse(null); + if (matchingInstanceof == null) { + return NO_MATCH; + } + return describeMatch( + firstStatement, + SuggestedFix.builder() + .delete(variableTree) + .postfixWith(matchingInstanceof, " " + variableTree.getName().toString()) + .build()); + } + + private ImmutableSet scanForInstanceOf(ExpressionTree condition) { + ImmutableSet.Builder instanceOfs = ImmutableSet.builder(); + new SimpleTreeVisitor() { + @Override + public Void visitParenthesized(ParenthesizedTree tree, Void unused) { + return visit(tree.getExpression(), null); + } + + @Override + public Void visitBinary(BinaryTree tree, Void unused) { + if (tree.getKind() != Kind.CONDITIONAL_AND) { + return null; + } + visit(tree.getLeftOperand(), null); + visit(tree.getRightOperand(), null); + return null; + } + + @Override + public Void visitInstanceOf(InstanceOfTree tree, Void unused) { + instanceOfs.add(tree); + return null; + } + }.visit(condition, null); + return instanceOfs.build(); + } +} 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 99dd823e182..e366d98c370 100644 --- a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java +++ b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java @@ -301,6 +301,7 @@ import com.google.errorprone.bugpatterns.ParameterComment; import com.google.errorprone.bugpatterns.ParameterName; import com.google.errorprone.bugpatterns.ParametersButNotParameterized; +import com.google.errorprone.bugpatterns.PatternMatchingInstanceof; import com.google.errorprone.bugpatterns.PreconditionsCheckNotNullRepeated; import com.google.errorprone.bugpatterns.PreconditionsInvalidPlaceholder; import com.google.errorprone.bugpatterns.PreferredInterfaceType; @@ -1030,6 +1031,7 @@ public static ScannerSupplier warningChecks() { OverridesGuiceInjectableMethod.class, OverridingMethodInconsistentArgumentNamesChecker.class, ParameterName.class, + PatternMatchingInstanceof.class, PreconditionsCheckNotNullRepeated.class, PrimitiveAtomicReference.class, ProtectedMembersInFinalClass.class, diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/PatternMatchingInstanceofTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/PatternMatchingInstanceofTest.java new file mode 100644 index 00000000000..57ee03e09a3 --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/PatternMatchingInstanceofTest.java @@ -0,0 +1,151 @@ +/* + * Copyright 2024 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.truth.TruthJUnit.assume; + +import com.google.errorprone.BugCheckerRefactoringTestHelper; +import com.google.errorprone.util.RuntimeVersion; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public final class PatternMatchingInstanceofTest { + private final BugCheckerRefactoringTestHelper helper = + BugCheckerRefactoringTestHelper.newInstance(PatternMatchingInstanceof.class, getClass()); + + @Test + public void positive() { + assume().that(RuntimeVersion.isAtLeast21()).isTrue(); + helper + .addInputLines( + "Test.java", + "class Test {", + " void test(Object o) {", + " if (o instanceof Test) {", + " Test test = (Test) o;", + " test(test);", + " }", + " }", + "}") + .addOutputLines( + "Test.java", + "class Test {", + " void test(Object o) {", + " if (o instanceof Test test) {", + " test(test);", + " }", + " }", + "}") + .doTest(); + } + + @Test + public void notDefinitelyChecked_noFinding() { + helper + .addInputLines( + "Test.java", + "class Test {", + " void test(Object o) {", + " if (o instanceof Test || o.hashCode() > 0) {", + " Test test = (Test) o;", + " test(test);", + " }", + " }", + "}") + .expectUnchanged() + .doTest(); + } + + @Test + public void moreChecksInIf_stillMatches() { + assume().that(RuntimeVersion.isAtLeast21()).isTrue(); + helper + .addInputLines( + "Test.java", + "class Test {", + " void test(Object o) {", + " if (o instanceof Test && o.hashCode() != 1) {", + " Test test = (Test) o;", + " test(test);", + " }", + " }", + "}") + .addOutputLines( + "Test.java", + "class Test {", + " void test(Object o) {", + " if (o instanceof Test test && o.hashCode() != 1) {", + " test(test);", + " }", + " }", + "}") + .doTest(); + } + + @Test + public void differentTypeToCheck_noFinding() { + helper + .addInputLines( + "Test.java", + "class Test {", + " void test(Object o) {", + " if (o instanceof Test) {", + " Integer test = (Integer) o;", + " test(test);", + " }", + " }", + "}") + .expectUnchanged() + .doTest(); + } + + @Test + public void noInstanceofAtAll_noFinding() { + helper + .addInputLines( + "Test.java", + "class Test {", + " void test(Object o) {", + " if (o.hashCode() > 0) {", + " Integer test = (Integer) o;", + " test(test);", + " }", + " }", + "}") + .expectUnchanged() + .doTest(); + } + + @Test + public void differentVariable() { + helper + .addInputLines( + "Test.java", + "class Test {", + " void test(Object x, Object y) {", + " if (x instanceof Test) {", + " Test test = (Test) y;", + " test(test, null);", + " }", + " }", + "}") + .expectUnchanged() + .doTest(); + } +}