Replace tree-sitter bindings with tree-house
#12972
Conversation
the-mikedavis
added
A-tree-sitter
the-mikedavis
force-pushed
the
tree-house
branch
from
e5a1f9a to
69b68df
Compare
|
This is fantastic!! It's great to have parameters retain their color even when the function definition goes out of the viewport. I just rebased #12768 on top of your PR and it works flawlessly. We don't have to inject into every argument anymore, positional injections work!!
|
|
While writing the code for query precedence in the injection query I was assuming I was fixing niche bugs (and sometimes questioned neither it was worth writing that quite subtle code). Seeing an actual usecase for it so quickly is great! |
|
Thanks a lot for doing this! I’m going to have a look at whether I can use it in |
|
I've been testing this along with #12768 and found a reproducible crash This panic happens if:
To reproduce, add this at the end of ((macro_invocation
macro:
[
(scoped_identifier
name: (_) @_macro_name)
(identifier) @_macro_name
]
(token_tree . (_) . (_) . (string_literal) @injection.content))
(#any-of? @_macro_name
; std
"some_macro")
(#set! injection.language "rust")
(#set! injection.include-children))Open a Rust project with this structure with cursor at /// ```
/// some_macro!((), (), "`rust` injection happens here");
/// ```
macro_rules! some_macro {
($a:expr, $b:expr, $c:literal) => {};
}
fn main() {
some_ma|
}When Rust analyzer auto-completes the If you remove the But in that case, my PR is no longer able to inject the |
the-mikedavis
force-pushed
the
tree-house
branch
from
69b68df to
ece473b
Compare
|
Looks like it was a bug in some of the precedence handling for injections and it could cause injection layers to get out of order. Fixed in helix-editor/tree-house@9e2063c |
the-mikedavis
force-pushed
the
tree-house
branch
from
ece473b to
691c9bd
Compare
|
Found a segfault Open this Rust file, with cursor at the end of the comment, after the //! L
//! - `a`
struct A {}
impl A {
//
pub fn a() {}
}When you try to insert a //! L
//! - `a`
struct A {}
impl A {
///
pub fn a() {}
}Instead we have a segfault: Note: Removing the Tested with the latest version of this PR (does not happen on master) What is bizarre is that if you copy paste the final version of the file then there is no segfault, and you can write doc comments just fine I tried to run this with with
|
|
Also, the
It is fairly common not to have an At the moment, what I'm thinking is that there can be a Then we can use this Edit: I was able to implement this in #13116
|
| @@ -1,6 +1,10 @@ | |||
| ([(line_comment) (block_comment)] @injection.content | |||
| ([(line_comment !doc) (block_comment !doc)] @injection.content | |||
There was a problem hiding this comment.
| ([(line_comment !doc) (block_comment !doc)] @injection.content | |
| ([(line_comment) (block_comment)] @injection.content |
With this PR the comment language doesn't get injected
I removed those !doc and it gets injected now.
And the documentation still gets highlighted
There was a problem hiding this comment.
I see some non-determinism in whether or not line comments (specifically those that are at the start of a line or preceded by whitespace rather than nodes in the root layer's syntax tree) are captured by the injections query and emitted QueryCursor. That issue needs further debugging but this is not a fix for it: I see the same behavior with this change.
The !doc makes this pattern distinct from the (doc_comment) one. Either a comment node has a doc: (doc_comment) field or it doesn't so the !doc means that either the first pattern or the second will capture a comment - it can't be captured by both. Removing the !doc is theoretically ok since the second pattern has higher precedence, but it shouldn't be necessary to remove the !doc.
I think the bug is related to initially loading the rust parser and queries from a different thread than the main thread (which performs highlighting). I can reproduce this only when opening application.rs as the first file, from the picker and letting the preview highlighting kick in, and not consistently. When this behavior happens, oddly, the comment injection does work for any line comments which are preceded by non-whitespace:
(note that NOTE: is not highlighted in the first line but it is after "grammo")
|
W.r.t. the segfault: I pushed a workaround in helix-editor/tree-house@8b94086. What's happening is that adding a new doc comment to the root layer adds a new range to the markdown combined injection, so it starts out covering ranges This doesn't happen on master because incremental injections are smarter in tree-house. On master we don't pass the old parse tree because we don't recognize that the new injection layer is the same as the old one. Backtrace...
|
the-mikedavis
force-pushed
the
tree-house
branch
from
d04f940 to
2e79bb5
Compare
|
Not sure if I am doing something wrong, and the upgrade process is now different than before, but I get the highlighting in the picker, but not the actual buffer. Picker: Buffer: |
|
I think I saw this as well it seems like there is something up with the timeout, at least that's what the logs where saying for me I believe |
What's stopping us from moving to a newer tree-sitter version now? I'm dependent on a newer ABI (15) but unable to use since we're stuck with 0.22 treesitter version. However seeing this newer tree-house library only deepens the problem as bunch of PRs might still use the old APIs of helix. It's already painful to keep rebasing the non-merged PRs and seeing such 2k+ changes makes me question my ability on rebasing further... |
|
Newer versions of the |
|
Will directives |
|
So my issue seems a little more interesting(?) than a timeout. When I have |
|
Actually, just changing |
I experienced this several times. Specifically I remember when I had a string inside of I even had a panic happen a few times when this happened I could not reproduce this consistently, unfortunately. But I always remember this happening when deleting the ends of |
|
Oh, sorry, I should have worded that better, I am changing this setting in The timeout was also in my logs, forgot to mention: |
|
The log is from here but I dont see any ERROR log it mentions for pub fn set_language(
&mut self,
language_config: Option<Arc<syntax::config::LanguageConfiguration>>,
loader: &syntax::Loader,
) {
self.language = language_config;
self.syntax = self.language.as_ref().and_then(|config| {
Syntax::new(self.text.slice(..), config.language(), loader)
.map_err(|err| {
// `NoRootConfig` means that there was an issue loading the language/syntax
// config for the root language of the document. An error must have already
// been logged by `LanguageData::syntax_config`.
if err != syntax::HighlighterError::NoRootConfig {
log::warn!("Error building syntax for '{}': {err}", self.display_name());
}
})
.ok()
});
}Setting In this case, the document is set to rust and no highlights The tree-sitter-scope also seem to show |
the-mikedavis
force-pushed
the
tree-house
branch
from
2e79bb5 to
17fb059
Compare
|
I think the spurious parser failures should be fixed in the latest push: I'm pretty sure it was some unsoundness in the code used for the new way that the C library accepts a timeout. I'll figure out a proper implementation for that before the old way goes away in v0.26 of the C library. In the meantime we can use the old
These aren't implemented currently but could be added to tree-house if there is a need. |
|
I think I am getting some live locks. helix isnt crashing, but just freezes, which necessitates a forced kill of the process. I was editing some rust code in a doc example and it happened. I will try to get a recording of it if it happens again. |
This comment was marked as outdated.
This comment was marked as outdated.
|
Actually that might be related to something else, as even without this branch it is still happening. Sorry for the noise. |
the-mikedavis
force-pushed
the
tree-house
branch
from
17fb059 to
813f771
Compare
|
Starting to use this in my build and now I am not seeing the comment injections: Not sure if this is what was alluded to in an earlier discussion or if this is unknown. Before I copied over the new injections it seemed to work, but I haven't copied over those in a while. So it may be from the precedence PR? |
Yes, I think its the same as this: |
|
Here are a few extra injections to add for (fenced_code_block
(info_string
(language) @__language)
(code_fence_content) @injection.content
; list of attributes for Rust syntax highlighting:
; https://doc.rust-lang.org/rustdoc/write-documentation/documentation-tests.html?highlight=no_run#attributes
(#match? @__language
"(ignore|should_panic|no_run|compile_fail|standalone_crate|custom|edition*)")
(#set! injection.language "rust")
(#set! injection.include-unnamed-children))Since at the moment there is no syntax highlighting for |
This type also exists on `Editor`. This change brings it to the `Document` as well because the replacement for `Syntax` in the child commits will eliminate `Syntax`'s copy of `syn_loader`. `Syntax` will also be responsible for returning the highlighter and query iterators (which will borrow the loader), so the loader must be separated from that type. In the long run, when we make a larger refactor to have `Document::apply` be a function of the `Editor` instead of the `Document`, we will be able to drop this field on `Document` - it is currently only necessary for `Document::apply`. Once we make that refactor, we will be able to eliminate the surrounding `Arc` in `Arc<ArcSwap<syntax::Loader>>` and use the `ArcSwap` directly instead.
Co-authored-by: Nik Revenco <[email protected]>
the-mikedavis
force-pushed
the
tree-house
branch
from
813f771 to
974caf0
Compare
This PR drops our dependency on the upstream
tree-sitterRust bindings and moves us to a new highlighter crate we've been working on:tree-house.The main benefit of the new highlighter is that it fixes correctness issues, especially since the reverse query precedence PR. It solves longstanding issues #1151 and #6491, and updates us to the latest tree-sitter C source (v0.22 -> v0.25), which will become important as more grammars adopt the new ABI. It's also a slight performance boost in my benchmarking (5-10%). We've hoped to separate out our highlighting machinery for a while so that other applications could take advantage and the hope is that
tree-houseis the right way to expose it (\cc @hadronized).The new highlighter crate makes some abstractions clearer. The types to care about are already established in the
syntaxmodule:Loader, an ECS-style holder of all languages (pub struct Language(pub u32)). This stores the language configuration as well as any grammar and query information and allows looking up a language by file-type, language name, shebang, etc.. Instead of being stored onSyntaxthis is now stored separately onDocumentand passed toSyntaxfor updates. In the long run we can eventually store a singleArcSwap<syntax::Loader>onEditorinstead of many clones ofArc<ArcSwap<syntax::Loader>>.Syntax, a wrapper of theSyntaxtype fromtree-house. It holds a tree of trees of the injection layers of a document and provides logarithmic lookups to determine an injection layer (including the layer's language, config, tree and queries) for a given byte range. It also has a query iterator that works across injections which can support indents, textobjects, etc. in the future.There are some differences compared to
masterthat are less significant:helix_core::syntax::config. This is not strictly necessary for this change but it seems like a good opportunity to move these largely unrelated types away from the syntax module.u32s instead ofusize. This causes some changes inhelix-coreand the commands modules but is otherwise an implementation detail.tree_sitter::Pointin the indents module has been replaced with byte indices. Generally we can avoid the point types and functions.syntax::generate_editsfor example now produces zero points.TreeCursortype that acts over injection layers is built intotree-houseand is basically free to create - it doesn't require any extra sorting of layers. That type intree-houseuses theTreeCursortype from the C library and so it should be more efficient.Highlighttype comes fromtree-housenow and is represented as a non-max u32 (enables the null pointer optimization forOption<Highlight>).... and some other changes that are worth reviewing:
syntax::mergein favor of a cursor-like API that matches the highlighter. See theOverlayHighlightsandOverlayHighlightertypes in the rewrittensyntaxmodule.cargo xtask query-checkcan now point out unknown properties like(#set! priority ..)and unknown predicates like(#has-ancestor? ..)and has a nice visual output. This especially benefits the indent query which previously used runtime panics for the same effect, seeIndentQuery.Example query compilation errors...
Fixes #1151
Fixes #3391
Fixes #6491