Skip to content

Commit a6456cc

Browse files
committed
refactor: fix bug in comments right before a Use
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.
1 parent 7ab6fcf commit a6456cc

3 files changed

Lines changed: 47 additions & 14 deletions

File tree

c2rust-refactor/src/rewrite/base.rs

Lines changed: 36 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -348,18 +348,36 @@ where
348348
// There's an item on the right corresponding to nothing on the left.
349349
// Insert the item before the current item on the left, rewriting
350350
// recursively.
351+
//
352+
// Rewrites and deletions of the neighboring old items claim
353+
// their comment-extended spans, so anchor the insertion
354+
// outside those extended spans; an insertion inside one
355+
// would conflict with it. Inserting exactly at the extended
356+
// start of the next item is fine: `cleanup_rewrites` orders
357+
// the insertion before the following rewrite.
358+
let extended_span = |idx: usize| {
359+
let span = ast(&old[idx]).splice_span();
360+
match old_ids[idx] {
361+
SeqItemId::Node(id) => extend_span_comments(&id, span, &rcx),
362+
_ => span,
363+
}
364+
};
351365
let before = if i > 0 {
352-
ast(&old[i - 1]).splice_span()
366+
extended_span(i - 1)
353367
} else {
354368
outer_span.shrink_to_lo()
355369
};
356370
let after = if i < old.len() {
357-
ast(&old[i]).splice_span()
371+
extended_span(i)
358372
} else {
359373
outer_span.shrink_to_hi()
360374
};
361375

362-
let old_span = if is_rewritable(before) {
376+
let old_span = if i == 0 && i < old.len() && is_rewritable(after) {
377+
// The outer span's start may fall inside the first item's
378+
// comment-extended span; insert before the comments.
379+
after.with_hi(after.lo())
380+
} else if is_rewritable(before) {
363381
before.with_lo(before.hi())
364382
} else if is_rewritable(after) {
365383
after.with_hi(after.lo())
@@ -460,18 +478,30 @@ where
460478
// There's an item on the right corresponding to nothing on the left.
461479
// Insert the item before the current item on the left, rewriting
462480
// recursively.
481+
//
482+
// As in `rewrite_seq`, anchor the insertion outside the
483+
// neighbors' comment-extended spans, which their own
484+
// rewrites and deletions claim.
485+
let extended_span = |idx: usize| match old_ids[idx] {
486+
SeqItemId::Node(id) => extend_span_comments(&id, old_spans[idx], &rcx),
487+
_ => old_spans[idx],
488+
};
463489
let before = if i > 0 {
464-
old_spans[i - 1]
490+
extended_span(i - 1)
465491
} else {
466492
outer_span.shrink_to_lo()
467493
};
468494
let after = if i < old.len() {
469-
old_spans[i]
495+
extended_span(i)
470496
} else {
471497
outer_span.shrink_to_hi()
472498
};
473499

474-
let old_span = if is_rewritable(before) {
500+
let old_span = if i == 0 && i < old.len() && is_rewritable(after) {
501+
// The outer span's start may fall inside the first item's
502+
// comment-extended span; insert before the comments.
503+
after.with_hi(after.lo())
504+
} else if is_rewritable(before) {
475505
before.with_lo(before.hi())
476506
} else if is_rewritable(after) {
477507
after.with_hi(after.lo())

c2rust-refactor/tests/snapshots/reorganize_split_renamed_import.rs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -44,11 +44,11 @@ pub mod dest {
4444
pub fn dest_fn() {}
4545
}
4646

47-
// The `user` module holds one simple import resolving in both the type and
48-
// value namespaces. After the reorganization the two targets have different
49-
// paths (`tick_1` vs `tick`), so a second import must be added for the value
50-
// namespace: the retained import only covers the renamed type.
5147
pub mod user {
48+
// One simple import resolving in both the type and value namespaces.
49+
// After the reorganization the two targets have different paths
50+
// (`tick_1` vs `tick`), so a second import must be added for the value
51+
// namespace: the retained import only covers the renamed type.
5252
use crate::dest::dest_h::tick;
5353

5454
pub fn make(x: i32) -> tick {

c2rust-refactor/tests/snapshots/snapshots__refactor-reorganize_definitions-reorganize_split_renamed_import.rs.snap

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -47,12 +47,15 @@ pub mod dest {
4747
pub fn dest_fn() {}
4848
}
4949

50-
// The `user` module holds one simple import resolving in both the type and
51-
// value namespaces. After the reorganization the two targets have different
52-
// paths (`tick_1` vs `tick`), so a second import must be added for the value
53-
// namespace: the retained import only covers the renamed type.
5450
pub mod user {
5551
use crate::dest::tick;
52+
// One simple import resolving in both the type and value namespaces.
53+
54+
// After the reorganization the two targets have different paths
55+
56+
// (`tick_1` vs `tick`), so a second import must be added for the value
57+
58+
// namespace: the retained import only covers the renamed type.
5659
use crate::dest::tick_1;
5760

5861
pub fn make(x: i32) -> crate::dest::tick_1 {

0 commit comments

Comments
 (0)