Rollforward of 654d1db: Handle Joiner.on(...) in AbstractToString.

Not yet handling the Iterable overload; that's a bit more involved given the lack of prior art. Handily there was a 'isInVarargsPosition' _right there_. PiperOrigin-RevId: 604758974
google · Feb 6, 2024 · 0bd7432 · 0bd7432
1 parent 5f20325
commit 0bd7432
Show file tree

Hide file tree

Showing 12 changed files with 135 additions and 2 deletions.
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/AbstractToString.java b/core/src/main/java/com/google/errorprone/bugpatterns/AbstractToString.java
@@ -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;
@@ -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;
@@ -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();
@@ -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)) {
@@ -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;
   }
 
@@ -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)) {

diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/AnnotationMirrorToString.java b/core/src/main/java/com/google/errorprone/bugpatterns/AnnotationMirrorToString.java
@@ -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;
@@ -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(
@@ -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;

diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/AnnotationValueToString.java b/core/src/main/java/com/google/errorprone/bugpatterns/AnnotationValueToString.java
@@ -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;
@@ -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(
@@ -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;

diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/ArrayToString.java b/core/src/main/java/com/google/errorprone/bugpatterns/ArrayToString.java
@@ -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;
@@ -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)
@@ -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;

diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/LiteProtoToString.java b/core/src/main/java/com/google/errorprone/bugpatterns/LiteProtoToString.java
@@ -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;
@@ -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(
@@ -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;

diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/ObjectToString.java b/core/src/main/java/com/google/errorprone/bugpatterns/ObjectToString.java
@@ -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;
@@ -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
@@ -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;

diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/StreamToString.java b/core/src/main/java/com/google/errorprone/bugpatterns/StreamToString.java
@@ -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(
@@ -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;

diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/SymbolToString.java b/core/src/main/java/com/google/errorprone/bugpatterns/SymbolToString.java
@@ -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;
@@ -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.
@@ -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;

diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/TreeToString.java b/core/src/main/java/com/google/errorprone/bugpatterns/TreeToString.java
@@ -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;
@@ -67,7 +68,9 @@ public class TreeToString extends AbstractToString {
           .withParameters("java.lang.Object");
 
   @Inject
-  TreeToString() {}
+  TreeToString(ErrorProneFlags flags) {
+    super(flags);
+  }
 
   @Override
   protected TypePredicate typePredicate() {

diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/TypeToString.java b/core/src/main/java/com/google/errorprone/bugpatterns/TypeToString.java
@@ -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;
@@ -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.
@@ -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;

diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/ArrayToStringTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/ArrayToStringTest.java
@@ -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);",
+            "  }",
+            "}")
+        .doTest();
+  }
 }
diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/StreamToStringTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/StreamToStringTest.java
@@ -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();
+  }
 }