uber / piranha

A tool for refactoring code related to feature flag APIs
Apache License 2.0
2.3k stars 198 forks source link

Kotlin cleanup rules to support `!true &&` and `!false ||` expressions #694

Open daveight opened 2 months ago

daveight commented 2 months ago

I noticed an issue when processing boolean expressions: if (!feature.isEnabled(FEATURE_A) && condition != "constant")

Piranha refactors it like this: if (!condition != "constant") which is syntactically and logically incorrect

I implemented two additional rules to fix it:

1) simplify not true in conjunction expression

!true && ... => the whole boolean expression will be refactored to false

2) simplify not false in disjunction expression

!false || ... => the whole boolean expression will be refactored to true

Looking forward to a feedback / review. Thanks!

CLAassistant commented 2 months ago

CLA assistant check
All committers have signed the CLA.

danieltrt commented 1 month ago

Hi @daveight

The problem here is not polyglot-piranha, but rather the tree sitter grammar for Kotlin. That entire expression is getting parsed this way (see below). I think we should try to fix the grammar (if this is not expected behavior? I'm not well-versed in Kotlin), rather than add this workaround rule

Screenshot 2024-10-10 at 5 32 32 PM
daveight commented 1 month ago

Hello, I totally agree with you.

However for my project introducing this workaround rule would be a fastest way to solve the problem, however the problem is that I cannot configure this rule externally, because in order this to work correctly - the rule should be executed in a specific order.

Is there a way to configure this order in Piranha for an external rule? Thanks

danieltrt commented 1 month ago

I think you can just inject these custom rules at run-time instead.

from polyglot_piranha import execute_piranha, PiranhaArguments, Rule, RuleGraph, OutgoingEdges

# Toy code snippet
code = """
class Sample {
   fun test_fn() {
    if (!feature.isEnabled(FEATURE_A) && condition != "constant") {
        // if branch
    } else {
        // else branch
    }
   }

}
"""

r1 = Rule(
    name="replace_method",
    query="""cs !:[x].isEnabled(@stale_flag_name) && :[rest+]  
    """, # if :[x].isEnabled(@stale_flag_name) is now permanently true
    replace_node="*",
    replace="false",
    holes={"stale_flag_name"},
    is_seed_rule=True
)

edge = OutgoingEdges("replace_method", to=["boolean_literal_cleanup"], scope="Parent")

# Create Piranha arguments
piranha_arguments = PiranhaArguments(
    code_snippet=code,
    language="kt",
    rule_graph=RuleGraph(rules=[r1], edges=[edge],),
    substitutions={"stale_flag_name": "FEATURE_A"}

)

piranha_summary = execute_piranha(piranha_arguments)
print(piranha_summary[0].content)

But to answer your question, polyglotpiranha works with a stack of global rules. The ones that show up first in the list are executed first.

danieltrt commented 1 month ago

@daveight Did it work for you?

daveight commented 4 weeks ago

Hello @danieltrt thanks for the reply. Is there a reference to learn this syntax? I am using tree-sitter queries, but this syntax I am not aware of. Thanks.