fix #894: Error Prone UnnecessaryLambdaArgumentParentheses (#1186)

Lambdas with a single parameter do not require argument parentheses.

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
 

baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/UnnecessaryLambdaArgumentParentheses.java

@@ -0,0 +1,97 @@
+/*
+ * (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.WARNING,
+        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;
+        }
+        // Avoid using getOffsetTokensForNode at this point because it's significantly more expensive
+        List<ErrorProneToken> tokens = state.getTokensForNode(tree);
+        int depth = 0;
+        int identifiers = 0;
+        for (int i = 0; i < tokens.size(); i++) {
+            ErrorProneToken token = tokens.get(i);
+            // 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 == 0) {
+                List<ErrorProneToken> 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;
+    }
+
+    // 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) == '(';
+    }
+}

baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/UnnecessaryLambdaArgumentParenthesesTest.java

@@ -0,0 +1,67 @@
+/*
+ * (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<Object> a = (value) -> value == null;",
+                        "    Predicate<Object> b = ( value ) -> value == null;",
+                        "    Predicate<Object> c = /* (value) -> value*/(value) -> value == null;",
+                        "    Predicate<Object> d = (value) /*(value) -> value*/ -> value == null;",
+                        "}")
+                .addOutputLines(
+                        "Test.java",
+                        "import " + Predicate.class.getName() + ';',
+                        "class Test {",
+                        "    Predicate<Object> a = value -> value == null;",
+                        "    Predicate<Object> b =  value  -> value == null;",
+                        "    Predicate<Object> c = /* (value) -> value*/value -> value == null;",
+                        "    Predicate<Object> 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<Object> a = value -> value == null;",
+                        "    Predicate<Object> b =  value  -> value == null;",
+                        "    Predicate<Object> c = /* (value) -> value*/value -> value == null;",
+                        "    Predicate<Object> d = value /*(value) -> value*/ -> value == null;",
+                        "    Predicate<?> e = (String value) -> value == null;",
+                        "}")
+                .expectUnchanged()
+                .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);
+    }
+
+    private RefactoringValidator fix() {
+        return RefactoringValidator.of(new UnnecessaryLambdaArgumentParentheses(), getClass());
+    }
+}

changelog/@unreleased/pr-1186.v2.yml

@@ -0,0 +1,8 @@
+type: improvement
+improvement:
+  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

gradle-baseline-java-config/resources/checkstyle/checkstyle.xml

@@ -410,7 +410,6 @@
             <message key="name.invalidPattern" value="Type name ''{0}'' must match pattern ''{1}''."/>
         </module>
         <module name="TypecastParenPad"/> <!-- Java Style Guide: Horizontal whitespace -->
-        <module name="UnnecessaryParentheses"/>
         <module name="UnusedImports"> <!-- Java Style Guide: No unused imports -->
             <property name="processJavadoc" value="true"/>
         </module>

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",