From 1dff9a01ae51c0ed25c1e9cdec4f446f698a7acf Mon Sep 17 00:00:00 2001 From: Liam Miller-Cushon Date: Fri, 14 Jul 2023 14:57:43 -0700 Subject: [PATCH] Add a check for mutability in Guice modules PiperOrigin-RevId: 548230672 --- .../bugpatterns/MutableGuiceModule.java | 97 +++++++++++++++++++ .../scanner/BuiltInCheckerSuppliers.java | 2 + .../bugpatterns/MutableGuiceModuleTest.java | 77 +++++++++++++++ docs/bugpattern/MutableGuiceModule.md | 1 + 4 files changed, 177 insertions(+) create mode 100644 core/src/main/java/com/google/errorprone/bugpatterns/MutableGuiceModule.java create mode 100644 core/src/test/java/com/google/errorprone/bugpatterns/MutableGuiceModuleTest.java create mode 100644 docs/bugpattern/MutableGuiceModule.md diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/MutableGuiceModule.java b/core/src/main/java/com/google/errorprone/bugpatterns/MutableGuiceModule.java new file mode 100644 index 000000000000..afcb60cbbdf9 --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/MutableGuiceModule.java @@ -0,0 +1,97 @@ +/* + * 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.prettyType; +import static com.google.errorprone.matchers.Description.NO_MATCH; +import static com.google.errorprone.util.ASTHelpers.enclosingClass; +import static com.google.errorprone.util.ASTHelpers.getSymbol; +import static com.google.errorprone.util.ASTHelpers.getType; + +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker.VariableTreeMatcher; +import com.google.errorprone.bugpatterns.threadsafety.WellKnownMutability; +import com.google.errorprone.fixes.SuggestedFixes; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.suppliers.Supplier; +import com.sun.source.tree.VariableTree; +import com.sun.tools.javac.code.Symbol; +import com.sun.tools.javac.code.Symbol.VarSymbol; +import com.sun.tools.javac.code.Type; +import javax.inject.Inject; +import javax.lang.model.element.ElementKind; +import javax.lang.model.element.Modifier; + +/** A {@link BugChecker}; see the associated {@link BugPattern} annotation for details. */ +@BugPattern(summary = "Fields in Guice modules should be final", severity = WARNING) +public class MutableGuiceModule extends BugChecker implements VariableTreeMatcher { + + private final WellKnownMutability wellKnownMutability; + + @Inject + MutableGuiceModule(WellKnownMutability wellKnownMutability) { + this.wellKnownMutability = wellKnownMutability; + } + + @Override + public Description matchVariable(VariableTree tree, VisitorState state) { + if (state.errorProneOptions().isTestOnlyTarget()) { + return NO_MATCH; + } + VarSymbol sym = getSymbol(tree); + if (sym == null) { + return NO_MATCH; + } + if (!sym.getKind().equals(ElementKind.FIELD)) { + return NO_MATCH; + } + Symbol abstractModule = ABSTRACT_MODULE.get(state); + if (abstractModule == null) { + return NO_MATCH; + } + if (!enclosingClass(sym).isSubClass(abstractModule, state.getTypes())) { + return NO_MATCH; + } + if (sym.isStatic()) { + return NO_MATCH; + } + if (!tree.getModifiers().getFlags().contains(Modifier.FINAL)) { + Description.Builder description = buildDescription(tree); + SuggestedFixes.addModifiers(tree, state, Modifier.FINAL) + .filter(f -> SuggestedFixes.compilesWithFix(f, state)) + .ifPresent(description::addFix); + state.reportMatch(description.build()); + } + Type type = getType(tree); + String nameStr = type.tsym.flatName().toString(); + if (wellKnownMutability.getKnownMutableClasses().contains(nameStr)) { + state.reportMatch( + buildDescription(tree) + .setMessage( + String.format( + "Fields in Guice modules should be immutable, but %s is mutable", + prettyType(type, state))) + .build()); + } + return NO_MATCH; + } + + private static final Supplier ABSTRACT_MODULE = + VisitorState.memoize(state -> state.getSymbolFromString("com.google.inject.AbstractModule")); +} 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 04d1dcefef44..231f61c83f56 100644 --- a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java +++ b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java @@ -249,6 +249,7 @@ import com.google.errorprone.bugpatterns.MultipleTopLevelClasses; import com.google.errorprone.bugpatterns.MultipleUnaryOperatorsInMethodCall; import com.google.errorprone.bugpatterns.MustBeClosedChecker; +import com.google.errorprone.bugpatterns.MutableGuiceModule; import com.google.errorprone.bugpatterns.MutablePublicArray; import com.google.errorprone.bugpatterns.NCopiesOfChar; import com.google.errorprone.bugpatterns.NamedLikeContextualKeyword; @@ -963,6 +964,7 @@ public static ScannerSupplier warningChecks() { ModifySourceCollectionInStream.class, MultipleParallelOrSequentialCalls.class, MultipleUnaryOperatorsInMethodCall.class, + MutableGuiceModule.class, MutablePublicArray.class, NamedLikeContextualKeyword.class, NarrowCalculation.class, diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/MutableGuiceModuleTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/MutableGuiceModuleTest.java new file mode 100644 index 000000000000..0cc9e3e3b575 --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/MutableGuiceModuleTest.java @@ -0,0 +1,77 @@ +/* + * 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.CompilationTestHelper; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public class MutableGuiceModuleTest { + private final CompilationTestHelper testHelper = + CompilationTestHelper.newInstance(MutableGuiceModule.class, getClass()); + + @Test + public void positive() { + testHelper + .addSourceLines( + "Test.java", + "import com.google.inject.AbstractModule;", + "class Test extends AbstractModule {", + " // BUG: Diagnostic contains:", + " String x = new String();", + "}") + .doTest(); + } + + @Test + public void positiveType() { + testHelper + .addSourceLines( + "Test.java", + "import com.google.inject.AbstractModule;", + "class Test extends AbstractModule {", + " // BUG: Diagnostic contains: Object is mutable", + " final Object x = new Object();", + "}") + .doTest(); + } + + @Test + public void negativeFinal() { + testHelper + .addSourceLines( + "Test.java", + "import com.google.inject.AbstractModule;", + "class Test extends AbstractModule {", + " final String x = new String();", + "}") + .doTest(); + } + + @Test + public void negativeNotAModule() { + testHelper + .addSourceLines( + "Test.java", // + "class Test {", + " String x = new String();", + "}") + .doTest(); + } +} diff --git a/docs/bugpattern/MutableGuiceModule.md b/docs/bugpattern/MutableGuiceModule.md new file mode 100644 index 000000000000..a617a3f3fa1c --- /dev/null +++ b/docs/bugpattern/MutableGuiceModule.md @@ -0,0 +1 @@ +Guice modules should not be mutable.