diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/AbstractToString.java b/core/src/main/java/com/google/errorprone/bugpatterns/AbstractToString.java index 6a6d03303e8..ce9d7a2bb7b 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/AbstractToString.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/AbstractToString.java @@ -16,6 +16,7 @@ package com.google.errorprone.bugpatterns; +import static com.google.errorprone.VisitorState.memoize; import static com.google.errorprone.matchers.Description.NO_MATCH; import static com.google.errorprone.matchers.Matchers.anyOf; import static com.google.errorprone.matchers.Matchers.instanceMethod; @@ -30,21 +31,27 @@ import com.google.errorprone.annotations.FormatMethod; import com.google.errorprone.bugpatterns.BugChecker.BinaryTreeMatcher; import com.google.errorprone.bugpatterns.BugChecker.CompoundAssignmentTreeMatcher; +import com.google.errorprone.bugpatterns.BugChecker.MemberReferenceTreeMatcher; import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; import com.google.errorprone.fixes.Fix; import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Matcher; import com.google.errorprone.predicates.TypePredicate; +import com.google.errorprone.suppliers.Supplier; import com.sun.source.tree.BinaryTree; import com.sun.source.tree.CompoundAssignmentTree; import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.MemberReferenceTree; import com.sun.source.tree.MethodInvocationTree; import com.sun.source.tree.Tree; import com.sun.source.tree.Tree.Kind; import com.sun.source.util.TreePath; +import com.sun.tools.javac.code.Symbol; +import com.sun.tools.javac.code.Symbol.MethodSymbol; import com.sun.tools.javac.code.Type; import com.sun.tools.javac.code.Type.ArrayType; import com.sun.tools.javac.code.Type.MethodType; +import com.sun.tools.javac.util.Name; import java.util.List; import java.util.Optional; import javax.lang.model.type.TypeKind; @@ -56,7 +63,10 @@ *

See examples in {@link StreamToString} and {@link ArrayToString}. */ public abstract class AbstractToString extends BugChecker - implements BinaryTreeMatcher, MethodInvocationTreeMatcher, CompoundAssignmentTreeMatcher { + implements BinaryTreeMatcher, + MethodInvocationTreeMatcher, + MemberReferenceTreeMatcher, + CompoundAssignmentTreeMatcher { /** The type to match on. */ protected abstract TypePredicate typePredicate(); @@ -93,6 +103,37 @@ protected boolean allowableToStringKind(ToStringKind toStringKind) { protected abstract Optional toStringFix( Tree toStringCall, ExpressionTree stringifiedExpr, VisitorState state); + /** + * Constructs a fix for a method reference to {@code toString()}, e.g. {@code Object::toString}. + * + * @param receiverType the type of the object {@code toString()} will be called on + */ + protected Optional memberReferenceFix( + MemberReferenceTree tree, Type receiverType, VisitorState state) { + return Optional.empty(); + } + + private static final Supplier TO_STRING_NAME = memoize(state -> state.getName("toString")); + + private static final Supplier STRING_VALUE_OF_OBJECT = + memoize( + state -> { + var stringSymbol = state.getSymtab().stringType.tsym; + for (Symbol sym : stringSymbol.members().getSymbolsByName(state.getName("valueOf"))) { + if (sym instanceof MethodSymbol methodSym + && methodSym.isStatic() + && methodSym.getParameters().size() == 1 + && state + .getTypes() + .isSameType( + methodSym.getParameters().get(0).asType(), + state.getSymtab().objectType)) { + return methodSym; + } + } + return null; + }); + private static final Matcher TO_STRING = instanceMethod().anyClass().named("toString").withNoParameters(); @@ -135,10 +176,12 @@ protected abstract Optional toStringFix( private final boolean handleJoinerVarargs; private final boolean handleJoinerIterable; + private final boolean handleMemberReference; protected AbstractToString(ErrorProneFlags flags) { this.handleJoinerVarargs = flags.getBoolean("AbstractToString:Joiner").orElse(true); this.handleJoinerIterable = flags.getBoolean("AbstractToString:JoinerIterable").orElse(true); + this.handleMemberReference = flags.getBoolean("AbstractToString:MemberReference").orElse(true); } private static boolean isInVarargsPosition( @@ -231,6 +274,7 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState elementType.getTypeArguments().getFirst(), argTree, argTree, + // TODO(cpovirk): Suggest a fix. ToStringKind.NONE, state); } @@ -239,6 +283,41 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState return NO_MATCH; } + @Override + public Description matchMemberReference(MemberReferenceTree tree, VisitorState state) { + if (!handleMemberReference) { + return NO_MATCH; + } + + MethodSymbol symbol = getSymbol(tree); + if (symbol == null || !isToStringOrIsStringValueOf(symbol, state)) { + return NO_MATCH; + } + + Type targetType = getType(tree); + if (targetType == null) { + return NO_MATCH; + } + + Type descriptorType = state.getTypes().findDescriptorType(targetType); + if (descriptorType == null || descriptorType.getParameterTypes().isEmpty()) { + return NO_MATCH; + } + + Type receiverType = descriptorType.getParameterTypes().get(0); + if (typePredicate().apply(receiverType, state)) { + handleStringifiedTree(receiverType, tree, tree, ToStringKind.MEMBER_REFERENCE, state); + } + + return NO_MATCH; + } + + private boolean isToStringOrIsStringValueOf(MethodSymbol symbol, VisitorState state) { + return (symbol.getSimpleName().equals(TO_STRING_NAME.get(state)) + && symbol.getParameters().isEmpty()) + || symbol.equals(STRING_VALUE_OF_OBJECT.get(state)); + } + @Override public Description matchBinary(BinaryTree tree, VisitorState state) { if (!state.getTypes().isSameType(getType(tree), state.getSymtab().stringType)) { @@ -293,7 +372,7 @@ private void handleStringifiedTree( stringifiedExpr, state, type, - getFix(stringifiedExpr, state, toStringCall, toStringKind))); + getFix(type, stringifiedExpr, state, toStringCall, toStringKind))); } private static Type type(ExpressionTree tree) { @@ -305,6 +384,7 @@ private static Type type(ExpressionTree tree) { } private Optional getFix( + Type type, ExpressionTree stringifiedExpr, VisitorState state, Tree toStringCall, @@ -312,6 +392,7 @@ private Optional getFix( return switch (toStringKind) { case IMPLICIT, FLOGGER, FORMAT_METHOD -> implicitToStringFix(stringifiedExpr, state); case EXPLICIT -> toStringFix(toStringCall, stringifiedExpr, state); + case MEMBER_REFERENCE -> memberReferenceFix((MemberReferenceTree) toStringCall, type, state); case NONE -> Optional.empty(); }; } @@ -331,6 +412,7 @@ enum ToStringKind { EXPLICIT, FORMAT_METHOD, FLOGGER, + MEMBER_REFERENCE, NONE, } } diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/ArrayToString.java b/core/src/main/java/com/google/errorprone/bugpatterns/ArrayToString.java index 155e656fdc7..3888627f99f 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/ArrayToString.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/ArrayToString.java @@ -17,7 +17,10 @@ package com.google.errorprone.bugpatterns; import static com.google.errorprone.BugPattern.SeverityLevel.ERROR; +import static com.google.errorprone.fixes.SuggestedFixes.qualifyType; import static com.google.errorprone.matchers.method.MethodMatchers.instanceMethod; +import static com.google.errorprone.util.ASTHelpers.getReceiver; +import static com.google.errorprone.util.ASTHelpers.getType; import com.google.errorprone.BugPattern; import com.google.errorprone.ErrorProneFlags; @@ -27,8 +30,8 @@ import com.google.errorprone.matchers.Matcher; import com.google.errorprone.predicates.TypePredicate; import com.google.errorprone.predicates.TypePredicates; -import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.MemberReferenceTree; import com.sun.source.tree.Tree; import com.sun.tools.javac.code.Type; import com.sun.tools.javac.code.Types; @@ -77,14 +80,14 @@ protected Optional toStringFix( // If the array is the result of calling e.getStackTrace(), replace // e.getStackTrace().toString() with Guava's Throwables.getStackTraceAsString(e). if (GET_STACK_TRACE.matches(stringifiedExpr, state)) { + var fix = SuggestedFix.builder(); return Optional.of( - SuggestedFix.builder() - .addImport("com.google.common.base.Throwables") - .replace( + fix.replace( toStringCall, String.format( - "Throwables.getStackTraceAsString(%s)", - state.getSourceForNode(ASTHelpers.getReceiver(stringifiedExpr)))) + "%s.getStackTraceAsString(%s)", + qualifyType(state, fix, "com.google.common.base.Throwables"), + state.getSourceForNode(getReceiver(stringifiedExpr)))) .build()); } // e.g. String.valueOf(theArray) -> Arrays.toString(theArray) @@ -93,21 +96,31 @@ protected Optional toStringFix( return fix(toStringCall, stringifiedExpr, state); } - private static Optional fix(Tree toStringCall, Tree stringifiedExpr, VisitorState state) { - String method = isNestedArray(stringifiedExpr, state) ? "deepToString" : "toString"; + @Override + protected Optional memberReferenceFix( + MemberReferenceTree tree, Type receiverType, VisitorState state) { + String method = isNestedArray(receiverType, state) ? "deepToString" : "toString"; + var fix = SuggestedFix.builder(); + return Optional.of( + fix.replace(tree, qualifyType(state, fix, "java.util.Arrays") + "::" + method).build()); + } + private static Optional fix(Tree toStringCall, Tree stringifiedExpr, VisitorState state) { + String method = isNestedArray(getType(stringifiedExpr), state) ? "deepToString" : "toString"; + var fix = SuggestedFix.builder(); return Optional.of( - SuggestedFix.builder() - .addImport("java.util.Arrays") - .replace( + fix.replace( toStringCall, - String.format("Arrays.%s(%s)", method, state.getSourceForNode(stringifiedExpr))) + String.format( + "%s.%s(%s)", + qualifyType(state, fix, "java.util.Arrays"), + method, + state.getSourceForNode(stringifiedExpr))) .build()); } - private static boolean isNestedArray(Tree stringifiedExpr, VisitorState state) { + private static boolean isNestedArray(Type type, VisitorState state) { Types types = state.getTypes(); - Type withType = ASTHelpers.getType(stringifiedExpr); - return withType != null && types.isArray(withType) && types.isArray(types.elemtype(withType)); + return type != null && types.isArray(type) && types.isArray(types.elemtype(type)); } } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/ArrayToStringTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/ArrayToStringTest.java index 74d8ce11d43..a4a3cc770f5 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/ArrayToStringTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/ArrayToStringTest.java @@ -487,4 +487,53 @@ String test(Joiner j, List a) { """) .doTest(); } + + @Test + public void methodReference() { + refactoringHelper + .addInputLines( + "Test.java", + """ + import static java.util.stream.Collectors.joining; + + import java.util.stream.Stream; + + class Test { + void f(Stream s) { + s.map(Object::toString).collect(joining(", ")); + } + + void g(Stream s) { + s.map(Object::toString).collect(joining(", ")); + } + + void h(Stream s) { + s.map(String::valueOf).collect(joining(", ")); + } + } + """) + .addOutputLines( + "Test.java", + """ + import static java.util.stream.Collectors.joining; + + import java.util.Arrays; + import java.util.stream.Stream; + + class Test { + void f(Stream s) { + s.map(Arrays::toString).collect(joining(", ")); + } + + void g(Stream s) { + s.map(Arrays::deepToString).collect(joining(", ")); + } + + void h(Stream s) { + s.map(Arrays::toString).collect(joining(", ")); + } + } + """) + .doTest(); + } }