tree-sitter / tree-sitter-javascript

Javascript grammar for tree-sitter
MIT License
314 stars 108 forks source link

Introduce condition alias for capturing the conditions #280

Closed ketkarameya closed 3 months ago

ketkarameya commented 4 months ago

This PR introduces condition for capturing condition field in while_statement and if_statement. Currently the condition field is captured by parenthesized_expression. condition is actually exactly like parenthesized_expression.

Why this change? This causes a incorrect match in Piranha. If we write a rewrite rule to replace (parenthesized_expression (true)) with true it will transform if (true) { ... } to if true { ... }. While Piranha could implement some awkward logic to handle this scenario, I thought updating the grammar itself would be cleaner.

Since the AST node captured by the condition field ( (...) ) is not really a expression, so I named it just condition and not suffix it with _expression.

There is a simialr PR I had merged for tree-sitter-java few months ago.

Checklist:

https://github.com/tree-sitter/tree-sitter-java/pull/145

amaanq commented 3 months ago

I'm not fully convinced doing it like this is right, how about hiding the condition with an underscore using a field for the condition instead so it fits in nicer? The ast playground seems to call the actual condition test

_condition: $ => seq('(', field('test', $.expression), ')'),
amaanq commented 3 months ago

I agree, I think that the condition field should suffice honestly

ketkarameya commented 3 months ago

Hey! Sorry I am late to this thread :| Actually, condition as a field is not enough because the query (parenthesized_expression) @p over-matches. In the below code snippet I would want it to match (true) only and not (someCondition()). I

if (someCondition()){
  boolean x = (true);
}

In theory, (someCondition())` is actually not a parenthesized expression? Like i do understand the reservations behind adding another node kind to the grammar. In our Piranha system it leads to incorrect code simplification :|

if we decide to go ahead with this change, based on my above argument; I think we will have to update the typescript grammar too

amaanq commented 3 months ago

No, and frankly I wish it hadn't been changed in Java either, but that predates my involvement in upstream and Max is too busy oftentimes to manage every grammar repo. I'll probably revert it sometime soon FYI. If you are looking for something that doesn't match a specific node, you can use predicates with some custom postprocessing on top of the actual query matches to get the results you want. e.g., you can check and filter these out with something like (#not-has-parent? @paren_exp if_statement) that you can implement for your use case.

amaanq commented 3 months ago

Actually, you can benefit from using negated fields. https://tree-sitter.github.io/tree-sitter/using-parsers

ketkarameya commented 3 months ago

negation won't work in this case i guess. Yes, we have mechanisms like (#not-has-parent? @paren_exp if_statement) in place, will fall back to that. Thanks :)