From 7e5178dede7f0868f83972e05a16956c59f41cd3 Mon Sep 17 00:00:00 2001 From: Andrei Homescu Date: Thu, 23 Jul 2026 20:55:17 -0700 Subject: [PATCH] refactor: fix bug in comments right before a Use MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Root cause (in `rewrite/base.rs`, not the transform): the `Right`/insert branches of `rewrite_seq` and `rewrite_seq_comma_sep` anchored insertions at the neighbors' raw `splice_span()` boundaries, while the neighbors' own rewrites and deletions claim their *comment-extended* spans (`extend_span_comments`) — so an insertion anchored at the raw start of a commented, rewritten item landed inside that item's claimed region and `cleanup_rewrites` panicked. The insert branches now extend the neighbor anchor spans over comments the same way the delete branch always did, and an insertion at position 0 anchors before the first item's comment-extended span (the sequence's outer span can start inside it). Inserting exactly at the extended start of the next item is safe: `cleanup_rewrites` sorts the insertion ahead of the following rewrite. The regression test keeps the comment attached to the import and so covers this fix too. --- c2rust-refactor/src/rewrite/base.rs | 42 ++++++++++++++++--- .../reorganize_split_renamed_import.rs | 8 ++-- ...ns-reorganize_split_renamed_import.rs.snap | 11 +++-- 3 files changed, 47 insertions(+), 14 deletions(-) diff --git a/c2rust-refactor/src/rewrite/base.rs b/c2rust-refactor/src/rewrite/base.rs index db98cbaa8f..a072aa7da7 100644 --- a/c2rust-refactor/src/rewrite/base.rs +++ b/c2rust-refactor/src/rewrite/base.rs @@ -348,18 +348,36 @@ where // There's an item on the right corresponding to nothing on the left. // Insert the item before the current item on the left, rewriting // recursively. + // + // Rewrites and deletions of the neighboring old items claim + // their comment-extended spans, so anchor the insertion + // outside those extended spans; an insertion inside one + // would conflict with it. Inserting exactly at the extended + // start of the next item is fine: `cleanup_rewrites` orders + // the insertion before the following rewrite. + let extended_span = |idx: usize| { + let span = ast(&old[idx]).splice_span(); + match old_ids[idx] { + SeqItemId::Node(id) => extend_span_comments(&id, span, &rcx), + _ => span, + } + }; let before = if i > 0 { - ast(&old[i - 1]).splice_span() + extended_span(i - 1) } else { outer_span.shrink_to_lo() }; let after = if i < old.len() { - ast(&old[i]).splice_span() + extended_span(i) } else { outer_span.shrink_to_hi() }; - let old_span = if is_rewritable(before) { + let old_span = if i == 0 && i < old.len() && is_rewritable(after) { + // The outer span's start may fall inside the first item's + // comment-extended span; insert before the comments. + after.with_hi(after.lo()) + } else if is_rewritable(before) { before.with_lo(before.hi()) } else if is_rewritable(after) { after.with_hi(after.lo()) @@ -460,18 +478,30 @@ where // There's an item on the right corresponding to nothing on the left. // Insert the item before the current item on the left, rewriting // recursively. + // + // As in `rewrite_seq`, anchor the insertion outside the + // neighbors' comment-extended spans, which their own + // rewrites and deletions claim. + let extended_span = |idx: usize| match old_ids[idx] { + SeqItemId::Node(id) => extend_span_comments(&id, old_spans[idx], &rcx), + _ => old_spans[idx], + }; let before = if i > 0 { - old_spans[i - 1] + extended_span(i - 1) } else { outer_span.shrink_to_lo() }; let after = if i < old.len() { - old_spans[i] + extended_span(i) } else { outer_span.shrink_to_hi() }; - let old_span = if is_rewritable(before) { + let old_span = if i == 0 && i < old.len() && is_rewritable(after) { + // The outer span's start may fall inside the first item's + // comment-extended span; insert before the comments. + after.with_hi(after.lo()) + } else if is_rewritable(before) { before.with_lo(before.hi()) } else if is_rewritable(after) { after.with_hi(after.lo()) diff --git a/c2rust-refactor/tests/snapshots/reorganize_split_renamed_import.rs b/c2rust-refactor/tests/snapshots/reorganize_split_renamed_import.rs index 40aa7fda1a..53f37c06d4 100644 --- a/c2rust-refactor/tests/snapshots/reorganize_split_renamed_import.rs +++ b/c2rust-refactor/tests/snapshots/reorganize_split_renamed_import.rs @@ -44,11 +44,11 @@ pub mod dest { pub fn dest_fn() {} } -// The `user` module holds one simple import resolving in both the type and -// value namespaces. After the reorganization the two targets have different -// paths (`tick_1` vs `tick`), so a second import must be added for the value -// namespace: the retained import only covers the renamed type. pub mod user { + // One simple import resolving in both the type and value namespaces. + // After the reorganization the two targets have different paths + // (`tick_1` vs `tick`), so a second import must be added for the value + // namespace: the retained import only covers the renamed type. use crate::dest::dest_h::tick; pub fn make(x: i32) -> tick { diff --git a/c2rust-refactor/tests/snapshots/snapshots__refactor-reorganize_definitions-reorganize_split_renamed_import.rs.snap b/c2rust-refactor/tests/snapshots/snapshots__refactor-reorganize_definitions-reorganize_split_renamed_import.rs.snap index ab912cabba..c713dfa5dc 100644 --- a/c2rust-refactor/tests/snapshots/snapshots__refactor-reorganize_definitions-reorganize_split_renamed_import.rs.snap +++ b/c2rust-refactor/tests/snapshots/snapshots__refactor-reorganize_definitions-reorganize_split_renamed_import.rs.snap @@ -48,12 +48,15 @@ pub mod dest { pub fn dest_fn() {} } -// The `user` module holds one simple import resolving in both the type and -// value namespaces. After the reorganization the two targets have different -// paths (`tick_1` vs `tick`), so a second import must be added for the value -// namespace: the retained import only covers the renamed type. pub mod user { use crate::dest::tick; + // One simple import resolving in both the type and value namespaces. + + // After the reorganization the two targets have different paths + + // (`tick_1` vs `tick`), so a second import must be added for the value + + // namespace: the retained import only covers the renamed type. use crate::dest::tick_1; pub fn make(x: i32) -> crate::dest::tick_1 {