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 is overly cautious around the use of = #330

Closed
blipper opened this issue Aug 16, 2024 · 2 comments · Fixed by #332
Closed

UseStringReplace is overly cautious around the use of = #330

blipper opened this issue Aug 16, 2024 · 2 comments · Fixed by #332
Assignees
Labels
bug Something isn't working good first issue Good for newcomers test provided

Comments

@blipper
Copy link
Contributor

blipper commented Aug 16, 2024

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

class A {
    void foo(String bar) {
        bar.replace("=","|");
    }
}

What did you see instead?

class A {
    void foo(String bar) {
        bar.replaceAll("=","|");
    }
}

https://github.com/openrewrite/rewrite-static-analysis/blob/main/src/main/java/org/openrewrite/staticanalysis/UseStringReplace.java#L77 seems to just scan?

Are you interested in contributing a fix to OpenRewrite?

@blipper blipper added the bug Something isn't working label Aug 16, 2024
@timtebeek
Copy link
Contributor

Hi @blipper ; Could it be that you flipped the before and after examples here? Since we go from replaceAll to replace when we can safely do so. Here's the test I think you meant that indeed replicates nothing is done currently.

@Test
@Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/330")
void overlyCautious() {
    //language=java
    rewriteRun(
      java(
        """
          class A {
              String foo(String bar) {
                  return bar.replaceAll("=","|");
              }
          }
          """,
        """
          class A {
              String foo(String bar) {
                  return bar.replace("=","|");
              }
          }
          """
      )
    );
}

@timtebeek
Copy link
Contributor

It looks like we're indeed cautious excluding that =, even if that's only in these patterns:
https://docs.oracle.com/javase/8/docs/api/java/util/regex/Pattern.html

Special constructs (named-capturing and non-capturing)

Construct Matchers
(?=X) X, via zero-width positive lookahead
(?<=X) X, via zero-width positive lookbehind

As a quick glance we can change that METACHARACTERS to something like the following to get the test to pass.

- private static final Pattern METACHARACTERS = Pattern.compile("[(\\[{\\\\^\\-=$!|\\]})?*+.]");
+ private static final Pattern METACHARACTERS = Pattern.compile("[(\\[{\\\\^\\-$!|\\]})?*+.]|\\?=|<=");

@timtebeek timtebeek moved this to Backlog in OpenRewrite Aug 17, 2024
@timtebeek timtebeek changed the title org.openrewrite.staticanalysis.UseStringReplace is overly cautious UseStringReplace is overly cautious around the use of = Aug 17, 2024
@github-project-automation github-project-automation bot moved this from Backlog to Done in OpenRewrite Aug 21, 2024
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
Development

Successfully merging a pull request may close this issue.

2 participants