feat: Support for nested languages #403

I'd like to avoid the indentation caused by if let Some. How about changing the first 2 lines to the following?

let Some(query_str) = lang_profile.injections else {
    return FxHashMap::default();
};
let mut node_id_to_injection_lang = FxHashMap::default();

Maybe it would even make sense to change the return type to an Option? Not sure about this one though

Sure! Is it more convenient for you if I amend the commits and force-push, or add those refactorings as separate commits on top of the branch?

That's a good question. On one hand, I do prefer a cleaner history. On the other hand, I think Codeberg doesn't quite like it when a change is pulled from under the feet a review comment (since it doesn't keep track of PR versions AFAICT). So let's play it safe and go with the second option

1. since the result of this if let Some() doesn't depend on the particular node, it can be pulled out of for_each
2. nodes.for_each( map.insert() ) can be replaced with map.extend(nodes)

if let Some(injected_lang) = LangProfile::find_by_name(&language) {
    node_id_to_injection_lang.extend(
        m.nodes_for_capture_index(content_capture_index)
            .map(|node| (node.id(), injected_lang)),
    );
}

shouldn't we maybe return an error if we don't support the injected language? Because I think that if we ignore its actual language, we'll end up handling it like the language it's embedded in, which might backfire

Because I think that if we ignore its actual language, we'll end up handling it like the language it's embedded in, which might backfire

no, in that case we just don't re-parse the injection recursively at all, and just leave it to how it was parsed by the parent grammar… which feels appropriate to me, given that the parent grammar will generally leave this part un-analyzed.

For instance, we currently don't support CSS, and the HTML grammar sets up injections for <style> elements to be parsed as CSS. Because we don't find a suitable language, the <style> block is left un-analyzed. The presence of a <style> block shouldn't prevent the HTML from parsing, since we can still resolve conflicts in the rest of the HTML tree.

Right, I since saw that explained at line 189. Maybe it would make sense to copy that comment into here as well

It took me longer than I'd like to admit to notice the yaml->yml change introduced by the left side. Maybe add a README like:

left: changed codeblock language from yaml to yml
right: renamed foo to foo2

or, if you want to be more terse:

left: s/yaml/yml
right: s/foo/foo2

My bad, actually I should have kept this test case in the PR that introduced the markdown parser, since it's independent from injections support…

I guess that would've stopped me from trying to look inside the codeblock... But all good:)
But I think having the README is helpful even despite that 😅

Ah, you did add it! Thanks:)

Nice reference 😅

This .into() turns the "injection.content" on the right into an owned Box<str>, since that's the type of property.key on the left.

Box<str> is an owned equivalent of &str (the only difference from String being that the latter can be resized, so the triple is isomorphic to Box<[T]> vs &[T] vs Vec<T>), and it implements Deref<Target=str>, so it can be coerced to a &str with a reborrow:

 if &*property.key == "injection.language"

Pretty much the same goes for property.value, except that there the Box<str> is wrapped in an Option, so we need to use Option::as_deref to reborrow the value. The rest should follow, but here's the solution just in case:)

diff --git i/src/ast.rs w/src/ast.rs
index 748fd584..114584e9 100644
--- i/src/ast.rs
+++ w/src/ast.rs
@@ -137,7 +137,7 @@ impl<'a> AstNode<'a> {
                 // first, check if the language is statically defined in this clause of the query as a property
                 let lang_property_value = pattern_properties.iter().find_map(|property| {
                     if &*property.key == "injection.language" {
-                        Some(property.value.clone()?.to_string())
+                        property.value.as_deref()
                     } else {
                         None
                     }
@@ -151,9 +151,9 @@ impl<'a> AstNode<'a> {
                         ))
                         .next()
                         .expect("injection.language capture didn't match any node");
-                    source[lang_node.byte_range()].to_owned()
+                    &source[lang_node.byte_range()]
                 });
-                if let Some(injected_lang) = LangProfile::find_by_name(&language) {
+                if let Some(injected_lang) = LangProfile::find_by_name(language) {
                     node_id_to_injection_lang.extend(
                         m.nodes_for_capture_index(content_capture_index)
                             .map(|node| (node.id(), injected_lang)),

Thanks a lot, you can tell I've struggled with those boxes :-D

I think extracting this into a variable would be a bit cleaner...

can use the Display impl for LangProfile here:)

"Injection query for language {lang_profile} set as an empty string, use None instead",

is there an example of this you know of? maybe even a test case that shows an injection being trimmed resulting in an incorrect output...

If you remove the condition on the injection, you should get a nice error when running examples/markdown/failing/nested_injections.

Interesting. But the fact that the test is failing even with this restriction in place could mean that the latter is not quite correct, no?

The error you mention appears in trailing_whitespace, in particular when (panickingly) slicing into self.source, so I tried replacing that with a get, but then the test output was a whole different kind of wrong... Maybe I'll admit my defeat here, and trust you to debug this in the future 😅

But why do children of injection expand beyond the node itself! That feels so wrong to me

The root of a tree as returned by tree-sitter generally (if not always) spans the whole source that you give it to parse. Because we do not to trim roots (condition node.parent().is_some()), this means that the containing node (the one picked out by the injection query) shouln't be trimmed either. This doesn't need to propagate further up the tree since the node picked out by the injection query is the one defining the source area to parse, so we know we're not expanding beyond that.

An alternative (which I would like to explore as a follow up) would be to trim the injected roots as well, and only not trim the real, actual root of the entire document.