diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/GuiceCreateInjectorWithCombineRefactor.java b/core/src/main/java/com/google/errorprone/bugpatterns/GuiceCreateInjectorWithCombineRefactor.java deleted file mode 100644 index 11753fb4787..00000000000 --- a/core/src/main/java/com/google/errorprone/bugpatterns/GuiceCreateInjectorWithCombineRefactor.java +++ /dev/null @@ -1,67 +0,0 @@ -/* - * 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.method.MethodMatchers.staticMethod; -import static java.util.stream.Collectors.joining; - -import com.google.errorprone.BugPattern; -import com.google.errorprone.VisitorState; -import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; -import com.google.errorprone.fixes.SuggestedFix; -import com.google.errorprone.matchers.Description; -import com.google.errorprone.matchers.Matcher; -import com.sun.source.tree.ExpressionTree; -import com.sun.source.tree.MethodInvocationTree; -import java.util.List; - -/** A {@link BugChecker}; see the associated {@link BugPattern} annotation for details. */ -@BugPattern( - summary = "Nesting Modules.combine() inside Guice.createInjector() is unnecessary.", - severity = WARNING) -public class GuiceCreateInjectorWithCombineRefactor extends BugChecker - implements MethodInvocationTreeMatcher { - private static final Matcher GUICE_CREATE_INJECTOR_METHOD = - staticMethod().onClass("com.google.inject.Guice").named("createInjector"); - - private static final Matcher MODULES_COMBINE_METHOD = - staticMethod().onClass("com.google.inject.util.Modules").named("combine"); - - /** Matches if Guice.createInjector() is called with a Modules.combine() argument. */ - @Override - public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { - if (!GUICE_CREATE_INJECTOR_METHOD.matches(tree, state)) { - return Description.NO_MATCH; - } - - SuggestedFix.Builder suggestedFixBuilder = SuggestedFix.builder(); - for (ExpressionTree arg : tree.getArguments()) { - if (MODULES_COMBINE_METHOD.matches(arg, state)) { - MethodInvocationTree combineInvocation = (MethodInvocationTree) arg; - List subModules = combineInvocation.getArguments(); - String replacement = - subModules.stream() - .map(module -> state.getSourceForNode(module)) - .collect(joining(", ")); - suggestedFixBuilder.replace(arg, replacement); - } - } - var fix = suggestedFixBuilder.build(); - return fix.isEmpty() ? Description.NO_MATCH : describeMatch(tree, fix); - } -} diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/GuiceNestedCombine.java b/core/src/main/java/com/google/errorprone/bugpatterns/GuiceNestedCombine.java new file mode 100644 index 00000000000..054ae75370a --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/GuiceNestedCombine.java @@ -0,0 +1,91 @@ +/* + * 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.collect.Iterables.getLast; +import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; +import static com.google.errorprone.matchers.Description.NO_MATCH; +import static com.google.errorprone.matchers.method.MethodMatchers.staticMethod; +import static com.google.errorprone.suppliers.Suppliers.typeFromString; +import static com.google.errorprone.util.ASTHelpers.getStartPosition; +import static com.google.errorprone.util.ASTHelpers.getSymbol; +import static com.google.errorprone.util.ASTHelpers.getType; +import static com.google.errorprone.util.ASTHelpers.isSubtype; + +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; +import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.suppliers.Supplier; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.tools.javac.code.Type; +import java.util.List; + +/** A {@link BugChecker}; see the associated {@link BugPattern} annotation for details. */ +@BugPattern(summary = "Nesting Modules.combine() here is unnecessary.", severity = WARNING) +public final class GuiceNestedCombine extends BugChecker implements MethodInvocationTreeMatcher { + private static final Matcher MODULES_COMBINE_METHOD = + staticMethod().onClass("com.google.inject.util.Modules").named("combine"); + + private static final Supplier MODULE = typeFromString("com.google.inject.Module"); + + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + if (!MODULES_COMBINE_METHOD.matches(tree, state)) { + return NO_MATCH; + } + var module = MODULE.get(state); + if (module == null + || tree.getArguments().isEmpty() + || !tree.getArguments().stream().allMatch(a -> isSubtype(getType(a), module, state))) { + return NO_MATCH; + } + + var parent = state.getPath().getParentPath().getLeaf(); + if (!(parent instanceof MethodInvocationTree)) { + return NO_MATCH; + } + if (!isInVarargsPosition(tree, (MethodInvocationTree) parent, state)) { + return NO_MATCH; + } + + var fix = + SuggestedFix.builder() + .replace(getStartPosition(tree), getStartPosition(tree.getArguments().get(0)), "") + .replace( + state.getEndPosition(getLast(tree.getArguments())), state.getEndPosition(tree), "") + .build(); + return describeMatch(tree, fix); + } + + private static boolean isInVarargsPosition( + ExpressionTree argTree, MethodInvocationTree methodInvocationTree, VisitorState state) { + var methodSymbol = getSymbol(methodInvocationTree); + if (!methodSymbol.isVarArgs()) { + return false; + } + int parameterCount = methodSymbol.getParameters().size(); + List arguments = methodInvocationTree.getArguments(); + // Don't match if we're passing an array into a varargs parameter, but do match if there are + // other parameters along with it. + return (arguments.size() > parameterCount || !state.getTypes().isArray(getType(argTree))) + && arguments.indexOf(argTree) >= parameterCount - 1; + } +} 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 e366d98c370..d265791377f 100644 --- a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java +++ b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java @@ -157,7 +157,7 @@ import com.google.errorprone.bugpatterns.GetClassOnAnnotation; import com.google.errorprone.bugpatterns.GetClassOnClass; import com.google.errorprone.bugpatterns.GetClassOnEnum; -import com.google.errorprone.bugpatterns.GuiceCreateInjectorWithCombineRefactor; +import com.google.errorprone.bugpatterns.GuiceNestedCombine; import com.google.errorprone.bugpatterns.HashtableContains; import com.google.errorprone.bugpatterns.HidingField; import com.google.errorprone.bugpatterns.ICCProfileGetInstance; @@ -918,7 +918,7 @@ public static ScannerSupplier warningChecks() { FragmentNotInstantiable.class, FutureReturnValueIgnored.class, GetClassOnEnum.class, - GuiceCreateInjectorWithCombineRefactor.class, + GuiceNestedCombine.class, HidingField.class, ICCProfileGetInstance.class, IdentityHashMapUsage.class, diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/GuiceCreateInjectorWithCombineRefactorTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/GuiceCreateInjectorWithCombineRefactorTest.java deleted file mode 100644 index 82c9c67419f..00000000000 --- a/core/src/test/java/com/google/errorprone/bugpatterns/GuiceCreateInjectorWithCombineRefactorTest.java +++ /dev/null @@ -1,112 +0,0 @@ -/* - * 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 com.google.errorprone.BugCheckerRefactoringTestHelper; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.junit.runners.JUnit4; - -@RunWith(JUnit4.class) -public final class GuiceCreateInjectorWithCombineRefactorTest { - private final BugCheckerRefactoringTestHelper refactoringTestHelper = - BugCheckerRefactoringTestHelper.newInstance( - GuiceCreateInjectorWithCombineRefactor.class, getClass()); - - @Test - public void positive() { - refactoringTestHelper - .addInputLines( - "Test.java", - "import com.google.inject.AbstractModule;", - "import com.google.inject.Guice;", - "import com.google.inject.util.Modules;", - "class Test {", - " private class ModuleA extends AbstractModule {}", - " private class ModuleB extends AbstractModule {}", - " private class ModuleC extends AbstractModule {}", - " public void test() {", - " Guice.createInjector(", - " new ModuleA(),", - " Modules.combine(new ModuleB(), new ModuleC()));", - " }", - "}") - .addOutputLines( - "Test.java", - "import com.google.inject.AbstractModule;", - "import com.google.inject.Guice;", - "import com.google.inject.util.Modules;", - "class Test {", - " private class ModuleA extends AbstractModule {}", - " private class ModuleB extends AbstractModule {}", - " private class ModuleC extends AbstractModule {}", - " public void test() {", - " Guice.createInjector(", - " new ModuleA(),", - " new ModuleB(),", - " new ModuleC());", - " }", - "}") - .doTest(); - } - - @Test - public void negative_whenNoCombine() { - refactoringTestHelper - .addInputLines( - "Test.java", - "import com.google.inject.AbstractModule;", - "import com.google.inject.Guice;", - "import com.google.inject.util.Modules;", - "class Test {", - " private class ModuleA extends AbstractModule {}", - " private class ModuleB extends AbstractModule {}", - " private class ModuleC extends AbstractModule {}", - " public void test() {", - " Guice.createInjector(", - " new ModuleA(),", - " new ModuleB(),", - " new ModuleC());", - " }", - "}") - .expectUnchanged() - .doTest(); - } - - @Test - public void negative_whenNoCreateInjector() { - refactoringTestHelper - .addInputLines( - "Test.java", - "import com.google.inject.AbstractModule;", - "import com.google.inject.Module;", - "import com.google.inject.util.Modules;", - "class Test {", - " private class ModuleA extends AbstractModule {}", - " private class ModuleB extends AbstractModule {}", - " private class ModuleC extends AbstractModule {}", - " public void test() {", - " Module extraModule = Modules.combine(", - " new ModuleA(),", - " new ModuleB(),", - " new ModuleC());", - " }", - "}") - .expectUnchanged() - .doTest(); - } -} diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/GuiceNestedCombineTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/GuiceNestedCombineTest.java new file mode 100644 index 00000000000..371d46271d4 --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/GuiceNestedCombineTest.java @@ -0,0 +1,307 @@ +/* + * 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 com.google.errorprone.BugCheckerRefactoringTestHelper; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public final class GuiceNestedCombineTest { + private final BugCheckerRefactoringTestHelper refactoringTestHelper = + BugCheckerRefactoringTestHelper.newInstance(GuiceNestedCombine.class, getClass()); + + @Test + public void positive() { + refactoringTestHelper + .addInputLines( + "Test.java", + "import com.google.inject.AbstractModule;", + "import com.google.inject.Guice;", + "import com.google.inject.util.Modules;", + "class Test {", + " private class ModuleA extends AbstractModule {}", + " private class ModuleB extends AbstractModule {}", + " private class ModuleC extends AbstractModule {}", + " public void test() {", + " Guice.createInjector(", + " new ModuleA(),", + " Modules.combine(new ModuleB(), new ModuleC()));", + " }", + "}") + .addOutputLines( + "Test.java", + "import com.google.inject.AbstractModule;", + "import com.google.inject.Guice;", + "import com.google.inject.util.Modules;", + "class Test {", + " private class ModuleA extends AbstractModule {}", + " private class ModuleB extends AbstractModule {}", + " private class ModuleC extends AbstractModule {}", + " public void test() {", + " Guice.createInjector(", + " new ModuleA(),", + " new ModuleB(),", + " new ModuleC());", + " }", + "}") + .doTest(); + } + + @Test + public void arbitraryVarargsMethod_combineCollapsed() { + refactoringTestHelper + .addInputLines( + "Test.java", + "import com.google.inject.AbstractModule;", + "import com.google.inject.Module;", + "import com.google.inject.util.Modules;", + "class Test {", + " private class ModuleA extends AbstractModule {}", + " private class ModuleB extends AbstractModule {}", + " private class ModuleC extends AbstractModule {}", + " public void test() {", + " foo(", + " new ModuleA(),", + " Modules.combine(new ModuleB(), new ModuleC()));", + " }", + " public void foo(Module... xs) {}", + "}") + .addOutputLines( + "Test.java", + "import com.google.inject.AbstractModule;", + "import com.google.inject.Module;", + "import com.google.inject.util.Modules;", + "class Test {", + " private class ModuleA extends AbstractModule {}", + " private class ModuleB extends AbstractModule {}", + " private class ModuleC extends AbstractModule {}", + " public void test() {", + " foo(", + " new ModuleA(),", + " new ModuleB(), new ModuleC());", + " }", + " public void foo(Module... xs) {}", + "}") + .doTest(); + } + + @Test + public void singleArgument_collapsed() { + refactoringTestHelper + .addInputLines( + "Test.java", + "import com.google.inject.AbstractModule;", + "import com.google.inject.Module;", + "import com.google.inject.util.Modules;", + "class Test {", + " private class ModuleA extends AbstractModule {}", + " public void test() {", + " foo(", + " new ModuleA(),", + " Modules.combine(new ModuleA()));", + " }", + " public void foo(Module... xs) {}", + "}") + .addOutputLines( + "Test.java", + "import com.google.inject.AbstractModule;", + "import com.google.inject.Module;", + "import com.google.inject.util.Modules;", + "class Test {", + " private class ModuleA extends AbstractModule {}", + " public void test() {", + " foo(", + " new ModuleA(),", + " new ModuleA());", + " }", + " public void foo(Module... xs) {}", + "}") + .doTest(); + } + + @Test + public void noArguments_ignored() { + refactoringTestHelper + .addInputLines( + "Test.java", + "import com.google.inject.AbstractModule;", + "import com.google.inject.Module;", + "import com.google.inject.util.Modules;", + "class Test {", + " private class ModuleA extends AbstractModule {}", + " public void test() {", + " foo(", + " new ModuleA(),", + " Modules.combine());", + " }", + " public void foo(Module... xs) {}", + "}") + .expectUnchanged() + .doTest(); + } + + @Test + public void iterableOverload_noFinding() { + refactoringTestHelper + .addInputLines( + "Test.java", + "import com.google.common.collect.ImmutableList;", + "import com.google.inject.AbstractModule;", + "import com.google.inject.Module;", + "import com.google.inject.util.Modules;", + "class Test {", + " private class ModuleA extends AbstractModule {}", + " public void test() {", + " foo(", + " new ModuleA(),", + " Modules.combine(ImmutableList.of(new ModuleA())));", + " }", + " public void foo(Module... xs) {}", + "}") + .expectUnchanged() + .doTest(); + } + + @Test + public void varargsMethod_arrayInputToCombine_noFinding() { + refactoringTestHelper + .addInputLines( + "Test.java", + "import com.google.inject.AbstractModule;", + "import com.google.inject.Module;", + "import com.google.inject.util.Modules;", + "class Test {", + " public void test(Module[] ms) {", + " foo(Modules.combine(ms));", + " }", + " public void foo(Module... xs) {}", + "}") + .expectUnchanged() + .doTest(); + } + + @Test + public void notVargs_noFinding() { + refactoringTestHelper + .addInputLines( + "Test.java", + "import com.google.inject.AbstractModule;", + "import com.google.inject.Module;", + "import com.google.inject.util.Modules;", + "class Test {", + " private class ModuleA extends AbstractModule {}", + " private class ModuleB extends AbstractModule {}", + " private class ModuleC extends AbstractModule {}", + " public void test() {", + " foo(", + " new ModuleA(),", + " Modules.combine(new ModuleB(), new ModuleC()));", + " }", + " public void foo(Module a, Module b) {}", + "}") + .expectUnchanged() + .doTest(); + } + + @Test + public void partialVarargs_collapsed() { + refactoringTestHelper + .addInputLines( + "Test.java", + "import com.google.inject.AbstractModule;", + "import com.google.inject.Module;", + "import com.google.inject.util.Modules;", + "class Test {", + " private class ModuleA extends AbstractModule {}", + " private class ModuleB extends AbstractModule {}", + " private class ModuleC extends AbstractModule {}", + " public void test() {", + " foo(", + " new ModuleA(),", + " Modules.combine(new ModuleB(), new ModuleC()),", + " Modules.combine(new ModuleB(), new ModuleC()));", + " }", + " public void foo(Module a, Module... b) {}", + "}") + .addOutputLines( + "Test.java", + "import com.google.inject.AbstractModule;", + "import com.google.inject.Module;", + "import com.google.inject.util.Modules;", + "class Test {", + " private class ModuleA extends AbstractModule {}", + " private class ModuleB extends AbstractModule {}", + " private class ModuleC extends AbstractModule {}", + " public void test() {", + " foo(", + " new ModuleA(),", + " new ModuleB(), new ModuleC(),", + " new ModuleB(), new ModuleC());", + " }", + " public void foo(Module a, Module... b) {}", + "}") + .doTest(); + } + + @Test + public void noCombine_noFinding() { + refactoringTestHelper + .addInputLines( + "Test.java", + "import com.google.inject.AbstractModule;", + "import com.google.inject.Guice;", + "import com.google.inject.util.Modules;", + "class Test {", + " private class ModuleA extends AbstractModule {}", + " private class ModuleB extends AbstractModule {}", + " private class ModuleC extends AbstractModule {}", + " public void test() {", + " Guice.createInjector(", + " new ModuleA(),", + " new ModuleB(),", + " new ModuleC());", + " }", + "}") + .expectUnchanged() + .doTest(); + } + + @Test + public void assignedToVariable_noFinding() { + refactoringTestHelper + .addInputLines( + "Test.java", + "import com.google.inject.AbstractModule;", + "import com.google.inject.Module;", + "import com.google.inject.util.Modules;", + "class Test {", + " private class ModuleA extends AbstractModule {}", + " private class ModuleB extends AbstractModule {}", + " private class ModuleC extends AbstractModule {}", + " public void test() {", + " Module extraModule = Modules.combine(", + " new ModuleA(),", + " new ModuleB(),", + " new ModuleC());", + " }", + "}") + .expectUnchanged() + .doTest(); + } +}