tree-sitter / tree-sitter-rust

Rust grammar for tree-sitter
MIT License
338 stars 96 forks source link

Support Doc Comments and General Cleanup of Scanner #195

Closed ProfDoof closed 6 months ago

ProfDoof commented 10 months ago

Hi all!

I was looking at some previous PRs for doc comments like #168 and thought I would take a crack at it addressing some of the issues raised by @the-mikedavis in the other PR to fix #147. I scope creeped myself while I was doing it though and also separated each of the processing steps in the scanner.c file out into their own functions to make it easier to read the file. I was struggling quite a bit to read it myself. I also went through and did some cleanup of functions based on what I read in the documentation for tree-sitter.

If I need to split this into 2 separate PRs, let me know, and I will address that as able.

Quick question though, is there a good way for me to benchmark my changes against the original solution? I want to make sure that the extra processing cost we would be incurring is worth it.

Changes Made

Grammar

Scanner

Test

ProfDoof commented 10 months ago

I have fixed the lint problems locally but now I'm hunting down the parser fuzzing error.

ProfDoof commented 10 months ago

The fuzzing error seen in the failure on here was caused by the shift in the install process for tree-sitter-cli from installing the api.h and parser.h files to only installing the api.h file. This means that you need to use the relative path to import the parser.h created by tree-sitter generate instead. That needs to be fixed in the vim repo as well it looks like.

However, there was also a recent update to the tree-sitter-fuzz-action which will likely cause an error as well. I will go make an issue on there and link it here.

ProfDoof commented 10 months ago

I looked deeper and make install is the one that no longer installs parser.h but cargo and npm still do. So, it's not an issue right now but may become one later.

ProfDoof commented 10 months ago

Hey, just wanted to check what the expected timeline on looking at this is.

maplant commented 10 months ago

Would love to have this added. Not parsing doc comments is very strange

ProfDoof commented 8 months ago

Hi! I wanted to check back in on this as it's been sitting in my backlog for a couple of month now. @amaanq is this something that you would need to look at?

TrueDoctor commented 7 months ago

For me this seems to only highlight the first two characters of the doc comment, https://github.com/tree-sitter/tree-sitter-rust/pull/168 does not have the same problem

ProfDoof commented 7 months ago

For me this seems to only highlight the first two characters of the doc comment, #168 does not have the same problem

Would you mind posting the code you were using to test it? I am willing to fix it, I just need to know what it is. @TrueDoctor

TrueDoctor commented 7 months ago

Would you mind posting the code you were using to test it? I am willing to fix it, I just need to know what it is. @TrueDoctor

I hooked it up to the helix editor by changing the url of the rust grammer to point to your pr and then adding (line_doc_comment) @comment.block.documentation to the file runtime/queries/rust/highlights.scm and then starting the editor (sometimes you have to manually ask helix to recompile the grammer by doing hx --grammar fetch and hx --grammar build)

ProfDoof commented 7 months ago

@TrueDoctor

Oh, I see the issue I think. line_doc_comment isn't a valid selection for the grammar I crafted. Instead, you want to add

(line_comment doc: (_)) @comment.block.documentation
(line_comment) @comment.line
(block_comment doc: (_)) @comment.block.documentation
(block_comment) @comment.block

or

(line_comment (doc_comment)) @comment.block.documentation
(line_comment) @comment.line
(block_comment (doc_comment)) @comment.block.documentation
(block_comment) @comment.block

Or something like that. I'm not an expert on crafting queries yet. Once I have time again (don't do a PhD, you'll never have your own life) I can try to drum up some better versions.

Oh, if you want to differentiate between inner and outer doc comments you can as well. You would do that like this I think:

(line_comment (doc_comment outer: (_))) @comment.block.documentation
(line_comment (doc_comment inner: (_))) @comment.block.documentation
(line_comment) @comment.line
(block_comment (doc_comment outer: (_))) @comment.block.documentation
(block_comment (doc_comment inner: (_))) @comment.block.documentation
(block_comment) @comment.block

or

(line_comment (doc_comment (outer_doc_comment))) @comment.block.documentation.outer
(line_comment (doc_comment (inner_doc_comment))) @comment.block.documentation.inner
(line_comment) @comment.line
(block_comment (doc_comment (outer_doc_comment))) @comment.block.documentation.outer
(block_comment (doc_comment (inner_doc_comment))) @comment.block.documentation.inner
(block_comment) @comment.block
TrueDoctor commented 7 months ago

Oh, sorry I think made a small error, I think I actually used the (doc_comment) @comment.block.documentation and just copy pasted the line from my current config without checking. Otherwise the editor would complain about missing nodes iirc

ProfDoof commented 7 months ago

@TrueDoctor

The doc_comment does not contain the contents of the node IIRC. Instead, doc_comment contains just the characters needed to determine if I'm dealing with an inner or outer comment (theoretically, it's been a while since I wrote this code). The rest of the contents are contained within the line_comment or block_comment. There might be a different way to consider it though. I'm welcome to suggestions.

amaanq commented 6 months ago

I'm sorry for taking so long to get to this, I had a bit of a hiatus November-January and then was just busy afterwards and forgot about this

This looks really good FYI, so thanks a ton @ProfDoof! There's a few things I changed around but otherwise I think we can get this in, most notably:

Might've forgotten something, but that's the meat of it. Thanks!

clason commented 6 months ago

Would be helpful to add queries for this (makes it easier for downstream to include).

amaanq commented 6 months ago

not much really needed tbh, there's still queries for comments but this enables querying for doc comments (which is not in the list of upstream's standard captures nvm it is) to be done much more easily

There's @comment.documentation highlight queries now

TrueDoctor commented 6 months ago

:partying_face: