Recipe to remove boolVariation and clean up if statement

[timtebeek]

What's changed?

Add recipe to remove calls like client.boolVariation("flag-key-123abc", context, false) with true or false.

What's your motivation?

Clean up feature flags and code paths.

Have you considered any alternatives or workarounds?

Not really. Quick time boxed (one flight) attempt.

Any additional context

• https://github.com/openrewrite/rewrite-launchdarkly/issues/5

[shanman190]

@timtebeek, really great start! Just a couple of general ideas:

1. Just a note in terms of the "Do no harm" principle, this'll walk a fine line which I think is fine. What I mean here is that the end user will have had to be consistent with how they used the flag. If they chose to flip flop between true and false cases within their own code, then this recipe will introduce what would be construed as a bug in behavior -- which should be caught during code review. But if they remained consistent always using the same condition statement, then this recipe will perform it's duties perfectly. Tim pointed out that there is a test case that covers negated usages, so that even if the flag is being used in an inverse way at another call site, that the overall condition is evaluated correctly.

2. Along the lines of your todo comment in the code related to applying the removal less bluntly, it could be a useful improvement of the RemoveUnusedLocalVariable if it's visitor could be customized with a specific variable(actual OpenRewrite LST element) or a type selector (com.launchdarkly.sdk.*) to remove from a filtering standpoint. This could allow upstream recipe authors to be more specific in terms of what they would like to see removed rather than just everything unused. Default could be to remove everything if the additional options weren't provided.

3. Should we also potentially consider scheduling a remove unused field visitor to cleanup potential LDClient cases?

[timtebeek]

Thanks for the feedback! I've gone over this one again, and I think we're at a point where we can merge an initial version, even if there's small things we could improve still.

Some future improvements or considerations:

• RemoveUnusedLocalVariables does not clean up LDContext when the assignment might have side effects, such as when using the builder. Might need a separate recipe that ignores the potential builder side effect.
• unused method arguments are not removed and cascade upwards to callers (no such recipe)
• local variables that become true or false are not inlined (no such recipe)
• the doAfterVisit recipes now affect the entire SourceFile containing a feature flag, but not other SourceFiles. I think that's good-enough for the majority of cases. Limiting those clean ups to just the affected variables or conditional would be considerably more effort.
• possibly reuse FindFeatureFlag to identify feature keys in more places
• combine with ReplaceConstant to inline constants using the same feature key