rewrite-static-analysis icon indicating copy to clipboard operation
rewrite-static-analysis copied to clipboard

org.openrewrite.staticanalysis.ReplaceDuplicateStringLiterals creates unnecessary duplicates

Open blipper opened this issue 1 year ago • 3 comments

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 avatar Oct 02 '24 21:10 blipper

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.

nielsdebruin avatar Oct 09 '24 15:10 nielsdebruin

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.

blipper avatar Oct 10 '24 18:10 blipper

This issue is stale because it has not had any activity for 60 days. Remove question label or comment or this will be closed in two weeks. Issues may be reopened when there is renewed interest.

github-actions[bot] avatar Jul 28 '25 11:07 github-actions[bot]