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

Jump to bottom

Rollforward of https:/google/error-prone/commit/654d1dbf1e6dd652cd6e8ca003643ddf02266ec2: Handle Joiner.on(...) in AbstractToString. #4233

Merged
merged 1 commit into from
Feb 6, 2024
Merged

Rollforward of https:/google/error-prone/commit/654d1dbf1e6dd652cd6e8ca003643ddf02266ec2: Handle Joiner.on(...) in AbstractToString. #4233

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
44 changes: 43 additions & 1 deletion core/src/main/java/com/google/errorprone/bugpatterns/AbstractToString.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import static com.google.errorprone.util.ASTHelpers.getSymbol;
import static com.google.errorprone.util.ASTHelpers.getType;

import com.google.errorprone.ErrorProneFlags;
import com.google.errorprone.VisitorState;
import com.google.errorprone.annotations.FormatMethod;
import com.google.errorprone.bugpatterns.BugChecker.BinaryTreeMatcher;
Expand All @@ -42,6 +43,7 @@
import com.sun.source.tree.Tree.Kind;
import com.sun.source.util.TreePath;
import com.sun.tools.javac.code.Type;
import com.sun.tools.javac.code.Type.ArrayType;
import com.sun.tools.javac.code.Type.MethodType;
import java.util.List;
import java.util.Optional;
Expand Down Expand Up @@ -117,6 +119,15 @@ protected abstract Optional<Fix> toStringFix(
.named("append")
.withParameters("java.lang.Object"));

private static final Matcher<ExpressionTree> JOINER =
instanceMethod().onDescendantOf("com.google.common.base.Joiner").named("join");

private final boolean handleJoiner;

protected AbstractToString(ErrorProneFlags flags) {
this.handleJoiner = flags.getBoolean("AbstractToString:Joiner").orElse(true);
}

private static boolean isInVarargsPosition(
ExpressionTree argTree, MethodInvocationTree methodInvocationTree, VisitorState state) {
int parameterCount = getSymbol(methodInvocationTree).getParameters().size();
Expand All @@ -127,6 +138,17 @@ private static boolean isInVarargsPosition(
&& arguments.indexOf(argTree) >= parameterCount - 1;
}

private static boolean isVarargsArray(
ExpressionTree argTree, MethodInvocationTree methodInvocationTree, VisitorState state) {
int parameterCount = getSymbol(methodInvocationTree).getParameters().size();
List<? extends ExpressionTree> arguments = methodInvocationTree.getArguments();
// Don't match if we're passing an array into a varargs parameter, but do match if there are
// other parameters along with it.
return arguments.size() == parameterCount
&& state.getTypes().isArray(getType(argTree))
&& arguments.indexOf(argTree) == parameterCount - 1;
}

@Override
public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
if (PRINT_STRING.matches(tree, state)) {
Expand Down Expand Up @@ -168,6 +190,22 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState
handleStringifiedTree(argTree, ToStringKind.FLOGGER, state);
}
}
if (handleJoiner && JOINER.matches(tree, state)) {
var symbol = getSymbol(tree);
if (symbol.isVarArgs()) {
for (ExpressionTree argTree : tree.getArguments()) {
if (isVarargsArray(argTree, tree, state)) {
handleStringifiedTree(
((ArrayType) getType(argTree)).getComponentType(),
argTree,
argTree,
ToStringKind.IMPLICIT,
state);
}
handleStringifiedTree(argTree, ToStringKind.IMPLICIT, state);
}
}
}
return NO_MATCH;
}

Expand Down Expand Up @@ -202,7 +240,11 @@ private void handleStringifiedTree(

private void handleStringifiedTree(
Tree parent, ExpressionTree tree, ToStringKind toStringKind, VisitorState state) {
Type type = type(tree);
handleStringifiedTree(type(tree), parent, tree, toStringKind, state);
}

private void handleStringifiedTree(
Type type, Tree parent, ExpressionTree tree, ToStringKind toStringKind, VisitorState state) {
if (type.getKind() == TypeKind.NULL
|| !typePredicate().apply(type, state)
|| allowableToStringKind(toStringKind)) {
Expand Down
7 changes: 7 additions & 0 deletions core/src/main/java/com/google/errorprone/bugpatterns/AnnotationMirrorToString.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import static com.google.errorprone.fixes.SuggestedFixes.qualifyType;

import com.google.errorprone.BugPattern;
import com.google.errorprone.ErrorProneFlags;
import com.google.errorprone.VisitorState;
import com.google.errorprone.fixes.Fix;
import com.google.errorprone.fixes.SuggestedFix;
Expand All @@ -28,6 +29,7 @@
import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.Tree;
import java.util.Optional;
import javax.inject.Inject;

/** A {@link BugChecker}; see the associated {@link BugPattern} annotation for details. */
@BugPattern(
Expand All @@ -40,6 +42,11 @@ public class AnnotationMirrorToString extends AbstractToString {
private static final TypePredicate TYPE_PREDICATE =
TypePredicates.isExactType("javax.lang.model.element.AnnotationMirror");

@Inject
AnnotationMirrorToString(ErrorProneFlags flags) {
super(flags);
}

@Override
protected TypePredicate typePredicate() {
return TYPE_PREDICATE;
Expand Down
7 changes: 7 additions & 0 deletions core/src/main/java/com/google/errorprone/bugpatterns/AnnotationValueToString.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import static com.google.errorprone.fixes.SuggestedFixes.qualifyType;

import com.google.errorprone.BugPattern;
import com.google.errorprone.ErrorProneFlags;
import com.google.errorprone.VisitorState;
import com.google.errorprone.fixes.Fix;
import com.google.errorprone.fixes.SuggestedFix;
Expand All @@ -28,6 +29,7 @@
import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.Tree;
import java.util.Optional;
import javax.inject.Inject;

/** A {@link BugChecker}; see the associated {@link BugPattern} annotation for details. */
@BugPattern(
Expand All @@ -40,6 +42,11 @@ public class AnnotationValueToString extends AbstractToString {
private static final TypePredicate TYPE_PREDICATE =
TypePredicates.isExactType("javax.lang.model.element.AnnotationValue");

@Inject
AnnotationValueToString(ErrorProneFlags flags) {
super(flags);
}

@Override
protected TypePredicate typePredicate() {
return TYPE_PREDICATE;
Expand Down
7 changes: 7 additions & 0 deletions core/src/main/java/com/google/errorprone/bugpatterns/ArrayToString.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import static com.google.errorprone.matchers.method.MethodMatchers.instanceMethod;

import com.google.errorprone.BugPattern;
import com.google.errorprone.ErrorProneFlags;
import com.google.errorprone.VisitorState;
import com.google.errorprone.fixes.Fix;
import com.google.errorprone.fixes.SuggestedFix;
Expand All @@ -32,6 +33,7 @@
import com.sun.tools.javac.code.Type;
import com.sun.tools.javac.code.Types;
import java.util.Optional;
import javax.inject.Inject;

/**
* @author [email protected] (Mike Edgar)
Expand All @@ -47,6 +49,11 @@ public class ArrayToString extends AbstractToString {

private static final TypePredicate IS_ARRAY = TypePredicates.isArray();

@Inject
ArrayToString(ErrorProneFlags flags) {
super(flags);
}

@Override
protected TypePredicate typePredicate() {
return IS_ARRAY;
Expand Down
7 changes: 7 additions & 0 deletions core/src/main/java/com/google/errorprone/bugpatterns/LiteProtoToString.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@

import com.google.common.collect.ImmutableSet;
import com.google.errorprone.BugPattern;
import com.google.errorprone.ErrorProneFlags;
import com.google.errorprone.VisitorState;
import com.google.errorprone.fixes.Fix;
import com.google.errorprone.predicates.TypePredicate;
Expand All @@ -35,6 +36,7 @@
import com.sun.source.tree.Tree;
import com.sun.tools.javac.code.Type;
import java.util.Optional;
import javax.inject.Inject;

/** Flags calls to {@code toString} on lite protos. */
@BugPattern(
Expand Down Expand Up @@ -69,6 +71,11 @@ public final class LiteProtoToString extends AbstractToString {
.add("v", "d", "i")
.build();

@Inject
LiteProtoToString(ErrorProneFlags flags) {
super(flags);
}

@Override
protected TypePredicate typePredicate() {
return LiteProtoToString::matches;
Expand Down
7 changes: 7 additions & 0 deletions core/src/main/java/com/google/errorprone/bugpatterns/ObjectToString.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

import com.google.common.collect.Iterables;
import com.google.errorprone.BugPattern;
import com.google.errorprone.ErrorProneFlags;
import com.google.errorprone.VisitorState;
import com.google.errorprone.fixes.Fix;
import com.google.errorprone.fixes.SuggestedFixes;
Expand All @@ -32,6 +33,7 @@
import com.sun.tools.javac.code.Types;
import com.sun.tools.javac.util.Names;
import java.util.Optional;
import javax.inject.Inject;

/**
* Warns against calling toString() on Objects which don't have toString() method overridden and
Expand Down Expand Up @@ -72,6 +74,11 @@ private static boolean finalNoOverrides(Type type, VisitorState state) {
&& m.overrides(toString, type.tsym, types, /* checkResult= */ false)));
}

@Inject
ObjectToString(ErrorProneFlags flags) {
super(flags);
}

@Override
protected TypePredicate typePredicate() {
return ObjectToString::finalNoOverrides;
Expand Down
7 changes: 7 additions & 0 deletions core/src/main/java/com/google/errorprone/bugpatterns/StreamToString.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,14 @@
import static com.google.errorprone.predicates.TypePredicates.isDescendantOf;

import com.google.errorprone.BugPattern;
import com.google.errorprone.ErrorProneFlags;
import com.google.errorprone.VisitorState;
import com.google.errorprone.fixes.Fix;
import com.google.errorprone.predicates.TypePredicate;
import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.Tree;
import java.util.Optional;
import javax.inject.Inject;

/** A {@link BugChecker}; see the associated {@link BugPattern} annotation for details. */
@BugPattern(
Expand All @@ -35,6 +37,11 @@ public class StreamToString extends AbstractToString {

private static final TypePredicate STREAM = isDescendantOf("java.util.stream.Stream");

@Inject
StreamToString(ErrorProneFlags flags) {
super(flags);
}

@Override
protected TypePredicate typePredicate() {
return STREAM;
Expand Down
7 changes: 7 additions & 0 deletions core/src/main/java/com/google/errorprone/bugpatterns/SymbolToString.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import static com.google.errorprone.util.ASTHelpers.isBugCheckerCode;

import com.google.errorprone.BugPattern;
import com.google.errorprone.ErrorProneFlags;
import com.google.errorprone.VisitorState;
import com.google.errorprone.fixes.Fix;
import com.google.errorprone.matchers.Matcher;
Expand All @@ -32,6 +33,7 @@
import com.sun.source.tree.Tree;
import com.sun.tools.javac.code.Type;
import java.util.Optional;
import javax.inject.Inject;

/**
* Flags {@code com.sun.tools.javac.code.Symbol#toString} usage in {@link BugChecker}s.
Expand All @@ -58,6 +60,11 @@ private static boolean symbolToStringInBugChecker(Type type, VisitorState state)
return IS_SYMBOL.apply(type, state) && STRING_EQUALS.matches(parentTree, state);
}

@Inject
SymbolToString(ErrorProneFlags flags) {
super(flags);
}

@Override
protected TypePredicate typePredicate() {
return SymbolToString::symbolToStringInBugChecker;
Expand Down
5 changes: 4 additions & 1 deletion core/src/main/java/com/google/errorprone/bugpatterns/TreeToString.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import static com.google.errorprone.util.ASTHelpers.isSubtype;

import com.google.errorprone.BugPattern;
import com.google.errorprone.ErrorProneFlags;
import com.google.errorprone.VisitorState;
import com.google.errorprone.fixes.Fix;
import com.google.errorprone.fixes.SuggestedFix;
Expand Down Expand Up @@ -67,7 +68,9 @@ public class TreeToString extends AbstractToString {
.withParameters("java.lang.Object");

@Inject
TreeToString() {}
TreeToString(ErrorProneFlags flags) {
super(flags);
}

@Override
protected TypePredicate typePredicate() {
Expand Down
7 changes: 7 additions & 0 deletions core/src/main/java/com/google/errorprone/bugpatterns/TypeToString.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import static com.google.errorprone.util.ASTHelpers.isBugCheckerCode;

import com.google.errorprone.BugPattern;
import com.google.errorprone.ErrorProneFlags;
import com.google.errorprone.VisitorState;
import com.google.errorprone.fixes.Fix;
import com.google.errorprone.matchers.Matcher;
Expand All @@ -32,6 +33,7 @@
import com.sun.source.tree.Tree;
import com.sun.tools.javac.code.Type;
import java.util.Optional;
import javax.inject.Inject;

/**
* Flags {@code com.sun.tools.javac.code.Type#toString} usage in {@link BugChecker}s.
Expand All @@ -58,6 +60,11 @@ private static boolean typeToStringInBugChecker(Type type, VisitorState state) {
return IS_TYPE.apply(type, state) && STRING_EQUALS.matches(parentTree, state);
}

@Inject
TypeToString(ErrorProneFlags flags) {
super(flags);
}

@Override
protected TypePredicate typePredicate() {
return TypeToString::typeToStringInBugChecker;
Expand Down
14 changes: 14 additions & 0 deletions core/src/test/java/com/google/errorprone/bugpatterns/ArrayToStringTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,4 +185,18 @@ public void positiveConcat() {
public void negativeConcat() {
compilationHelper.addSourceFile("ArrayToStringConcatenationNegativeCases.java").doTest();
}

@Test
public void arrayPassedToJoiner() {
compilationHelper
.addSourceLines(
"Test.java",
"import com.google.common.base.Joiner;",
"class Test {",
" String test(Joiner j, Object[] a) {",
" return j.join(a);",
" }",
Copy link
Contributor

Choose a reason for hiding this comment

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

@graememorgan Do you intend to support the Joiner.join overload where the varargs parameter has other fixed parameters before it?

One of the codebases I maintain has this example:

    private static String makePath(Namespace namespace, Project project, String... path) {
        return "/" + Joiner.on("/").join(namespace.name(), project.name(), (Object[]) path);
    }

And it's now erroring with:

error: [ArrayToString] Calling toString on an array does not provide useful information
        return "/" + Joiner.on("/").join(namespace.name(), project.name(), (Object[]) path);
                                                                           ^
    (see https://errorprone.info/bugpattern/ArrayToString)
  Did you mean 'return "/" + Joiner.on("/").join(namespace.name(), project.name(), Arrays.toString((Object[]) path));'?

The suggestion is an unwanted behavior change, because it introduces the array format and causes tests to start failing with:

Expected :"/NamespacePath/ProjectPath/folder1/folder2/folder3"
Actual   :"/NamespacePath/ProjectPath/[folder1, folder2, folder3]"

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ash211 thanks for the comment, there's a fix in progress for that

"}")
.doTest();
}
}
18 changes: 18 additions & 0 deletions core/src/test/java/com/google/errorprone/bugpatterns/StreamToStringTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,4 +82,22 @@ public void withinStreamClass() {
"}")
.doTest();
}

@Test
public void viaJoiner() {
compilationHelper
.addSourceLines(
"Test.java",
"import com.google.common.base.Joiner;",
"import java.util.stream.Stream;",
"class Test {",
" public void s(Stream<String> xs) {",
" // BUG: Diagnostic contains:",
" var x = Joiner.on(\"foo\").join(xs, xs);",
" // BUG: Diagnostic contains:",
" var y = Joiner.on(\"foo\").join(null, null, new Stream[]{xs});",
" }",
"}")
.doTest();
}
}
Loading