From b1523470aa1aef9d67cc433b4249cc81d5140bdd Mon Sep 17 00:00:00 2001 From: Andrei Homescu Date: Fri, 24 Jul 2026 16:44:53 -0700 Subject: [PATCH 1/2] refactor: add test for redirecting paths to a private no-mangle definition MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Regression test: `test_reorganize_private_no_mangle_def` (`tests/snapshots/reorganize_private_no_mangle_def.rs`). `def::counter` carries `#[no_mangle]` but no `pub`, and `user` includes a header declaring it. `match_defs` only considers definitions that `is_exported` accepts, which for a value means it carries `#[no_mangle]` or `#[export_name]` — a statement about the C symbol being visible to another translation unit, not about Rust visibility. So the declaration is matched to the definition and `user::read` is rewritten to `crate::def::counter`, naming an item that is private to `def`: error[E0603]: static `counter` is private The recorded output shows the rewritten path next to the still-private definition, with `new_expect_compile_error` marking the state as broken. This is the second reason the output of `test_reorganize_definitions` does not compile, reduced to one definition and one reference. --- c2rust-refactor/tests/snapshots.rs | 11 +++++ .../reorganize_private_no_mangle_def.rs | 43 +++++++++++++++++++ ...s-reorganize_private_no_mangle_def.rs.snap | 40 +++++++++++++++++ 3 files changed, 94 insertions(+) create mode 100644 c2rust-refactor/tests/snapshots/reorganize_private_no_mangle_def.rs create mode 100644 c2rust-refactor/tests/snapshots/snapshots__refactor-reorganize_definitions-reorganize_private_no_mangle_def.rs.snap diff --git a/c2rust-refactor/tests/snapshots.rs b/c2rust-refactor/tests/snapshots.rs index 13a603f494..5becffba64 100644 --- a/c2rust-refactor/tests/snapshots.rs +++ b/c2rust-refactor/tests/snapshots.rs @@ -549,6 +549,17 @@ fn test_reorganize_non_ascii_ident() { .test(); } +/// A definition that a header declaration is matched to has to be nameable +/// from the modules whose paths are rewritten to point at it, even when it +/// is only "exported" in the sense of carrying `#[no_mangle]`. +#[test] +fn test_reorganize_private_no_mangle_def() { + refactor("reorganize_definitions") + .named("reorganize_private_no_mangle_def.rs") + .new_expect_compile_error(true) + .test(); +} + #[test] fn test_reorganize_self_import_destination() { refactor("reorganize_definitions") diff --git a/c2rust-refactor/tests/snapshots/reorganize_private_no_mangle_def.rs b/c2rust-refactor/tests/snapshots/reorganize_private_no_mangle_def.rs new file mode 100644 index 0000000000..c940683f93 --- /dev/null +++ b/c2rust-refactor/tests/snapshots/reorganize_private_no_mangle_def.rs @@ -0,0 +1,43 @@ +#![feature(rustc_private)] +#![feature(register_tool)] +#![register_tool(c2rust)] +#![allow(non_upper_case_globals)] +#![allow(non_camel_case_types)] +#![allow(dead_code)] + +extern crate libc; + +// `counter` carries `#[no_mangle]`, so it is visible to the linker from +// another translation unit, but it is private to its module in Rust terms. +// The header declaration in `user` is matched to it and every path to the +// declaration is rewritten to point here, from outside this module. + +pub mod def { + use libc; + + #[no_mangle] + #[c2rust::src_loc = "10:0"] + static mut counter: libc::c_int = 0; +} + +pub mod user { + use libc; + + #[c2rust::header_src = "/home/user/some/workspace/counter.h:1"] + pub mod counter_h { + use super::libc; + + extern "C" { + #[c2rust::src_loc = "2:0"] + pub static mut counter: libc::c_int; + } + } + + use counter_h::counter; + + pub unsafe fn read() -> libc::c_int { + counter + } +} + +fn main() {} diff --git a/c2rust-refactor/tests/snapshots/snapshots__refactor-reorganize_definitions-reorganize_private_no_mangle_def.rs.snap b/c2rust-refactor/tests/snapshots/snapshots__refactor-reorganize_definitions-reorganize_private_no_mangle_def.rs.snap new file mode 100644 index 0000000000..51f55424aa --- /dev/null +++ b/c2rust-refactor/tests/snapshots/snapshots__refactor-reorganize_definitions-reorganize_private_no_mangle_def.rs.snap @@ -0,0 +1,40 @@ +--- +source: c2rust-refactor/tests/snapshots.rs +expression: c2rust-refactor reorganize_definitions --rewrite-mode alongside -- tests/snapshots/reorganize_private_no_mangle_def.rs --edition 2021 +--- +#![feature(rustc_private)] +#![feature(register_tool)] +#![register_tool(c2rust)] +#![allow(non_upper_case_globals)] +#![allow(non_camel_case_types)] +#![allow(dead_code)] + +pub mod counter_h { + use ::libc; +} +extern crate libc; + +// `counter` carries `#[no_mangle]`, so it is visible to the linker from +// another translation unit, but it is private to its module in Rust terms. +// The header declaration in `user` is matched to it and every path to the +// declaration is rewritten to point here, from outside this module. + +pub mod def { + use libc; + + #[no_mangle] + + static mut counter: libc::c_int = 0; +} + +pub mod user { + use libc; + + use crate::def::counter; + + pub unsafe fn read() -> libc::c_int { + crate::def::counter + } +} + +fn main() {} From 8498d9bc3eb4c2db86f6232553c77835af8409bd Mon Sep 17 00:00:00 2001 From: Andrei Homescu Date: Fri, 24 Jul 2026 16:48:11 -0700 Subject: [PATCH 2/2] refactor: widen definitions that header declarations are matched to MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `match_defs` replaces a header declaration with a definition found elsewhere in the crate and rewrites every path to the declaration to point at that definition. It only accepts a definition `is_exported` agrees is exported, but for a value that asks whether the item carries `#[no_mangle]` or `#[export_name]` — whether the C symbol reaches another translation unit, which says nothing about Rust visibility. A `#[no_mangle]` static private to its module qualifies, and the paths rewritten to name it come from other modules, which cannot: error[E0603]: static `counter` is private `match_defs` now records each matched definition that is not `pub`, and `widen_matched_defs` joins its visibility with `pub(crate)` — enough because every path that now points at it is in this crate. This is the pass the commented-out `imports` field on `HeaderDeclarations` anticipated ("must be made pub(crate) at least"). With this and the previous commit, the output of `test_reorganize_definitions` compiles for the first time, so its `new_expect_compile_error` is dropped too; the only change to that snapshot here is `Bar` gaining `pub(crate)`. --- .../src/transform/reorganize_definitions.rs | 39 +++++++++++++++++++ c2rust-refactor/tests/snapshots.rs | 5 +-- ...s-reorganize_private_no_mangle_def.rs.snap | 2 +- ...s__refactor-reorganize_definitions.rs.snap | 2 +- 4 files changed, 42 insertions(+), 6 deletions(-) diff --git a/c2rust-refactor/src/transform/reorganize_definitions.rs b/c2rust-refactor/src/transform/reorganize_definitions.rs index 0c7cf6b735..2cd7537cb0 100644 --- a/c2rust-refactor/src/transform/reorganize_definitions.rs +++ b/c2rust-refactor/src/transform/reorganize_definitions.rs @@ -72,6 +72,14 @@ pub struct Reorganizer<'a, 'tcx: 'a> { // Counter used by `unique_ident` ident_counter: HashMap, + + // Definitions that `match_defs` redirected a header declaration to, and + // which are not `pub`. `is_exported` accepts a `#[no_mangle]` value + // whatever its Rust visibility, because that is what decides whether the + // C symbol is visible to another translation unit; but the paths that now + // point at the definition come from other modules, which a private item + // cannot be named from. `widen_matched_defs` widens these. + widened_defs: HashSet, } #[derive(Clone)] @@ -124,6 +132,7 @@ impl<'a, 'tcx> Reorganizer<'a, 'tcx> { bitfield_ty_defs: HashMap::new(), stdlib_id: DUMMY_NODE_ID, ident_counter: HashMap::new(), + widened_defs: HashSet::new(), } } @@ -135,6 +144,7 @@ impl<'a, 'tcx> Reorganizer<'a, 'tcx> { let mut header_decls = self.remove_header_items(krate); self.match_defs(&mut header_decls, krate); + self.widen_matched_defs(krate); self.update_module_info_items(krate); self.move_items(header_decls, krate); @@ -518,6 +528,9 @@ impl<'a, 'tcx> Reorganizer<'a, 'tcx> { DeclKind::ForeignItem(foreign, _) => foreign_equiv(&foreign, item), }); if !decl_ids.is_empty() { + if !item.vis.kind.is_pub() { + self.widened_defs.insert(item.id); + } let def_id = self.cx.node_def_id(item.id); let dest_path = self.cx.def_path(def_id); let ldid = def_id.expect_local(); @@ -680,6 +693,32 @@ impl<'a, 'tcx> Reorganizer<'a, 'tcx> { } } + /// Widen the visibility of the definitions `match_defs` redirected header + /// declarations to, so that the rewritten paths can name them. + /// + /// `match_defs` accepts a definition that `is_exported` calls exported, + /// which for a value means it carries `#[no_mangle]` or `#[export_name]` — + /// a statement about the C symbol, not about Rust visibility. Such a + /// definition can be private to its module while the declarations it + /// replaces were used from other modules, whose paths now point at it. + fn widen_matched_defs(&self, krate: &mut Crate) { + if self.widened_defs.is_empty() { + return; + } + // The users are all in this crate, so `pub(crate)` is enough. + let crate_vis = VisibilityKind::Restricted { + path: P(Path::from_ident(Ident::new(kw::Crate, DUMMY_SP))), + id: DUMMY_NODE_ID, + shorthand: true, + }; + FlatMapNodes::visit(krate, |mut item: P| { + if self.widened_defs.contains(&item.id) { + item.vis.kind = join_visibility(&item.vis.kind, &crate_vis); + } + smallvec![item] + }); + } + /// Update items set in ModuleInfos with current remaining items in that /// module so that we don't override an existing item fn update_module_info_items(&mut self, krate: &Crate) { diff --git a/c2rust-refactor/tests/snapshots.rs b/c2rust-refactor/tests/snapshots.rs index 5becffba64..bb5c947169 100644 --- a/c2rust-refactor/tests/snapshots.rs +++ b/c2rust-refactor/tests/snapshots.rs @@ -440,9 +440,7 @@ fn test_reorder_derives() { #[cfg(target_os = "linux")] // `statvfs` and `statfs64` are Linux only. #[test] fn test_reorganize_definitions() { - refactor("reorganize_definitions") - .new_expect_compile_error(true) - .test(); + refactor("reorganize_definitions").test(); } #[test] @@ -556,7 +554,6 @@ fn test_reorganize_non_ascii_ident() { fn test_reorganize_private_no_mangle_def() { refactor("reorganize_definitions") .named("reorganize_private_no_mangle_def.rs") - .new_expect_compile_error(true) .test(); } diff --git a/c2rust-refactor/tests/snapshots/snapshots__refactor-reorganize_definitions-reorganize_private_no_mangle_def.rs.snap b/c2rust-refactor/tests/snapshots/snapshots__refactor-reorganize_definitions-reorganize_private_no_mangle_def.rs.snap index 51f55424aa..fef10c7825 100644 --- a/c2rust-refactor/tests/snapshots/snapshots__refactor-reorganize_definitions-reorganize_private_no_mangle_def.rs.snap +++ b/c2rust-refactor/tests/snapshots/snapshots__refactor-reorganize_definitions-reorganize_private_no_mangle_def.rs.snap @@ -24,7 +24,7 @@ pub mod def { #[no_mangle] - static mut counter: libc::c_int = 0; + pub(crate) static mut counter: libc::c_int = 0; } pub mod user { 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 0914695284..ac996c745e 100644 --- a/c2rust-refactor/tests/snapshots/snapshots__refactor-reorganize_definitions.rs.snap +++ b/c2rust-refactor/tests/snapshots/snapshots__refactor-reorganize_definitions.rs.snap @@ -96,7 +96,7 @@ pub mod bar { type FooInt = i32; #[no_mangle] - static mut Bar: crate::bar::bar_t = crate::bar::bar_t { + pub(crate) static mut Bar: crate::bar::bar_t = crate::bar::bar_t { alloc: 0 as *mut libc::c_char, data: 0 as *mut libc::c_char, i: 0,