Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix #1431: Avoid rewriting lambdas into ambiguous references #1432

Merged
merged 3 commits into from
Jul 1, 2020
Merged

fix #1431: Avoid rewriting lambdas into ambiguous references #1432

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
45182fb
fix #1431: Avoid rewriting lambdas into ambiguous references
carterkozak Jun 25, 2020
f6c2a48
Add generated changelog entries
carterkozak Jun 25, 2020
4d0cdd3
fmt
carterkozak Jun 25, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 3 additions & 6 deletions ...ine-error-prone/src/main/java/com/palantir/baseline/errorprone/LambdaMethodReference.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,6 @@
@SuppressWarnings("checkstyle:CyclomaticComplexity")
public final class LambdaMethodReference extends BugChecker implements BugChecker.LambdaExpressionTreeMatcher {

private static final String MESSAGE = "Lambda should be a method reference";

@Override
public Description matchLambdaExpression(LambdaExpressionTree tree, VisitorState state) {
LambdaExpressionTree.BodyKind bodyKind = tree.getBodyKind();
Expand Down Expand Up @@ -150,8 +148,7 @@ private Description convertVariableInstanceMethods(
return Description.NO_MATCH;
}
return buildFix(methodSymbol, methodInvocation, root, state, isLocal(methodInvocation))
.map(fix ->
buildDescription(root).setMessage(MESSAGE).addFix(fix).build())
.map(fix -> buildDescription(root).addFix(fix).build())
.orElse(Description.NO_MATCH);
}

Expand All @@ -170,8 +167,8 @@ private Description convertMethodInvocations(
}

return buildFix(methodSymbol, methodInvocation, root, state, isLocal(methodInvocation))
.map(fix ->
buildDescription(root).setMessage(MESSAGE).addFix(fix).build())
.filter(fix -> SuggestedFixes.compilesWithFix(fix, state))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a quick and dirty fix, we can write something more precise in the future if we find performance suffers.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, I didn't see a good way to do the ambiguous check on the lambda/method reference receiver, but this would catch it. Should we put this filter in the buildFix method itself to catch any other potential invalid transforms?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not aware of other paths that result in ambiguous behavior, I'd prefer to catch specific causes so we can add test cases for them, and hopefully write more precise fixes that don't involve recompilation.

.map(fix -> buildDescription(root).addFix(fix).build())
.orElse(Description.NO_MATCH);
}

Expand Down
25 changes: 25 additions & 0 deletions ...error-prone/src/test/java/com/palantir/baseline/errorprone/LambdaMethodReferenceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -727,4 +727,29 @@ public void testGuavaToJavaUtilOptional() {
.expectUnchanged()
.doTest();
}

@Test
public void testSuperAmbiguous() {
refactor()
.addInputLines(
"Test.java",
"import java.util.function.BiConsumer;",
"import java.util.function.Consumer;",
"class Test {",
" interface One {",
" void call(Consumer<String> a);",
" void call(BiConsumer<String, String> a);",
" }",
" interface Two {",
" void apply(String a, String b);",
" void apply(String a);",
" }",
" void f(One one, Two two) {",
// Lambda conversion requires a cast which wouldn't necessarily be cleaner.
" one.call(value -> two.apply(value));",
" }",
"}")
.expectUnchanged()
.doTest();
}
}
5 changes: 5 additions & 0 deletions changelog/@unreleased/pr-1432.v2.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
type: fix
fix:
description: Avoid rewriting lambdas into ambiguous references
links:
- https://github.com/palantir/gradle-baseline/pull/1432