Enhancements to remove feature flag recipes

[timtebeek]

What problem are you trying to solve?

We identified a couple of possible improvements following the first recipe to removed a feature flag.

• Recipe to remove boolVariation and clean up if statement #10 (comment)

Describe the solution you'd like

• 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.
• the doAfterVisit recipes now affect the entire SourceFile containing a feature flag, but not other SourceFiles. we might want to limit that further, although it'd be more effort
• possibly reuse FindFeatureFlag to identify feature keys in more places
• combine with ReplaceConstant to inline constants using the same feature key
• unused method arguments are not removed and cascade upwards to callers (no such recipe)
• local variables that become true or false (or String literal) are not inlined (no such recipe)
  • InlineOneTimeUsageVariable rewrite-static-analysis#367

Have you considered any alternatives or workarounds?

Some of these can be separate recipes in for instance rewrite-static-analysis, that we then reuse here.

[kavitha186]

@timtebeek Can I work on this issue?

[timtebeek]

@timtebeek Can I work on this issue?

Hi @kavitha186 yes thanks for your offer to help! Which element specifically would you like to tackle first? A draft PR with a failing test case is typically a great way to start

[kavitha186]

@timtebeek Could you please check my draft PR

• Remove unused local variables despite potential side effects after removing feature flag #35)

I have few questions on attempting to tackle "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."

Should I expect all to be cleared with respect to LDContext? eg: if there is any methods used from LDCotext https://launchdarkly.github.io/java-server-sdk/com/launchdarkly/sdk/LDContext.html .

[kavitha186]

@timtebeek Can I take up possibly reuse FindFeatureFlag to identify feature keys in more places

Can you please help me to understand where the feature keys are not found currently with findFeature flag?

[timtebeek]

@timtebeek Can I take up possibly reuse FindFeatureFlag to identify feature keys in more places

Can you please help me to understand where the feature keys are not found currently with findFeature flag?

Hi @kavitha186 ; appreciate the continued offer to help out! These two PRs already improved what constants we can detect:

• Use new ConstantFold.findConstantExpr #20
• Prevent TraitErrors seen with custom method pattern #24

You'll also see that reflected in a test like this one 6f90bd4. Where we currently fail is a slight alteration where the constant is in another class:

    @Test
    void removeWhenFeatureFlagIsAnExternalConstant() {
        rewriteRun(
          spec -> spec.recipe(new RemoveBooleanFlag("com.acme.bank.CustomLaunchDarklyWrapper featureFlagEnabled(String, boolean)", "flag-key-123abc", true)),
          // language=java
          java(
            """
              package com.acme.bank;
              
              public class CustomLaunchDarklyWrapper {
                  public static final String FEATURE_TOGGLE = "flag-key-123abc";
                  public boolean featureFlagEnabled(String key, boolean fallback) {
                      return fallback;
                  }
              }
              """,
            SourceSpec::skip
          ),
          // language=java
          java(
            """
              import com.acme.bank.CustomLaunchDarklyWrapper;
              import static com.acme.bank.CustomLaunchDarklyWrapper.FEATURE_TOGGLE;
              class Foo {
              
                  private CustomLaunchDarklyWrapper wrapper = new CustomLaunchDarklyWrapper();
                  void bar() {
                      if (wrapper.featureFlagEnabled(FEATURE_TOGGLE, false)) {
                          // Application code to show the feature
                          System.out.println("Feature is on");
                      }
                      else {
                        // The code to run if the feature is off
                          System.out.println("Feature is off");
                      }
                  }
              }
              """,
            """
              class Foo {
                  void bar() {
                      // Application code to show the feature
                      System.out.println("Feature is on");
                  }
              }
              """
          )
        );
    }

I'd expect that to be fairly common, and worth supporting, as folks might very well extract those feature keys to a shared constant. Perhaps this is something we can solve in ConstantFold, but I don't know the full details there to know if that's indeed possible.

We did previously use this other method to determine the literal value for a given constant; that might work here once we've determined that the first argument is a constant, not a literal.

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

Lines 81 to 86 in e481645

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

So it's a bit up in the air how to approach finding more such feature flags usages, but a welcome addition to phase them out anywhere regardless of how they are used.