language: Only invalidate injections owned by the reparsed layer - #62437
language: Only invalidate injections owned by the reparsed layer#62437Artin0123 wants to merge 3 commits into
Conversation
dinocosta
left a comment
There was a problem hiding this comment.
Hey @Artin0123 🙂
Thank you for suggesting these changes! Was taking a look at the code and I'm suspecting it might be a duplicate of or, at least, attempt to fix the same issue as #62370 .
I'm going to close this Pull Request because of that, but feel free to let me know if there's any detail I might be missing that is contemplated in this Pull Request but not on #62370 . In case that's something that could be easily added to #62370 , feel free to comment there.
Thanks!
|
@dinocosta Thanks for taking a look, and sorry for the confusion — the symptom in both reports To make sure I wasn't guessing, I measured it. I checked out a single base commit
In the "#62370 only" column, the output for A/B/C is byte-for-byte identical to the That is because the two changes are in different functions and address different
The user-visible difference is exactly that: #62127 recovers on save, this one does Would you mind reopening this PR? Happy to rebase it onto current |
|
Thank you for taking a thorough look @Artin0123 ! I'll reopen this then 🙂 In the meantime, and maybe the existing test already does this, would you mind adding or updating existing tests to cover the scenarios you've listed that are only fixed by the changes in this Pull Request and not by #62370 ? Thanks! P.S.: Applied the |
|
Thanks for reopening, @dinocosta! 🎉 I think this is already covered by the test in this PR: A and C should come down to the same clamp in If you'd still like them covered explicitly, I'm happy to add the extra edits and assertions, or to restructure the test if there's a shape you'd prefer. |
The lost highlighting is not specific to HTML comments: any highlight produced by the injected layer disappears, so assert an inline tag in an adjacent list item as well.
Clamping the expanded ranges to the reparsed layer's own range still let the invalidation discard the injections of a layer that merely begins where the reparsed layer ends, such as the paragraph following an HTML block in Markdown. That layer is not reparsed, so its discarded injections are never recreated. Record which layer each changed region belongs to and skip layers that are not nested inside of it. This subsumes the clamping, so the expanded ranges are back to their original form.
|
Pushed two more commits and rewrote the description, which now stands on its own. The regression test now asserts an inline tag in addition to the comments, because the loss is not specific to HTML comments — a plain More importantly, the clamping this PR did before only fixes part of the problem. Testing more reports of the same symptom turned up a case it misses: a layer that begins exactly where the reparsed layer ends. Typing at the end of an HTML block still discards the HTML injected into the paragraph on the next line, because the clamped range and that layer touch at the same offset. The fix now records which layer each changed region belongs to and only invalidates layers nested inside of it, which subsumes the clamping and covers both cases. The description has the comparison with the alternatives I ruled out. |
Objective
Syntax highlighting inside nested language injections can be lost permanently after editing a nearby line. It only comes back after editing the affected line again or restarting Zed; saving the file does not help.
This is most visible with inline HTML in Markdown, where every paragraph and list item is a separate injection layer:
output.mp4
After a layer is reparsed,
SyntaxMapregisters its changed rows, expanded by one row in each direction, as invalidated regions for the next nesting depth. Nothing checks that the layers those regions discard actually belong to the reparsed layer. A layer only recomputes its injections when its own content changed, so a neighbouring layer whose text was untouched skipsget_injections, and the child layer discarded on its behalf is never recreated.Solution
Record which layer each
ChangedRegionbelongs to, and only invalidate layers nested inside of it. A layer owns exactly the injections inside its own range, and it is the only layer that will recreate them.Two narrower fixes were tried first and are not sufficient:
ITERATIONS=300random editsClamping leaves the second case, because the clamped range and the abutting layer still touch at the same offset. Biasing the endpoints inwards cannot distinguish a child that legitimately begins at that offset from an unrelated layer that does, so it keeps stale layers alive: with that variant, a 300-seed sweep of
test_random_syntax_map_edits_rust_macrosends up with three layers surviving where a fresh parse has one. Ownership distinguishes the two: a child is nested inside the owner, an abutting layer is not.Ownership subsumes clamping, so the expanded ranges themselves are unchanged from
main.#62370 changes
splice_included_ranges, which is a different function and a different bug: it produces a wrong range list within a single layer, so a full reparse repairs it. The tests added here fail with it applied.Testing
test_injections_are_preserved_when_a_sibling_layer_is_reparsededits one Markdown list item and asserts that the inline HTML of the adjacent items keeps its highlighting, for both comments and tags.test_injections_are_preserved_when_an_abutting_layer_is_reparsededits the last line of an HTML block and asserts that the HTML injected into the following paragraph keeps its highlighting.cargo test -p language(154 tests)ITERATIONS=300 cargo test -p language test_random_syntax_map_editsSelf-Review Checklist:
Release Notes: