Skip to content

Commit

Permalink
Inline local variables if assigned from feature flag call (#41)
Browse files Browse the repository at this point in the history
  • Loading branch information
@timtebeek
timtebeek authored Dec 22, 2024
1 parent 7244dde commit c448e2a
Show file tree
Hide file tree
Showing 6 changed files with 104 additions and 41 deletions.
56 changes: 45 additions & 11 deletions src/main/java/org/openrewrite/featureflags/RemoveBooleanFlag.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,13 @@

import lombok.EqualsAndHashCode;
import lombok.Value;
import org.jspecify.annotations.Nullable;
import org.openrewrite.*;
import org.openrewrite.analysis.constantfold.ConstantFold;
import org.openrewrite.analysis.util.CursorUtil;
import org.openrewrite.java.JavaVisitor;
import org.openrewrite.java.MethodMatcher;
import org.openrewrite.java.search.SemanticallyEqual;
import org.openrewrite.java.search.UsesMethod;
import org.openrewrite.java.tree.Expression;
import org.openrewrite.java.tree.J;
Expand Down Expand Up @@ -65,25 +67,57 @@ public String getDescription() {
public TreeVisitor<?, ExecutionContext> getVisitor() {
final MethodMatcher methodMatcher = new MethodMatcher(methodPattern, true);
JavaVisitor<ExecutionContext> visitor = new JavaVisitor<ExecutionContext>() {
@Override
public @Nullable J visitVariableDeclarations(J.VariableDeclarations multiVariable, ExecutionContext ctx) {
if (multiVariable.getVariables().size() == 1 && matches(multiVariable.getVariables().get(0).getInitializer())) {
// Remove the variable declaration and inline any references to the variable with the literal value
J.Identifier identifierToReplaceWithLiteral = multiVariable.getVariables().get(0).getName();
doAfterVisit(new JavaVisitor<ExecutionContext>() {
@Override
public J visitIdentifier(J.Identifier ident, ExecutionContext ctx) {
if (SemanticallyEqual.areEqual(ident , identifierToReplaceWithLiteral)) {
return buildLiteral().withPrefix(ident.getPrefix());
}
return ident;
}
});
cleanUpAfterReplacements();
return null;
}
return super.visitVariableDeclarations(multiVariable, ctx);
}

@Override
public J visitMethodInvocation(J.MethodInvocation method, ExecutionContext ctx) {
J.MethodInvocation mi = (J.MethodInvocation) super.visitMethodInvocation(method, ctx);
if (methodMatcher.matches(mi) && isFeatureKey(mi.getArguments().get(0))) {
doAfterVisit(new SimplifyConstantIfBranchExecution().getVisitor());
doAfterVisit(Repeat.repeatUntilStable(new RemoveUnusedLocalVariables(null, true).getVisitor(), 3));
doAfterVisit(new RemoveUnusedPrivateFields().getVisitor());
J.Literal literal = new J.Literal(Tree.randomId(), Space.SINGLE_SPACE, Markers.EMPTY, replacementValue, String.valueOf(replacementValue), null, JavaType.Primitive.Boolean);
return literal.withPrefix(mi.getPrefix());
if (matches(mi)) {
cleanUpAfterReplacements();
return buildLiteral().withPrefix(mi.getPrefix());
}
return mi;
}

private boolean isFeatureKey(Expression firstArgument) {
return CursorUtil.findCursorForTree(getCursor(), firstArgument)
.bind(c -> ConstantFold.findConstantLiteralValue(c, String.class))
.map(featureKey::equals)
.orSome(false);
private boolean matches(@Nullable Expression mi) {
if (methodMatcher.matches(mi)) {
Expression firstArgument = ((J.MethodInvocation) mi).getArguments().get(0);
return CursorUtil.findCursorForTree(getCursor(), firstArgument)
.bind(c -> ConstantFold.findConstantLiteralValue(c, String.class))
.map(featureKey::equals)
.orSome(false);
}
return false;
}

private void cleanUpAfterReplacements() {
doAfterVisit(new SimplifyConstantIfBranchExecution().getVisitor());
doAfterVisit(Repeat.repeatUntilStable(new RemoveUnusedLocalVariables(null, true).getVisitor(), 3));
doAfterVisit(new RemoveUnusedPrivateFields().getVisitor());
}

private J.Literal buildLiteral() {
return new J.Literal(Tree.randomId(), Space.SINGLE_SPACE, Markers.EMPTY, replacementValue, String.valueOf(replacementValue), null, JavaType.Primitive.Boolean);
}

};
return Preconditions.check(new UsesMethod<>(methodMatcher), visitor);
}
Expand Down
55 changes: 44 additions & 11 deletions src/main/java/org/openrewrite/featureflags/RemoveStringFlag.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,13 @@

import lombok.EqualsAndHashCode;
import lombok.Value;
import org.jspecify.annotations.Nullable;
import org.openrewrite.*;
import org.openrewrite.analysis.constantfold.ConstantFold;
import org.openrewrite.analysis.util.CursorUtil;
import org.openrewrite.java.JavaVisitor;
import org.openrewrite.java.MethodMatcher;
import org.openrewrite.java.search.SemanticallyEqual;
import org.openrewrite.java.search.UsesMethod;
import org.openrewrite.java.tree.Expression;
import org.openrewrite.java.tree.J;
Expand Down Expand Up @@ -65,24 +67,55 @@ public String getDescription() {
public TreeVisitor<?, ExecutionContext> getVisitor() {
final MethodMatcher methodMatcher = new MethodMatcher(methodPattern, true);
JavaVisitor<ExecutionContext> visitor = new JavaVisitor<ExecutionContext>() {
@Override
public @Nullable J visitVariableDeclarations(J.VariableDeclarations multiVariable, ExecutionContext ctx) {
if (multiVariable.getVariables().size() == 1 && matches(multiVariable.getVariables().get(0).getInitializer())) {
// Remove the variable declaration and inline any references to the variable with the literal value
J.Identifier identifierToReplaceWithLiteral = multiVariable.getVariables().get(0).getName();
doAfterVisit(new JavaVisitor<ExecutionContext>() {
@Override
public J visitIdentifier(J.Identifier ident, ExecutionContext ctx) {
if (SemanticallyEqual.areEqual(ident , identifierToReplaceWithLiteral)) {
return buildLiteral().withPrefix(ident.getPrefix());
}
return ident;
}
});
cleanUpAfterReplacements();
return null;
}
return super.visitVariableDeclarations(multiVariable, ctx);
}

@Override
public J visitMethodInvocation(J.MethodInvocation method, ExecutionContext ctx) {
J.MethodInvocation mi = (J.MethodInvocation) super.visitMethodInvocation(method, ctx);
if (methodMatcher.matches(mi) && isFeatureKey(mi.getArguments().get(0))) {
doAfterVisit(new SimplifyConstantIfBranchExecution().getVisitor());
doAfterVisit(Repeat.repeatUntilStable(new RemoveUnusedLocalVariables(null, true).getVisitor(), 3));
doAfterVisit(new RemoveUnusedPrivateFields().getVisitor());
J.Literal literal = new J.Literal(Tree.randomId(), Space.SINGLE_SPACE, Markers.EMPTY, replacementValue, '"' + replacementValue + '"', null, JavaType.Primitive.String);
return literal.withPrefix(mi.getPrefix());
if (matches(mi)) {
cleanUpAfterReplacements();
return buildLiteral().withPrefix(mi.getPrefix());
}
return mi;
}

private boolean isFeatureKey(Expression firstArgument) {
return CursorUtil.findCursorForTree(getCursor(), firstArgument)
.bind(c -> ConstantFold.findConstantLiteralValue(c, String.class))
.map(featureKey::equals)
.orSome(false);
private boolean matches(@Nullable Expression expression) {
if (methodMatcher.matches(expression)) {
Expression firstArgument = ((J.MethodInvocation) expression).getArguments().get(0);
return CursorUtil.findCursorForTree(getCursor(), firstArgument)
.bind(c -> ConstantFold.findConstantLiteralValue(c, String.class))
.map(featureKey::equals)
.orSome(false);
}
return false;
}

private void cleanUpAfterReplacements() {
doAfterVisit(new SimplifyConstantIfBranchExecution().getVisitor());
doAfterVisit(Repeat.repeatUntilStable(new RemoveUnusedLocalVariables(null, true).getVisitor(), 3));
doAfterVisit(new RemoveUnusedPrivateFields().getVisitor());
}

private J.Literal buildLiteral() {
return new J.Literal(Tree.randomId(), Space.SINGLE_SPACE, Markers.EMPTY, replacementValue, '"' + replacementValue + '"', null, JavaType.Primitive.String);
}
};
return Preconditions.check(new UsesMethod<>(methodMatcher), visitor);
Expand Down
13 changes: 7 additions & 6 deletions src/test/java/org/openrewrite/featureflags/RemoveBooleanFlagTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ void customMethodPatternForWrapper() {
java(
"""
package com.acme.bank;
public class CustomLaunchDarklyWrapper {
public boolean featureFlagEnabled(String key, boolean fallback) {
return fallback;
Expand All @@ -50,7 +50,8 @@ public boolean featureFlagEnabled(String key, boolean fallback) {
class Foo {
private CustomLaunchDarklyWrapper wrapper = new CustomLaunchDarklyWrapper();
void bar() {
if (wrapper.featureFlagEnabled("flag-key-123abc", false)) {
boolean enabled = wrapper.featureFlagEnabled("flag-key-123abc", false);
if (enabled) {
// Application code to show the feature
System.out.println("Feature is on");
}
Expand Down Expand Up @@ -84,7 +85,7 @@ void customMethodPatternNoConstants() {
package com.osd.util;
import java.util.Map;
import java.util.HashMap;
public class ToggleChecker {
public boolean isToggleEnabled(String toggleName, boolean fallback) {
Map<String,Boolean> toggleMap = new HashMap<>();
Expand Down Expand Up @@ -134,7 +135,7 @@ void removeWhenFeatureFlagIsAConstant() {
java(
"""
package com.acme.bank;
public class CustomLaunchDarklyWrapper {
public boolean featureFlagEnabled(String key, boolean fallback) {
return fallback;
Expand All @@ -149,7 +150,7 @@ public boolean featureFlagEnabled(String key, boolean fallback) {
import com.acme.bank.CustomLaunchDarklyWrapper;
class Foo {
private static final String FEATURE_TOGGLE = "flag-key-123abc";
private CustomLaunchDarklyWrapper wrapper = new CustomLaunchDarklyWrapper();
void bar() {
if (wrapper.featureFlagEnabled(FEATURE_TOGGLE, false)) {
Expand Down Expand Up @@ -183,7 +184,7 @@ void removeUnnecessaryTernary() {
java(
"""
package com.acme.bank;
public class CustomLaunchDarklyWrapper {
public boolean featureFlagEnabled(String key, boolean fallback) {
return fallback;
Expand Down
4 changes: 1 addition & 3 deletions src/test/java/org/openrewrite/featureflags/RemoveStringFlagTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
*/
package org.openrewrite.featureflags;

import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Test;
import org.openrewrite.DocumentExample;
import org.openrewrite.java.JavaParser;
Expand Down Expand Up @@ -62,8 +61,7 @@ void bar() {
"""
class Foo {
void bar() {
String topic = "topic-456";
System.out.println("Publishing to topic: " + topic);
System.out.println("Publishing to topic: " + "topic-456");
}
}
"""
Expand Down
14 changes: 6 additions & 8 deletions src/test/java/org/openrewrite/featureflags/launchdarkly/RemoveBoolVariationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import org.junit.jupiter.api.Test;
import org.openrewrite.DocumentExample;
import org.openrewrite.InMemoryExecutionContext;
import org.openrewrite.Issue;
import org.openrewrite.java.JavaParser;
import org.openrewrite.test.RecipeSpec;
import org.openrewrite.test.RewriteTest;
Expand Down Expand Up @@ -77,10 +78,10 @@ void keyInConstant() {
"""
import com.launchdarkly.sdk.LDContext;
import com.launchdarkly.sdk.server.LDClient;
class Foo {
private static final String FEATURE_FLAG_123ABC = "flag-key-123abc";
private LDClient client = new LDClient("sdk-key-123abc");
void bar() {
LDContext context = null;
Expand Down Expand Up @@ -333,6 +334,7 @@ void bar() {
);
}

@Issue("https://github.com/openrewrite/rewrite-feature-flags/issues/40")
@Test
void localVariablesNotInlined() {
// language=java
Expand All @@ -357,12 +359,8 @@ void bar() {
"""
class Foo {
void bar() {
// Local variables not yet inlined
boolean flagEnabled = true;
if (flagEnabled) {
// Application code to show the feature
System.out.println("Feature is on");
}
// Application code to show the feature
System.out.println("Feature is on");
}
}
"""
Expand Down
3 changes: 1 addition & 2 deletions src/test/java/org/openrewrite/featureflags/launchdarkly/RemoveStringVariationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,7 @@ void bar() {
"""
class Foo {
void bar() {
String topic = "topic-456";
System.out.println("Publishing to topic: " + topic);
System.out.println("Publishing to topic: " + "topic-456");
}
}
"""
Expand Down

0 comments on commit c448e2a

Please sign in to comment.