Remove unused local variables despite potential side effects after removing feature flag

[kavitha186]

@timtebeek I would like to first fix this one 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.

I'm thinking to approach this as below

1. To update the below recipe to handle the Local variable removal when there is a builder assigned and if the variable is not use in the method? https://github.com/openrewrite/rewrite-static-analysis/blob/cba8ef0a14f87056d42e1d7f4d26b0617af4e614/src/main/java/org/openrewrite/staticanalysis/RemoveUnusedLocalVariables.java#L37

Please confirm if it would work or should I create a new recipe to handle below special cases while removing local variable.

• with builder assigned
• if it just used in logging context.toString()
• if it is just used in logging as log.info("log the context", context.toString())

[kavitha186]

@timtebeek I had 2 test cases and one of them where LDContext had assigned builder methods - Expectation is the builder methods also has to be cleaned up.

An another test cases where I tried to use Create method on LDContext and builderWithContext, I'm expecting it to clear everything , but both the actual and expected result is so unexpected . Can you please what is expected in the testcase "removeUnusedLDContextWithBuilderContext". thanks .

[timtebeek]

hi @kavitha186 ! Great to see these test cases added; I think indeed you're right in expecting these LDContext objects to be cleared out where possible.

Rather than a separate recipe it might make most sense to add a new nullable argument to RemoveUnusedLocalVariables for something like: Boolean withSideEffects, and then never set mightSideEffect to true for method invocations here. We could tackle that as a separate contribution, such that this PR ideally only needs to add that new argument to constructor in

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

Line 73 in 2c9a406

doAfterVisit(new RemoveUnusedLocalVariables(null).getVisitor());

Would you be willing to push up that matching change as well? Much appreciated that you're chiming in here for the improvements. :)

[timtebeek]

I've pushed up a small PR based on the above that should unblock this use case here; thanks again for the suggestion!

• Allow removal of unused local variables with side effects in initializer rewrite-static-analysis#378

[timtebeek]

Great addition @kavitha186 ; I think we can merge this increment already, even if there's potentially still those unused fields to clear out, as this is already a nice improvement.

[kavitha186]

Thanks @timtebeek for merging this Pr. I will pass boolean flag from this RemoveBoolVariation as true and will implement the get visitor on Removeunusedlocalvariable to check for any builder or any other methods used from LDContext.

[timtebeek]

Hi @kavitha186 that should mostly be covered already with the changes I added here. But let me know if there's anything left to do still!

[kavitha186]

yes looks like all covered . Will take up next item from the issues..thanks