From df14397f105db2b6ed623d6f228dc138f8343066 Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Mon, 1 Jun 2020 16:48:10 -0400 Subject: [PATCH] validate against ambiguous references (#1372) --- .../errorprone/LambdaMethodReference.java | 72 +++++++++- .../errorprone/LambdaMethodReferenceTest.java | 136 +++++++++++++----- 2 files changed, 166 insertions(+), 42 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 56523c484..2b6809a25 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 @@ -28,6 +28,7 @@ import com.google.errorprone.util.ASTHelpers; import com.google.errorprone.util.ErrorProneToken; import com.sun.source.tree.BlockTree; +import com.sun.source.tree.ClassTree; import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.IdentifierTree; import com.sun.source.tree.LambdaExpressionTree; @@ -43,6 +44,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( @@ -52,6 +55,7 @@ providesFix = BugPattern.ProvidesFix.REQUIRES_HUMAN_ATTENTION, severity = BugPattern.SeverityLevel.SUGGESTION, summary = "Lambda should be a method reference") +@SuppressWarnings("checkstyle:CyclomaticComplexity") public final class LambdaMethodReference extends BugChecker implements BugChecker.LambdaExpressionTreeMatcher { private static final String MESSAGE = "Lambda should be a method reference"; @@ -144,11 +148,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( @@ -184,12 +187,59 @@ private static Optional 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 matching = ASTHelpers.findMatchingMethods( + 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 matching = ASTHelpers.findMatchingMethods( + symbol.name, + sym -> sym != null + && sym.isStatic() + && sym.getParameters().size() == 1 + && state.getTypes() + .isAssignable( + state.getTypes().erasure(receiverType), + state.getTypes() + .erasure(sym.params().get(0).type)), + receiverType, + state.getTypes()); + return !matching.isEmpty(); + } + } + private static Optional qualifyTarget( Symbol.MethodSymbol symbol, MethodInvocationTree invocation, @@ -198,7 +248,17 @@ private static Optional qualifyTarget( VisitorState state, boolean isLocal) { if (!symbol.isStatic() && isLocal) { - return Optional.of("this." + symbol.name.toString()); + // Validate teh local method is defined in this class + ClassTree enclosingClass = ASTHelpers.findEnclosingNode(state.getPath(), ClassTree.class); + if (enclosingClass == null) { + return Optional.empty(); + } + Type.ClassType enclosingType = ASTHelpers.getType(enclosingClass); + if (!ASTHelpers.findMatchingMethods(symbol.name, symbol::equals, enclosingType, state.getTypes()) + .isEmpty()) { + return Optional.of("this." + symbol.name.toString()); + } + return Optional.empty(); } ExpressionTree receiver = ASTHelpers.getReceiver(invocation); 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 a2118b7bc..b2bd0c435 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 @@ -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() { + 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() + ';', @@ -55,7 +53,7 @@ public void testMethodReference() { @Test void testInstanceMethod() { - compilationHelper + compile() .addSourceLines( "Test.java", "import " + Optional.class.getName() + ';', @@ -70,7 +68,7 @@ void testInstanceMethod() { @Test void testLocalInstanceMethod() { - compilationHelper + compile() .addSourceLines( "Test.java", "import " + Optional.class.getName() + ';', @@ -88,7 +86,7 @@ void testLocalInstanceMethod() { @Test public void testLocalInstanceMethodSupplier() { - compilationHelper + compile() .addSourceLines( "Test.java", "import " + ImmutableList.class.getName() + ';', @@ -108,7 +106,7 @@ public void testLocalInstanceMethodSupplier() { @Test void testLocalStaticMethod_multiParam() { - compilationHelper + compile() .addSourceLines( "Test.java", "import " + Map.class.getName() + ';', @@ -126,7 +124,7 @@ void testLocalStaticMethod_multiParam() { @Test public void testLocalMethodSupplier_block() { - compilationHelper + compile() .addSourceLines( "Test.java", "import " + ImmutableList.class.getName() + ';', @@ -146,7 +144,7 @@ public void testLocalMethodSupplier_block() { @Test public void testStaticMethod_block() { - compilationHelper + compile() .addSourceLines( "Test.java", "import " + ImmutableList.class.getName() + ';', @@ -163,7 +161,7 @@ public void testStaticMethod_block() { @Test public void testAutoFix_staticMethod_block() { - refactoringValidator + refactor() .addInputLines( "Test.java", "import " + ImmutableList.class.getName() + ';', @@ -189,7 +187,7 @@ public void testAutoFix_staticMethod_block() { @Test public void testAutoFix_staticMethodWithParam() { - refactoringValidator + refactor() .addInputLines( "Test.java", "import " + ImmutableList.class.getName() + ';', @@ -215,7 +213,7 @@ public void testAutoFix_staticMethodWithParam() { @Test void testAutoFix_InstanceMethod() { - refactoringValidator + refactor() .addInputLines( "Test.java", "import " + Optional.class.getName() + ';', @@ -237,7 +235,7 @@ void testAutoFix_InstanceMethod() { @Test void testNegative_InstanceMethodWithType() { - refactoringValidator + refactor() .addInputLines( "Test.java", "import " + Optional.class.getName() + ';', @@ -250,9 +248,60 @@ void testNegative_InstanceMethodWithType() { .doTest(); } + @Test + void testNegative_ambiguousStaticReference() { + refactor() + .addInputLines( + "Test.java", + "import " + Optional.class.getName() + ';', + "class Test {", + " public Optional foo(Optional 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 foo(Optional optional) {", + " return optional.map(value -> value.toString());", + " }", + "}") + .expectUnchanged() + .doTest(); + } + + @Test + void testNegative_ambiguousThis() { + refactor() + .addInputLines( + "Test.java", + "import " + Supplier.class.getName() + ';', + "class Test {", + " class Inner {", + " public Supplier foo() {", + // this::bar is incorrect because 'this' is Inner and 'bar' is defined on 'Test'. + " return () -> bar();", + " }", + " }", + " private String bar() {", + " return \"\";", + " }", + "}") + .expectUnchanged() + .doTest(); + } + @Test void testAutoFix_SpecificInstanceMethod() { - refactoringValidator + refactor() .addInputLines( "Test.java", "import " + Optional.class.getName() + ';', @@ -274,7 +323,7 @@ void testAutoFix_SpecificInstanceMethod() { @Test void testAutoFix_SpecificInstanceMethod_withTypeParameters() { - refactoringValidator + refactor() .addInputLines( "Test.java", "import " + Optional.class.getName() + ';', @@ -296,7 +345,7 @@ void testAutoFix_SpecificInstanceMethod_withTypeParameters() { @Test void testAutoFix_localInstanceMethod() { - refactoringValidator + refactor() .addInputLines( "Test.java", "import " + Optional.class.getName() + ';', @@ -324,7 +373,7 @@ void testAutoFix_localInstanceMethod() { @Test void testAutoFix_localInstanceMethod_explicitThis() { - refactoringValidator + refactor() .addInputLines( "Test.java", "import " + Optional.class.getName() + ';', @@ -352,7 +401,7 @@ void testAutoFix_localInstanceMethod_explicitThis() { @Test void testAutoFix_localStaticMethod() { - refactoringValidator + refactor() .addInputLines( "Test.java", "import " + Optional.class.getName() + ';', @@ -380,7 +429,7 @@ void testAutoFix_localStaticMethod() { @Test void testAutoFix_localStaticMethod_multiParam() { - refactoringValidator + refactor() .addInputLines( "Test.java", "import " + Map.class.getName() + ';', @@ -408,7 +457,7 @@ void testAutoFix_localStaticMethod_multiParam() { @Test void testAutoFix_StaticMethod_multiParam() { - refactoringValidator + refactor() .addInputLines( "Test.java", "import " + Map.class.getName() + ';', @@ -432,7 +481,7 @@ void testAutoFix_StaticMethod_multiParam() { @Test public void testAutoFix_block_localMethod() { - refactoringValidator + refactor() .addInputLines( "Test.java", "import " + ImmutableList.class.getName() + ';', @@ -464,7 +513,7 @@ public void testAutoFix_block_localMethod() { @Test public void testNegative_block() { - compilationHelper + compile() .addSourceLines( "Test.java", "import " + ImmutableList.class.getName() + ';', @@ -480,7 +529,7 @@ public void testNegative_block() { @Test public void testPositive_expression() { - compilationHelper + compile() .addSourceLines( "Test.java", "import " + ImmutableList.class.getName() + ';', @@ -497,7 +546,7 @@ public void testPositive_expression() { @Test public void testAutoFix_expression() { - refactoringValidator + refactor() .addInputLines( "Test.java", "import " + ImmutableList.class.getName() + ';', @@ -523,7 +572,7 @@ public void testAutoFix_expression() { @Test public void testNegative_expression_staticMethod() { - compilationHelper + compile() .addSourceLines( "Test.java", "import " + ImmutableList.class.getName() + ';', @@ -538,7 +587,7 @@ public void testNegative_expression_staticMethod() { @Test public void testAutoFix_expression_localMethod() { - refactoringValidator + refactor() .addInputLines( "Test.java", "import " + ImmutableList.class.getName() + ';', @@ -570,7 +619,7 @@ public void testAutoFix_expression_localMethod() { @Test public void testAutoFix_expression_referenceMethod() { - refactoringValidator + refactor() .addInputLines( "Test.java", "import " + ImmutableList.class.getName() + ';', @@ -598,7 +647,7 @@ public void testAutoFix_expression_referenceMethod() { @Test void testNegative_LocalStaticMethod_multiParam() { - compilationHelper + compile() .addSourceLines( "Test.java", "import " + Map.class.getName() + ';', @@ -615,7 +664,7 @@ void testNegative_LocalStaticMethod_multiParam() { @Test public void testNegative_expression() { - compilationHelper + compile() .addSourceLines( "Test.java", "import " + ImmutableList.class.getName() + ';', @@ -631,7 +680,7 @@ public void testNegative_expression() { @Test public void testNegative_expression_chain() { - compilationHelper + compile() .addSourceLines( "Test.java", "import " + ImmutableList.class.getName() + ';', @@ -649,7 +698,7 @@ public void testNegative_expression_chain() { @Test public void testNegative_dont_eagerly_capture_reference() { - compilationHelper + compile() .addSourceLines( "Test.java", "import " + Supplier.class.getName() + ';', @@ -663,4 +712,19 @@ public void testNegative_dont_eagerly_capture_reference() { "}") .doTest(); } + + @Test + public void testGuavaToJavaUtilOptional() { + refactor() + .addInputLines( + "Test.java", + "import java.util.stream.Stream;", + "class Test {", + " Stream> f(Stream> in) {", + " return in.map(value -> value.toJavaUtil());", + " }", + "}") + .expectUnchanged() + .doTest(); + } }