diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ExecutorSubmitRunnableFutureIgnored.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ExecutorSubmitRunnableFutureIgnored.java new file mode 100644 index 000000000..efb01ecfc --- /dev/null +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ExecutorSubmitRunnableFutureIgnored.java @@ -0,0 +1,66 @@ +/* + * (c) Copyright 2018 Palantir Technologies Inc. All rights reserved. + * + * 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.palantir.baseline.errorprone; + +import com.google.auto.service.AutoService; +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.AbstractReturnValueIgnored; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.fixes.SuggestedFixes; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.matchers.method.MethodMatchers; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.MethodInvocationTree; +import java.util.concurrent.ExecutorService; + +@AutoService(BugChecker.class) +@BugPattern( + name = "ExecutorSubmitRunnableFutureIgnored", + link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks", + linkType = BugPattern.LinkType.CUSTOM, + providesFix = BugPattern.ProvidesFix.REQUIRES_HUMAN_ATTENTION, + severity = BugPattern.SeverityLevel.ERROR, + summary = "Uncaught exceptions from ExecutorService.submit are not logged by the uncaught exception handler " + + "because it is assumed that the returned future is used to watch for failures.\n" + + "When the returned future is ignored, using ExecutorService.execute is preferred because " + + "failures are recorded.") +public final class ExecutorSubmitRunnableFutureIgnored extends AbstractReturnValueIgnored { + + private static final Matcher MATCHER = MethodMatchers.instanceMethod() + .onDescendantOf(ExecutorService.class.getName()) + .named("submit") + .withParameters(Runnable.class.getName()); + + @Override + public Matcher specializedMatcher() { + return MATCHER; + } + + // Override matchMethodInvocation from AbstractReturnValueIgnored to apply our suggested fix. + @Override + public Description matchMethodInvocation(MethodInvocationTree methodInvocationTree, VisitorState state) { + Description description = super.matchMethodInvocation(methodInvocationTree, state); + if (Description.NO_MATCH.equals(description)) { + return description; + } + return buildDescription(methodInvocationTree) + .addFix(SuggestedFixes.renameMethodInvocation(methodInvocationTree, "execute", state)) + .build(); + } +} diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ExecutorSubmitRunnableFutureIgnoredTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ExecutorSubmitRunnableFutureIgnoredTest.java new file mode 100644 index 000000000..f25cbdad6 --- /dev/null +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ExecutorSubmitRunnableFutureIgnoredTest.java @@ -0,0 +1,74 @@ +/* + * (c) Copyright 2018 Palantir Technologies Inc. All rights reserved. + * + * 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.palantir.baseline.errorprone; + +import com.google.errorprone.BugCheckerRefactoringTestHelper; +import com.google.errorprone.CompilationTestHelper; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Future; +import org.junit.jupiter.api.Test; + +class ExecutorSubmitRunnableFutureIgnoredTest { + + @Test + void testFix() { + BugCheckerRefactoringTestHelper.newInstance(new ExecutorSubmitRunnableFutureIgnored(), getClass()) + .addInputLines( + "Test.java", + "import " + ExecutorService.class.getName() + ';', + "class Test {", + " void f(ExecutorService exec) {", + " exec.submit(() -> System.out.println(\"Hello\"));", + " }", + "}") + .addOutputLines( + "Test.java", + "import " + ExecutorService.class.getName() + ';', + "class Test {", + " void f(ExecutorService exec) {", + " exec.execute(() -> System.out.println(\"Hello\"));", + " }", + "}") + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + + @Test + void testNegative() { + CompilationTestHelper.newInstance(ExecutorSubmitRunnableFutureIgnored.class, getClass()).addSourceLines( + "Test.java", + "import " + ExecutorService.class.getName() + ';', + "import " + Future.class.getName() + ';', + "class Test {", + " void f(ExecutorService exec) {", + " Future future = exec.submit(() -> System.out.println(\"Hello\"));", + " }", + "}").doTest(); + } + + @Test + void testMessage() { + CompilationTestHelper.newInstance(ExecutorSubmitRunnableFutureIgnored.class, getClass()).addSourceLines( + "Test.java", + "import " + ExecutorService.class.getName() + ';', + "class Test {", + " void f(ExecutorService exec) {", + " // BUG: Diagnostic contains: not logged by the uncaught exception handler", + " exec.submit(() -> System.out.println(\"Hello\"));", + " }", + "}").doTest(); + } +} diff --git a/baseline-refaster-rules/src/main/java/com/palantir/baseline/refaster/ExecutorSubmitRunnableFutureIgnored.java b/baseline-refaster-rules/src/main/java/com/palantir/baseline/refaster/ExecutorSubmitRunnableFutureIgnored.java deleted file mode 100644 index 7d97d4bba..000000000 --- a/baseline-refaster-rules/src/main/java/com/palantir/baseline/refaster/ExecutorSubmitRunnableFutureIgnored.java +++ /dev/null @@ -1,42 +0,0 @@ -/* - * (c) Copyright 2019 Palantir Technologies Inc. All rights reserved. - * - * 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.palantir.baseline.refaster; - -import com.google.errorprone.refaster.annotation.AfterTemplate; -import com.google.errorprone.refaster.annotation.BeforeTemplate; -import java.util.concurrent.ExecutorService; - -/** - * Uncaught exceptions from {@link ExecutorService#submit(Runnable)} are not logged by the uncaught exception handler - * because it is assumed that the returned future is used to watch for failures. - * When the returned future is ignored, using {@link ExecutorService#execute(Runnable)} is preferred because - * failures are recorded. - */ -public final class ExecutorSubmitRunnableFutureIgnored { - - @BeforeTemplate - @SuppressWarnings("FutureReturnValueIgnored") - void submit(ExecutorService executor, Runnable runnable) { - executor.submit(runnable); - } - - @AfterTemplate - void execute(ExecutorService executor, Runnable runnable) { - executor.execute(runnable); - } - -} diff --git a/baseline-refaster-rules/src/test/java/com/palantir/baseline/refaster/ExecutorSubmitRunnableFutureIgnoredTest.java b/baseline-refaster-rules/src/test/java/com/palantir/baseline/refaster/ExecutorSubmitRunnableFutureIgnoredTest.java deleted file mode 100644 index cfda6a612..000000000 --- a/baseline-refaster-rules/src/test/java/com/palantir/baseline/refaster/ExecutorSubmitRunnableFutureIgnoredTest.java +++ /dev/null @@ -1,63 +0,0 @@ -/* - * (c) Copyright 2019 Palantir Technologies Inc. All rights reserved. - * - * 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.palantir.baseline.refaster; - -import org.junit.Test; - -public class ExecutorSubmitRunnableFutureIgnoredTest { - - @Test - public void test() { - RefasterTestHelper - .forRefactoring(ExecutorSubmitRunnableFutureIgnored.class) - .withInputLines( - "Test", - "import java.util.concurrent.Executors;", - "public class Test {", - " static {", - " Executors.newSingleThreadExecutor().submit(() -> {});", - " }", - "}") - .hasOutputLines( - "import java.util.concurrent.Executors;", - "public class Test {", - " static {", - " Executors.newSingleThreadExecutor().execute(() -> {});", - " }", - "}"); - } - - @Test - public void testConsumedFutureUnchanged() { - RefasterTestHelper - .forRefactoring(ExecutorSubmitRunnableFutureIgnored.class) - .withInputLines( - "Test", - "import java.util.concurrent.Executors;", - "import java.util.concurrent.Future;", - "public class Test {", - " Future future = Executors.newSingleThreadExecutor().submit(() -> {});", - "}") - .hasOutputLines( - "import java.util.concurrent.Executors;", - "import java.util.concurrent.Future;", - "public class Test {", - " Future future = Executors.newSingleThreadExecutor().submit(() -> {});", - "}"); - } - -} diff --git a/changelog/@unreleased/pr-943.v2.yml b/changelog/@unreleased/pr-943.v2.yml new file mode 100644 index 000000000..2222d8f42 --- /dev/null +++ b/changelog/@unreleased/pr-943.v2.yml @@ -0,0 +1,5 @@ +type: improvement +improvement: + description: ExecutorSubmitRunnableFutureIgnored as error prone ERROR + links: + - https://github.com/palantir/gradle-baseline/pull/943 diff --git a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/extensions/BaselineErrorProneExtension.java b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/extensions/BaselineErrorProneExtension.java index 65b57e295..af6e1b9bf 100644 --- a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/extensions/BaselineErrorProneExtension.java +++ b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/extensions/BaselineErrorProneExtension.java @@ -24,6 +24,7 @@ public class BaselineErrorProneExtension { private static final ImmutableList DEFAULT_PATCH_CHECKS = ImmutableList.of( // Baseline checks + "ExecutorSubmitRunnableFutureIgnored", "LambdaMethodReference", "OptionalOrElseMethodInvocation", "PreferBuiltInConcurrentKeySet",