refactor: compare foreign function declarations by resolved type

c2rust-refactor/src/context.rs

@@ -1147,26 +1147,47 @@ pub struct CalleeInfo<'tcx> {
 
 type DefMapping = HashMap<DefId, DefId>;
 
+#[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> {
     pub fn new(cx: &'a RefactorCtxt<'a, 'tcx>) -> Self {
         Self {
             cx,
             def_mapping: None,
+            exact_array_lens: false,
         }
     }
 
     pub fn new_with_mapping(cx: &'a RefactorCtxt<'a, 'tcx>, def_mapping: &'b DefMapping) -> Self {
         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;
                 }

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 {

c2rust-refactor/tests/snapshots.rs

@@ -472,6 +472,15 @@ 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")
+        .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]

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::<buf>();
+        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::<buf>();
+        fill(&mut b)
+    }
+}
+
+fn main() {}

c2rust-refactor/tests/snapshots/snapshots__refactor-reorganize_definitions-reorganize_foreign_fn_type_identity.rs.snap

@@ -0,0 +1,68 @@
+---
+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;
+
+        #[link_name = "fill"]
+        pub fn fill_1(b: *mut crate::io_h::buf_1) -> 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::buf>();
+        crate::io_h::fill(&mut b)
+    }
+}
+
+pub mod b {
+    use libc;
+
+    use crate::io_h::buf_1;
+    use crate::io_h::fill_1;
+
+    pub unsafe fn run() -> libc::c_int {
+        let mut b = std::mem::zeroed::<crate::io_h::buf_1>();
+        crate::io_h::fill_1(&mut b)
+    }
+}
+
+fn main() {}

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_1>() };
-        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>() };