From 9e6f469812d9b740885973dec54cfc1100a104eb Mon Sep 17 00:00:00 2001 From: Andrei Homescu Date: Fri, 24 Jul 2026 12:23:19 -0700 Subject: [PATCH 1/2] refactor: add test for nondeterministic extern block ordering MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Regression test: `test_reorganize_extern_block_order` (`tests/snapshots/reorganize_extern_block_order.rs`). Two `/usr/include` headers are used so both merge into the same `stdlib` destination — that is the only way to get declarations from two headers into one `into_items` batch, since the normal destination heuristic only matches a header against the module sharing its name. One header declares foreign items in two ABIs (`"C"` and `"sysv64"`) so both failure modes show up at once. --- c2rust-refactor/tests/snapshots.rs | 18 +++++ .../reorganize_extern_block_order.rs | 69 +++++++++++++++++++ ...ions-reorganize_extern_block_order.rs.snap | 50 ++++++++++++++ 3 files changed, 137 insertions(+) create mode 100644 c2rust-refactor/tests/snapshots/reorganize_extern_block_order.rs create mode 100644 c2rust-refactor/tests/snapshots/snapshots__refactor-reorganize_definitions-reorganize_extern_block_order.rs.snap diff --git a/c2rust-refactor/tests/snapshots.rs b/c2rust-refactor/tests/snapshots.rs index 031bbe56f5..2766f3eaa7 100644 --- a/c2rust-refactor/tests/snapshots.rs +++ b/c2rust-refactor/tests/snapshots.rs @@ -463,6 +463,24 @@ fn test_reorganize_bitfield_ty() { .test(); } +/// TODO Broken; ignored because it is *nondeterministic*, not because it +/// always fails. Foreign items are grouped into one `extern` block per ABI by +/// iterating a `HashMap`, so the two blocks are emitted in a different +/// order from run to run and the stored snapshot (which records one of the two +/// orders) only matches some of the time. The blocks are also hoisted ahead of +/// every regular item *after* the `BEGIN`/`END` header comments have been +/// assigned in source order, so the comments describe a layout that is not the +/// one emitted: the header comments on the foreign items are lost entirely, +/// and the surviving ones read `END aa_h` / `BEGIN bb_h` across a boundary the +/// hoisted `extern` blocks used to sit on. Un-ignore once `into_items` orders +/// the ABI groups deterministically and assigns the comments after hoisting. +#[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] diff --git a/c2rust-refactor/tests/snapshots/reorganize_extern_block_order.rs b/c2rust-refactor/tests/snapshots/reorganize_extern_block_order.rs new file mode 100644 index 0000000000..dac3a6979b --- /dev/null +++ b/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` 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() {} diff --git a/c2rust-refactor/tests/snapshots/snapshots__refactor-reorganize_definitions-reorganize_extern_block_order.rs.snap b/c2rust-refactor/tests/snapshots/snapshots__refactor-reorganize_definitions-reorganize_extern_block_order.rs.snap new file mode 100644 index 0000000000..4c689fd3af --- /dev/null +++ b/c2rust-refactor/tests/snapshots/snapshots__refactor-reorganize_definitions-reorganize_extern_block_order.rs.snap @@ -0,0 +1,50 @@ +--- +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 { + extern "C" { + pub fn aa_c_fn() -> i32; + pub fn bb_c_fn() -> i32; + } + extern "sysv64" { + + pub fn aa_sysv_fn() -> i32; + } + // =============== 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() {} From e166ab7b5ab79275e042b437a221b6b14f350bc6 Mon Sep 17 00:00:00 2001 From: Andrei Homescu Date: Fri, 24 Jul 2026 12:49:36 -0700 Subject: [PATCH 2/2] refactor: fix nondeterministic ordering for foreign items MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `foreign_items` was a `HashMap>` iterated to build the `extern` blocks, so their order varied run to run. Separately, the `BEGIN/END` header comments were assigned while walking the *sorted interleaved* declaration sequence, but foreign items were then regrouped by ABI and hoisted ahead of all items, so the comments described a layout that was never emitted. `into_items` now keys the map on `(header_ident, Abi)` rather than `Abi` alone, and uses an `IndexMap`, so the groups are emitted in the order they are first encountered — which the sort just above derives deterministically from the header line and `src_loc`. Keying on the header too means every `extern` block belongs to exactly one header, which is what lets a single header comment describe it; previously a block could span several headers, and no comment could have been correct for it. The `BEGIN`/`END` comments are then assigned in a second pass over the final `foreign_mods.chain(items)` sequence, so they describe the layout as emitted. Because the blocks are still hoisted ahead of the regular items, a header can now legitimately be entered, left and entered again in one module; the comments say so. Each `extern` block also needs a `NodeId` to hang its comment on, and comments on the foreign items *inside* a block never reach the output (the block is pretty-printed as one new item). `CommandState::next_node_id` only hands out an unused id, so `NodeMap` never learns the node exists and `add_comment` silently drops anything keyed on it — the warning at `command.rs:485` fires. Added `CommandState::register_new_node_id`, which registers the id the same way parsed nodes are registered, and the transform calls it for each block it creates. --- c2rust-refactor/src/command.rs | 8 +++ .../src/transform/reorganize_definitions.rs | 71 +++++++++++-------- c2rust-refactor/tests/snapshots.rs | 18 ++--- ...ions-reorganize_extern_block_order.rs.snap | 11 ++- ...nitions-reorganize_multi_namespace.rs.snap | 1 + ...reorganize_split_namespace_imports.rs.snap | 1 + ...ns-reorganize_split_renamed_import.rs.snap | 9 ++- ...s__refactor-reorganize_definitions.rs.snap | 3 +- 8 files changed, 74 insertions(+), 48 deletions(-) diff --git a/c2rust-refactor/src/command.rs b/c2rust-refactor/src/command.rs index 746b02965c..fffa488ab6 100644 --- a/c2rust-refactor/src/command.rs +++ b/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(); diff --git a/c2rust-refactor/src/transform/reorganize_definitions.rs b/c2rust-refactor/src/transform/reorganize_definitions.rs index d77b30678a..0c7cf6b735 100644 --- a/c2rust-refactor/src/transform/reorganize_definitions.rs +++ b/c2rust-refactor/src/transform/reorganize_definitions.rs @@ -1987,49 +1987,60 @@ impl<'a, 'tcx> HeaderDeclarations<'a, 'tcx> { } }); - let mut items: Vec> = Vec::new(); - let mut foreign_items: HashMap>> = 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)> = 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>> = 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>( diff --git a/c2rust-refactor/tests/snapshots.rs b/c2rust-refactor/tests/snapshots.rs index 2766f3eaa7..13a603f494 100644 --- a/c2rust-refactor/tests/snapshots.rs +++ b/c2rust-refactor/tests/snapshots.rs @@ -463,17 +463,13 @@ fn test_reorganize_bitfield_ty() { .test(); } -/// TODO Broken; ignored because it is *nondeterministic*, not because it -/// always fails. Foreign items are grouped into one `extern` block per ABI by -/// iterating a `HashMap`, so the two blocks are emitted in a different -/// order from run to run and the stored snapshot (which records one of the two -/// orders) only matches some of the time. The blocks are also hoisted ahead of -/// every regular item *after* the `BEGIN`/`END` header comments have been -/// assigned in source order, so the comments describe a layout that is not the -/// one emitted: the header comments on the foreign items are lost entirely, -/// and the surviving ones read `END aa_h` / `BEGIN bb_h` across a boundary the -/// hoisted `extern` blocks used to sit on. Un-ignore once `into_items` orders -/// the ABI groups deterministically and assigns the comments after hoisting. +/// 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") diff --git a/c2rust-refactor/tests/snapshots/snapshots__refactor-reorganize_definitions-reorganize_extern_block_order.rs.snap b/c2rust-refactor/tests/snapshots/snapshots__refactor-reorganize_definitions-reorganize_extern_block_order.rs.snap index 4c689fd3af..036ee21558 100644 --- a/c2rust-refactor/tests/snapshots/snapshots__refactor-reorganize_definitions-reorganize_extern_block_order.rs.snap +++ b/c2rust-refactor/tests/snapshots/snapshots__refactor-reorganize_definitions-reorganize_extern_block_order.rs.snap @@ -11,14 +11,23 @@ expression: c2rust-refactor reorganize_definitions --rewrite-mode alongside -- t // 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; - pub fn bb_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)] diff --git a/c2rust-refactor/tests/snapshots/snapshots__refactor-reorganize_definitions-reorganize_multi_namespace.rs.snap b/c2rust-refactor/tests/snapshots/snapshots__refactor-reorganize_definitions-reorganize_multi_namespace.rs.snap index c2ec86737f..765411a729 100644 --- a/c2rust-refactor/tests/snapshots/snapshots__refactor-reorganize_definitions-reorganize_multi_namespace.rs.snap +++ b/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; } } diff --git a/c2rust-refactor/tests/snapshots/snapshots__refactor-reorganize_definitions-reorganize_split_namespace_imports.rs.snap b/c2rust-refactor/tests/snapshots/snapshots__refactor-reorganize_definitions-reorganize_split_namespace_imports.rs.snap index d7f4b0cf9e..1dd58a8fd3 100644 --- a/c2rust-refactor/tests/snapshots/snapshots__refactor-reorganize_definitions-reorganize_split_namespace_imports.rs.snap +++ b/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)] 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 c713dfa5dc..93b674eb04 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 @@ -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, } diff --git a/c2rust-refactor/tests/snapshots/snapshots__refactor-reorganize_definitions.rs.snap b/c2rust-refactor/tests/snapshots/snapshots__refactor-reorganize_definitions.rs.snap index a22fe72a23..0914695284 100644 --- a/c2rust-refactor/tests/snapshots/snapshots__refactor-reorganize_definitions.rs.snap +++ b/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;