on master help sign in
Fix initial highlight layer sort order (#5196)

The purpose of this change is to remove the mutable self borrow on `HighlightIterLayer::sort_key` so that we can sort layers with the correct ordering using the `Vec::sort` function family. `HighlightIterLayer::sort_key` needs `&mut self` since it calls `Peekable::peek` which needs `&mut self`. `Vec::sort` functions only give immutable borrows of the elements to ensure the correctness of the sort. We could instead approach this by creating an eager Peekable and using that instead of `std::iter::Peekable` to wrap `QueryCaptures`: ```rust struct EagerPeekable<I: Iterator> { iter: I, peeked: Option<I::Item>, } impl<I: Iterator> EagerPeekable<I> { fn new(mut iter: I) -> Self { let peeked = iter.next(); Self { iter, peeked } } fn peek(&self) -> Option<&I::Item> { self.peeked.as_ref() } } impl<I: Iterator> Iterator for EagerPeekable<I> { type Item = I::Item; fn next(&mut self) -> Option<Self::Item> { std::mem::replace(&mut self.peeked, self.iter.next()) } } ``` This would be a cleaner approach (notice how `EagerPeekable::peek` takes `&self` rather than `&mut self`), however this doesn't work in practice because the Items emitted by the `tree_sitter::QueryCaptures` Iterator must be consumed before the next Item is returned. `Iterator::next` on `tree_sitter::QueryCaptures` modifies the `QueryMatch` returned by the last call of `next`. This behavior is not currently reflected in the lifetimes/structure of `QueryCaptures`. This fixes an issue with layers being out of order when using combined injections since the old code only checked the first range in the layer. Layers being out of order could cause missing highlights for combined-injections content.

commitd5f17d3f6947abbfd0684246a440751355e48f9d authorMichael Davis <mcarsondavis@gmail.com> dateWed 1 Feb 2023, 15:28 (4 years ago) parentb225187
1 file changed+14−20
helix-core/src/syntax.rs+14−20
helix-core/src/syntax.rs+14−20
@@ -1092,21 +1092,14 @@ impl Syntax {
10921092 }],
10931093 cursor,
10941094 _tree: None,
1095− captures,
1095+ captures: RefCell::new(captures),
10961096 config: layer.config.as_ref(), // TODO: just reuse `layer`
10971097 depth: layer.depth, // TODO: just reuse `layer`
1098− ranges: &layer.ranges, // TODO: temp
10991098 })
11001099 })
11011100 .collect::<Vec<_>>();
11021101
1103− // HAXX: arrange layers by byte range, with deeper layers positioned first
1104− layers.sort_by_key(|layer| {
1105− (
1106− layer.ranges.first().cloned(),
1107− std::cmp::Reverse(layer.depth),
1108− )
1109− });
1102+ layers.sort_unstable_by_key(|layer| layer.sort_key());
11101103
11111104 let mut result = HighlightIter {
11121105 source,
@@ -1424,12 +1417,11 @@ impl<'a> TextProvider<'a> for RopeProvider<'a> {
14241417struct HighlightIterLayer<'a> {
14251418 _tree: Option<Tree>,
14261419 cursor: QueryCursor,
1427− captures: iter::Peekable<QueryCaptures<'a, 'a, RopeProvider<'a>>>,
1420+ captures: RefCell<iter::Peekable<QueryCaptures<'a, 'a, RopeProvider<'a>>>>,
14281421 config: &'a HighlightConfiguration,
14291422 highlight_end_stack: Vec<usize>,
14301423 scope_stack: Vec<LocalScope<'a>>,
14311424 depth: u32,
1432− ranges: &'a [Range],
14331425}
14341426
14351427impl<'a> fmt::Debug for HighlightIterLayer<'a> {
@@ -1610,10 +1602,11 @@ impl<'a> HighlightIterLayer<'a> {
16101602 // First, sort scope boundaries by their byte offset in the document. At a
16111603 // given position, emit scope endings before scope beginnings. Finally, emit
16121604 // scope boundaries from deeper layers first.
1613− fn sort_key(&mut self) -> Option<(usize, bool, isize)> {
1605+ fn sort_key(&self) -> Option<(usize, bool, isize)> {
16141606 let depth = -(self.depth as isize);
16151607 let next_start = self
16161608 .captures
1609+ .borrow_mut()
16171610 .peek()
16181611 .map(|(m, i)| m.captures[*i].node.start_byte());
16191612 let next_end = self.highlight_end_stack.last().cloned();
@@ -1838,7 +1831,8 @@ impl<'a> Iterator for HighlightIter<'a> {
18381831 // Get the next capture from whichever layer has the earliest highlight boundary.
18391832 let range;
18401833 let layer = &mut self.layers[0];
1841− if let Some((next_match, capture_index)) = layer.captures.peek() {
1834+ let captures = layer.captures.get_mut();
1835+ if let Some((next_match, capture_index)) = captures.peek() {
18421836 let next_capture = next_match.captures[*capture_index];
18431837 range = next_capture.node.byte_range();
18441838
@@ -1861,7 +1855,7 @@ impl<'a> Iterator for HighlightIter<'a> {
18611855 return self.emit_event(self.source.len_bytes(), None);
18621856 };
18631857
1864− let (mut match_, capture_index) = layer.captures.next().unwrap();
1858+ let (mut match_, capture_index) = captures.next().unwrap();
18651859 let mut capture = match_.captures[capture_index];
18661860
18671861 // Remove from the local scope stack any local scopes that have already ended.
@@ -1937,11 +1931,11 @@ impl<'a> Iterator for HighlightIter<'a> {
19371931 }
19381932
19391933 // Continue processing any additional matches for the same node.
1940− if let Some((next_match, next_capture_index)) = layer.captures.peek() {
1934+ if let Some((next_match, next_capture_index)) = captures.peek() {
19411935 let next_capture = next_match.captures[*next_capture_index];
19421936 if next_capture.node == capture.node {
19431937 capture = next_capture;
1944− match_ = layer.captures.next().unwrap().0;
1938+ match_ = captures.next().unwrap().0;
19451939 continue;
19461940 }
19471941 }
@@ -1964,11 +1958,11 @@ impl<'a> Iterator for HighlightIter<'a> {
19641958 // highlighting patterns that are disabled for local variables.
19651959 if definition_highlight.is_some() || reference_highlight.is_some() {
19661960 while layer.config.non_local_variable_patterns[match_.pattern_index] {
1967− if let Some((next_match, next_capture_index)) = layer.captures.peek() {
1961+ if let Some((next_match, next_capture_index)) = captures.peek() {
19681962 let next_capture = next_match.captures[*next_capture_index];
19691963 if next_capture.node == capture.node {
19701964 capture = next_capture;
1971− match_ = layer.captures.next().unwrap().0;
1965+ match_ = captures.next().unwrap().0;
19721966 continue;
19731967 }
19741968 }
@@ -1983,10 +1977,10 @@ impl<'a> Iterator for HighlightIter<'a> {
19831977 // for a given node are ordered by pattern index, so these subsequent
19841978 // captures are guaranteed to be for highlighting, not injections or
19851979 // local variables.
1986− while let Some((next_match, next_capture_index)) = layer.captures.peek() {
1980+ while let Some((next_match, next_capture_index)) = captures.peek() {
19871981 let next_capture = next_match.captures[*next_capture_index];
19881982 if next_capture.node == capture.node {
1989− layer.captures.next();
1983+ captures.next();
19901984 } else {
19911985 break;
19921986 }