openrewrite / rewrite-static-analysis

OpenRewrite recipes for identifying and fixing static analysis issues.
Apache License 2.0
27 stars 43 forks source link

`UseStringReplace` should not rewrite when special replacement string #301

Closed protocol7 closed 6 days ago

protocol7 commented 3 weeks ago

\ 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?

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");
              }
            }
            """));
  }
}
timtebeek commented 3 weeks ago

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. https://github.com/openrewrite/rewrite-static-analysis/blob/8a196e2a5d6236457d302e1df9fc7b41c000c9d3/src/main/java/org/openrewrite/staticanalysis/UseStringReplace.java#L84-L89

protocol7 commented 6 days ago

PR for this here: https://github.com/openrewrite/rewrite-static-analysis/pull/306