remake else if as repeat($.else_clause)

[cormacrelf]

trafficstars

Motivation: https://github.com/andymass/vim-matchup/pull/140 -- see the query file https://github.com/andymass/vim-matchup/pull/140/files#diff-d74a0aa0bf2225f5b3d440f88938de06c9c0373c035643ba22aed5a34d5ffcc5 for the specific queries.

Before, it looked like this:

if ... {
} else if let ... = ... {
} else if ... {
} else {
}
(if_expression condition: ... consequence: (block)
  alternative: (else_clause (if_expression condition: (let_condition pattern:  ... value: ...)
                                           consequence: (block)
      alternative: (else_clause (if_expression condition: ... consequence: (block)
          alternative: (else_clause (block)))))))

After, it looks like this:

if ... {
} else if let ... = ... {
} else if ... {
} else {
}
(if_expression condition: ... consequence: (block)
  (else_clause condition: (let_condition pattern: ... value: ...)
               consequence: (block))
  (else_clause condition: ... consequence: (block))
  (else_clause (block)))

Previously, the "else" and "if" were not adjacent tokens, and therefore could not be grouped together in an ("else" "if") @match query. The main motivation here is highlighting each if/else if/else branch with vim-matchup.

It was also difficult to query only a complete if/else expression, and exclude if_expressions that were simply the "if" in an "else if". It is maybe wrong to say that the latter is actually an expression, but moreover if you wanted to query only the former, you would have to either list all the contexts where an if_expression can occur (except else_clause) or use an #if-not? to exclude the (else_clause (if_expression) @not_this). Again, the motivation is attempting to navigate between if/else branches in the editor using vim matchup, which requires matching one single (if_expression) @scope.if to link all the branches to, and not creating a bunch of smaller scopes on all the if_expressions contained within.

The resulting tree is flatter. There is no need for the alternative: field name as (else_clause) only appears in the tail of an (if_expression) and never at the top level in the condition/consequence, hence writing (else_clause) in a query unambiguously selects nodes at the tail. And the previous two problems are solved:

• (else_clause "else" "if") can match now, and it will not match a final else {} clause. Previously it was an impossible pattern.
• (if_expression) will only match once in a chain of if {} else if {} ...s, with the match over the entire expression. No need for hacky tricks to avoid matches on inner if_expressions.

[aryx]

@maxbrunsfeld what do you think? Should we accept the PR because it helps some indentation tools outside even though it makes the grammar slighly more complicated? What do we do for other languages with a similar elseif structures?

[maxbrunsfeld]

I'm ok with this change.

You're right @aryx that we shouldn't make grammars significantly more complicated in order to support the limitations of specific text editors/tools. Sometimes, a tool just may need to generalize the shapes of syntax trees that it can deal with.

But in this case, the added complexity is pretty limited, and we're hearing feedback from multiple consumers of this parser that the proposed tree would be easier to work with. I want to be receptive to that kind of feedback.

Thanks everyone for weighing in; it makes it easier to make judgment calls when multiple folks share their experience using the parser.

[cormacrelf]

Thanks Max and aryx, and thanks everyone else for the discussion. I'll have a quick go at the improvement I alluded to and see if it works, otherwise I'll shape it up for a merge.

[aryx]

@cormacrelf can you rebase with the latest version to resolve the conflicts and then it should be good to go since max approved it.

[cormacrelf]

I found this again, sorry I didn't manage to batch this with the recent breaking changes to if expressions to support let-else statements. Notably, the question of whether we wanted else_clause in there is resolved, because those recent changes introduced an else_clause.

There are a couple of outstanding questions, I think:

1. Do we want ... else {} to parse as (else_clause (block)) or (else_clause consequence: (block))? I think perhaps the latter, this would mean the condition is conceptually just an optional field, and you can uniformly query for the consequence block using (else_clause consequence: (block ...) @the-block).
2. else_clause's fields are only loosely linked to the corresponding fields on if_expression. Should we somehow restore the ability to query the condition or the consequence with only one query rule, rather than splitting it out into e.g. (if_expression condition: (...) @cond) + (else_clause condition: (...) @cond)? I think the answer is that we don't really care / not worth it / you can still query everything you need, just slightly more verbose
3. Do we want to ensure the final else_clause is the only one without a condition? Should tree-sitter error if you write if c1 {} else {} else if c2 {}? I personally don't think it matters at all. The nested approach does not accept it but there is no real harm if we do that I can see.

[amaanq]

Can you rebase @cormacrelf

And for your questions:

1. the entire else_clause should be a field named 'alternative'
2. maybe an alternative would be to have a repeat($.else_if_clause) followed by an optional else_clause to allow easier querying w/ the if_expression instead of adding an _else_if node
3. solved by my suggestion in 2