-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Use split_parser branch for markdown grammar #3108
Use split_parser branch for markdown grammar #3108
Conversation
fe19775
to
7a3906c
Compare
Changing this to ready since I found a solution to the problems in #3129. |
cb4e6db
to
b7ee7af
Compare
I tried this branch locally but I don't see any qualitative improvements to performance (we'll need some measurements like from #940 to be definitive). Specifically I see the some slowness with incremental updates while editing (for example typing quickly in insert mode) on a reasonably large and complicated markdown document like the changelog. Is this branch of the grammar(s) more correct than master? This doesn't feel like a regression so I'm certainly not against merging it but I'm curious what the improvement is of splitting the parsers? |
I'm no longer maintaining the master branch, I just kept it because some projects seem to depend on it. It was not really possible to implement a correct enough markdown parser in a single grammar. The specs also recommend to parse markdown in 2 passes. Now after splitting it into 2 grammars they work closer to that recommendation. They're more correct now and with some optimizations to incremental parsing I think they'll also be faster. (But that'll be topic of future PRs). |
Specifically I think that tree-sitter reparses all injected ranges if they contain "conflicts" no matter if they changed or not. That means that parsing is linear with the number of injected ranges. I think we could just not reparse ranges if they did not change at all (parsing should be deterministic with regards to the input anyways). |
I'm not very familiar with the changes but I think archseer might've covered that when implementing combined injections: 6728e44...7c9ebd0 We don't use the stock tree-sitter-cli highlighting code since it isn't incremental |
Just looking at the code that doesn't seem to be the case, but I should do some more investigation. |
It's related to this TODO: helix/helix-core/src/syntax.rs Line 724 in 681c0a9
Just going one step further and also avoid parsing layers that are after the edits |
This works well for me locally but let's await further review/merging in #3129 |
Can you resolve the conflict? Looks good to merge otherwise 👍🏻 |
0802082
to
840e2d7
Compare
840e2d7
to
c750fa3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works great! Thanks for working on this plus going the extra mile to add the new predicate :)
See #3072.
Only a draft because
markdown_inline
grammar is not injected properly, but to me it seems like this is a problem with helix and not the language config? After a few experiments I think the grammar is injected, but highlighting is still not done according to the injected language. I'm not very sure though.Another weird behavior is with fenced code blocks. They seem to work to some degree, but if you enter something like
then
let
is highlighted correctly while"bar"
is not highlighted at all.