From 75330200fad656dd4a66c0ec8d40f5b128cffed3 Mon Sep 17 00:00:00 2001 From: Andrei Homescu Date: Thu, 23 Jul 2026 20:36:33 -0700 Subject: [PATCH 1/2] refactor: add test for multi-namespace imports splitting Regression test: `test_reorganize_split_renamed_import` (`tests/snapshots/reorganize_split_renamed_import.rs`). The original two-external-crates scenario cannot be built in the single-file snapshot harness, so the test exercises the equivalent local case the old parent-id comparison also got wrong: a collision-renamed type target (`tick_1`) and a value target (`tick`) in the *same* destination module. Pre-fix output omits the value-namespace `use crate::dest::tick;`. --- c2rust-refactor/tests/snapshots.rs | 10 +++ .../reorganize_split_renamed_import.rs | 63 +++++++++++++++++ ...ns-reorganize_split_renamed_import.rs.snap | 67 +++++++++++++++++++ 3 files changed, 140 insertions(+) create mode 100644 c2rust-refactor/tests/snapshots/reorganize_split_renamed_import.rs create mode 100644 c2rust-refactor/tests/snapshots/snapshots__refactor-reorganize_definitions-reorganize_split_renamed_import.rs.snap diff --git a/c2rust-refactor/tests/snapshots.rs b/c2rust-refactor/tests/snapshots.rs index e7e88cce5e..d5959ddb59 100644 --- a/c2rust-refactor/tests/snapshots.rs +++ b/c2rust-refactor/tests/snapshots.rs @@ -541,6 +541,16 @@ fn test_reorganize_split_namespace_imports() { .test(); } +/// A multi-namespace import whose type-namespace target is collision-renamed +/// must still get a second import for the value namespace: coverage is +/// decided by comparing target paths, not parent modules. +#[test] +fn test_reorganize_split_renamed_import() { + refactor("reorganize_definitions") + .named("reorganize_split_renamed_import.rs") + .test(); +} + #[test] fn test_sink_lets() { refactor("sink_lets").test(); diff --git a/c2rust-refactor/tests/snapshots/reorganize_split_renamed_import.rs b/c2rust-refactor/tests/snapshots/reorganize_split_renamed_import.rs new file mode 100644 index 0000000000..40aa7fda1a --- /dev/null +++ b/c2rust-refactor/tests/snapshots/reorganize_split_renamed_import.rs @@ -0,0 +1,63 @@ +#![feature(register_tool)] +#![register_tool(c2rust)] +#![allow(non_camel_case_types)] +#![allow(dead_code)] +#![allow(unused_imports)] + +pub mod other { + #[c2rust::header_src = "/home/user/some/workspace/other.h:1"] + pub mod other_h { + // This incompatible struct shares the `tick` spelling and is + // processed first, so the struct in dest.h below is renamed to + // `tick_1` when both are moved out of their headers. + #[derive(Copy, Clone)] + #[repr(C)] + #[c2rust::src_loc = "2:0"] + pub struct tick { + pub y: f64, + } + } + + pub fn other_fn() {} +} + +pub mod dest { + #[c2rust::header_src = "/home/user/some/workspace/dest.h:1"] + pub mod dest_h { + // A type and a value sharing the `tick` spelling. Both move into + // `dest`, but the struct is renamed to `tick_1` by the collision + // with other.h while the static keeps its name, so their new paths + // differ even though they land in the same module. + #[derive(Copy, Clone)] + #[repr(C)] + #[c2rust::src_loc = "3:0"] + pub struct tick { + pub x: i32, + } + + extern "C" { + #[c2rust::src_loc = "9:0"] + pub static tick: i32; + } + } + + 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::dest_h::tick; + + pub fn make(x: i32) -> tick { + tick { x } + } + + pub fn read() -> i32 { + unsafe { tick } + } +} + +fn main() {} 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 new file mode 100644 index 0000000000..0ee9b363e1 --- /dev/null +++ b/c2rust-refactor/tests/snapshots/snapshots__refactor-reorganize_definitions-reorganize_split_renamed_import.rs.snap @@ -0,0 +1,67 @@ +--- +source: c2rust-refactor/tests/snapshots.rs +expression: c2rust-refactor reorganize_definitions --rewrite-mode alongside -- tests/snapshots/reorganize_split_renamed_import.rs --edition 2021 +--- +#![feature(register_tool)] +#![register_tool(c2rust)] +#![allow(non_camel_case_types)] +#![allow(dead_code)] +#![allow(unused_imports)] + +pub mod other { + + // =============== BEGIN other_h ================ + + // This incompatible struct shares the `tick` spelling and is + + // processed first, so the struct in dest.h below is renamed to + + // `tick_1` when both are moved out of their headers. + #[derive(Copy, Clone)] + #[repr(C)] + pub struct tick { + pub y: f64, + } + + pub fn other_fn() {} +} + +pub mod dest { + extern "C" { + pub static tick: i32; + } + // =============== BEGIN dest_h ================ + + // A type and a value sharing the `tick` spelling. Both move into + + // `dest`, but the struct is renamed to `tick_1` by the collision + + // with other.h while the static keeps its name, so their new paths + + // differ even though they land in the same module. + #[derive(Copy, Clone)] + #[repr(C)] + pub struct tick_1 { + pub x: i32, + } + + 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_1; + + pub fn make(x: i32) -> crate::dest::tick_1 { + crate::dest::tick_1 { x } + } + + pub fn read() -> i32 { + unsafe { crate::dest::tick } + } +} + +fn main() {} From 5748cf2a9a100c5c3c3add79694042f0a52d292f Mon Sep 17 00:00:00 2001 From: Andrei Homescu Date: Thu, 23 Jul 2026 19:12:01 -0700 Subject: [PATCH 2/2] refactor: reorganize_definitions: fix multi-namespace imports splitting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The split loop now reconstructs the path the retained import was rewritten to and compares target paths with `ast_equiv` instead of comparing parent module NodeIds, so external targets (all `DUMMY_NODE_ID`) are handled correctly. Regression test: `test_reorganize_split_renamed_import` (`tests/snapshots/reorganize_split_renamed_import.rs`). The original two-external-crates scenario cannot be built in the single-file snapshot harness, so the test exercises the equivalent local case the old parent-id comparison also got wrong: a collision-renamed type target (`tick_1`) and a value target (`tick`) in the *same* destination module. Pre-fix output omits the value-namespace `use crate::dest::tick;`. Follow-on bug found while writing the test: the split-import insertion had never actually fired in any test before, and when the synthesized `use` is inserted next to the rewritten retained import *with a comment attached directly above it*, the rewriter panics with "conflicting rewrites" — the sequence splice absorbs the comment into its span and overlaps the item's own path rewrite. The test sidesteps it by keeping comments away from the import; the rewriter/seq-edit interplay still needs a real fix. `:1144-1172`. `match_exports` stores `parent: DUMMY_NODE_ID` for all external replacements (`:673`), and `other_mod_id` falls back to `DUMMY_NODE_ID` (`:1153`). So when a single `use` resolves in two namespaces to targets in two *different* external crates/modules, `other_mod_id != *parent` compares `DUMMY == DUMMY`, concludes "same module", and skips emitting the second import — the retained import is rewritten to the first namespace's path and the other binding is lost. The redundancy check further down explicitly documents this exact pitfall ("DUMMY_NODE_ID represents every external module and cannot prove path equality", `:1286-1295`) but this earlier check doesn't apply the same rule. --- .../src/transform/reorganize_definitions.rs | 44 +++++++++++-------- ...ns-reorganize_split_renamed_import.rs.snap | 1 + 2 files changed, 26 insertions(+), 19 deletions(-) diff --git a/c2rust-refactor/src/transform/reorganize_definitions.rs b/c2rust-refactor/src/transform/reorganize_definitions.rs index 24f8cb9880..843652bfb3 100644 --- a/c2rust-refactor/src/transform/reorganize_definitions.rs +++ b/c2rust-refactor/src/transform/reorganize_definitions.rs @@ -1136,24 +1136,27 @@ impl<'a, 'tcx> Reorganizer<'a, 'tcx> { let mut items = smallvec![]; if let ItemKind::Use(_) = &item.kind { if let Some((path, def_ids)) = multi_namespace_uses.get(&item.id) { - // The retained import uses the first (type/value/ - // macro ordered) resolution. Determine where that - // resolution lives even when it was not remapped. - let other_mod_id = remapped_paths - .get(&item.id) - .map(|&(mod_id, _)| mod_id) - .or_else(|| { - let ldid = def_ids.first()?.1.as_local()?; - let mod_hir_id = - self.cx.ty_ctxt().parent_module_from_def_id(ldid); - Some(self.cx.hir_map().local_def_id_to_node_id(mod_hir_id)) - }) - .unwrap_or(DUMMY_NODE_ID); + // The retained import was rewritten (by the path + // folding above) to the path of its first (type/ + // value/macro ordered) resolution. Reconstruct + // that path and add an import for each remaining + // resolution the retained path does not cover. + // Parent module NodeIds cannot decide coverage: + // DUMMY_NODE_ID stands for every external module, + // so only path equality proves it. + let first_def = def_ids.first().unwrap().1; + let retained_path = match self.path_mapping.get(&first_def) { + Some(replacement) => replacement.path.clone(), + None if is_relative_path(&path) => { + self.cx.def_qpath(first_def).1 + } + None => path.clone(), + }; for &(namespace, def_id) in &def_ids[1..] { if let Some(Replacement { path, parent, .. }) = self.path_mapping.get(&def_id) { - if other_mod_id != *parent { + if !path.ast_equiv(&retained_path) { let new_node_id = self.st.next_node_id(); let inserted = remapped_paths .insert(new_node_id, (*parent, def_id)) @@ -1179,7 +1182,8 @@ impl<'a, 'tcx> Reorganizer<'a, 'tcx> { .cx .hir_map() .local_def_id_to_node_id(mod_hir_id); - if other_mod_id != mod_id { + let target_path = self.cx.def_path(def_id); + if !target_path.ast_equiv(&retained_path) { let new_node_id = self.st.next_node_id(); let inserted = remapped_paths .insert(new_node_id, (mod_id, def_id)) @@ -1189,10 +1193,12 @@ impl<'a, 'tcx> Reorganizer<'a, 'tcx> { new_node_id, smallvec![(namespace, mod_id)], ); - items.push(mk().id(new_node_id).use_simple_item( - self.cx.def_path(def_id), - None::, - )); + items.push( + mk().id(new_node_id).use_simple_item( + target_path, + None::, + ), + ); } } } 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 0ee9b363e1..ab912cabba 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 @@ -53,6 +53,7 @@ pub mod dest { // 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; use crate::dest::tick_1; pub fn make(x: i32) -> crate::dest::tick_1 {