From f47ce45afcaef3d0e4aeded2fbfe38726ce42cf1 Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Mon, 25 Nov 2019 13:43:07 -0500 Subject: [PATCH] RedundantMethodReference check to avoid unnecessary method references (#1069) RedundantMethodReference check to avoid unnecessary method references --- README.md | 1 + .../errorprone/RedundantMethodReference.java | 88 +++++++++++++++++++ .../RedundantMethodReferenceTest.java | 85 ++++++++++++++++++ changelog/@unreleased/pr-1069.v2.yml | 5 ++ .../BaselineErrorProneExtension.java | 1 + 5 files changed, 180 insertions(+) create mode 100644 baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/RedundantMethodReference.java create mode 100644 baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/RedundantMethodReferenceTest.java create mode 100644 changelog/@unreleased/pr-1069.v2.yml diff --git a/README.md b/README.md index 317542fd4..3ca794bd5 100644 --- a/README.md +++ b/README.md @@ -181,6 +181,7 @@ Safe Logging can be found at [github.com/palantir/safe-logging](https://github.c - `Slf4jThrowable`: Slf4j loggers require throwables to be the last parameter otherwise a stack trace is not produced. - `JooqResultStreamLeak`: Autocloseable streams and cursors from jOOQ results should be obtained in a try-with-resources statement. - `StreamOfEmpty`: Stream.of() should be replaced with Stream.empty() to avoid unnecessary varargs allocation. +- `RedundantMethodReference`: Redundant method reference to the same type. ### Programmatic Application diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/RedundantMethodReference.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/RedundantMethodReference.java new file mode 100644 index 000000000..1750e2d2d --- /dev/null +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/RedundantMethodReference.java @@ -0,0 +1,88 @@ +/* + * (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.errorprone; + +import com.google.auto.service.AutoService; +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.MemberReferenceTree; +import com.sun.tools.javac.code.Symbol; +import com.sun.tools.javac.code.Type; +import com.sun.tools.javac.tree.JCTree; +import java.util.Set; +import javax.lang.model.element.Modifier; + +@AutoService(BugChecker.class) +@BugPattern( + name = "RedundantMethodReference", + link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks", + linkType = BugPattern.LinkType.CUSTOM, + severity = BugPattern.SeverityLevel.SUGGESTION, + summary = "Redundant method reference to the same type") +public final class RedundantMethodReference extends BugChecker implements BugChecker.MemberReferenceTreeMatcher { + + @Override + public Description matchMemberReference(MemberReferenceTree tree, VisitorState state) { + if (tree.getMode() != MemberReferenceTree.ReferenceMode.INVOKE) { + return Description.NO_MATCH; + } + if (!(tree instanceof JCTree.JCMemberReference)) { + return Description.NO_MATCH; + } + JCTree.JCMemberReference jcMemberReference = (JCTree.JCMemberReference) tree; + // Only support expression::method, not static method references or unbound method references (e.g. List::add) + if (jcMemberReference.kind != JCTree.JCMemberReference.ReferenceKind.BOUND) { + return Description.NO_MATCH; + } + Type rawResultType = ASTHelpers.getResultType(tree.getQualifierExpression()); + if (rawResultType == null) { + return Description.NO_MATCH; + } + Type treeType = ASTHelpers.getType(tree); + if (treeType == null) { + return Description.NO_MATCH; + } + if (!state.getTypes().isAssignable(rawResultType, treeType)) { + return Description.NO_MATCH; + } + Symbol.MethodSymbol methodSymbol = ASTHelpers.getSymbol(tree); + if (methodSymbol == null) { + return Description.NO_MATCH; + } + // Make sure the same method is being overridden, it's important not to change method invocations on types + // that also happen to implement the resulting functional interface. + Set matching = ASTHelpers.findMatchingMethods( + methodSymbol.name, + symbol -> symbol.getModifiers().contains(Modifier.ABSTRACT) + && methodSymbol.overrides(symbol, symbol.enclClass(), state.getTypes(), true), + treeType, + state.getTypes()); + // Do not allow any ambiguity, size must be exactly one. + if (matching.size() != 1) { + return Description.NO_MATCH; + } + return buildDescription(tree) + .addFix(SuggestedFix.builder() + .replace(tree, state.getSourceForNode(tree.getQualifierExpression())) + .build()) + .build(); + } +} diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/RedundantMethodReferenceTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/RedundantMethodReferenceTest.java new file mode 100644 index 000000000..c9143b531 --- /dev/null +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/RedundantMethodReferenceTest.java @@ -0,0 +1,85 @@ +/* + * (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.errorprone; + +import com.google.errorprone.BugCheckerRefactoringTestHelper; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.parallel.Execution; +import org.junit.jupiter.api.parallel.ExecutionMode; + +@Execution(ExecutionMode.CONCURRENT) +class RedundantMethodReferenceTest { + + @Test + void testFix() { + fix() + .addInputLines( + "Test.java", + "import java.util.*;", + "import java.util.function.*;", + "class Test {", + " String func(Optional in, Supplier defaultValue, Collection col) {", + " Comparator c = Comparator.comparing(Objects::nonNull);", + " in.ifPresent(col::add);", + " return in.orElseGet(defaultValue::get);", + " }", + "}") + .addOutputLines( + "Test.java", + "import java.util.*;", + "import java.util.function.*;", + "class Test {", + " String func(Optional in, Supplier defaultValue, Collection col) {", + " Comparator c = Comparator.comparing(Objects::nonNull);", + " in.ifPresent(col::add);", + " return in.orElseGet(defaultValue);", + " }", + "}") + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + + @Test + void testFunctionalInterfaceAdditionalMethod() { + fix() + .addInputLines( + "Test.java", + "import java.util.*;", + "import java.util.stream.*;", + "import java.util.function.*;", + "class Test {", + " void func(Stream in, List items) {", + " Ambiguous ambiguous = new Ambiguous();", + " in.forEach(ambiguous::printError);", + " }", + " static class Ambiguous implements Consumer {", + " @Override", + " public void accept(String value) {", + " System.out.println(value);", + " }", + " public void printError(String value) {", + " System.err.println(value);", + " }", + " }", + "}") + .expectUnchanged() + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + + private RefactoringValidator fix() { + return RefactoringValidator.of(new RedundantMethodReference(), getClass()); + } +} diff --git a/changelog/@unreleased/pr-1069.v2.yml b/changelog/@unreleased/pr-1069.v2.yml new file mode 100644 index 000000000..e6d9eadeb --- /dev/null +++ b/changelog/@unreleased/pr-1069.v2.yml @@ -0,0 +1,5 @@ +type: improvement +improvement: + description: RedundantMethodReference check to avoid unnecessary method references + links: + - https://github.com/palantir/gradle-baseline/pull/1069 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 024d94ce9..ad241f1ce 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 @@ -34,6 +34,7 @@ public class BaselineErrorProneExtension { "PreferSafeLoggableExceptions", "PreferSafeLoggingPreconditions", "ReadReturnValueIgnored", + "RedundantMethodReference", "RedundantModifier", "Slf4jLevelCheck", "Slf4jLogsafeArgs",