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

Convert multi param lambdas and local method invocations to method references #1365

Merged
merged 9 commits into from
Jun 1, 2020
+589 −93
Merged

Convert multi param lambdas and local method invocations to method references #1365

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
3fa6f1b
baby steps
May 29, 2020
c7fa4b0
convert local methods and multi param methods
May 29, 2020
474512d
Add generated changelog entries
May 29, 2020
9ac79db
handle explicit this
May 29, 2020
7f47bb7
simplify
May 29, 2020
0ecf78c
Merge branch 'fo/lambda-methods' of github.com:palantir/gradle-baseli…
May 29, 2020
6369791
demonstrate static method with param
May 29, 2020
7a43e41
Ckozak/fo/lambda methods/fix (#1369)
carterkozak May 29, 2020
df14397
validate against ambiguous references (#1372)
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
94 changes: 70 additions & 24 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 @@ -17,6 +17,7 @@
package com.palantir.baseline.errorprone;

import com.google.auto.service.AutoService;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
import com.google.errorprone.BugPattern;
import com.google.errorprone.VisitorState;
Expand All @@ -27,13 +28,16 @@
import com.google.errorprone.util.ASTHelpers;
import com.sun.source.tree.BlockTree;
import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.IdentifierTree;
import com.sun.source.tree.LambdaExpressionTree;
import com.sun.source.tree.MethodInvocationTree;
import com.sun.source.tree.ReturnTree;
import com.sun.source.tree.StatementTree;
import com.sun.source.tree.Tree;
import com.sun.tools.javac.code.Symbol;
import com.sun.tools.javac.code.Type;
import java.util.List;
import java.util.Objects;
import java.util.Optional;

@AutoService(BugChecker.class)
Expand All @@ -50,12 +54,6 @@ public final class LambdaMethodReference extends BugChecker implements BugChecke

@Override
public Description matchLambdaExpression(LambdaExpressionTree tree, VisitorState state) {
// Only handle simple no-arg method references for the time being, don't worry about
// simplifying map.forEach((k, v) -> func(k, v)) to map.forEach(this::func)
if (tree.getParameters().size() > 1) {
return Description.NO_MATCH;
}

LambdaExpressionTree.BodyKind bodyKind = tree.getBodyKind();
Tree body = tree.getBody();
// n.b. These checks are meant to avoid any and all cleverness. The goal is to be confident
Expand Down Expand Up @@ -91,49 +89,97 @@ public Description matchLambdaExpression(LambdaExpressionTree tree, VisitorState

private Description checkMethodInvocation(
MethodInvocationTree methodInvocation, LambdaExpressionTree root, VisitorState state) {
if (!methodInvocation.getArguments().isEmpty()
|| !methodInvocation.getTypeArguments().isEmpty()) {
Symbol.MethodSymbol methodSymbol = ASTHelpers.getSymbol(methodInvocation);
if (methodSymbol == null || !methodInvocation.getTypeArguments().isEmpty()) {
return Description.NO_MATCH;
}
Symbol.MethodSymbol methodSymbol = ASTHelpers.getSymbol(methodInvocation);
if (methodSymbol == null || shouldIgnore(methodSymbol, root, methodInvocation)) {

if (methodInvocation.getArguments().isEmpty()
&& (root.getParameters().size() == 1 || root.getParameters().isEmpty())) {
return convertSuppliersAndVariableInstanceMethods(methodSymbol, methodInvocation, root, state);
}

if (methodInvocation.getArguments().size() == root.getParameters().size()) {
return convertMethodInvocations(methodSymbol, methodInvocation, root, state);
}

return Description.NO_MATCH;
}

private Description convertSuppliersAndVariableInstanceMethods(
ferozco marked this conversation as resolved.
Show resolved Hide resolved
Symbol.MethodSymbol methodSymbol,
MethodInvocationTree methodInvocation,
LambdaExpressionTree root,
VisitorState state) {
if (!root.getParameters().isEmpty()) {
Symbol paramSymbol = ASTHelpers.getSymbol(Iterables.getOnlyElement(root.getParameters()));
Symbol receiverSymbol = ASTHelpers.getSymbol(ASTHelpers.getReceiver(methodInvocation));
if (!paramSymbol.equals(receiverSymbol)) {
return Description.NO_MATCH;
}
}

ExpressionTree receiver = ASTHelpers.getReceiver(methodInvocation);
boolean isLocal = receiver == null;
if (!isLocal && !(receiver instanceof IdentifierTree)) {
return Description.NO_MATCH;
}

return buildDescription(root)
.setMessage(MESSAGE)
.addFix(buildFix(methodSymbol, methodInvocation, root, state))
.addFix(buildFix(methodSymbol, methodInvocation, root, state, isLocal))
.build();
}

private static boolean shouldIgnore(
Symbol.MethodSymbol methodSymbol, LambdaExpressionTree root, MethodInvocationTree methodInvocation) {
if (!methodSymbol.isStatic()) {
if (root.getParameters().size() == 1) {
Symbol paramSymbol = ASTHelpers.getSymbol(Iterables.getOnlyElement(root.getParameters()));
Symbol receiverSymbol = ASTHelpers.getSymbol(ASTHelpers.getReceiver(methodInvocation));
return !paramSymbol.equals(receiverSymbol);
}
return true;
private Description convertMethodInvocations(
Symbol.MethodSymbol methodSymbol,
MethodInvocationTree methodInvocation,
LambdaExpressionTree root,
VisitorState state) {
List<Symbol> methodParams = getSymbols(methodInvocation.getArguments());
List<Symbol> lambdaParam = getSymbols(root.getParameters());

// We are guaranteed that all of root params are symbols so equality should handle cases where methodInvocation
// arguments are not symbols or are out of order
if (!methodParams.equals(lambdaParam)) {
return Description.NO_MATCH;
}
return !root.getParameters().isEmpty();

boolean isLocal = ASTHelpers.getReceiver(methodInvocation) == null;
ferozco marked this conversation as resolved.
Show resolved Hide resolved
return buildDescription(root)
.setMessage(MESSAGE)
.addFix(buildFix(methodSymbol, methodInvocation, root, state, isLocal))
.build();
}

private static List<Symbol> getSymbols(List<? extends Tree> params) {
return params.stream()
.map(ASTHelpers::getSymbol)
.filter(Objects::nonNull)
.collect(ImmutableList.toImmutableList());
}

private static Optional<SuggestedFix> buildFix(
Symbol.MethodSymbol symbol,
MethodInvocationTree invocation,
LambdaExpressionTree root,
VisitorState state) {
VisitorState state,
boolean isLocal) {
SuggestedFix.Builder builder = SuggestedFix.builder();
return toMethodReference(qualifyTarget(symbol, invocation, builder, state))
return toMethodReference(qualifyTarget(symbol, invocation, builder, state, isLocal))
.map(qualified -> builder.replace(root, qualified).build());
}

private static String qualifyTarget(
Symbol.MethodSymbol symbol,
MethodInvocationTree invocation,
SuggestedFix.Builder builder,
VisitorState state) {
VisitorState state,
boolean isLocal) {
if (!symbol.isStatic() && isLocal) {
return "this." + symbol.name.toString();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there might be some oddness if the method is. It provided by this, but an enclosing class, requiring prefix “Enclosing.this.“. Worth a test to verify, I’d probably exclude that from the check rather than attempt to refactor since it’s fairly uncommon.

}

Type receiverType = ASTHelpers.getReceiverType(invocation);
if (receiverType == null || receiverType.getLowerBound() != null || receiverType.getUpperBound() != null) {
return SuggestedFixes.qualifyType(state, builder, symbol);
Expand Down
Loading