Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

daveight
Copy link

@daveight daveight commented Sep 9, 2024

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

  1. simplify not false in disjunction expression

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

Looking forward to a feedback / review. Thanks!

- simplify not true in conjunction expression
- simplify not true in disjunction expression
@CLAassistant
Copy link

CLAassistant commented Sep 9, 2024

CLA assistant check
All committers have signed the CLA.

@danieltrt
Copy link
Collaborator

danieltrt commented Oct 10, 2024

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
Copy link
Author

daveight commented Oct 10, 2024

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
Copy link
Collaborator

danieltrt commented Oct 10, 2024

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
Copy link
Collaborator

@daveight Did it work for you?

@daveight
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants