From b9e01a8d4f2a7a822eaf6b8f95f72f2f0ca72912 Mon Sep 17 00:00:00 2001 From: ghm Date: Fri, 1 Sep 2023 15:42:24 -0700 Subject: [PATCH] Introduce a check to flag unnecessary use of async primitives in local (and hence single-threaded) scopes. Flume: unknown commit PiperOrigin-RevId: 562065758 --- .../bugpatterns/UnnecessaryAsync.java | 120 ++++++++++++++++++ .../scanner/BuiltInCheckerSuppliers.java | 2 + .../bugpatterns/UnnecessaryAsyncTest.java | 111 ++++++++++++++++ 3 files changed, 233 insertions(+) create mode 100644 core/src/main/java/com/google/errorprone/bugpatterns/UnnecessaryAsync.java create mode 100644 core/src/test/java/com/google/errorprone/bugpatterns/UnnecessaryAsyncTest.java diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/UnnecessaryAsync.java b/core/src/main/java/com/google/errorprone/bugpatterns/UnnecessaryAsync.java new file mode 100644 index 00000000000..d50f037e660 --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/UnnecessaryAsync.java @@ -0,0 +1,120 @@ +/* + * 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.common.collect.ImmutableList.toImmutableList; +import static com.google.errorprone.matchers.Description.NO_MATCH; +import static com.google.errorprone.matchers.Matchers.anyOf; +import static com.google.errorprone.matchers.Matchers.constructor; +import static com.google.errorprone.util.ASTHelpers.getSymbol; +import static com.google.errorprone.util.ASTHelpers.isConsideredFinal; + +import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.SeverityLevel; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker.VariableTreeMatcher; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matcher; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.IdentifierTree; +import com.sun.source.tree.LambdaExpressionTree; +import com.sun.source.tree.MemberSelectTree; +import com.sun.source.tree.MethodTree; +import com.sun.source.tree.Tree; +import com.sun.source.tree.VariableTree; +import com.sun.source.util.TreePathScanner; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.stream.Stream; +import javax.lang.model.element.ElementKind; + +/** A BugPattern; see the summary. */ +@BugPattern( + severity = SeverityLevel.WARNING, + summary = + "Variables which are initialized and do not escape the current scope do not need to worry" + + " about concurrency. Using the non-concurrent type will reduce overhead and" + + " verbosity.") +public final class UnnecessaryAsync extends BugChecker implements VariableTreeMatcher { + private static final Matcher NEW_SYNCHRONIZED_THING = + anyOf( + Stream.of( + "java.util.concurrent.atomic.AtomicBoolean", + "java.util.concurrent.atomic.AtomicReference", + "java.util.concurrent.atomic.AtomicInteger", + "java.util.concurrent.atomic.AtomicLong", + "java.util.concurrent.ConcurrentHashMap") + .map(x -> constructor().forClass(x)) + .collect(toImmutableList())); + + @Override + public Description matchVariable(VariableTree tree, VisitorState state) { + var symbol = getSymbol(tree); + if (!symbol.getKind().equals(ElementKind.LOCAL_VARIABLE) || !isConsideredFinal(symbol)) { + return NO_MATCH; + } + var initializer = tree.getInitializer(); + if (initializer == null || !NEW_SYNCHRONIZED_THING.matches(initializer, state)) { + return NO_MATCH; + } + AtomicBoolean escapes = new AtomicBoolean(false); + new TreePathScanner() { + int lambdaDepth = 0; + + @Override + public Void visitMethod(MethodTree tree, Void unused) { + lambdaDepth++; + var ret = super.visitMethod(tree, null); + lambdaDepth--; + return ret; + } + + @Override + public Void visitLambdaExpression(LambdaExpressionTree tree, Void unused) { + lambdaDepth++; + var ret = super.visitLambdaExpression(tree, null); + lambdaDepth--; + return ret; + } + + @Override + public Void visitIdentifier(IdentifierTree tree, Void unused) { + if (!getSymbol(tree).equals(symbol)) { + return super.visitIdentifier(tree, null); + } + // We're in a lambda, so our symbol implicitly escapes. + if (lambdaDepth > 0) { + escapes.set(true); + return super.visitIdentifier(tree, null); + } + var parentTree = getCurrentPath().getParentPath().getLeaf(); + // Anything other than a method invocation on our symbol constitutes a reference to it + // escaping. + if (isVariableDeclarationItself(parentTree) || parentTree instanceof MemberSelectTree) { + return super.visitIdentifier(tree, null); + } + escapes.set(true); + return super.visitIdentifier(tree, null); + } + + private boolean isVariableDeclarationItself(Tree parentTree) { + return parentTree instanceof VariableTree && getSymbol(parentTree).equals(symbol); + } + }.scan(state.getPath().getParentPath(), null); + // TODO(ghm): Include an attempted fix, if possible. + return escapes.get() ? NO_MATCH : describeMatch(tree); + } +} 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 6b8dfd93716..c6a6a6740d9 100644 --- a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java +++ b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java @@ -391,6 +391,7 @@ import com.google.errorprone.bugpatterns.UnnecessarilyVisible; import com.google.errorprone.bugpatterns.UnnecessaryAnonymousClass; import com.google.errorprone.bugpatterns.UnnecessaryAssignment; +import com.google.errorprone.bugpatterns.UnnecessaryAsync; import com.google.errorprone.bugpatterns.UnnecessaryBoxedAssignment; import com.google.errorprone.bugpatterns.UnnecessaryBoxedVariable; import com.google.errorprone.bugpatterns.UnnecessaryDefaultInEnumSwitch; @@ -1048,6 +1049,7 @@ public static ScannerSupplier warningChecks() { UndefinedEquals.class, UnicodeEscape.class, UnnecessaryAssignment.class, + UnnecessaryAsync.class, UnnecessaryLambda.class, UnnecessaryLongToIntConversion.class, UnnecessaryMethodInvocationMatcher.class, diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/UnnecessaryAsyncTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/UnnecessaryAsyncTest.java new file mode 100644 index 00000000000..cf6ed680cfa --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/UnnecessaryAsyncTest.java @@ -0,0 +1,111 @@ +/* + * 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 final class UnnecessaryAsyncTest { + private final CompilationTestHelper helper = + CompilationTestHelper.newInstance(UnnecessaryAsync.class, getClass()); + + @Test + public void positive() { + helper + .addSourceLines( + "Test.java", + "import java.util.concurrent.atomic.AtomicInteger;", + "class Test {", + " int test() {", + " // BUG: Diagnostic contains:", + " var ai = new AtomicInteger();", + " ai.set(1);", + " return ai.get();", + " }", + "}") + .doTest(); + } + + @Test + public void negative_escapesScope() { + helper + .addSourceLines( + "Test.java", + "import java.util.concurrent.atomic.AtomicInteger;", + "class Test {", + " AtomicInteger test() {", + " var ai = new AtomicInteger();", + " ai.set(1);", + " return ai;", + " }", + "}") + .doTest(); + } + + @Test + public void negative_passedToAnotherMethod() { + helper + .addSourceLines( + "Test.java", + "import java.util.concurrent.atomic.AtomicInteger;", + "class Test {", + " void test() {", + " var ai = new AtomicInteger();", + " ai.set(1);", + " frobnicate(ai);", + " }", + " void frobnicate(Number n) {}", + "}") + .doTest(); + } + + @Test + public void positive_uselessConcurrentMap() { + helper + .addSourceLines( + "Test.java", + "import java.util.concurrent.ConcurrentHashMap;", + "class Test {", + " int test() {", + " // BUG: Diagnostic contains:", + " var chm = new ConcurrentHashMap<>();", + " chm.put(1, 2);", + " return chm.size();", + " }", + "}") + .doTest(); + } + + @Test + public void negative_capturedByLambda() { + helper + .addSourceLines( + "Test.java", + "import java.util.concurrent.atomic.AtomicInteger;", + "import java.util.List;", + "class Test {", + " long test(List xs) {", + " var ai = new AtomicInteger();", + " return xs.stream().mapToLong(x -> ai.getAndIncrement()).sum();", + " }", + "}") + .doTest(); + } +}