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

Add rust support via tree-sitter #140

Closed
wants to merge 4 commits into from
Closed

Conversation

cormacrelf
Copy link
Contributor

Draft as this depends on and includes the previous PR #139

Supports:

  • fn/return
  • async/await
  • if/else
    • caveat 1: else if cannot be highlighted together, due to structure of the rust grammar treating it as if c {} else ... where ... is either a block or another if_expression.
    • caveat 2: only ifs at the top level inside a block is supported. if as a sub-expression e.g. in let x = if y {} else {}; can't be matched lest an else-if branch introduce a new child scope.
  • loop/while/for + break/continue, excluding labels eg break 'label; and also excluding break with a expression break expr;

The if/else could be improved if the rust grammar is changed. I'm not sure if they would, because currently if_expression and if_let_expression are handled separately and adding another else_if and else_if_let would be a bit annoying to write, but it would make for easier to use output trees.

Previously, placing the cursor on line 1 would highlight every elif and
else in the whole block of code. % would jump to line 4, then % from
there would cycle between lines 2, 4 and 6.

 1   if noice:
 2       if yeah:
 3           pass
 4       elif no:
 5           pass
 6       else:
 7           pass
 8   elif blah:
 9       pass
10   else:
11       pass

With the change, which I think the code had contemplated given there was
already an unused `M.containing_scope(node, bufnr, info.key)` call, % will
only move between ifs and elses that are in the same `@scope.if_`, and
not to any inner scopes. Hence the example will have two mutually
exclusive %-cycles: [1, 8, 10] and [2, 4, 6].
now the first | matches any contained return statements,
and those returns do not match the surrounding `fn`.
@andymass
Copy link
Owner

Thanks for this!

Re else if, I think the current code assumes that the matched/highlighted portion is a single unnamed node like "def". This was done purely for convenience for python but if Rust is structured differently we definitely want to change that to allow higher order nodes to be highlighted entirely.

I don't know Rust, is it possible you could provide examples and desired matching, showing the caveats you mentioned so they can become test cases for my development?

@cormacrelf
Copy link
Contributor Author

cormacrelf commented Mar 17, 2021

I would lean towards saying that's how rust's grammar is but it probably shouldn't be that way. Everyone thinks of else if as a single token, so much so that many languages combine them into elif/elsif. It is also not a particularly accurate description of the rust grammar that else if has an actual "if expression or block" because if that were really true you would be allowed to put any old expression in that slot and you are absolutely not. So any time anyone tries to build tree restructuring code for it they are going to come across this issue -- having to special case if_expressions within an else because they can't be replaced by any old expression and eg do not really make sense as a text object for "change in expression" etc. As noted in the query file there you can't even do that special casing in neovim today without more predicates being implemented, which is the second caveat and you can't solve it from matchup alone. Fixing the grammar would hit both. I'll have a look at that first.

Re tests yes can do.

@cormacrelf
Copy link
Contributor Author

Also higher order nodes in matchup should in general be pretty useless, because TS queries can already do ad hoc grouping like so: ("else" "if") @name. I think this only works if the two tokens are adjacent in the tree, and this is why grammars should be written such that any two tokens you might want to group together are actually adjacent. (Maybe also works if they have tokens in between, but point is they can't be in a child node like ("else" (if_expression "if")).)

@andymass
Copy link
Owner

andymass commented Apr 3, 2021

Apparently the predicate syntax is #not-is?. But I tried this for a little while and couldn't get the query to work in the nvim ts playground.

The other question I had now is if the cursor is here: else [x] if should the whole else if be highlighted?

As you point out there's no way to write a query that targets the "else if" specifically. But we want that combination to act like a single token for the purpose of matching somehow. This will require a bit of re-engineering as currently the "seed" token must be a single unnamed node. In case of else if, it cannot be a node or a single capture, but must instead be formed from multiple captures next to each other. So far I have tried to avoid adding language meta-data (in lua or vim support files) in addition to the query files, but it might become necessary.

Note my goal is to find a general way to support all languages. Rust is a good second target. Even if the PR you raised on the grammar is merged, it's good to resolve these concerns. I agree with you that in this case, grouping could work under a new grammar, I'm afraid that other languages will have similar cases.

@cormacrelf
Copy link
Contributor Author

No it's definitely is-not? ts repo. The neovim bindings have not implemented it yet so it won't work.

If you want a general way to resolve this, the best idea I have is to:

  • Have one match group for the actual targetable token
  • Have one match group name for anything else you want to be highlighted at the same time

So the else would be targetable with % via @mid._if.1 but the "if" in "else if" could be turned blue as well with @highlight._if.

@andymass
Copy link
Owner

I found a solution to highlighting else if using nvim-treesitter's #make-range! directive. It allows us to create pseudo-node ranges which span other nodes. The main downside is somewhat worse performance since we have to do the initial match against actual queries, but I think it should not be a major problem. I haven't written the full query, but the following works (with the latest set of commits);

(block (if_expression "if" @open.if_) @scope.if_)
(else_clause "else" @mid.if_.1 (block))
(else_clause
  "else" @_start (if_expression "if" @_end)
  (#make-range! "mid.if_.2" @_start @_end))

@cormacrelf
Copy link
Contributor Author

cormacrelf commented Apr 22, 2021

Just for linking: c39903d and https://github.com/nvim-treesitter/nvim-treesitter/blob/4df26673037a67686a4da91615cd129b39b60dd9/lua/nvim-treesitter/query.lua#L168

That's pretty cool! Nice solution. I'll try it out and patch this PR.

@dsully
Copy link

dsully commented Sep 12, 2021

Any update on getting this merged? Thanks!

andymass added a commit that referenced this pull request Sep 12, 2021
andymass added a commit that referenced this pull request Oct 9, 2021
@andymass
Copy link
Owner

andymass commented Oct 9, 2021

I've incorporated the rest of the queries from this PR. If there are any problems we can handle them in separate issues.
Thank you for your contribution @cormacrelf!

@andymass andymass closed this Oct 9, 2021
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