RemoveStringFlag should remove method calls & conditions when comparing equal Strings

[bsgongang]

I was using the OpenRewrite rewrite-feature-flags, RemoveStringFlag recipe to remove a feature flag in code.

The recipe replaced the feature flag call with the expected value, but could not use the replaced value to simplify the code.

Here is my code:

type: specs.openrewrite.org/v1beta/recipe
name: com.openrewrite.featureflags.RemoveFlag
displayName: Remove feature flags
recipeList:
 -org.openrewrite.featureflags.RemoveStringFlag:
      methodPattern: com.my.org.impl.Client getFlagAssignment(String, String, String)
      featureKey: “feature_266154"
      replacementValue: "T1” 

code to refactor with above recipe:

private com.my.org.impl.Client client; 

String treatment = client.getFlagAssignment(“feature_266154”, session, clientId); 

if (T1.equals(treatment)) { 
doTreatment(); 
} else { 
doNonTreatment(); 
}

After running the recipe the output

code transformation output

private com.my.org.impl.Client client; 

String treatment = T1; 

if (T1.equals(treatment)) { 
doTreatment(); 
} else { 
doNonTreatment(); 
}

Expectation

doTreatment();

issue: if (T1.equals(treatment)) is not being simplified with the treatment value which is T1; I was expecting further simplification, if (T1.equants(treament == T1)) => if (true)

Describe the solution you'd like

We should enhance the remove feature flag recipe to evaluate expressions with the targeted value, simplify the code, and generate a diff that developers wont have any change to do

Have you considered any alternatives or workarounds?

Trying to add some static analysis recipes or use ast-grep rules to evaluate all expressions with the feature flag assigned value

Additional context

Are you interested in contributing this feature to OpenRewrite? Yes, I am interested

Sure, I would like to work on this. I need a POC from openrewrite with whom i can collaborate to get it done

[timtebeek]

hi @bsgongang ; thanks for the detailed description and offer to help. I think there's (at least) two ways we could go about this.

One would be to inline local variables used in the assignment from your call out to the client, before we call out to SimplifyConstantIfBranchExecution. This could be achieved in this recipe by replacing each assigned variable occurrence with the replacementValue, or using a reusable recipe like this one:

• InlineOneTimeUsageVariable rewrite-static-analysis#363

Another would be to have SimplifyConstantIfBranchExecution use CursorUtil to decide when to simplify a constant if branch, similar to how we look for feature keys that are defined elsewhere.

rewrite-feature-flags/src/main/java/org/openrewrite/featureflags/RemoveStringFlag.java

Lines 81 to 86 in f9ffb53

	private boolean isFeatureKey(Expression firstArgument) {
	return CursorUtil.findCursorForTree(getCursor(), firstArgument)
	.bind(c -> ConstantFold.findConstantLiteralValue(c, String.class))
	.map(featureKey::equals)
	.orSome(false);
	}

Either approach could work here. A fix to SimplifyConstantIfBranchExecution would mean we get those benefits in other recipes as well, so is perhaps worth exploring first.

[timtebeek]

I've pushed up a first stage that should help some of these cases to be replaced more easily in openrewrite/rewrite@2882fdc
eadaf02

[timtebeek]

Thanks for helping explore this @bsgongang ! I've pushed up a change just now that also inlines locally assigned variables.