From bd2cc9c6432f46006d307b0eacd5a0350dc0b107 Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Wed, 4 Dec 2024 10:54:42 +0100 Subject: [PATCH] StringFormatted should not wrap first argument by default in Java 17 upgrade (#618) Fixes https://github.com/openrewrite/rewrite-migrate-java/issues/616 --- .../java/migrate/lang/StringFormatted.java | 106 ++++++++++-------- .../META-INF/rewrite/java-version-17.yml | 3 +- .../migrate/lang/StringFormattedTest.java | 38 +++++-- 3 files changed, 92 insertions(+), 55 deletions(-) diff --git a/src/main/java/org/openrewrite/java/migrate/lang/StringFormatted.java b/src/main/java/org/openrewrite/java/migrate/lang/StringFormatted.java index 4ef8347b04..6da39fbd65 100644 --- a/src/main/java/org/openrewrite/java/migrate/lang/StringFormatted.java +++ b/src/main/java/org/openrewrite/java/migrate/lang/StringFormatted.java @@ -17,15 +17,16 @@ import lombok.EqualsAndHashCode; import lombok.Value; -import org.openrewrite.ExecutionContext; -import org.openrewrite.Preconditions; -import org.openrewrite.Recipe; -import org.openrewrite.TreeVisitor; +import org.jspecify.annotations.Nullable; +import org.openrewrite.*; import org.openrewrite.java.JavaVisitor; import org.openrewrite.java.MethodMatcher; import org.openrewrite.java.search.UsesJavaVersion; import org.openrewrite.java.search.UsesMethod; -import org.openrewrite.java.tree.*; +import org.openrewrite.java.tree.Expression; +import org.openrewrite.java.tree.J; +import org.openrewrite.java.tree.JRightPadded; +import org.openrewrite.java.tree.Space; import org.openrewrite.marker.Markers; import java.time.Duration; @@ -40,6 +41,13 @@ public class StringFormatted extends Recipe { private static final MethodMatcher STRING_FORMAT = new MethodMatcher("java.lang.String format(String, ..)"); + @Option(displayName = "Add parentheses around the first argument", + description = "Add parentheses around the first argument if it is not a simple expression. " + + "Default true; if false no change will be made. ", + required = false) + @Nullable + Boolean addParentheses; + @Override public String getDisplayName() { return "Prefer `String.formatted(Object...)`"; @@ -51,52 +59,56 @@ public String getDescription() { } @Override - public TreeVisitor getVisitor() { - return Preconditions.check( - Preconditions.and(new UsesJavaVersion<>(17), new UsesMethod<>(STRING_FORMAT)), - new StringFormattedVisitor()); + public Duration getEstimatedEffortPerOccurrence() { + return Duration.ofMinutes(1); } - private static class StringFormattedVisitor extends JavaVisitor { - @Override - public J visitMethodInvocation(J.MethodInvocation methodInvocation, ExecutionContext ctx) { - methodInvocation = (J.MethodInvocation) super.visitMethodInvocation(methodInvocation, ctx); - if (!STRING_FORMAT.matches(methodInvocation) || methodInvocation.getMethodType() == null) { - return methodInvocation; - } + @Override + public TreeVisitor getVisitor() { + TreeVisitor check = Preconditions.and(new UsesJavaVersion<>(17), new UsesMethod<>(STRING_FORMAT)); + return Preconditions.check(check, new JavaVisitor() { + @Override + public J visitMethodInvocation(J.MethodInvocation methodInvocation, ExecutionContext ctx) { + methodInvocation = (J.MethodInvocation) super.visitMethodInvocation(methodInvocation, ctx); + if (!STRING_FORMAT.matches(methodInvocation) || methodInvocation.getMethodType() == null) { + return methodInvocation; + } - maybeRemoveImport("java.lang.String.format"); - J.MethodInvocation mi = methodInvocation.withName(methodInvocation.getName().withSimpleName("formatted")); - mi = mi.withMethodType(methodInvocation.getMethodType().getDeclaringType().getMethods().stream() - .filter(it -> it.getName().equals("formatted")) - .findAny() - .orElse(null)); - if (mi.getName().getType() != null) { - mi = mi.withName(mi.getName().withType(mi.getMethodType())); - } - List arguments = methodInvocation.getArguments(); - mi = mi.withSelect(wrapperNotNeeded(arguments.get(0)) ? arguments.get(0).withPrefix(Space.EMPTY) : - new J.Parentheses<>(randomId(), Space.EMPTY, Markers.EMPTY, - JRightPadded.build(arguments.get(0)))); - mi = mi.withArguments(arguments.subList(1, arguments.size())); - if (mi.getArguments().isEmpty()) { - // To store spaces between the parenthesis of a method invocation argument list - // Ensures formatting recipes chained together with this one will still work as expected - mi = mi.withArguments(singletonList(new J.Empty(randomId(), Space.EMPTY, Markers.EMPTY))); - } - return maybeAutoFormat(methodInvocation, mi, ctx); - } + // No change when change might be controversial, such as string concatenation + List arguments = methodInvocation.getArguments(); + boolean wrapperNeeded = wrapperNeeded(arguments.get(0)); + if (Boolean.FALSE.equals(addParentheses) && wrapperNeeded) { + return methodInvocation; + } - private static boolean wrapperNotNeeded(Expression expression) { - return expression instanceof J.Identifier || - expression instanceof J.Literal || - expression instanceof J.MethodInvocation || - expression instanceof J.FieldAccess; - } - } + maybeRemoveImport("java.lang.String.format"); + J.MethodInvocation mi = methodInvocation.withName(methodInvocation.getName().withSimpleName("formatted")); + mi = mi.withMethodType(methodInvocation.getMethodType().getDeclaringType().getMethods().stream() + .filter(it -> it.getName().equals("formatted")) + .findAny() + .orElse(null)); + if (mi.getName().getType() != null) { + mi = mi.withName(mi.getName().withType(mi.getMethodType())); + } + mi = mi.withSelect(wrapperNeeded ? + new J.Parentheses<>(randomId(), Space.EMPTY, Markers.EMPTY, + JRightPadded.build(arguments.get(0))) : + arguments.get(0).withPrefix(Space.EMPTY)); + mi = mi.withArguments(arguments.subList(1, arguments.size())); + if (mi.getArguments().isEmpty()) { + // To store spaces between the parenthesis of a method invocation argument list + // Ensures formatting recipes chained together with this one will still work as expected + mi = mi.withArguments(singletonList(new J.Empty(randomId(), Space.EMPTY, Markers.EMPTY))); + } + return maybeAutoFormat(methodInvocation, mi, ctx); + } - @Override - public Duration getEstimatedEffortPerOccurrence() { - return Duration.ofMinutes(1); + private boolean wrapperNeeded(Expression expression) { + return !(expression instanceof J.Identifier || + expression instanceof J.Literal || + expression instanceof J.MethodInvocation || + expression instanceof J.FieldAccess); + } + }); } } diff --git a/src/main/resources/META-INF/rewrite/java-version-17.yml b/src/main/resources/META-INF/rewrite/java-version-17.yml index 5bfa377819..3d8b681440 100644 --- a/src/main/resources/META-INF/rewrite/java-version-17.yml +++ b/src/main/resources/META-INF/rewrite/java-version-17.yml @@ -28,7 +28,6 @@ tags: recipeList: - org.openrewrite.java.migrate.Java8toJava11 - org.openrewrite.java.migrate.UpgradeBuildToJava17 - - org.openrewrite.java.migrate.lang.StringFormatted - org.openrewrite.staticanalysis.InstanceOfPatternMatch - org.openrewrite.staticanalysis.AddSerialAnnotationToSerialVersionUID - org.openrewrite.java.migrate.RemovedRuntimeTraceMethods @@ -36,6 +35,8 @@ recipeList: - org.openrewrite.java.migrate.RemovedModifierAndConstantBootstrapsConstructors - org.openrewrite.java.migrate.lang.UseTextBlocks: convertStringsWithoutNewlines: false + - org.openrewrite.java.migrate.lang.StringFormatted: + addParentheses: false - org.openrewrite.java.migrate.DeprecatedJavaxSecurityCert - org.openrewrite.java.migrate.DeprecatedLogRecordThreadID - org.openrewrite.java.migrate.RemovedLegacySunJSSEProviderName diff --git a/src/test/java/org/openrewrite/java/migrate/lang/StringFormattedTest.java b/src/test/java/org/openrewrite/java/migrate/lang/StringFormattedTest.java index 2b24078b2c..4647775316 100755 --- a/src/test/java/org/openrewrite/java/migrate/lang/StringFormattedTest.java +++ b/src/test/java/org/openrewrite/java/migrate/lang/StringFormattedTest.java @@ -27,7 +27,7 @@ class StringFormattedTest implements RewriteTest { @Override public void defaults(RecipeSpec spec) { - spec.recipe(new StringFormatted()); + spec.recipe(new StringFormatted(null)); } @Test @@ -72,12 +72,36 @@ void concatenatedText() { class A { String str = String.format("foo" + "%s", "a"); - }""", """ + } + """, + """ package com.example.app; class A { String str = ("foo" + "%s").formatted("a"); - }""" + } + """ + ), + 17 + ) + ); + } + + @Issue("https://github.com/openrewrite/rewrite-migrate-java/issues/616") + @Test + void concatenatedFormatStringNotConvertedIfIndicated() { + //language=java + rewriteRun( + spec -> spec.recipe(new StringFormatted(false)), + version( + java( + """ + package com.example.app; + class A { + String str = String.format("foo" + + "%s", "a"); + } + """ ), 17 ) @@ -92,10 +116,10 @@ void callingFunction() { java( """ package com.example.app; - + class A { String str = String.format(getTemplateString(), "a"); - + private String getTemplateString() { return "foo %s"; } @@ -103,10 +127,10 @@ private String getTemplateString() { """, """ package com.example.app; - + class A { String str = getTemplateString().formatted("a"); - + private String getTemplateString() { return "foo %s"; }