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

org.openrewrite.staticanalysis.ReplaceDuplicateStringLiterals creates unnecessary duplicates #351

Open
blipper opened this issue Oct 2, 2024 · 2 comments
Labels
bug Something isn't working

Comments

@blipper
Copy link
Contributor

blipper commented Oct 2, 2024

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

class A {
  public static final Map<String, String> test = Map.of("Android");
  public static final String ANDROID = "Android";
  void foo() {
   System.out.println("Android");
   }
}

What did you expect to see?

class A {
  public static final String ANDROID = "Android";
  public static final Map<String, String> test = Map.of(ANROID);
  void foo() {
   System.out.println(ANDROID);
   }
}

What did you see instead?

class A {
  private static final String ANDROID_1 = "Android";
  public static final Map<String, String> test = Map.of(ANROID_1);
  public static final String ANDROID = ANROID_1;
  void foo() {
   System.out.println(ANDROID);
   }
}

What is the full stack trace of any errors you encountered?

stacktrace output here

Are you interested in contributing a fix to OpenRewrite?

@blipper blipper added the bug Something isn't working label Oct 2, 2024
@timtebeek timtebeek moved this to Backlog in OpenRewrite Oct 6, 2024
@nielsdebruin
Copy link
Contributor

nielsdebruin commented Oct 9, 2024

I believe this behavior might be a "feature" ;) rather than a bug? The behavior arises due to the ANDROID constant field being public. The recipe always generate private static final fields to replace any duplicate literals. If this result in a naming conflict, with another field that either does not share the same value, or is not private static final...Then the recipe will generate a new private static final variable to ensure immutability and proper encapsulation. Hence, if ANDROID were private instead of public, the expected behavior in your example would occur and ANDROID will be directly referenced without creating an additional private variable.

@blipper
Copy link
Contributor Author

blipper commented Oct 10, 2024

It is still arguably a bug. It is static and the same value. Nothing prevents a future refactor if you want to diverge the value for internal purposes. If there is a public static IMO it should use that. Immutability is the only property required not encapsulation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Backlog
Development

No branches or pull requests

2 participants