Skip to content

Commit

Permalink
Auto merge of rust-lang#18166 - ChayimFriedman2:dollar-crate-root, r=…
Browse files Browse the repository at this point in the history
…Veykril

fix: Fix a bug in span map merge, and add explanations of how span maps are stored

Because it took me hours to figure out that contrary to common sense, the offset stored is the *end* of the node, and we search by the *start*. Which is why we need a convoluted `partition_point()` instead of a simple `binary_search()`. And this was not documented at all. Which made me make mistakes with my implementation of `SpanMap::merge()`.

The other bug fixed about span map merging is correctly keeping track of the current offset in presence of multiple sibling macro invocations. Unrelated, but because of the previous issue it took me hours to debug, so I figured out I'll put them together for posterity.

Fixes rust-lang#18163.
  • Loading branch information
bors committed Sep 24, 2024
2 parents a5f028b + b14cd71 commit ceb495a
Show file tree
Hide file tree
Showing 2 changed files with 33 additions and 5 deletions.
12 changes: 9 additions & 3 deletions src/tools/rust-analyzer/crates/ide/src/expand_macro.rs
Original file line number Diff line number Diff line change
Expand Up @@ -149,14 +149,14 @@ fn expand_macro_recur(
expanded.text_range().len(),
&expansion_span_map,
);
Some(expand(sema, expanded, result_span_map, offset_in_original_node))
Some(expand(sema, expanded, result_span_map, u32::from(offset_in_original_node) as i32))
}

fn expand(
sema: &Semantics<'_, RootDatabase>,
expanded: SyntaxNode,
result_span_map: &mut SpanMap<SyntaxContextId>,
offset_in_original_node: TextSize,
mut offset_in_original_node: i32,
) -> SyntaxNode {
let children = expanded.descendants().filter_map(ast::Item::cast);
let mut replacements = Vec::new();
Expand All @@ -166,8 +166,14 @@ fn expand(
sema,
&child,
result_span_map,
offset_in_original_node + child.syntax().text_range().start(),
TextSize::new(
(offset_in_original_node + (u32::from(child.syntax().text_range().start()) as i32))
as u32,
),
) {
offset_in_original_node = offset_in_original_node
+ (u32::from(new_node.text_range().len()) as i32)
- (u32::from(child.syntax().text_range().len()) as i32);
// check if the whole original syntax is replaced
if expanded == *child.syntax() {
return new_node;
Expand Down
26 changes: 24 additions & 2 deletions src/tools/rust-analyzer/crates/span/src/map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ use crate::{
/// Maps absolute text ranges for the corresponding file to the relevant span data.
#[derive(Debug, PartialEq, Eq, Clone, Hash)]
pub struct SpanMap<S> {
/// The offset stored here is the *end* of the node.
spans: Vec<(TextSize, SpanData<S>)>,
/// Index of the matched macro arm on successful expansion for declarative macros.
// FIXME: Does it make sense to have this here?
Expand Down Expand Up @@ -109,11 +110,32 @@ where
///
/// The length of the replacement node needs to be `other_size`.
pub fn merge(&mut self, other_range: TextRange, other_size: TextSize, other: &SpanMap<S>) {
// I find the following diagram helpful to illustrate the bounds and why we use `<` or `<=`:
// --------------------------------------------------------------------
// 1 3 5 6 7 10 11 <-- offsets we store
// 0-1 1-3 3-5 5-6 6-7 7-10 10-11 <-- ranges these offsets refer to
// 3 .. 7 <-- other_range
// 3-5 5-6 6-7 <-- ranges we replace (len = 7-3 = 4)
// ^^^^^^^^^^^ ^^^^^^^^^^
// remove shift
// 2 3 5 9 <-- offsets we insert
// 0-2 2-3 3-5 5-9 <-- ranges we insert (other_size = 9-0 = 9)
// ------------------------------------
// 1 3
// 0-1 1-3 <-- these remain intact
// 5 6 8 12
// 3-5 5-6 6-8 8-12 <-- we shift these by other_range.start() and insert them
// 15 16
// 12-15 15-16 <-- we shift these by other_size-other_range.len() = 9-4 = 5
// ------------------------------------
// 1 3 5 6 8 12 15 16 <-- final offsets we store
// 0-1 1-3 3-5 5-6 6-8 8-12 12-15 15-16 <-- final ranges

self.spans.retain_mut(|(offset, _)| {
if other_range.contains(*offset) {
if other_range.start() < *offset && *offset <= other_range.end() {
false
} else {
if *offset >= other_range.end() {
if *offset > other_range.end() {
*offset += other_size;
*offset -= other_range.len();
}
Expand Down

0 comments on commit ceb495a

Please sign in to comment.