Skip to content

Do not adopt Arrays.toString for varargs arguments #246

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

Merged
merged 4 commits into from
Jan 31, 2024
Merged

Do not adopt Arrays.toString for varargs arguments #246

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
51aeea5
Do not adopt Arrays.toString for varargs arguments
timtebeek Jan 19, 2024
b2c11dd
Validate an additional case
timtebeek Jan 19, 2024
312a2fa
Merge branch 'main' into no-arrays.tostring-on-varargs
timtebeek Jan 19, 2024
5d63da6
Merge branch 'main' into no-arrays.tostring-on-varargs
timtebeek Jan 22, 2024
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
17 changes: 14 additions & 3 deletions src/main/java/org/openrewrite/staticanalysis/RemoveToStringCallsFromArrayInstances.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,10 @@
import org.openrewrite.java.JavaTemplate;
import org.openrewrite.java.JavaVisitor;
import org.openrewrite.java.MethodMatcher;
import org.openrewrite.java.tree.*;
import org.openrewrite.java.tree.Expression;
import org.openrewrite.java.tree.J;
import org.openrewrite.java.tree.JavaType;
import org.openrewrite.java.tree.TypedTree;

import java.util.Arrays;
import java.util.Collections;
Expand Down Expand Up @@ -75,9 +78,17 @@ public J visitMethodInvocation(J.MethodInvocation mi, ExecutionContext ctx) {
return buildReplacement(select, mi);
} else if (METHOD_MATCHERS.stream().anyMatch(matcher -> matcher.matches(mi))) {
// deals with edge cases where .toString() is called implicitly
JavaType.Method methodType = mi.getMethodType();
if (methodType == null) {
return mi;
}
List<JavaType> parameterTypes = methodType.getParameterTypes();
Copy link
Contributor

@knutwannheden knutwannheden Jan 31, 2024

Choose a reason for hiding this comment

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

An alternative would be to rely on Flag.Varargs which when set on the JavaType.Method means that it is a varargs method.

Copy link
Member Author

@timtebeek timtebeek Jan 31, 2024

Choose a reason for hiding this comment

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

Didn't know that flag, thanks! But then that only applies to the entire method right? Yet we'd still want to do replacements on String.format and friends when we can. Say String.format gets called with 6 arguments, one of them is an array. The current implementation would then still wrap that one array in Arrays.toString. Or how would you apply that flag here?

Copy link
Contributor

@knutwannheden knutwannheden Jan 31, 2024

Choose a reason for hiding this comment

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

Not sure. I would probably just use the flag to check what the recipe should do with the arguments corresponding to the (last) varargs parameter, if any. Probably no huge improvement...

List<Expression> arguments = mi.getArguments();
for (Expression arg : arguments) {
if (arg.getType() instanceof JavaType.Array) {
for (int i = 0; i < arguments.size(); i++) {
Expression arg = arguments.get(i);
if (arg.getType() instanceof JavaType.Array &&
(i > parameterTypes.size() - 1 ||
!(parameterTypes.get(i) instanceof JavaType.Array))) {
getCursor().putMessage("METHOD_KEY", mi);
break;
}
Expand Down
69 changes: 42 additions & 27 deletions src/test/java/org/openrewrite/staticanalysis/RemoveToStringCallsFromArrayInstancesTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,7 @@
public class RemoveToStringCallsFromArrayInstancesTest implements RewriteTest {
@Override
public void defaults(RecipeSpec spec) {
spec
.recipe(new RemoveToStringCallsFromArrayInstances());
spec.recipe(new RemoveToStringCallsFromArrayInstances());
}

@Test
Expand Down Expand Up @@ -227,16 +226,6 @@ public static void main(String[] args) {
System.out.println(String.format("s=%s", s));
}
}
""",
"""
import java.util.Arrays;

class SomeClass {
public static void main(String[] args) {
int[] s = new int[]{1, 2, 3};
System.out.println(String.format("s=%s", Arrays.toString(s)));
}
}
"""
)
);
Expand Down Expand Up @@ -429,7 +418,7 @@ public static void main(String[] args) {
}

@Test
void worksWithPrintStreamFormat() {
void doesNotRunOnPrintStreamFormat() {
//language=java
rewriteRun(
java(
Expand All @@ -445,20 +434,6 @@ public static void main(String[] args) {
ps.flush();
}
}
""",
"""
import java.io.PrintStream;
import java.util.Arrays;

class SomeClass {
public static void main(String[] args) {
PrintStream ps = new PrintStream(System.out);
String[] arr = new String[]{"test", "array"};

ps.format("formatting array: %s", Arrays.toString(arr));
ps.flush();
}
}
"""
)
);
Expand Down Expand Up @@ -500,4 +475,44 @@ public static void main(String[] args) {
);
}

@Test
void varargs() {
//language=java
rewriteRun(
java(
"""
class SomeClass {
String foo(Object[] strings) {
return String.format("%s %s", strings);
}
}
"""
)
);
}

@Test
void varargsButTwoArrays() {
//language=java
rewriteRun(
java(
"""
class SomeClass {
String foo(Object[] array1, Object[] array2) {
return String.format("%s %s", array1, array2);
}
}
""",
"""
import java.util.Arrays;

class SomeClass {
String foo(Object[] array1, Object[] array2) {
return String.format("%s %s", Arrays.toString(array1), Arrays.toString(array2));
}
}
"""
)
);
}
}