Go Cleanup Built-in Rules#265
Conversation
dvmarcilio
force-pushed
the
go_develop
branch
3 times, most recently
from
ee5aa14 to
1db699d
Compare
dvmarcilio
force-pushed
the
go_develop
branch
from
1db699d to
df23eba
Compare
| (binary_expression | ||
| left: [(true) (parenthesized_expression (true))] | ||
| operator:"&&" | ||
| right : (_)* @rhs |
There was a problem hiding this comment.
Why do we need * ? There has to be one RHS in the binary expression right?
| ( | ||
| (binary_expression | ||
| left: ( | ||
| [ |
There was a problem hiding this comment.
What if we just use a wildcard for @lhs and @rhs ?
| replace = "@nested.statements" | ||
| replace_node = "nested.block" | ||
|
|
||
| # This rule is not deleting multiple statements after return. |
There was a problem hiding this comment.
IS that some issue with the grammar? Can we look into it ?
There was a problem hiding this comment.
We can now successfully match all statements after return_statement in a similar way as e.g., Java and Javascript.
Accounting for the \nterminator did the trick:
(((statement) "\\n"?)+ @post)
However, replace is only working for the first statement. This is also happening in Java (draft PR #284).
The self-edge makes it work for the current implementation.
There was a problem hiding this comment.
Introduced dummy node return_cleanup and now we have a cycle
dvmarcilio
force-pushed
the
go_develop
branch
2 times, most recently
from
c10f4a8 to
50e4937
Compare
| query = """ | ||
| ( | ||
| (binary_expression | ||
| left : (_)* @lhs |
There was a problem hiding this comment.
| left : (_)* @lhs | |
| left : (_) @lhs |
| ) | ||
| """ | ||
| queries = [ | ||
| """ |
There was a problem hiding this comment.
Add more constraints.
Delete var declaration if all assignments' rhs eq init of var decl;
Delete all assignments if no var decl exists in scope
There was a problem hiding this comment.
It turns out that this delete_parent_assignment rule was doing nothing. I removed it, and everything is working as expected.
Well-spotted that the constraint was not working.
dvmarcilio
force-pushed
the
go_develop
branch
from
50e4937 to
db4c6e6
Compare
| # | ||
| # Note that this rule *won't* rewrite when @lhs is a call. | ||
| [[rules]] | ||
| groups = ["boolean_expression_simplify"] |
There was a problem hiding this comment.
We have to think of a strategy to prevent it from simplifying after <something> || true
There was a problem hiding this comment.
@ketkarameya should we deal with this in this PR? or do we open a new issue after this is accepted?
There was a problem hiding this comment.
i think a follow up would be ok. We can address this alongside the fix for java/kt
| ##### | ||
| # Dummy rule to introduce a cycle for `delete_statement_after_return` | ||
| [[rules]] | ||
| name = "return_cleanup" |
There was a problem hiding this comment.
I think there is some naming convention for dummy rules in the readme
There was a problem hiding this comment.
Renamed to return_statement_cleanup
| holes = ["stale_flag_name"] | ||
|
|
||
| [[rules]] | ||
| name = "replace_identifier_with_str_literal" |
There was a problem hiding this comment.
what is the purpose of this rule (And the above one)?
There was a problem hiding this comment.
Thanks for the review, @ketkarameya. I agree that this rule is just noise. I restructured this test to have two rules:
find_const_str_literal: finds a constant (@const_idwith a value equal tostale_flag_namepiranha_argument.false_flag: replaces withfalsecalls toBoolValue()with anidentifierargument equal to@const_id(hole).
Now the test doesn't remove the constant declaration.
- Having the `\n` terminator reproduces the exact matching as languages such as Java and JavaScript
- `@treated` for flag value - `groups` in the end of rule definition - rule that matches first declared first - `false_flag` -> `update_feature_flag_api`
dvmarcilio
force-pushed
the
go_develop
branch
from
ab1c9fb to
4ce2547
Compare
2 participants
Ongoing work on writing built-in cleanup rules for Go.