uber / piranha

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

Issue with Rule #699

Open rajeshchinnam opened 2 months ago

rajeshchinnam commented 2 months ago

Hi @danieltrt

I have below rule , which is working fine on tree sitter play ground [[rules]] name = "replace_boolean_parse_with_literal" query = """ ( (if_statement condition: ( parenthesized_expression (method_invocation object: (identifier) @object name: (identifier) @name arguments: (argument_list (true) ) ) @method_invocation ) ) @if_statement ) (#eq? @object "Boolean") (#eq? @name "parseBoolean") """ replace_node = "parenthesized_expression" replace = "true" groups = ["replace_expression_with_boolean_literal"]

but when I am running on piranha

I am getting below error

pyo3_runtime.PanicException: Could not parse the query : "(\n\t(if_statement\n\t condition: (\n\t\tparenthesized_expression\n\t\t(method_invocation\n\t\t object: (identifier) @object\n\t\t name: (identifier) @name\n\t\t arguments: (argument_list\n\t\t\t(true)\n\t\t )\n\t\t) @method_invocation\n\t )\n\t) @if_statement\n)\n(#eq? @object \"Boolean\")\n(#eq? @name \"parseBoolean\")\n" Some(QueryError { row: 2, column: 3, offset: 20, message: "\t condition: (\n ^", kind: Structure })

any help on this is well appreciated Thanks in advance

ketkarameya commented 2 months ago

@rajeshchinnam I will get back to u in a day or two. I am busy with something urgent at work for next two days.

rajeshchinnam commented 2 months ago

sure @ketkarameya I will be waiting for your response

one more scenario I have my method

public String get(String requestNumberPrefix, String request) { if (tenantConfiguration.get(TenantConfigurationKey.FEATURE_SEQUENTIAL_NUMBERS_FOR_WARRANTY).equals("true")) { log.info("request number prefix is {}", requestNumberPrefix); Long nextSequenceId = getNextSequenceIdAndSave(requestNumberPrefix, request); String sequenceNumber = String.format(REQUEST_NUMBER_FORMAT, nextSequenceId); return requestNumberPrefix + sequenceNumber; } return generateRandomNumber(); }

with Rule (( (method_invocation (methodinvocation name: () @name arguments: (argument_list [ (fieldaccess field: () @argument) (_) @argument ] ) ) name: (identifier) @identifiereq arguments: (argument_list [ (string_literal) @trueValue ] ) ) ) (#eq? @name "get") (#eq? @argument "TenantConfigurationKey.FEATURE_SEQUENTIAL_NUMBERS_FOR_WARRANTY") (#eq? @identifiereq "equals") (#eq? @trueValue "\"true\"") ) This rule highlighting get(TenantConfigurationKey.FEATURE_SEQUENTIAL_NUMBERS_FOR_WARRANTY).equals("true") in piranha playground but when I run piranha its not finding the match

rajeshchinnam commented 1 month ago

@ketkarameya can you please let me know what could be the issue here when you get some time.

ketkarameya commented 1 month ago

@rajeshchinnam can you please make a demo repository with your code and whatever python script you have written? It will be easier to collaborate.

For instance, in your above rule I would do something like :

[[rules]]
name = "replace_boolean_parse_with_literal"
query = """
(
(method_invocation
object: (identifier) @object
name: (identifier) @name
arguments: (argument_list
(true)
)
) @method_invocation
)
(#eq? @object "Boolean")
(#eq? @name "parseBoolean")
"""
replace_node = "method_invocation"
replace = "true"
groups = ["replace_expression_with_boolean_literal"]

^ Just try to match your ff-api in the python script (using piranha's python api) and follow this example https://github.com/uber/piranha/blob/master/POLYGLOT_README.md#stale-feature-flag-cleanup

rajeshchinnam commented 1 month ago

Thanks @ketkarameya

Please refer

https://github.com/rajeshchinnam/piranha/blob/main/feature_flag_cleanup.py https://github.com/rajeshchinnam/piranha/blob/main/configurations/rules.toml

I can able to fix the rule and I am trying to fix another use case

https://github.com/rajeshchinnam/piranha/tree/main/configurations

eg:

I want to remove the test method for the stale feature flags

@Test void shouldGetRandomValueIfFeatureIsDisabled() { when(TenantConfiguration.get(TenantConfigurationKey.FEATURE_SEQUENTIAL_NUMBERS_FOR_WARRANTY)).thenReturn("false"); // TODO }

can you please suggest the rule

ketkarameya commented 1 month ago

you actually do not need to match the context. So if u want to match TenantConfiguration.get(TenantConfigurationKey.FEATURE_SEQUENTIAL_NUMBERS_FOR_WARRANTY) just match this snippet replace it with true/false. Then you can add an edge that cleans up tautological expressions like when(true).thenReturn(true)

rajeshchinnam commented 1 month ago

@ketkarameya

Please review my updated rule https://github.com/rajeshchinnam/piranha/blob/main/configurations/rules.toml

remove_method_with_specific_line : is updating when(TenantConfiguration.get(TenantConfigurationKey.FEATURE_SEQUENTIAL_NUMBERS_FOR_WARRANTY)).thenReturn("false");

as

when("true").thenReturn("false");

https://github.com/rajeshchinnam/piranha/blob/main/src/main/java/com/piranha/DocumentInfoServiceTest.java

and my rule is

https://github.com/rajeshchinnam/piranha/blob/main/configurations/rules.toml

when i am testing this on play sitter its matching but running on piranha its not matching

Please suggest

rajeshchinnam commented 1 month ago

I fixed it @ketkarameya