refactor: Update reorganize_definitions test and fix some issues (#1484)

* Fix reorganize_definitions test: the test (and probably all the
refactoring tests) has bitrotted; get it working again
* Add repro of statvfs issue from python2: reproduce the issue
encountered by the python2 test on Ubuntu 22.04 where the `statvfs`
structure has a private field that we cannot use
* Check visibility when comparing structural equivalence for function
signatures
* Deep match visibility when comparing function signatures: updates to
the algorithm we use to match equivalent types and function signatures.
* Enable more transforms on the python2 integration test.

When matching between two types to determine whether they can be
unified, we now account for differences in visibility because importing
a structure with private types from an external crate (e.g. `libc`)
makes them unusable to us when trying to construct new values of those
types. The motivating example is `statvfs`:

```rust
    pub struct statvfs {
        pub f_bsize: c_ulong,
        pub f_frsize: c_ulong,
        pub f_blocks: crate::fsblkcnt_t,
        pub f_bfree: crate::fsblkcnt_t,
        pub f_bavail: crate::fsblkcnt_t,
        pub f_files: crate::fsfilcnt_t,
        pub f_ffree: crate::fsfilcnt_t,
        pub f_favail: crate::fsfilcnt_t,
        pub f_fsid: c_ulong,
        pub f_flag: c_ulong,
        pub f_namemax: c_ulong,
        __f_spare: [c_int; 6],
    }
```

Author: kkysen

c2rust-refactor/src/context.rs

@@ -788,9 +788,12 @@ impl<'a, 'tcx> RefactorCtxt<'a, 'tcx> {
         Some(path)
     }
 
-    /// Compare two items for type compatibility under the C definition
-    pub fn compatible_types(&self, item1: &Item, item2: &Item) -> bool {
-        TypeCompare::new(self).compatible_types(item1, item2)
+    /// Compare two items for type compatibility under the C definition.
+    ///
+    /// If `match_vis` is `true`, the visibility of all `struct`/`enum`/`union`
+    /// fields (i.e. if they are `pub`) must match between the two items.
+    pub fn compatible_types(&self, item1: &Item, item2: &Item, match_vis: bool) -> bool {
+        TypeCompare::new(self).compatible_types(item1, item2, match_vis)
     }
 
     /// Compare two function declarations for equivalent argument and return types,
@@ -1062,8 +1065,11 @@ impl<'a, 'tcx, 'b> TypeCompare<'a, 'tcx, 'b> {
         }
     }
 
-    /// Compare two items for type compatibility under the C definition
-    pub fn compatible_types(&self, item1: &Item, item2: &Item) -> bool {
+    /// Compare two items for type compatibility under the C definition.
+    ///
+    /// If `match_vis` is `true`, the visibility of all `struct`/`enum`/`union`
+    /// fields (i.e. if they are `pub`) must match between the two items.
+    pub fn compatible_types(&self, item1: &Item, item2: &Item, match_vis: bool) -> bool {
         use rustc_ast::ItemKind::*;
         match (&item1.kind, &item2.kind) {
             // * Assure that these two items are in fact of the same type, just to be safe.
@@ -1072,7 +1078,13 @@ impl<'a, 'tcx, 'b> TypeCompare<'a, 'tcx, 'b> {
                     self.cx.opt_node_type(item1.id),
                     self.cx.opt_node_type(item2.id),
                 ) {
-                    (Some(ty1), Some(ty2)) => self.structural_eq_tys(ty1, ty2),
+                    (Some(ty1), Some(ty2)) => {
+                        if match_vis {
+                            self.structural_eq_tys_with_vis(ty1, ty2)
+                        } else {
+                            self.structural_eq_tys(ty1, ty2)
+                        }
+                    }
                     _ => {
                         // TODO: handle type aliases in traits; for now we don't
                         // care about them because C2Rust does not emit traits
@@ -1081,7 +1093,7 @@ impl<'a, 'tcx, 'b> TypeCompare<'a, 'tcx, 'b> {
 
                         if ta1.generics.params.is_empty() && ta2.generics.params.is_empty() {
                             // TODO: compare the other fields
-                            self.structural_eq_ast_tys(ty1, ty2)
+                            self.structural_eq_ast_tys(ty1, ty2, match_vis)
                                 && ta1.defaultness.unnamed_equiv(&ta2.defaultness)
                         } else {
                             // FIXME: handle generics (we don't need to for now)
@@ -1102,7 +1114,7 @@ impl<'a, 'tcx, 'b> TypeCompare<'a, 'tcx, 'b> {
                             && def1.unnamed_equiv(def2)
                     }
                     _ => {
-                        self.structural_eq_ast_tys(ty1, ty2)
+                        self.structural_eq_ast_tys(ty1, ty2, match_vis)
                             && expr1.unnamed_equiv(expr2)
                             && def1.unnamed_equiv(def2)
                     }
@@ -1139,11 +1151,27 @@ impl<'a, 'tcx, 'b> TypeCompare<'a, 'tcx, 'b> {
                     self.cx.opt_node_type(item1.id),
                     self.cx.opt_node_type(item2.id),
                 ) {
-                    (Some(ty1), Some(ty2)) => self.structural_eq_tys(ty1, ty2),
-                    _ => {
+                    (Some(ty1), Some(ty2)) => {
+                        if match_vis {
+                            self.structural_eq_tys_with_vis(ty1, ty2)
+                        } else {
+                            self.structural_eq_tys(ty1, ty2)
+                        }
+                    }
+                    _ => 'match_fields: {
+                        if variant1.fields().len() != variant2.fields().len() {
+                            break 'match_fields false;
+                        }
+
                         let mut fields = variant1.fields().iter().zip(variant2.fields().iter());
                         fields.all(|(field1, field2)| {
-                            self.structural_eq_ast_tys(&field1.ty, &field2.ty)
+                            // TODO: either Visibility or VisibilityKind should implement
+                            // PartialEq; until then, the closest we have is `is_pub`
+                            if match_vis && field1.vis.kind.is_pub() != field2.vis.kind.is_pub() {
+                                return false;
+                            }
+
+                            self.structural_eq_ast_tys(&field1.ty, &field2.ty, match_vis)
                         })
                     }
                 }
@@ -1159,6 +1187,10 @@ impl<'a, 'tcx, 'b> TypeCompare<'a, 'tcx, 'b> {
                         .zip(variant2.data.fields().iter())
                 });
                 fields.all(|(field1, field2)| {
+                    if match_vis && field1.vis.kind.is_pub() != field2.vis.kind.is_pub() {
+                        return false;
+                    }
+
                     match (
                         self.cx.opt_node_type(field1.id),
                         self.cx.opt_node_type(field2.id),
@@ -1177,7 +1209,13 @@ impl<'a, 'tcx, 'b> TypeCompare<'a, 'tcx, 'b> {
                         self.cx.opt_node_type(item1.id),
                         self.cx.opt_node_type(item2.id),
                     ) {
-                        (Some(ty1), Some(ty2)) => return self.structural_eq_tys(ty1, ty2),
+                        (Some(ty1), Some(ty2)) => {
+                            if match_vis {
+                                return self.structural_eq_tys_with_vis(ty1, ty2);
+                            } else {
+                                return self.structural_eq_tys(ty1, ty2);
+                            }
+                        }
                         _ => {}
                     }
                 }
@@ -1192,7 +1230,7 @@ impl<'a, 'tcx, 'b> TypeCompare<'a, 'tcx, 'b> {
     /// ignoring argument names.
     pub fn compatible_fn_prototypes(&self, decl1: &FnDecl, decl2: &FnDecl) -> bool {
         let mut args = decl1.inputs.iter().zip(decl2.inputs.iter());
-        if !args.all(|(arg1, arg2)| self.structural_eq_ast_tys(&arg1.ty, &arg2.ty)) {
+        if !args.all(|(arg1, arg2)| self.structural_eq_ast_tys(&arg1.ty, &arg2.ty, true)) {
             return false;
         }
 
@@ -1208,7 +1246,7 @@ impl<'a, 'tcx, 'b> TypeCompare<'a, 'tcx, 'b> {
             FnRetTy::Ty(ty) => &ty,
         };
 
-        self.structural_eq_ast_tys(ty1, ty2)
+        self.structural_eq_ast_tys(ty1, ty2, true)
     }
 
     /// Compare two ty function signatures for equivalent argument and return
@@ -1223,24 +1261,35 @@ 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(arg_ty1, arg_ty2) {
+            if !self.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(out_ty1, out_ty2)
+        self.structural_eq_tys_with_vis(out_ty1, out_ty2)
     }
 
     /// Compare two AST types for structural equivalence, ignoring names.
-    fn structural_eq_ast_tys(&self, ty1: &rustc_ast::Ty, ty2: &rustc_ast::Ty) -> bool {
+    ///
+    /// If `match_vis` is `true`, the visibility of all `struct`/`enum`/`union`
+    /// fields (i.e. if they are `pub`) must match between the two types.
+    fn structural_eq_ast_tys(
+        &self,
+        ty1: &rustc_ast::Ty,
+        ty2: &rustc_ast::Ty,
+        match_vis: bool,
+    ) -> bool {
         match (self.cx.opt_node_type(ty1.id), self.cx.opt_node_type(ty2.id)) {
+            (Some(ty1), Some(ty2)) if match_vis => {
+                return self.structural_eq_tys_with_vis(ty1, ty2)
+            }
             (Some(ty1), Some(ty2)) => return self.structural_eq_tys(ty1, ty2),
             _ => {}
         }
         match (self.cx.try_resolve_ty(ty1), self.cx.try_resolve_ty(ty2)) {
-            (Some(did1), Some(did2)) => self.structural_eq_defs(did1, did2, false),
+            (Some(did1), Some(did2)) => self.structural_eq_defs(did1, did2, match_vis),
             _ => ty1.unnamed_equiv(ty2),
         }
     }

c2rust-refactor/src/transform/reorganize_definitions.rs

@@ -361,7 +361,7 @@ impl<'a, 'tcx> Reorganizer<'a, 'tcx> {
 
             let decl_ids = declarations.remove_matching_defs(ns, item.ident, |decl| {
                 match decl {
-                    DeclKind::Item(decl) => self.cx.compatible_types(&decl, item),
+                    DeclKind::Item(decl) => self.cx.compatible_types(&decl, item, true),
                     DeclKind::ForeignItem(foreign, _) => foreign_equiv(&foreign, item),
                 }
             });
@@ -1547,7 +1547,7 @@ impl<'a, 'tcx> HeaderDeclarations<'a, 'tcx> {
                         // Otherwise make sure these items are structurally
                         // equivalent.
                         _ => {
-                            if self.cx.compatible_types(&item, &existing_item) {
+                            if self.cx.compatible_types(&item, &existing_item, true) {
                                 return ContainsDecl::Equivalent(existing_decl);
                             }
                         }

c2rust-refactor/tests/reorganize_definitions/new.rs

@@ -1,5 +1,6 @@
 #![feature(extern_types)]
 #![feature(rustc_private)]
+#![feature(register_tool)]
 #![register_tool(c2rust)]
 #![allow(non_upper_case_globals)]
 #![allow(non_camel_case_types)]
@@ -8,27 +9,68 @@
 #![allow(mutable_transmutes)]
 #![allow(unused_mut)]
 
-pub mod compat_h_0 {
-    pub struct conflicting {
-        pub y: libc::c_char,
-    }
-}
 pub mod compat_h {
     pub struct conflicting {
         pub x: libc::c_char,
     }
+
+    pub struct conflicting_1 {
+        pub y: libc::c_char,
+    }
 }
+extern crate libc;
 
 type outside = i32;
 
 pub mod bar {
 
+    extern "C" {
+        pub fn statvfs(path: *const libc::c_char, buf: *mut crate::bar::statvfs) -> libc::c_int;
+    }
     // =============== BEGIN bar_h ================
 
     // Test relative paths
     use crate::outside;
     //test2
     use libc;
+    // Import both the statfs64 type and function declaration from
+    // libc exactly as they are defined in that crate. However, since
+    // both definitions have a private field, the transform shouldn't
+    // unify them across crates.
+    #[repr(C)]
+
+    pub struct statvfs {
+        pub f_bsize: libc::c_ulong,
+        pub f_frsize: libc::c_ulong,
+        pub f_blocks: libc::fsblkcnt_t,
+        pub f_bfree: libc::fsblkcnt_t,
+        pub f_bavail: libc::fsblkcnt_t,
+        pub f_files: libc::fsfilcnt_t,
+        pub f_ffree: libc::fsfilcnt_t,
+        pub f_favail: libc::fsfilcnt_t,
+        pub f_fsid: libc::c_ulong,
+        pub f_flag: libc::c_ulong,
+        pub f_namemax: libc::c_ulong,
+        __f_spare: [libc::c_int; 6],
+    }
+    // Slightly different version of the structure: all fields are public.
+    // This shouldn't get unified either.
+    #[repr(C)]
+
+    pub struct statvfs_1 {
+        pub f_bsize: libc::c_ulong,
+        pub f_frsize: libc::c_ulong,
+        pub f_blocks: libc::fsblkcnt_t,
+        pub f_bfree: libc::fsblkcnt_t,
+        pub f_bavail: libc::fsblkcnt_t,
+        pub f_files: libc::fsfilcnt_t,
+        pub f_ffree: libc::fsfilcnt_t,
+        pub f_favail: libc::fsfilcnt_t,
+        pub f_fsid: libc::c_ulong,
+        pub f_flag: libc::c_ulong,
+        pub f_namemax: libc::c_ulong,
+        pub __f_spare: [libc::c_int; 6],
+    }
     //test1
     type OtherInt = i32;
     // Comment on bar_t
@@ -45,12 +87,10 @@ pub mod bar {
     type FooInt = i32;
 
     #[no_mangle]
-    static mut Bar: crate::bar::bar_t = unsafe {
-        crate::bar::bar_t {
-            alloc: 0 as *mut libc::c_char,
-            data: 0 as *mut libc::c_char,
-            i: 0,
-        }
+    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,
     };
 }
 
@@ -59,7 +99,7 @@ pub mod foo {
 
     use crate::bar::bar_t;
     use crate::bar::Bar;
-    use crate::compat_h_0::conflicting;
+    use crate::compat_h::conflicting_1;
 
     // Comment on foo_t
 
@@ -71,7 +111,30 @@ pub mod foo {
     }
 
     unsafe fn foo() -> *const crate::bar::bar_t {
-        let c = crate::compat_h_0::conflicting { y: 10 };
+        // Use the local definitions.
+        let mut buf = unsafe { std::mem::zeroed::<crate::bar::statvfs>() };
+        crate::bar::statvfs(core::ptr::null(), &mut buf);
+
+        // 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);
+
+        // Use the definitions that are identical to libc.
+        let mut buf = unsafe { std::mem::zeroed::<::libc::statfs64>() };
+        ::libc::statfs64(
+            core::ptr::null(),
+            &mut buf as *mut _ as *mut ::libc::statfs64,
+        );
+
+        // Use the definitions that are identical to libc.
+        let mut buf = unsafe { std::mem::zeroed::<::libc::statfs64>() };
+        ::libc::statfs64(
+            core::ptr::null(),
+            &mut buf as *mut _ as *mut ::libc::statfs64,
+        );
+
+        let _c = crate::compat_h::conflicting_1 { y: 10 };
         &crate::bar::Bar as *const crate::bar::bar_t
     }
 }