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

validate against ambiguous references #1372

Merged
merged 4 commits into from
Jun 1, 2020
Merged

validate against ambiguous references #1372

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
11a7faa
Avoid ambiguity
carterkozak May 31, 2020
7929ba1
validate against ambiguous references
carterkozak Jun 1, 2020
720adb9
fix ambiguous 'this'
carterkozak Jun 1, 2020
4c5c509
broaden ambiguous method check
carterkozak Jun 1, 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
55 changes: 50 additions & 5 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 @@ -43,6 +43,8 @@
import java.util.List;
import java.util.Objects;
import java.util.Optional;
import java.util.Set;
import javax.annotation.Nullable;

@AutoService(BugChecker.class)
@BugPattern(
Expand Down Expand Up @@ -144,11 +146,10 @@ private Description convertVariableInstanceMethods(
if (!paramSymbol.equals(receiverSymbol)) {
return Description.NO_MATCH;
}

return buildDescription(root)
.setMessage(MESSAGE)
.addFix(buildFix(methodSymbol, methodInvocation, root, state, isLocal(methodInvocation)))
.build();
return buildFix(methodSymbol, methodInvocation, root, state, isLocal(methodInvocation))
.map(fix ->
buildDescription(root).setMessage(MESSAGE).addFix(fix).build())
.orElse(Description.NO_MATCH);
}

private Description convertMethodInvocations(
Expand Down Expand Up @@ -184,12 +185,56 @@ private static Optional<SuggestedFix> buildFix(
LambdaExpressionTree root,
VisitorState state,
boolean isLocal) {
if (isAmbiguousMethod(symbol, ASTHelpers.getReceiver(invocation), state)) {
return Optional.empty();
}

SuggestedFix.Builder builder = SuggestedFix.builder();
return qualifyTarget(symbol, invocation, root, builder, state, isLocal)
.flatMap(LambdaMethodReference::toMethodReference)
.map(qualified -> builder.replace(root, qualified).build());
}

private static boolean isAmbiguousMethod(
Symbol.MethodSymbol symbol, @Nullable ExpressionTree receiver, VisitorState state) {
if (symbol.isStatic()) {
if (symbol.params().size() != 1) {
return false;
}
Symbol.ClassSymbol classSymbol = ASTHelpers.enclosingClass(symbol);
if (classSymbol == null) {
return false;
}
Set<Symbol.MethodSymbol> matching = ASTHelpers.findMatchingMethods(
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a super cool helper method! didn't know it existed

symbol.name,
sym -> sym != null && !sym.isStatic() && sym.getParameters().isEmpty(),
classSymbol.type,
state.getTypes());
return !matching.isEmpty();
} else {
if (!symbol.params().isEmpty()) {
return false;
}
if (receiver == null) {
return false;
}
Type receiverType = ASTHelpers.getType(receiver);
if (receiverType == null) {
return false;
}
Set<Symbol.MethodSymbol> matching = ASTHelpers.findMatchingMethods(
symbol.name,
sym -> sym != null
&& sym.isStatic()
&& sym.getParameters().size() == 1
&& state.getTypes()
.isAssignable(receiverType, sym.params().get(0).type),
receiverType,
state.getTypes());
return !matching.isEmpty();
}
}

private static Optional<String> qualifyTarget(
Symbol.MethodSymbol symbol,
MethodInvocationTree invocation,
Expand Down
100 changes: 64 additions & 36 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 @@ -23,23 +23,21 @@
import java.util.Map;
import java.util.Optional;
import java.util.function.Supplier;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;

public class LambdaMethodReferenceTest {

private CompilationTestHelper compilationHelper;
private RefactoringValidator refactoringValidator;
private CompilationTestHelper compile() {
Copy link
Contributor

Choose a reason for hiding this comment

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

gotta keep those tests fast 😛

Copy link
Contributor Author

Choose a reason for hiding this comment

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

heh, most of our tests are structured this way so I moved it over. I think I migrated most of them after hitting a few cases where tests flaked after helper/validator instances were reused (due to either parallelism or multiple asserts in a single test). This way they're always clean regardless of other state!

return CompilationTestHelper.newInstance(LambdaMethodReference.class, getClass());
}

@BeforeEach
public void before() {
compilationHelper = CompilationTestHelper.newInstance(LambdaMethodReference.class, getClass());
refactoringValidator = RefactoringValidator.of(new LambdaMethodReference(), getClass());
private RefactoringValidator refactor() {
return RefactoringValidator.of(new LambdaMethodReference(), getClass());
}

@Test
public void testMethodReference() {
compilationHelper
compile()
.addSourceLines(
"Test.java",
"import " + ImmutableList.class.getName() + ';',
Expand All @@ -55,7 +53,7 @@ public void testMethodReference() {

@Test
void testInstanceMethod() {
compilationHelper
compile()
.addSourceLines(
"Test.java",
"import " + Optional.class.getName() + ';',
Expand All @@ -70,7 +68,7 @@ void testInstanceMethod() {

@Test
void testLocalInstanceMethod() {
compilationHelper
compile()
.addSourceLines(
"Test.java",
"import " + Optional.class.getName() + ';',
Expand All @@ -88,7 +86,7 @@ void testLocalInstanceMethod() {

@Test
public void testLocalInstanceMethodSupplier() {
compilationHelper
compile()
.addSourceLines(
"Test.java",
"import " + ImmutableList.class.getName() + ';',
Expand All @@ -108,7 +106,7 @@ public void testLocalInstanceMethodSupplier() {

@Test
void testLocalStaticMethod_multiParam() {
compilationHelper
compile()
.addSourceLines(
"Test.java",
"import " + Map.class.getName() + ';',
Expand All @@ -126,7 +124,7 @@ void testLocalStaticMethod_multiParam() {

@Test
public void testLocalMethodSupplier_block() {
compilationHelper
compile()
.addSourceLines(
"Test.java",
"import " + ImmutableList.class.getName() + ';',
Expand All @@ -146,7 +144,7 @@ public void testLocalMethodSupplier_block() {

@Test
public void testStaticMethod_block() {
compilationHelper
compile()
.addSourceLines(
"Test.java",
"import " + ImmutableList.class.getName() + ';',
Expand All @@ -163,7 +161,7 @@ public void testStaticMethod_block() {

@Test
public void testAutoFix_staticMethod_block() {
refactoringValidator
refactor()
.addInputLines(
"Test.java",
"import " + ImmutableList.class.getName() + ';',
Expand All @@ -189,7 +187,7 @@ public void testAutoFix_staticMethod_block() {

@Test
public void testAutoFix_staticMethodWithParam() {
refactoringValidator
refactor()
.addInputLines(
"Test.java",
"import " + ImmutableList.class.getName() + ';',
Expand All @@ -215,7 +213,7 @@ public void testAutoFix_staticMethodWithParam() {

@Test
void testAutoFix_InstanceMethod() {
refactoringValidator
refactor()
.addInputLines(
"Test.java",
"import " + Optional.class.getName() + ';',
Expand All @@ -237,7 +235,7 @@ void testAutoFix_InstanceMethod() {

@Test
void testNegative_InstanceMethodWithType() {
refactoringValidator
refactor()
.addInputLines(
"Test.java",
"import " + Optional.class.getName() + ';',
Expand All @@ -250,9 +248,39 @@ void testNegative_InstanceMethodWithType() {
.doTest();
}

@Test
void testNegative_ambiguousStaticReference() {
refactor()
.addInputLines(
"Test.java",
"import " + Optional.class.getName() + ';',
"class Test {",
" public Optional<String> foo(Optional<Long> optional) {",
" return optional.map(value -> Long.toString(value));",
" }",
"}")
.expectUnchanged()
.doTest();
}

@Test
void testNegative_ambiguousInstanceReference() {
refactor()
.addInputLines(
"Test.java",
"import " + Optional.class.getName() + ';',
"class Test {",
" public Optional<String> foo(Optional<Long> optional) {",
" return optional.map(value -> value.toString());",
" }",
"}")
.expectUnchanged()
.doTest();
}

@Test
void testAutoFix_SpecificInstanceMethod() {
refactoringValidator
refactor()
.addInputLines(
"Test.java",
"import " + Optional.class.getName() + ';',
Expand All @@ -274,7 +302,7 @@ void testAutoFix_SpecificInstanceMethod() {

@Test
void testAutoFix_SpecificInstanceMethod_withTypeParameters() {
refactoringValidator
refactor()
.addInputLines(
"Test.java",
"import " + Optional.class.getName() + ';',
Expand All @@ -296,7 +324,7 @@ void testAutoFix_SpecificInstanceMethod_withTypeParameters() {

@Test
void testAutoFix_localInstanceMethod() {
refactoringValidator
refactor()
.addInputLines(
"Test.java",
"import " + Optional.class.getName() + ';',
Expand Down Expand Up @@ -324,7 +352,7 @@ void testAutoFix_localInstanceMethod() {

@Test
void testAutoFix_localInstanceMethod_explicitThis() {
refactoringValidator
refactor()
.addInputLines(
"Test.java",
"import " + Optional.class.getName() + ';',
Expand Down Expand Up @@ -352,7 +380,7 @@ void testAutoFix_localInstanceMethod_explicitThis() {

@Test
void testAutoFix_localStaticMethod() {
refactoringValidator
refactor()
.addInputLines(
"Test.java",
"import " + Optional.class.getName() + ';',
Expand Down Expand Up @@ -380,7 +408,7 @@ void testAutoFix_localStaticMethod() {

@Test
void testAutoFix_localStaticMethod_multiParam() {
refactoringValidator
refactor()
.addInputLines(
"Test.java",
"import " + Map.class.getName() + ';',
Expand Down Expand Up @@ -408,7 +436,7 @@ void testAutoFix_localStaticMethod_multiParam() {

@Test
void testAutoFix_StaticMethod_multiParam() {
refactoringValidator
refactor()
.addInputLines(
"Test.java",
"import " + Map.class.getName() + ';',
Expand All @@ -432,7 +460,7 @@ void testAutoFix_StaticMethod_multiParam() {

@Test
public void testAutoFix_block_localMethod() {
refactoringValidator
refactor()
.addInputLines(
"Test.java",
"import " + ImmutableList.class.getName() + ';',
Expand Down Expand Up @@ -464,7 +492,7 @@ public void testAutoFix_block_localMethod() {

@Test
public void testNegative_block() {
compilationHelper
compile()
.addSourceLines(
"Test.java",
"import " + ImmutableList.class.getName() + ';',
Expand All @@ -480,7 +508,7 @@ public void testNegative_block() {

@Test
public void testPositive_expression() {
compilationHelper
compile()
.addSourceLines(
"Test.java",
"import " + ImmutableList.class.getName() + ';',
Expand All @@ -497,7 +525,7 @@ public void testPositive_expression() {

@Test
public void testAutoFix_expression() {
refactoringValidator
refactor()
.addInputLines(
"Test.java",
"import " + ImmutableList.class.getName() + ';',
Expand All @@ -523,7 +551,7 @@ public void testAutoFix_expression() {

@Test
public void testNegative_expression_staticMethod() {
compilationHelper
compile()
.addSourceLines(
"Test.java",
"import " + ImmutableList.class.getName() + ';',
Expand All @@ -538,7 +566,7 @@ public void testNegative_expression_staticMethod() {

@Test
public void testAutoFix_expression_localMethod() {
refactoringValidator
refactor()
.addInputLines(
"Test.java",
"import " + ImmutableList.class.getName() + ';',
Expand Down Expand Up @@ -570,7 +598,7 @@ public void testAutoFix_expression_localMethod() {

@Test
public void testAutoFix_expression_referenceMethod() {
refactoringValidator
refactor()
.addInputLines(
"Test.java",
"import " + ImmutableList.class.getName() + ';',
Expand Down Expand Up @@ -598,7 +626,7 @@ public void testAutoFix_expression_referenceMethod() {

@Test
void testNegative_LocalStaticMethod_multiParam() {
compilationHelper
compile()
.addSourceLines(
"Test.java",
"import " + Map.class.getName() + ';',
Expand All @@ -615,7 +643,7 @@ void testNegative_LocalStaticMethod_multiParam() {

@Test
public void testNegative_expression() {
compilationHelper
compile()
.addSourceLines(
"Test.java",
"import " + ImmutableList.class.getName() + ';',
Expand All @@ -631,7 +659,7 @@ public void testNegative_expression() {

@Test
public void testNegative_expression_chain() {
compilationHelper
compile()
.addSourceLines(
"Test.java",
"import " + ImmutableList.class.getName() + ';',
Expand All @@ -649,7 +677,7 @@ public void testNegative_expression_chain() {

@Test
public void testNegative_dont_eagerly_capture_reference() {
compilationHelper
compile()
.addSourceLines(
"Test.java",
"import " + Supplier.class.getName() + ';',
Expand Down