From f641390b71e356c8ceb338ffd0c555353b5e1f77 Mon Sep 17 00:00:00 2001 From: Andrei Homescu Date: Fri, 24 Jul 2026 16:41:12 -0700 Subject: [PATCH 1/3] refactor: add test for foreign declarations merged on spelling alone Add a regression test for a bug where foreign function declarations get merged based purely on how their types are spelled. Two translation units each define their own struct named `buf`, and since the structs differ, the transform correctly renames the second one to `buf_1`. But both units also declare `fill(b: *mut buf)`. When `find_foreign_item` compares these declarations via `compatible_fn_prototypes`, it cannot resolve the types semantically (foreign items have no body, hence no typeck results), so it falls back to a syntactic comparison: `*mut buf` vs `*mut buf` looks identical, even though each refers to a different struct. The duplicate declaration is dropped and all callers are pointed at the first one, so code written against `buf_1` ends up passing it to a function expecting `*mut buf`: error[E0308]: mismatched types --- c2rust-refactor/tests/snapshots.rs | 10 +++ .../reorganize_foreign_fn_type_identity.rs | 74 +++++++++++++++++++ ...eorganize_foreign_fn_type_identity.rs.snap | 65 ++++++++++++++++ 3 files changed, 149 insertions(+) create mode 100644 c2rust-refactor/tests/snapshots/reorganize_foreign_fn_type_identity.rs create mode 100644 c2rust-refactor/tests/snapshots/snapshots__refactor-reorganize_definitions-reorganize_foreign_fn_type_identity.rs.snap diff --git a/c2rust-refactor/tests/snapshots.rs b/c2rust-refactor/tests/snapshots.rs index 3c4f5b6671..1862f3683e 100644 --- a/c2rust-refactor/tests/snapshots.rs +++ b/c2rust-refactor/tests/snapshots.rs @@ -472,6 +472,16 @@ fn test_reorganize_foreign_fn_arity() { .test(); } +/// Two foreign declarations whose parameters are spelled the same but name +/// different types describe different functions and must not be merged. +#[test] +fn test_reorganize_foreign_fn_type_identity() { + refactor("reorganize_definitions") + .named("reorganize_foreign_fn_type_identity.rs") + .new_expect_compile_error(true) + .test(); +} + /// A foreign item, `static` or `fn`, that is renamed to avoid a collision must /// keep naming the symbol it linked against before the rename. #[test] diff --git a/c2rust-refactor/tests/snapshots/reorganize_foreign_fn_type_identity.rs b/c2rust-refactor/tests/snapshots/reorganize_foreign_fn_type_identity.rs new file mode 100644 index 0000000000..baa0ef1c54 --- /dev/null +++ b/c2rust-refactor/tests/snapshots/reorganize_foreign_fn_type_identity.rs @@ -0,0 +1,74 @@ +#![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; + +// Each translation unit has its own `buf`, and the two differ in field +// visibility, so they are not unified and the second is renamed. Both units +// declare `fill` taking a `*mut buf` — spelled identically, but naming a +// different type in each unit. The two declarations therefore describe +// different functions and must not be collapsed into one. + +pub mod a { + use libc; + + #[c2rust::header_src = "/home/user/some/workspace/io.h:1"] + pub mod io_h { + use super::libc; + + #[repr(C)] + #[c2rust::src_loc = "2:0"] + pub struct buf { + pub len: libc::c_int, + pad: libc::c_int, + } + + extern "C" { + #[c2rust::src_loc = "3:0"] + pub fn fill(b: *mut buf) -> libc::c_int; + } + } + + use io_h::{buf, fill}; + + pub unsafe fn run() -> libc::c_int { + let mut b = std::mem::zeroed::(); + fill(&mut b) + } +} + +pub mod b { + use libc; + + #[c2rust::header_src = "/home/user/some/workspace/io.h:1"] + pub mod io_h { + use super::libc; + + // Same fields as the other `buf`, but all of them are public, so the + // two are not interchangeable. + #[repr(C)] + #[c2rust::src_loc = "2:0"] + pub struct buf { + pub len: libc::c_int, + pub pad: libc::c_int, + } + + extern "C" { + #[c2rust::src_loc = "3:0"] + pub fn fill(b: *mut buf) -> libc::c_int; + } + } + + use io_h::{buf, fill}; + + pub unsafe fn run() -> libc::c_int { + let mut b = std::mem::zeroed::(); + fill(&mut b) + } +} + +fn main() {} diff --git a/c2rust-refactor/tests/snapshots/snapshots__refactor-reorganize_definitions-reorganize_foreign_fn_type_identity.rs.snap b/c2rust-refactor/tests/snapshots/snapshots__refactor-reorganize_definitions-reorganize_foreign_fn_type_identity.rs.snap new file mode 100644 index 0000000000..292dbc6755 --- /dev/null +++ b/c2rust-refactor/tests/snapshots/snapshots__refactor-reorganize_definitions-reorganize_foreign_fn_type_identity.rs.snap @@ -0,0 +1,65 @@ +--- +source: c2rust-refactor/tests/snapshots.rs +expression: c2rust-refactor reorganize_definitions --rewrite-mode alongside -- tests/snapshots/reorganize_foreign_fn_type_identity.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 io_h { + extern "C" { + pub fn fill(b: *mut crate::io_h::buf) -> libc::c_int; + } + use ::libc; + + #[repr(C)] + + pub struct buf { + pub len: libc::c_int, + pad: libc::c_int, + } + // Same fields as the other `buf`, but all of them are public, so the + // two are not interchangeable. + #[repr(C)] + + pub struct buf_1 { + pub len: libc::c_int, + pub pad: libc::c_int, + } +} +extern crate libc; + +// Each translation unit has its own `buf`, and the two differ in field +// visibility, so they are not unified and the second is renamed. Both units +// declare `fill` taking a `*mut buf` — spelled identically, but naming a +// different type in each unit. The two declarations therefore describe +// different functions and must not be collapsed into one. + +pub mod a { + use libc; + + use crate::io_h::buf; + use crate::io_h::fill; + + pub unsafe fn run() -> libc::c_int { + let mut b = std::mem::zeroed::(); + crate::io_h::fill(&mut b) + } +} + +pub mod b { + use libc; + + use crate::io_h::buf_1; + use crate::io_h::fill; + + pub unsafe fn run() -> libc::c_int { + let mut b = std::mem::zeroed::(); + crate::io_h::fill(&mut b) + } +} + +fn main() {} From a973d8770847d1f5eaf7e255210766b87522b6c3 Mon Sep 17 00:00:00 2001 From: Andrei Homescu Date: Fri, 24 Jul 2026 23:42:00 -0700 Subject: [PATCH 2/3] refactor: compare array lengths exactly in function signatures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Type comparison lets a zero-length array match an array of any length. That leniency exists so an `extern` array declaration, whose omitted C length we translate as 0, can match its definition — but it also leaked into function signature comparison, where `*mut [c_int; 0]` and `*mut [c_int; 4]` are distinct, non-coercible types. As a result, `test_reorganize_foreign_fn_rename` merged two different `compute` declarations and rewrote a call to pass an argument of the wrong type. Add an `exact_array_lens` flag to `TypeCompare`: off by default so extern arrays still match their definitions, and turned on by `compatible_fn_sigs` and `compatible_fn_prototypes` so array lengths anywhere in a signature must match exactly. --- c2rust-refactor/src/context.rs | 41 +++++++++++++++++++++++++++++----- 1 file changed, 36 insertions(+), 5 deletions(-) diff --git a/c2rust-refactor/src/context.rs b/c2rust-refactor/src/context.rs index c4db085547..e7758c6149 100644 --- a/c2rust-refactor/src/context.rs +++ b/c2rust-refactor/src/context.rs @@ -1147,12 +1147,17 @@ pub struct CalleeInfo<'tcx> { type DefMapping = HashMap; +#[derive(Clone, Copy)] pub struct TypeCompare<'a, 'tcx: 'a, 'b> { cx: &'a RefactorCtxt<'a, 'tcx>, /// Mapping from old DefId to new DefId for defs that have been replaced /// after types were resolved. def_mapping: Option<&'b DefMapping>, + + /// Require array lengths to match exactly, instead of letting a + /// zero-length array match an array of any length. + exact_array_lens: bool, } impl<'a, 'tcx, 'b> TypeCompare<'a, 'tcx, 'b> { @@ -1160,6 +1165,7 @@ impl<'a, 'tcx, 'b> TypeCompare<'a, 'tcx, 'b> { Self { cx, def_mapping: None, + exact_array_lens: false, } } @@ -1167,6 +1173,21 @@ impl<'a, 'tcx, 'b> TypeCompare<'a, 'tcx, 'b> { Self { cx, def_mapping: Some(def_mapping), + exact_array_lens: false, + } + } + + /// Return a copy of this comparison that requires array lengths to match + /// exactly. + /// + /// The zero-length leniency exists for an `extern` array declaration, + /// which C lets omit the length that its definition gives. Nothing else + /// makes a zero-length array interchangeable with a longer one: they are + /// distinct Rust types, and Rust will not coerce between them. + fn with_exact_array_lens(self) -> Self { + Self { + exact_array_lens: true, + ..self } } @@ -1378,6 +1399,11 @@ impl<'a, 'tcx, 'b> TypeCompare<'a, 'tcx, 'b> { /// Compare two function declarations for equivalent argument and return types, /// ignoring argument names. pub fn compatible_fn_prototypes(&self, decl1: &FnDecl, decl2: &FnDecl) -> bool { + // A parameter or return type is passed by value at every call site, so + // the two signatures have to agree on it exactly; the zero-length + // array leniency only holds for an `extern` array declaration. + let strict_cmp = self.with_exact_array_lens(); + // `zip` below stops at the shorter parameter list, so the lengths have // to be compared separately. Otherwise a declaration is compatible // with any other one that merely extends it, which is exactly the @@ -1389,7 +1415,7 @@ impl<'a, 'tcx, 'b> TypeCompare<'a, 'tcx, 'b> { } let mut args = decl1.inputs.iter().zip(decl2.inputs.iter()); - if !args.all(|(arg1, arg2)| self.structural_eq_ast_tys(&arg1.ty, &arg2.ty, true)) { + if !args.all(|(arg1, arg2)| strict_cmp.structural_eq_ast_tys(&arg1.ty, &arg2.ty, true)) { return false; } @@ -1405,12 +1431,16 @@ impl<'a, 'tcx, 'b> TypeCompare<'a, 'tcx, 'b> { FnRetTy::Ty(ty) => &ty, }; - self.structural_eq_ast_tys(ty1, ty2, true) + strict_cmp.structural_eq_ast_tys(ty1, ty2, true) } /// Compare two ty function signatures for equivalent argument and return /// types, ignoring argument names. pub fn compatible_fn_sigs(&self, sig1: &FnSig<'tcx>, sig2: &FnSig<'tcx>) -> bool { + // See `compatible_fn_prototypes` for why a signature compares array + // lengths exactly. + let strict_cmp = self.with_exact_array_lens(); + if sig1.inputs().len() != sig2.inputs().len() { return false; } @@ -1420,14 +1450,14 @@ impl<'a, 'tcx, 'b> TypeCompare<'a, 'tcx, 'b> { } for (&arg_ty1, &arg_ty2) in sig1.inputs().iter().zip(sig2.inputs().iter()) { - if !self.structural_eq_tys_with_vis(arg_ty1, arg_ty2) { + if !strict_cmp.structural_eq_tys_with_vis(arg_ty1, arg_ty2) { return false; } } let out_ty1 = sig1.output(); let out_ty2 = sig2.output(); - self.structural_eq_tys_with_vis(out_ty1, out_ty2) + strict_cmp.structural_eq_tys_with_vis(out_ty1, out_ty2) } /// Compare two AST types for structural equivalence, ignoring names. @@ -1559,7 +1589,8 @@ impl<'a, 'tcx, 'b> TypeCompare<'a, 'tcx, 'b> { // array types with global array definitions, but it should be // apply in practice as we translate empty extern array lengths // into 0 length extern arrays. - if len1 != len2 && len1 != Some(0) && len2 != Some(0) { + let lenient = !self.exact_array_lens && (len1 == Some(0) || len2 == Some(0)); + if len1 != len2 && !lenient { trace!("Array lengths don't match: {:?} and {:?}", n1, n2); return false; } From b19824ceb9f20b663434811f02049b659e16b2ef Mon Sep 17 00:00:00 2001 From: Andrei Homescu Date: Fri, 24 Jul 2026 16:43:19 -0700 Subject: [PATCH 3/3] refactor: compare foreign function declarations by resolved type MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `find_foreign_item` compared foreign functions via `compatible_fn_prototypes`, which resolves parameters through `opt_node_type` — but foreign items have no typeck results, so declarations from different modules that were merely spelled alike merged, and callers of the dropped one were rewritten to a function taking a different type. Compare with `compatible_fn_sigs` instead, matching what `match_exports` already does for external functions. The syntactic path remains as a fallback for signatures that will not resolve, which is why the earlier arity check still matters. --- .../src/transform/reorganize_definitions.rs | 45 ++++++++++++++++++- c2rust-refactor/tests/snapshots.rs | 1 - ...eorganize_foreign_fn_type_identity.rs.snap | 7 ++- ...s__refactor-reorganize_definitions.rs.snap | 6 ++- 4 files changed, 54 insertions(+), 5 deletions(-) diff --git a/c2rust-refactor/src/transform/reorganize_definitions.rs b/c2rust-refactor/src/transform/reorganize_definitions.rs index df617c506c..e64f73b6fc 100644 --- a/c2rust-refactor/src/transform/reorganize_definitions.rs +++ b/c2rust-refactor/src/transform/reorganize_definitions.rs @@ -2180,7 +2180,13 @@ impl<'a, 'tcx> HeaderDeclarations<'a, 'tcx> { ( ForeignItemKind::Fn(box Fn { sig: sig1, .. }), ForeignItemKind::Fn(box Fn { sig: sig2, .. }), - ) => self.cx.compatible_fn_prototypes(&sig1.decl, &sig2.decl), + ) => compatible_foreign_fns( + self.cx, + existing_foreign, + item, + &sig1.decl, + &sig2.decl, + ), _ => existing_foreign.ast_equiv(item), }; @@ -2210,6 +2216,43 @@ enum ContainsDecl<'a> { Use(&'a mut MovedDecl), } +// Returns `true` if two foreign function declarations declare the same +// function, and so may be collapsed into one. +// +// The two declarations come from different modules, so comparing their +// written-out signatures is not enough: a parameter spelled `*mut stat` in +// each can name a different `stat` in each module, and merging on that basis +// silently repoints one module's calls at the other module's type. A foreign +// item has no body, so its signature carries no node types for +// `compatible_fn_prototypes` to compare, and it falls back to comparing the +// syntax. Compare the resolved signatures instead, and only fall back to the +// syntactic comparison when a signature cannot be resolved. +// +// Resolution fails (`no_bound_vars` returns `None`) when a signature contains +// late-bound lifetimes, e.g. an elided or function-scoped lifetime in a +// reference parameter such as `fn f(x: &u8)` or `fn f<'a>(x: &'a Foo)`. +// Transpiled declarations use raw pointers and are unaffected, but hand-edited +// code can carry references; two such signatures cannot be compared without +// instantiating their bound regions, so they take the syntactic path. +fn compatible_foreign_fns( + cx: &RefactorCtxt, + foreign1: &ForeignItem, + foreign2: &ForeignItem, + decl1: &FnDecl, + decl2: &FnDecl, +) -> bool { + let tcx = cx.ty_ctxt(); + let sig_of = |foreign: &ForeignItem| { + tcx.fn_sig(cx.node_def_id(foreign.id)) + .subst_identity() + .no_bound_vars() + }; + match (sig_of(foreign1), sig_of(foreign2)) { + (Some(sig1), Some(sig2)) => cx.compatible_fn_sigs(&sig1, &sig2), + _ => cx.compatible_fn_prototypes(decl1, decl2), + } +} + /// Returns true if the given ForeignItem can be a declaration for the given /// Item definition. fn foreign_equiv(foreign: &ForeignItem, item: &Item) -> bool { diff --git a/c2rust-refactor/tests/snapshots.rs b/c2rust-refactor/tests/snapshots.rs index 1862f3683e..563dfdde93 100644 --- a/c2rust-refactor/tests/snapshots.rs +++ b/c2rust-refactor/tests/snapshots.rs @@ -478,7 +478,6 @@ fn test_reorganize_foreign_fn_arity() { fn test_reorganize_foreign_fn_type_identity() { refactor("reorganize_definitions") .named("reorganize_foreign_fn_type_identity.rs") - .new_expect_compile_error(true) .test(); } diff --git a/c2rust-refactor/tests/snapshots/snapshots__refactor-reorganize_definitions-reorganize_foreign_fn_type_identity.rs.snap b/c2rust-refactor/tests/snapshots/snapshots__refactor-reorganize_definitions-reorganize_foreign_fn_type_identity.rs.snap index 292dbc6755..65ea8feff6 100644 --- a/c2rust-refactor/tests/snapshots/snapshots__refactor-reorganize_definitions-reorganize_foreign_fn_type_identity.rs.snap +++ b/c2rust-refactor/tests/snapshots/snapshots__refactor-reorganize_definitions-reorganize_foreign_fn_type_identity.rs.snap @@ -12,6 +12,9 @@ expression: c2rust-refactor reorganize_definitions --rewrite-mode alongside -- t pub mod io_h { extern "C" { pub fn fill(b: *mut crate::io_h::buf) -> libc::c_int; + + #[link_name = "fill"] + pub fn fill_1(b: *mut crate::io_h::buf_1) -> libc::c_int; } use ::libc; @@ -54,11 +57,11 @@ pub mod b { use libc; use crate::io_h::buf_1; - use crate::io_h::fill; + use crate::io_h::fill_1; pub unsafe fn run() -> libc::c_int { let mut b = std::mem::zeroed::(); - crate::io_h::fill(&mut b) + crate::io_h::fill_1(&mut b) } } 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 a7bb339d58..a22fe72a23 100644 --- a/c2rust-refactor/tests/snapshots/snapshots__refactor-reorganize_definitions.rs.snap +++ b/c2rust-refactor/tests/snapshots/snapshots__refactor-reorganize_definitions.rs.snap @@ -30,6 +30,10 @@ pub mod bar { 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 ================ @@ -121,7 +125,7 @@ pub mod foo { // Use the definitions that have all public fields. // The transform should not reuse any of the libc declarations. let mut buf = unsafe { std::mem::zeroed::() }; - crate::bar::statvfs(core::ptr::null(), &mut buf); + crate::bar::statvfs_1(core::ptr::null(), &mut buf); // Use the definitions that are identical to libc. let mut buf = unsafe { std::mem::zeroed::<::libc::statfs64>() };