From a046d80714d6f2ca05558c5b0d5124bea28809a6 Mon Sep 17 00:00:00 2001 From: ghm Date: Thu, 7 Sep 2023 09:52:14 -0700 Subject: [PATCH] Add a check for iterating over `multimap.keys()`, which almost always seems to be a mistake. PiperOrigin-RevId: 563462231 --- .../errorprone/bugpatterns/MultimapKeys.java | 65 +++++++++++ .../scanner/BuiltInCheckerSuppliers.java | 2 + .../bugpatterns/MultimapKeysTest.java | 110 ++++++++++++++++++ 3 files changed, 177 insertions(+) create mode 100644 core/src/main/java/com/google/errorprone/bugpatterns/MultimapKeys.java create mode 100644 core/src/test/java/com/google/errorprone/bugpatterns/MultimapKeysTest.java diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/MultimapKeys.java b/core/src/main/java/com/google/errorprone/bugpatterns/MultimapKeys.java new file mode 100644 index 00000000000..64261a515df --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/MultimapKeys.java @@ -0,0 +1,65 @@ +/* + * 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.renameMethodInvocation; +import static com.google.errorprone.matchers.Description.NO_MATCH; +import static com.google.errorprone.matchers.method.MethodMatchers.instanceMethod; + +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matcher; +import com.sun.source.tree.EnhancedForLoopTree; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.MemberSelectTree; +import com.sun.source.tree.MethodInvocationTree; + +/** A BugPattern; see the summary. */ +@BugPattern( + summary = + "Iterating over `Multimap.keys()` does not collapse duplicates. Did you mean `keySet()`?", + severity = WARNING) +public final class MultimapKeys extends BugChecker implements MethodInvocationTreeMatcher { + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + return KEYS.matches(tree, state) && isBeingIteratedOver(state) + ? describeMatch(tree, renameMethodInvocation(tree, "keySet", state)) + : NO_MATCH; + } + + private static boolean isBeingIteratedOver(VisitorState state) { + var parent = state.getPath().getParentPath().getLeaf(); + if (parent instanceof EnhancedForLoopTree) { + return true; + } + if (parent instanceof MemberSelectTree) { + var grandparent = state.getPath().getParentPath().getParentPath().getLeaf(); + return grandparent instanceof MethodInvocationTree + && FOR_EACH.matches((ExpressionTree) grandparent, state); + } + return false; + } + + private static final Matcher KEYS = + instanceMethod().onDescendantOf("com.google.common.collect.Multimap").named("keys"); + + private static final Matcher FOR_EACH = + instanceMethod().onDescendantOf("java.util.Collection").named("forEach"); +} 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 c6d1533a419..332c5d80a3a 100644 --- a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java +++ b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java @@ -247,6 +247,7 @@ import com.google.errorprone.bugpatterns.ModifySourceCollectionInStream; import com.google.errorprone.bugpatterns.ModifyingCollectionWithItself; import com.google.errorprone.bugpatterns.MultiVariableDeclaration; +import com.google.errorprone.bugpatterns.MultimapKeys; import com.google.errorprone.bugpatterns.MultipleParallelOrSequentialCalls; import com.google.errorprone.bugpatterns.MultipleTopLevelClasses; import com.google.errorprone.bugpatterns.MultipleUnaryOperatorsInMethodCall; @@ -971,6 +972,7 @@ public static ScannerSupplier warningChecks() { ModifiedButNotUsed.class, ModifyCollectionInEnhancedForLoop.class, ModifySourceCollectionInStream.class, + MultimapKeys.class, MultipleParallelOrSequentialCalls.class, MultipleUnaryOperatorsInMethodCall.class, MutablePublicArray.class, diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/MultimapKeysTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/MultimapKeysTest.java new file mode 100644 index 00000000000..1a7b52a5227 --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/MultimapKeysTest.java @@ -0,0 +1,110 @@ +/* + * 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 org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public final class MultimapKeysTest { + private final BugCheckerRefactoringTestHelper refactoring = + BugCheckerRefactoringTestHelper.newInstance(MultimapKeys.class, getClass()); + + @Test + public void positive() { + refactoring + .addInputLines( + "Test.java", + "import com.google.common.collect.Multimap;", + "class Test {", + " void test(Multimap m) {", + " for (String k : m.keys()) {}", + " }", + "}") + .addOutputLines( + "Test.java", + "import com.google.common.collect.Multimap;", + "class Test {", + " void test(Multimap m) {", + " for (String k : m.keySet()) {}", + " }", + "}") + .doTest(); + } + + @Test + public void positiveSubclass() { + refactoring + .addInputLines( + "Test.java", + "import com.google.common.collect.SetMultimap;", + "class Test {", + " void test(SetMultimap m) {", + " for (String k : m.keys()) {}", + " }", + "}") + .addOutputLines( + "Test.java", + "import com.google.common.collect.SetMultimap;", + "class Test {", + " void test(SetMultimap m) {", + " for (String k : m.keySet()) {}", + " }", + "}") + .doTest(); + } + + @Test + public void positiveFunctional() { + refactoring + .addInputLines( + "Test.java", + "import com.google.common.collect.Multimap;", + "class Test {", + " void test(Multimap m) {", + " m.keys().forEach(x -> {});", + " }", + "}") + .addOutputLines( + "Test.java", + "import com.google.common.collect.Multimap;", + "class Test {", + " void test(Multimap m) {", + " m.keySet().forEach(x -> {});", + " }", + "}") + .doTest(); + } + + @Test + public void negative() { + refactoring + .addInputLines( + "Test.java", + "import com.google.common.collect.Multimap;", + "import com.google.common.collect.Multiset;", + "class Test {", + " Multiset test(Multimap m) {", + " return m.keys();", + " }", + "}") + .expectUnchanged() + .doTest(); + } +}