Rewrite Merge to take two HighlightEvent Iterators #3695

* Refactor Merge to take two HighlightEvent Iterators * Add an Iterator for subslicing overlapping spans

In these cases, right or left has finished, respectively. We drain right or left's queue and emit those events but previously we forgot to then move the iterator to the next, discarding the Source being emitted in these branches. The result was duplicated emitted text after the last diagnostic in a file, or after the last selection.

The ranges for highlight spans are inclusive, so the span in right may be discarded if its end is equal to left's start. For a graphical example: |----------------| L |---| R Here R ends as L starts. In this case we discard R. Without this change, we subsliced R and made it into a Source which starts at L and ends at L which fails the min_width_1 invariant. Also included is a change that eagerly discards the following HighlightEnd events in the iterator and queued HighlightStart events. This case is technically covered by the outer `loop { match { .. } }` but it's better to do it eagerly: we don't need to branch on all of the possibilities, just blindly empty the right_queue and drop all HighlightEnds.

This change adds two counters which are incremented for every queued HighlightStart and decremented with every dequeued HighlightEnd. This is used to track the invariant that HighlightStarts and HighlightEnds are balanced in the left iterator. In the right iterator, the same checks are made except that the counter is also used to balance the number of HighlightStarts and HighlightEnds when the right iterator is being stopped before having finished. Without this change, the first test case from this commit would be missing a HighlightEnd. Also, the second test case would provoke a panic attempting to match on `(None, Some(HighlightEnd))`. This has been fixed by the change which allows a `(None, Some(event))` pattern. Output-wise, that change allows a `HighlightStart, HighlightEnd` sequence from `right` to end up in the emitted events. These are slightly wasteful but are otherwise benign. The given test case reflects a panic from running on the rainbow brackets branch.

Rather than transforming the input `Vec<Span>` into an iterator, this version of SpanIter keeps the Vec as-is and iterates through it manually with an index. This is necessary because in cases where many spans overlap one another, the SpanIter implementation needs to look arbitrarily far ahead in the Vec to find all ranges which start on the same point. The basic structure of this version is the same as the prior version. The following are the differences: * The position in the Vec is tracked manually with an index rather than automatically with an Iterator. * Subslicing applies to all spans with the same starting point. * All spans with the same start point are emitted simultaneously. The new version covers a real breakage noticed from diagnostics produced by rust-analyzer.

Selection highlights are already known to be satisfy all of the invariants in syntax::merge so syntax::span_iter is pure overhead. This change introduces another iterator with a naive flat_map implementation that splits Spans into HighlightEvents without checking if any spans are overlapping or sorted. We wrap the iterator in a new type to avoid any performance penalty of a `Box<dyn Iterator<Item = HighlightEvent>>` (which uses dynamic dispatch).

This change covers an edge-case that can arise in the SpanIter from subslicing spans. Spans which are subsliced may start after some other span later in the span Vec (see the new test case). To handle this, we re-sort the span Vec after subslicing any spans. Since the subslice only affects the starts of some spans and otherwise leaves the ordering unchanged, we accomplish the sort by finding any spans which should come before the subsliced spans and rotate them to come first. This preserves the ordering in the subsliced spans and in the rotated spans but ensures that the global ordering of the Vec remains correct after the subslice.

The span iterators and tests took up a significant portion of the syntax module but were fairly self-contained. In addition, the Span type was an informal (usize, std::ops::Range<usize>) which was a bit tricky to deal with: * the type was long to write out for all producers/consumers * std::ops::Range<usize> does not implement Copy Plus we don't end up using any Range methods except its Ord/PartialOrd implementations. Given its first-class usage for diagnostics and selections, it seems appropriate to separate it into its own struct type and module.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite Merge to take two HighlightEvent Iterators #3695

Rewrite Merge to take two HighlightEvent Iterators #3695

Commits on Sep 22, 2022