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 031bbe56f5..13a603f494 100644 --- a/c2rust-refactor/tests/snapshots.rs +++ 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] 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..036ee21558 --- /dev/null +++ b/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() {} 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;