refactor: reorganize_definitions: fix multi-namespace imports splitting

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::<String>,
-                                                ));
+                                                items.push(
+                                                    mk().id(new_node_id).use_simple_item(
+                                                        target_path,
+                                                        None::<String>,
+                                                    ),
+                                                );
                                             }
                                         }
                                     }

c2rust-refactor/tests/snapshots.rs

@@ -532,6 +532,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();

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() {}

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() {}