Skip to content

Commit

Permalink
Improve the RedundantStringConversion check (#193)
Browse files Browse the repository at this point in the history 
- Describe the set of well-known string conversion methods more
  concisely.
- Extend said set to include all relevant `String#valueOf` overloads.
  • Loading branch information
@Stephan202
Stephan202 authored Sep 10, 2022
1 parent 7daabee commit 63c3aa8
Show file tree
Hide file tree
Showing 2 changed files with 68 additions and 57 deletions.
100 changes: 43 additions & 57 deletions ...e-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RedundantStringConversion.java
View file
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
package tech.picnic.errorprone.bugpatterns;

import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static com.google.errorprone.BugPattern.LinkType.NONE;
import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION;
import static com.google.errorprone.BugPattern.StandardTags.SIMPLIFICATION;
import static com.google.errorprone.matchers.Matchers.allOf;
import static com.google.errorprone.matchers.Matchers.anyMethod;
import static com.google.errorprone.matchers.Matchers.anyOf;
import static com.google.errorprone.matchers.Matchers.argumentCount;
import static com.google.errorprone.matchers.Matchers.isNonNullUsingDataflow;
import static com.google.errorprone.matchers.Matchers.isSameType;
import static com.google.errorprone.matchers.Matchers.isSubtypeOf;
Expand All @@ -13,7 +16,9 @@
import static com.google.errorprone.matchers.method.MethodMatchers.staticMethod;

import com.google.auto.service.AutoService;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
import com.google.common.primitives.Primitives;
import com.google.errorprone.BugPattern;
import com.google.errorprone.ErrorProneFlags;
import com.google.errorprone.VisitorState;
Expand All @@ -24,6 +29,7 @@
import com.google.errorprone.fixes.SuggestedFix;
import com.google.errorprone.matchers.Description;
import com.google.errorprone.matchers.Matcher;
import com.google.errorprone.suppliers.Suppliers;
import com.sun.source.tree.BinaryTree;
import com.sun.source.tree.CompoundAssignmentTree;
import com.sun.source.tree.ExpressionTree;
Expand All @@ -41,6 +47,7 @@
import java.util.Locale;
import java.util.Objects;
import java.util.Optional;
import java.util.stream.Stream;
import tech.picnic.errorprone.bugpatterns.util.MethodMatcherFactory;
import tech.picnic.errorprone.bugpatterns.util.SourceCode;

Expand Down Expand Up @@ -69,49 +76,30 @@ public final class RedundantStringConversion extends BugChecker
allOf(STRING, isNonNullUsingDataflow());
private static final Matcher<ExpressionTree> NOT_FORMATTABLE =
not(isSubtypeOf(Formattable.class));
private static final Matcher<ExpressionTree> WELL_KNOWN_STRING_CONVERSION_METHODS =
private static final Matcher<MethodInvocationTree> WELL_KNOWN_STRING_CONVERSION_METHODS =
anyOf(
instanceMethod().onDescendantOfAny(Object.class.getName()).named("toString"),
staticMethod()
.onClass(Objects.class.getName())
.named("toString")
.withParameters(Object.class.getName()),
staticMethod()
.onClass(String.class.getName())
.named("valueOf")
.withParameters(Object.class.getName()),
staticMethod()
.onClass(String.class.getName())
.named("valueOf")
.withParameters(String.class.getName()),
staticMethod()
.onClass(Byte.class.getName())
.named("toString")
.withParameters(byte.class.getName()),
staticMethod()
.onClass(Character.class.getName())
.named("toString")
.withParameters(char.class.getName()),
staticMethod()
.onClass(Short.class.getName())
.named("toString")
.withParameters(short.class.getName()),
staticMethod()
.onClass(Integer.class.getName())
.named("toString")
.withParameters(int.class.getName()),
staticMethod()
.onClass(Long.class.getName())
.named("toString")
.withParameters(long.class.getName()),
staticMethod()
.onClass(Float.class.getName())
.named("toString")
.withParameters(float.class.getName()),
staticMethod()
.onClass(Double.class.getName())
instanceMethod()
.onDescendantOfAny(Object.class.getName())
.named("toString")
.withParameters(double.class.getName()));
.withNoParameters(),
allOf(
argumentCount(1),
anyOf(
staticMethod()
.onClassAny(
Stream.concat(
Primitives.allWrapperTypes().stream(), Stream.of(Objects.class))
.map(Class::getName)
.collect(toImmutableSet()))
.named("toString"),
allOf(
staticMethod().onClass(String.class.getName()).named("valueOf"),
not(
anyMethod()
.anyClass()
.withAnyName()
.withParametersOfType(
ImmutableList.of(Suppliers.arrayOf(Suppliers.CHAR_TYPE))))))));
private static final Matcher<ExpressionTree> STRINGBUILDER_APPEND_INVOCATION =
instanceMethod()
.onDescendantOf(StringBuilder.class.getName())
Expand All @@ -127,17 +115,10 @@ public final class RedundantStringConversion extends BugChecker
staticMethod().onClass(String.class.getName()).named("format"),
instanceMethod().onDescendantOf(Formatter.class.getName()).named("format"),
instanceMethod()
.onDescendantOf(PrintStream.class.getName())
.namedAnyOf("format", "printf"),
instanceMethod()
.onDescendantOf(PrintStream.class.getName())
.namedAnyOf("print", "println")
.withParameters(Object.class.getName()),
instanceMethod()
.onDescendantOf(PrintWriter.class.getName())
.onDescendantOfAny(PrintStream.class.getName(), PrintWriter.class.getName())
.namedAnyOf("format", "printf"),
instanceMethod()
.onDescendantOf(PrintWriter.class.getName())
.onDescendantOfAny(PrintStream.class.getName(), PrintWriter.class.getName())
.namedAnyOf("print", "println")
.withParameters(Object.class.getName()),
staticMethod()
Expand All @@ -156,7 +137,7 @@ public final class RedundantStringConversion extends BugChecker
.onDescendantOf("org.slf4j.Logger")
.namedAnyOf("trace", "debug", "info", "warn", "error");

private final Matcher<ExpressionTree> conversionMethodMatcher;
private final Matcher<MethodInvocationTree> conversionMethodMatcher;

/** Instantiates the default {@link RedundantStringConversion}. */
public RedundantStringConversion() {
Expand Down Expand Up @@ -186,7 +167,7 @@ public Description matchBinary(BinaryTree tree, VisitorState state) {

List<SuggestedFix.Builder> fixes = new ArrayList<>();

// XXX: Not so nice: we try to simplify the RHS twice.
// XXX: Avoid trying to simplify the RHS twice.
ExpressionTree preferredRhs = trySimplify(rhs, state).orElse(rhs);
if (STRING.matches(preferredRhs, state)) {
tryFix(lhs, state, ANY_EXPR).ifPresent(fixes::add);
Expand Down Expand Up @@ -276,15 +257,15 @@ private Optional<SuggestedFix.Builder> tryFixGuavaGuard(

// XXX: Write another check which checks that SLF4J patterns don't use `%s` and have a matching
// number of arguments of the appropriate type. Also flag explicit conversions from `Throwable` to
// string as the last logger argument. Suggests either dropping the converison or going with
// string as the last logger argument. Suggests either dropping the conversion or going with
// `Throwable#getMessage()` instead.
private Optional<SuggestedFix.Builder> tryFixSlf4jLogger(
List<? extends ExpressionTree> arguments, VisitorState state) {
/*
* SLF4J treats the final argument to a log statement specially if it is a `Throwabe`: it
* SLF4J treats the final argument to a log statement specially if it is a `Throwable`: it
* will always choose to render the associated stacktrace, even if the argument has a
* matching `{}` placeholder. (In this case the `{}` will simply be logged verbatim.) So if
* a log statement's final argument is the string representation of a `Throwble`, then we
* a log statement's final argument is the string representation of a `Throwable`, then we
* must not strip this explicit string conversion, as that would change the statement's
* semantics.
*/
Expand Down Expand Up @@ -338,11 +319,15 @@ private Optional<ExpressionTree> trySimplify(
}

private Optional<ExpressionTree> trySimplify(ExpressionTree tree, VisitorState state) {
if (tree.getKind() != Kind.METHOD_INVOCATION || !conversionMethodMatcher.matches(tree, state)) {
if (tree.getKind() != Kind.METHOD_INVOCATION) {
return Optional.empty();
}

MethodInvocationTree methodInvocation = (MethodInvocationTree) tree;
if (!conversionMethodMatcher.matches(methodInvocation, state)) {
return Optional.empty();
}

switch (methodInvocation.getArguments().size()) {
case 0:
return trySimplifyNullaryMethod(methodInvocation, state);
Expand Down Expand Up @@ -383,7 +368,8 @@ private Description finalize(Tree tree, Optional<SuggestedFix.Builder> fixes) {
.orElse(Description.NO_MATCH);
}

private static Matcher<ExpressionTree> createConversionMethodMatcher(ErrorProneFlags flags) {
private static Matcher<MethodInvocationTree> createConversionMethodMatcher(
ErrorProneFlags flags) {
// XXX: ErrorProneFlags#getList splits by comma, but method signatures may also contain commas.
// For this class methods accepting more than one argument are not valid, but still: not nice.
return flags
Expand Down
25 changes: 25 additions & 0 deletions ...ntrib/src/test/java/tech/picnic/errorprone/bugpatterns/RedundantStringConversionTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,9 +69,14 @@ void identificationWithinMutatingAssignment() {
" // BUG: Diagnostic contains:",
" s += String.valueOf(i);",
" // BUG: Diagnostic contains:",
" s += String.valueOf(0);",
" // BUG: Diagnostic contains:",
" s += String.valueOf((String) null);",
" s += String.valueOf(null);",
" s += String.valueOf(new char[0]);",
" s += String.valueOf(new char[0], 0, 0);",
" // BUG: Diagnostic contains:",
" s += Boolean.toString(false);",
" // BUG: Diagnostic contains:",
" s += Byte.toString((byte) 0);",
" // BUG: Diagnostic contains:",
Expand Down Expand Up @@ -121,9 +126,12 @@ void identificationWithinBinaryOperation() {
" // BUG: Diagnostic contains:",
" s + String.valueOf(i),",
" // BUG: Diagnostic contains:",
" s + String.valueOf(0),",
" // BUG: Diagnostic contains:",
" s + String.valueOf((String) null),",
" s + String.valueOf(null),",
" s + String.valueOf(new char[0]),",
" s + String.valueOf(new char[0], 0, 0),",
" //",
" 42 + this.toString(),",
" 42 + super.toString(),",
Expand All @@ -134,6 +142,7 @@ void identificationWithinBinaryOperation() {
" 42 + String.valueOf((String) null),",
" 42 + String.valueOf(null),",
" 42 + String.valueOf(new char[0]),",
" 42 + String.valueOf(new char[0], 0, 0),",
"",
" // BUG: Diagnostic contains:",
" this.toString() + s,",
Expand All @@ -144,19 +153,24 @@ void identificationWithinBinaryOperation() {
" // BUG: Diagnostic contains:",
" String.valueOf(i) + s,",
" // BUG: Diagnostic contains:",
" String.valueOf(0) + s,",
" // BUG: Diagnostic contains:",
" String.valueOf((String) null) + s,",
" String.valueOf(null) + s,",
" String.valueOf(new char[0]) + s,",
" String.valueOf(new char[0], 0, 0) + s,",
" //",
" this.toString() + 42,",
" super.toString() + 42,",
" i.toString() + 42,",
" i.toString(16) + 42,",
" String.valueOf(i) + 42,",
" String.valueOf(0) + 42,",
" // BUG: Diagnostic contains:",
" String.valueOf((String) null) + 42,",
" String.valueOf(null) + 42,",
" String.valueOf(new char[0]) + 42,",
" String.valueOf(new char[0], 0, 0) + 42,",
"",
" // BUG: Diagnostic contains:",
" this.toString() + this.toString(),",
Expand All @@ -167,9 +181,12 @@ void identificationWithinBinaryOperation() {
" // BUG: Diagnostic contains:",
" String.valueOf(i) + String.valueOf(i),",
" // BUG: Diagnostic contains:",
" String.valueOf(0) + String.valueOf(0),",
" // BUG: Diagnostic contains:",
" String.valueOf((String) null) + String.valueOf((String) null),",
" String.valueOf(null) + String.valueOf(null),",
" String.valueOf(new char[0]) + String.valueOf(new char[0]),",
" String.valueOf(new char[0], 0, 0) + String.valueOf(new char[0], 0, 0),",
" };",
" }",
"",
Expand Down Expand Up @@ -204,9 +221,12 @@ void identificationWithinStringBuilderMethod() {
" // BUG: Diagnostic contains:",
" sb.append(String.valueOf(i));",
" // BUG: Diagnostic contains:",
" sb.append(String.valueOf(0));",
" // BUG: Diagnostic contains:",
" sb.append(String.valueOf((String) null));",
" sb.append(String.valueOf(null));",
" sb.append(String.valueOf(new char[0]));",
" sb.append(String.valueOf(new char[0], 0, 0));",
" sb.append(s);",
" sb.append(\"constant\");",
"",
Expand All @@ -218,9 +238,12 @@ void identificationWithinStringBuilderMethod() {
" // BUG: Diagnostic contains:",
" sb.insert(0, String.valueOf(i));",
" // BUG: Diagnostic contains:",
" sb.insert(0, String.valueOf(0));",
" // BUG: Diagnostic contains:",
" sb.insert(0, String.valueOf((String) null));",
" sb.insert(0, String.valueOf(null));",
" sb.insert(0, String.valueOf(new char[0]));",
" sb.insert(0, String.valueOf(new char[0], 0, 0));",
" sb.insert(0, s);",
" sb.insert(0, \"constant\");",
"",
Expand Down Expand Up @@ -434,6 +457,8 @@ void identificationOfCustomConversionMethod() {
" // BUG: Diagnostic contains:",
" s + String.valueOf(b),",
" // BUG: Diagnostic contains:",
" s + Boolean.toString(false),",
" // BUG: Diagnostic contains:",
" s + Byte.toString((byte) 0),",
" // BUG: Diagnostic contains:",
" s + Character.toString((char) 0),",
Expand Down

0 comments on commit 63c3aa8

Please sign in to comment.