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.

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

UseStringReplace should not rewrite when special replacement string #301

Closed
protocol7 opened this issue Jun 11, 2024 · 2 comments · Fixed by #306
Closed

UseStringReplace should not rewrite when special replacement string #301

protocol7 opened this issue Jun 11, 2024 · 2 comments · Fixed by #306
Assignees
Labels
bug Something isn't working good first issue Good for newcomers test provided

Comments

@protocol7
Copy link
Contributor

\ and $ in the replacement string of String.replaceAll() has special meaning (https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/util/regex/Matcher.html#replaceAll(java.lang.String)) and should not be rewritten.

What version of OpenRewrite are you using?

  • org.openrewrite:rewrite-core:jar:8.27.4
  • org.openrewrite.recipe:rewrite-static-analysis:jar:1.9.0

What is the smallest, simplest way to reproduce the problem?

import static org.openrewrite.java.Assertions.java;

import org.junit.jupiter.api.Test;
import org.openrewrite.java.JavaParser;
import org.openrewrite.staticanalysis.UseStringReplace;
import org.openrewrite.test.RecipeSpec;
import org.openrewrite.test.RewriteTest;

public class KnownBugsTest implements RewriteTest {

  @Override
  public void defaults(RecipeSpec spec) {
    spec.parser(JavaParser.fromJavaVersion());
  }

  @Test
  public void replaceAllBackslashReplacement() {
    rewriteRun(
        spec -> spec.recipe(new UseStringReplace()),
        java(
            // replacement strings with \ escapes output characters and should not be rewritten
            """
            package com.helloworld;

            class Hello {
              public String hello() {
                return "abc".replaceAll("b", "\\\\\\\\");
              }
            }
            """));
  }

  @Test
  public void replaceAllReferenceReplacement() {
    rewriteRun(
        spec -> spec.recipe(new UseStringReplace()),
        java(
            // replacement strings with $ references capture groups and should not be rewritten
            """
            package com.helloworld;

            class Hello {
              public String hello() {
                return "abc".replaceAll("b", "$0");
              }
            }
            """));
  }
}

@protocol7 protocol7 added the bug Something isn't working label Jun 11, 2024
@timtebeek timtebeek moved this to Backlog in OpenRewrite Jun 13, 2024
@timtebeek
Copy link
Contributor

Thanks for the clear runnable bug report @protocol7 ! Should fit right into UseStringReplaceTest.java. Would you be open to creating a draft pull request using just these failing tests?

Looks like we could then next add a new conditional that checks there's no special characters in the replacement string, and if so skip making changes and return invocation early.

//Checks if method invocation matches with String#replaceAll
if (REPLACE_ALL.matches(invocation)) {
Expression firstArgument = invocation.getArguments().get(0);
//Checks if the first argument is a String literal
if (isStringLiteral(firstArgument)) {

@timtebeek timtebeek changed the title org.openrewrite.staticanalysis.UseStringReplace should not rewrite when special replacement string UseStringReplace should not rewrite when special replacement string Jun 13, 2024
protocol7 pushed a commit to protocol7/rewrite-static-analysis that referenced this issue Jun 29, 2024
…ring

If the replacement string of String.replaceAll contains $ or \, we
should not rewrite it as these indicate special replacements:
https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/util/regex/Matcher.html#replaceAll(java.lang.String)

Fixes openrewrite#301
protocol7 pushed a commit to protocol7/rewrite-static-analysis that referenced this issue Jun 29, 2024
…ring

If the replacement string of String.replaceAll contains $ or \, we
should not rewrite it as these indicate special replacements:
https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/util/regex/Matcher.html#replaceAll(java.lang.String)

Fixes openrewrite#301
@protocol7
Copy link
Contributor Author

PR for this here: #306

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers test provided
Projects
Archived in project
2 participants