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.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..ab912cabba --- /dev/null +++ b/c2rust-refactor/tests/snapshots/snapshots__refactor-reorganize_definitions-reorganize_split_renamed_import.rs.snap @@ -0,0 +1,68 @@ +--- +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; + 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() {}