From 43bbb38a0bee6df32531acc3aafd086149d04ffd Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Fri, 23 Aug 2019 10:55:14 -0400 Subject: [PATCH 1/3] LambdaMethodReference avoids suggestions for nonstatic methods No change to fixes that are automatically applied. Now the check avoids making suggestions without suggested fixes to eliminate noise. --- .../errorprone/LambdaMethodReference.java | 28 +++++++++++-------- .../errorprone/LambdaMethodReferenceTest.java | 23 ++++++++++++--- 2 files changed, 36 insertions(+), 15 deletions(-) diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/LambdaMethodReference.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/LambdaMethodReference.java index 8260f7ff8..b3aa1bb16 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/LambdaMethodReference.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/LambdaMethodReference.java @@ -91,25 +91,31 @@ private Description checkMethodInvocation( || !methodInvocation.getTypeArguments().isEmpty()) { return Description.NO_MATCH; } + Symbol.MethodSymbol methodSymbol = ASTHelpers.getSymbol(methodInvocation); + if (methodSymbol == null) { + // Should only ever occur if there are errors in the AST, allow the compiler to fail later + return Description.NO_MATCH; + } + if (!methodSymbol.isStatic()) { + // Only support static invocations for the time being to avoid erroneously + // rewriting '() -> foo()' to 'ClassName::foo' instead of 'this::foo' + // or suggesting '() -> foo.doWork().bar()' should be rewritten to 'foo.doWork()::bar', + // which executes 'doWork' eagerly, even when the supplier is not used. + return Description.NO_MATCH; + } return buildDescription(root) .setMessage(MESSAGE) - .addFix(buildFix(methodInvocation, root, state)) + .addFix(buildFix(methodSymbol, root, state)) .build(); } private static Optional buildFix( - MethodInvocationTree invocation, + Symbol.MethodSymbol symbol, LambdaExpressionTree root, VisitorState state) { - return Optional.ofNullable(ASTHelpers.getSymbol(invocation)) - // Only support static invocations for the time being to avoid erroneously - // rewriting '() -> foo()' to 'ClassName::foo' instead of 'this::foo' - .filter(Symbol::isStatic) - .flatMap(symbol -> { - SuggestedFix.Builder builder = SuggestedFix.builder(); - return toMethodReference(SuggestedFixes.qualifyType(state, builder, symbol)) - .map(qualified -> builder.replace(root, qualified).build()); - }); + SuggestedFix.Builder builder = SuggestedFix.builder(); + return toMethodReference(SuggestedFixes.qualifyType(state, builder, symbol)) + .map(qualified -> builder.replace(root, qualified).build()); } private static Optional toMethodReference(String qualifiedMethodName) { diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/LambdaMethodReferenceTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/LambdaMethodReferenceTest.java index 2e32ecf40..d946a62f4 100644 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/LambdaMethodReferenceTest.java +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/LambdaMethodReferenceTest.java @@ -89,7 +89,7 @@ public void testAutoFix_block() { } @Test - public void testPositive_block_localMethod() { + public void testNegative_block_localMethod() { compilationHelper.addSourceLines( "Test.java", "import " + ImmutableList.class.getName() + ';', @@ -97,7 +97,7 @@ public void testPositive_block_localMethod() { "import " + Optional.class.getName() + ';', "class Test {", " public List foo(Optional> optional) {", - " // BUG: Diagnostic contains: Lambda should be a method reference", + // A future improvement may rewrite the following to 'orElseGet(this::bar)' " return optional.orElseGet(() -> { return bar(); });", " }", " private List bar() {", @@ -195,7 +195,7 @@ public void testAutoFix_expression() { } @Test - public void testPositive_expression_localMethod() { + public void testNegative_expression_localMethod() { compilationHelper.addSourceLines( "Test.java", "import " + ImmutableList.class.getName() + ';', @@ -203,7 +203,7 @@ public void testPositive_expression_localMethod() { "import " + Optional.class.getName() + ';', "class Test {", " public List foo(Optional> optional) {", - " // BUG: Diagnostic contains: Lambda should be a method reference", + // A future improvement may rewrite the following to 'orElseGet(this::bar)' " return optional.orElseGet(() -> bar());", " }", " private List bar() {", @@ -258,4 +258,19 @@ public void testNegative_expression() { " }", "}").doTest(); } + + @Test + public void testNegative_expression_chain() { + compilationHelper.addSourceLines( + "Test.java", + "import " + ImmutableList.class.getName() + ';', + "import " + Optional.class.getName() + ';', + "class Test {", + " public Object foo(Optional optional) {", + // It's important that this is not rewritten to 'optional.orElseGet(ImmutableList.of(1)::size)' + // which create a new list eagerly, and returns a supplier for the new instances 'size()' function. + " return optional.orElseGet(() -> ImmutableList.of(1).size());", + " }", + "}").doTest(); + } } From b2830d65c8edc46585058fa891efaf183114439e Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Fri, 23 Aug 2019 14:55:14 +0000 Subject: [PATCH 2/3] Add generated changelog entries --- changelog/@unreleased/pr-771.v2.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelog/@unreleased/pr-771.v2.yml diff --git a/changelog/@unreleased/pr-771.v2.yml b/changelog/@unreleased/pr-771.v2.yml new file mode 100644 index 000000000..ba0db07e5 --- /dev/null +++ b/changelog/@unreleased/pr-771.v2.yml @@ -0,0 +1,5 @@ +type: improvement +improvement: + description: LambdaMethodReference avoids suggestions for non-static methods + links: + - https://github.com/palantir/gradle-baseline/pull/771 From 7d21e08f9014f69b43d9ca5dcdc4b2d4ac786f4b Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Fri, 23 Aug 2019 14:55:14 +0000 Subject: [PATCH 3/3] Add generated changelog entries --- changelog/@unreleased/pr-771.v2.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/changelog/@unreleased/pr-771.v2.yml b/changelog/@unreleased/pr-771.v2.yml index ba0db07e5..137b3c005 100644 --- a/changelog/@unreleased/pr-771.v2.yml +++ b/changelog/@unreleased/pr-771.v2.yml @@ -1,5 +1,5 @@ -type: improvement -improvement: +type: fix +fix: description: LambdaMethodReference avoids suggestions for non-static methods links: - https://github.com/palantir/gradle-baseline/pull/771