refactor: widen definitions that header declarations are matched to

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<Ident, usize>,
+
+    // 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<NodeId>,
 }
 
 #[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<Item>| {
+            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) {

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]
@@ -558,6 +556,16 @@ 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")
+        .test();
+}
+
 #[test]
 fn test_reorganize_self_import_destination() {
     refactor("reorganize_definitions")

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

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]
+
+    pub(crate) 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() {}

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,