refactor: fix nondeterministic ordering for foreign items

c2rust-refactor/src/command.rs

@@ -735,6 +735,14 @@ impl CommandState {
         self.node_id_counter.next()
     }
 
+    /// Register a `NodeId` handed out by `next_node_id` as belonging to a node
+    /// this command built itself, the same way ids of parsed nodes are
+    /// registered. Without this the `NodeMap` never learns the node exists, so
+    /// anything keyed on it - `add_comment` in particular - is dropped.
+    pub fn register_new_node_id(&self, id: NodeId) {
+        self.new_parsed_node_ids.borrow_mut().push(id);
+    }
+
     /// Transfer marks on `old` to a fresh NodeId, and return that fresh NodeId.
     pub fn transfer_marks(&self, old: NodeId) -> NodeId {
         let new = self.next_node_id();

c2rust-refactor/src/transform/reorganize_definitions.rs

@@ -1987,49 +1987,60 @@ impl<'a, 'tcx> HeaderDeclarations<'a, 'tcx> {
             }
         });
 
-        let mut items: Vec<P<Item>> = Vec::new();
-        let mut foreign_items: HashMap<Abi, Vec<P<ForeignItem>>> = HashMap::new();
-        let mut last_item_mod = None;
-        let mut last_foreign_item_mod = None;
+        // Each item is paired with the header it came from, so the
+        // `BEGIN`/`END` comments can be assigned once the final order is known.
+        let mut items: Vec<(Ident, P<Item>)> = Vec::new();
+        // Foreign items are grouped by header as well as by ABI: keying on the
+        // ABI alone would produce `extern` blocks spanning several headers,
+        // which no single header comment can describe. An `IndexMap` keeps the
+        // groups in the order they are first encountered, which is derived from
+        // the sort above, instead of in `HashMap` iteration order (which varies
+        // between runs).
+        let mut foreign_items: IndexMap<(Ident, Abi), Vec<P<ForeignItem>>> = IndexMap::new();
         for item in all_items {
             let cur_mod_name = item.parent_header.ident;
             match item.kind {
-                DeclKind::Item(i) => {
-                    if last_item_mod != Some(cur_mod_name) {
-                        st.add_comment(i.id, make_header_comment(last_item_mod, cur_mod_name));
-                        last_item_mod = Some(cur_mod_name);
-                    }
-                    items.push(i);
-                }
+                DeclKind::Item(i) => items.push((cur_mod_name, i)),
                 DeclKind::ForeignItem(fi, abi) => {
-                    if last_foreign_item_mod != Some(cur_mod_name) {
-                        st.add_comment(
-                            fi.id,
-                            make_header_comment(last_foreign_item_mod, cur_mod_name),
-                        );
-                        last_foreign_item_mod = Some(cur_mod_name);
-                    }
-                    foreign_items.entry(abi).or_default().push(fi);
+                    foreign_items
+                        .entry((cur_mod_name, abi))
+                        .or_default()
+                        .push(fi);
                 }
             }
 
             // If there is an impl item, add it now.
             if let Some(r#impl) = item.r#impl {
-                let cur_mod_name = r#impl.parent_header.ident;
-                let i = r#impl.item;
-                if last_item_mod != Some(cur_mod_name) {
-                    st.add_comment(i.id, make_header_comment(last_item_mod, cur_mod_name));
-                    last_item_mod = Some(cur_mod_name);
-                }
-                items.push(i);
+                items.push((r#impl.parent_header.ident, r#impl.item));
             }
         }
 
-        let foreign_mods = foreign_items
-            .into_iter()
-            .map(|(abi, items)| mk().extern_(abi).foreign_items(items));
+        let foreign_mods = foreign_items.into_iter().map(|((header, abi), items)| {
+            // A fresh, registered `NodeId` so the header comment below has
+            // something to attach to; comments on the foreign items inside the
+            // block never reach the output, since the whole block is
+            // pretty-printed as one new item.
+            let id = st.next_node_id();
+            st.register_new_node_id(id);
+            (header, mk().id(id).extern_(abi).foreign_items(items))
+        });
 
-        foreign_mods.chain(items.into_iter()).collect()
+        // Assign the header comments while walking the items in the order they
+        // are emitted. The `extern` blocks are hoisted ahead of every other
+        // item, so a header can be entered, left, and entered again in the
+        // final layout; walking the declarations in their pre-hoisting order
+        // instead would describe a layout that never gets written out.
+        let mut last_mod = None;
+        foreign_mods
+            .chain(items)
+            .map(|(header, item)| {
+                if last_mod != Some(header) {
+                    st.add_comment(item.id, make_header_comment(last_mod, header));
+                    last_mod = Some(header);
+                }
+                item
+            })
+            .collect()
     }
 
     fn find_item<'b>(

c2rust-refactor/tests/snapshots.rs

@@ -463,6 +463,20 @@ fn test_reorganize_bitfield_ty() {
         .test();
 }
 
+/// Foreign items are grouped into `extern` blocks by header and ABI, and the
+/// groups are emitted in the order they are first encountered rather than in
+/// `HashMap` iteration order, so the output is the same from run to run. The
+/// `BEGIN`/`END` header comments are assigned over the final item sequence, so
+/// they describe the layout that is actually emitted: `aa_h` is entered twice
+/// here, once for its `extern` blocks and again for its struct, because the
+/// blocks are hoisted ahead of every regular item.
+#[test]
+fn test_reorganize_extern_block_order() {
+    refactor("reorganize_definitions")
+        .named("reorganize_extern_block_order.rs")
+        .test();
+}
+
 /// Two foreign declarations of the same function that differ in the number
 /// of parameters are not interchangeable and must not be merged.
 #[test]

c2rust-refactor/tests/snapshots/reorganize_extern_block_order.rs

@@ -0,0 +1,69 @@
+#![feature(register_tool)]
+#![register_tool(c2rust)]
+#![allow(non_camel_case_types)]
+#![allow(dead_code)]
+
+// The destination all the standard library headers below are merged into.
+// It has to already exist for the `BEGIN`/`END` header comments to make it
+// into the output.
+pub mod stdlib {
+    pub const STDLIB_MARKER: i32 = 0;
+}
+
+pub mod api {
+    // Both headers are under `/usr/include`, so they are treated as standard
+    // library headers and all of their declarations land in the same `stdlib`
+    // destination module. Each header mixes a regular item with foreign items,
+    // and `aa.h` declares two different ABIs.
+    //
+    // `into_items` groups the foreign items by ABI into one `extern` block
+    // each and hoists every block ahead of every regular item, so:
+    //
+    // - the relative order of the two `extern` blocks comes from iterating a
+    //   `HashMap<Abi, _>` and varies from run to run, and
+    // - the `BEGIN`/`END` header comments are assigned while walking the
+    //   declarations in source order, before the hoisting, so they describe a
+    //   layout that isn't the one that gets emitted.
+    #[c2rust::header_src = "/usr/include/aa.h:1"]
+    pub mod aa_h {
+        #[c2rust::src_loc = "2:0"]
+        #[derive(Copy, Clone)]
+        #[repr(C)]
+        pub struct aa_ty {
+            pub x: i32,
+        }
+
+        extern "C" {
+            #[c2rust::src_loc = "3:0"]
+            pub fn aa_c_fn() -> i32;
+        }
+
+        extern "sysv64" {
+            #[c2rust::src_loc = "4:0"]
+            pub fn aa_sysv_fn() -> i32;
+        }
+    }
+
+    #[c2rust::header_src = "/usr/include/bb.h:5"]
+    pub mod bb_h {
+        extern "C" {
+            #[c2rust::src_loc = "6:0"]
+            pub fn bb_c_fn() -> i32;
+        }
+
+        #[c2rust::src_loc = "7:0"]
+        #[derive(Copy, Clone)]
+        #[repr(C)]
+        pub struct bb_ty {
+            pub y: i32,
+        }
+    }
+
+    pub unsafe fn go() -> i32 {
+        let a = aa_h::aa_ty { x: 1 };
+        let b = bb_h::bb_ty { y: 2 };
+        a.x + b.y + aa_h::aa_c_fn() + aa_h::aa_sysv_fn() + bb_h::bb_c_fn()
+    }
+}
+
+fn main() {}

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

@@ -0,0 +1,59 @@
+---
+source: c2rust-refactor/tests/snapshots.rs
+expression: c2rust-refactor reorganize_definitions --rewrite-mode alongside -- tests/snapshots/reorganize_extern_block_order.rs --edition 2021
+---
+#![feature(register_tool)]
+#![register_tool(c2rust)]
+#![allow(non_camel_case_types)]
+#![allow(dead_code)]
+
+// The destination all the standard library headers below are merged into.
+// It has to already exist for the `BEGIN`/`END` header comments to make it
+// into the output.
+pub mod stdlib {
+
+    // =============== BEGIN aa_h ================
+    extern "C" {
+
+        pub fn aa_c_fn() -> i32;
+    }
+    extern "sysv64" {
+
+        pub fn aa_sysv_fn() -> i32;
+    }
+    // ================ END aa_h ================
+    // =============== BEGIN bb_h ================
+    extern "C" {
+
+        pub fn bb_c_fn() -> i32;
+    }
+    // ================ END bb_h ================
+    // =============== BEGIN aa_h ================
+    #[derive(Copy, Clone)]
+    #[repr(C)]
+    pub struct aa_ty {
+        pub x: i32,
+    }
+    // ================ END aa_h ================
+    // =============== BEGIN bb_h ================
+    #[derive(Copy, Clone)]
+    #[repr(C)]
+    pub struct bb_ty {
+        pub y: i32,
+    }
+    pub const STDLIB_MARKER: i32 = 0;
+}
+
+pub mod api {
+
+    pub unsafe fn go() -> i32 {
+        let a = crate::stdlib::aa_ty { x: 1 };
+        let b = crate::stdlib::bb_ty { y: 2 };
+        a.x + b.y
+            + crate::stdlib::aa_c_fn()
+            + crate::stdlib::aa_sysv_fn()
+            + crate::stdlib::bb_c_fn()
+    }
+}
+
+fn main() {}

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

@@ -10,6 +10,7 @@ expression: c2rust-refactor reorganize_definitions --rewrite-mode alongside -- t
 
 pub mod dest_h {
     extern "C" {
+
         pub fn tick(x: i32) -> i32;
     }
 }

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

@@ -10,6 +10,7 @@ expression: c2rust-refactor reorganize_definitions --rewrite-mode alongside -- t
 
 pub mod stdlib {
     extern "C" {
+
         pub fn item(v: *const crate::stdlib::item) -> i32;
     }
     #[derive(Copy, Clone)]

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

@@ -27,20 +27,19 @@ pub mod other {
 }
 
 pub mod dest {
+
+    // =============== BEGIN dest_h ================
     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,
     }

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

@@ -28,14 +28,15 @@ type outside = i32;
 
 pub mod bar {
 
+    // =============== BEGIN bar_h ================
     extern "C" {
+
         pub fn statvfs(path: *const libc::c_char, buf: *mut crate::bar::statvfs) -> libc::c_int;
 
         #[link_name = "statvfs"]
         pub fn statvfs_1(path: *const libc::c_char, buf: *mut crate::bar::statvfs_1)
             -> libc::c_int;
     }
-    // =============== BEGIN bar_h ================
 
     // Test relative paths
     use crate::outside;