From 179095c77246401a694fb89841447afdf4825db2 Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Thu, 23 Jan 2020 11:18:45 -0500 Subject: [PATCH 1/4] fix #894: Error Prone UnnecessaryLambdaArgumentParentheses Lambdas with a single parameter do not require argument parentheses. ```diff - (value) -> value != null + value -> value != null ``` --- README.md | 1 + .../UnnecessaryLambdaArgumentParentheses.java | 100 ++++++++++++++++++ ...ecessaryLambdaArgumentParenthesesTest.java | 66 ++++++++++++ .../BaselineErrorProneExtension.java | 1 + 4 files changed, 168 insertions(+) create mode 100644 baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/UnnecessaryLambdaArgumentParentheses.java create mode 100644 baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/UnnecessaryLambdaArgumentParenthesesTest.java diff --git a/README.md b/README.md index 407342831..4e4243dc3 100644 --- a/README.md +++ b/README.md @@ -186,6 +186,7 @@ Safe Logging can be found at [github.com/palantir/safe-logging](https://github.c - `BracesRequired`: Require braces for loops and if expressions. - `CollectionStreamForEach`: Collection.forEach is more efficient than Collection.stream().forEach. - `LoggerEnclosingClass`: Loggers created using getLogger(Class) must reference their enclosing class. +- `UnnecessaryLambdaArgumentParentheses`: Lambdas with a single parameter do not require argument parentheses. ### Programmatic Application diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/UnnecessaryLambdaArgumentParentheses.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/UnnecessaryLambdaArgumentParentheses.java new file mode 100644 index 000000000..c7ce566c9 --- /dev/null +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/UnnecessaryLambdaArgumentParentheses.java @@ -0,0 +1,100 @@ +/* + * (c) Copyright 2020 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.ErrorProneToken; +import com.sun.source.tree.LambdaExpressionTree; +import com.sun.tools.javac.parser.Tokens; +import com.sun.tools.javac.tree.JCTree; +import java.util.List; + +/** + * UnnecessaryLambdaArgumentParentheses provides similar functionality to the upstream UnnecessaryParentheses, but + * specifically for single-parameter lambda arguments which are not covered by the existing check. Perhaps this can be + * contributed upstream. There's an argument against combining the two because parentheses around lambda arguments + * cannot be parsed directly from the AST where other parenthesis checked by UnnecessaryParentheses can. + */ +@AutoService(BugChecker.class) +@BugPattern( + name = "UnnecessaryLambdaArgumentParentheses", + link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks", + linkType = BugPattern.LinkType.CUSTOM, + providesFix = BugPattern.ProvidesFix.REQUIRES_HUMAN_ATTENTION, + severity = BugPattern.SeverityLevel.SUGGESTION, + summary = "Lambdas with a single parameter do not require argument parentheses.") +public final class UnnecessaryLambdaArgumentParentheses extends BugChecker + implements BugChecker.LambdaExpressionTreeMatcher { + + @Override + public Description matchLambdaExpression(LambdaExpressionTree tree, VisitorState state) { + if (!quickCheck(tree, state)) { + return Description.NO_MATCH; + } + List tokens = state.getOffsetTokensForNode(tree); + if (tokens.size() <= 3) { + return Description.NO_MATCH; + } + ErrorProneToken firstToken = tokens.get(0); + if (firstToken.kind() != Tokens.TokenKind.LPAREN) { + return Description.NO_MATCH; + } + // skip the first token, we've already validated it + int depth = 1; + for (int i = 1; i < tokens.size(); i++) { + ErrorProneToken token = tokens.get(i); + if (token.kind() == Tokens.TokenKind.ARROW) { + return Description.NO_MATCH; + } else if (token.kind() == Tokens.TokenKind.LPAREN) { + depth++; + } else if (token.kind() == Tokens.TokenKind.RPAREN) { + depth--; + if (depth == 0) { + return buildDescription(tree.getParameters().get(0)) + .addFix(SuggestedFix.builder() + .replace(firstToken.pos(), firstToken.endPos(), "") + .replace(token.pos(), token.endPos(), "") + .build()) + .build(); + } + } + } + return Description.NO_MATCH; + } + + // Fast check to rule out lambdas that don't violate this check without tokenizing the source. + private static boolean quickCheck(LambdaExpressionTree tree, VisitorState state) { + if (tree.getParameters().size() != 1) { + return false; + } + int start = ((JCTree) tree).getStartPosition(); + if (start == -1) { + return false; + } + CharSequence source = state.getSourceCode(); + if (source == null) { + return false; + } + // Fast check to avoid tokenizing all lambdas unnecessarily. + return source.charAt(start) == '('; + } +} diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/UnnecessaryLambdaArgumentParenthesesTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/UnnecessaryLambdaArgumentParenthesesTest.java new file mode 100644 index 000000000..823da9196 --- /dev/null +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/UnnecessaryLambdaArgumentParenthesesTest.java @@ -0,0 +1,66 @@ +/* + * (c) Copyright 2020 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 java.util.function.Predicate; +import org.junit.jupiter.api.Test; + +class UnnecessaryLambdaArgumentParenthesesTest { + + @Test + public void testFix() { + fix().addInputLines( + "Test.java", + "import " + Predicate.class.getName() + ';', + "class Test {", + " Predicate a = (value) -> value == null;", + " Predicate b = ( value ) -> value == null;", + " Predicate c = /* (value) -> value*/(value) -> value == null;", + " Predicate d = (value) /*(value) -> value*/ -> value == null;", + "}") + .addOutputLines( + "Test.java", + "import " + Predicate.class.getName() + ';', + "class Test {", + " Predicate a = value -> value == null;", + " Predicate b = value -> value == null;", + " Predicate c = /* (value) -> value*/value -> value == null;", + " Predicate d = value /*(value) -> value*/ -> value == null;", + "}") + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + + @Test + public void testNegative() { + fix().addInputLines( + "Test.java", + "import " + Predicate.class.getName() + ';', + "class Test {", + " Predicate a = value -> value == null;", + " Predicate b = value -> value == null;", + " Predicate c = /* (value) -> value*/value -> value == null;", + " Predicate d = value /*(value) -> value*/ -> value == null;", + "}") + .expectUnchanged() + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + + private RefactoringValidator fix() { + return RefactoringValidator.of(new UnnecessaryLambdaArgumentParentheses(), getClass()); + } +} 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 5bb907edc..a58f2383e 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 @@ -48,6 +48,7 @@ public class BaselineErrorProneExtension { "StrictUnusedVariable", "StringBuilderConstantParameters", "ThrowError", + "UnnecessaryLambdaArgumentParentheses", // TODO(ckozak): re-enable pending scala check // "ThrowSpecificity", "UnsafeGaugeRegistration", From 7da02dfe4eba0e72d10492643f16d805236da9c3 Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Thu, 23 Jan 2020 16:18:45 +0000 Subject: [PATCH 2/4] Add generated changelog entries --- changelog/@unreleased/pr-1186.v2.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelog/@unreleased/pr-1186.v2.yml diff --git a/changelog/@unreleased/pr-1186.v2.yml b/changelog/@unreleased/pr-1186.v2.yml new file mode 100644 index 000000000..1e7660daf --- /dev/null +++ b/changelog/@unreleased/pr-1186.v2.yml @@ -0,0 +1,5 @@ +type: improvement +improvement: + description: Lambdas with a single parameter do not require argument parentheses. + links: + - https://github.com/palantir/gradle-baseline/pull/1186 From 8a1e882a1fc0a194a6d9b97b3184b0523510c418 Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Thu, 23 Jan 2020 11:51:49 -0500 Subject: [PATCH 3/4] Ignore arguments with explicit types --- .../UnnecessaryLambdaArgumentParentheses.java | 41 +++++++++---------- ...ecessaryLambdaArgumentParenthesesTest.java | 1 + 2 files changed, 20 insertions(+), 22 deletions(-) diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/UnnecessaryLambdaArgumentParentheses.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/UnnecessaryLambdaArgumentParentheses.java index c7ce566c9..d3fc721d3 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/UnnecessaryLambdaArgumentParentheses.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/UnnecessaryLambdaArgumentParentheses.java @@ -50,32 +50,29 @@ public Description matchLambdaExpression(LambdaExpressionTree tree, VisitorState if (!quickCheck(tree, state)) { return Description.NO_MATCH; } - List tokens = state.getOffsetTokensForNode(tree); - if (tokens.size() <= 3) { - return Description.NO_MATCH; - } - ErrorProneToken firstToken = tokens.get(0); - if (firstToken.kind() != Tokens.TokenKind.LPAREN) { - return Description.NO_MATCH; - } - // skip the first token, we've already validated it - int depth = 1; - for (int i = 1; i < tokens.size(); i++) { + // Avoid using getOffsetTokensForNode at this point because it's significantly more expensive + List tokens = state.getTokensForNode(tree); + int depth = 0; + int identifiers = 0; + for (int i = 0; i < tokens.size(); i++) { ErrorProneToken token = tokens.get(i); - if (token.kind() == Tokens.TokenKind.ARROW) { + // parameters with types require parens + if (token.kind() == Tokens.TokenKind.IDENTIFIER && ++identifiers > 1) { + return Description.NO_MATCH; + } else if (token.kind() == Tokens.TokenKind.ARROW) { return Description.NO_MATCH; } else if (token.kind() == Tokens.TokenKind.LPAREN) { depth++; - } else if (token.kind() == Tokens.TokenKind.RPAREN) { - depth--; - if (depth == 0) { - return buildDescription(tree.getParameters().get(0)) - .addFix(SuggestedFix.builder() - .replace(firstToken.pos(), firstToken.endPos(), "") - .replace(token.pos(), token.endPos(), "") - .build()) - .build(); - } + } else if (token.kind() == Tokens.TokenKind.RPAREN && --depth == 0) { + List offsetTokens = state.getOffsetTokensForNode(tree); + ErrorProneToken firstToken = offsetTokens.get(0); + ErrorProneToken offsetToken = offsetTokens.get(i); + return buildDescription(tree.getParameters().get(0)) + .addFix(SuggestedFix.builder() + .replace(firstToken.pos(), firstToken.endPos(), "") + .replace(offsetToken.pos(), offsetToken.endPos(), "") + .build()) + .build(); } } return Description.NO_MATCH; diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/UnnecessaryLambdaArgumentParenthesesTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/UnnecessaryLambdaArgumentParenthesesTest.java index 823da9196..571aa1109 100644 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/UnnecessaryLambdaArgumentParenthesesTest.java +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/UnnecessaryLambdaArgumentParenthesesTest.java @@ -55,6 +55,7 @@ public void testNegative() { " Predicate b = value -> value == null;", " Predicate c = /* (value) -> value*/value -> value == null;", " Predicate d = value /*(value) -> value*/ -> value == null;", + " Predicate e = (String value) -> value == null;", "}") .expectUnchanged() .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); From f9a6e85ab17611b6febcf0f0a113eb31e42a4e59 Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Thu, 23 Jan 2020 12:12:29 -0500 Subject: [PATCH 4/4] replace checkstyle --- .../errorprone/UnnecessaryLambdaArgumentParentheses.java | 2 +- changelog/@unreleased/pr-1186.v2.yml | 5 ++++- .../resources/checkstyle/checkstyle.xml | 1 - 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/UnnecessaryLambdaArgumentParentheses.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/UnnecessaryLambdaArgumentParentheses.java index d3fc721d3..17a160f89 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/UnnecessaryLambdaArgumentParentheses.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/UnnecessaryLambdaArgumentParentheses.java @@ -40,7 +40,7 @@ link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks", linkType = BugPattern.LinkType.CUSTOM, providesFix = BugPattern.ProvidesFix.REQUIRES_HUMAN_ATTENTION, - severity = BugPattern.SeverityLevel.SUGGESTION, + severity = BugPattern.SeverityLevel.WARNING, summary = "Lambdas with a single parameter do not require argument parentheses.") public final class UnnecessaryLambdaArgumentParentheses extends BugChecker implements BugChecker.LambdaExpressionTreeMatcher { diff --git a/changelog/@unreleased/pr-1186.v2.yml b/changelog/@unreleased/pr-1186.v2.yml index 1e7660daf..a2b27d9b3 100644 --- a/changelog/@unreleased/pr-1186.v2.yml +++ b/changelog/@unreleased/pr-1186.v2.yml @@ -1,5 +1,8 @@ type: improvement improvement: - description: Lambdas with a single parameter do not require argument parentheses. + description: + Replace the checkstyle UnnecessaryParentheses check with error-prone. The existing upstream + UnnecessaryParentheses check covers most cases, a new UnnecessaryLambdaArgumentParentheses + check covers the rest. links: - https://github.com/palantir/gradle-baseline/pull/1186 diff --git a/gradle-baseline-java-config/resources/checkstyle/checkstyle.xml b/gradle-baseline-java-config/resources/checkstyle/checkstyle.xml index cd19cae45..dcac6f1b5 100644 --- a/gradle-baseline-java-config/resources/checkstyle/checkstyle.xml +++ b/gradle-baseline-java-config/resources/checkstyle/checkstyle.xml @@ -410,7 +410,6 @@ -