Rollup merge of #130301 - RalfJung:clashing_extern_declarations, r=compiler-errors

some fixes for clashing_extern_declarations lint

There were two issues with the clashing_extern_declarations lint:
- It would accept non-`repr(C)` structs as compatible with each other by comparing their fields in declaration order, but the fields could have different memory order (and with `-Zrandomize-layout`, this can really happen).
- It would accept two types as compatible if `compare_layouts` returns `true`, but that function actually just compared the *ABI*, not the fully layout -- and all sized structs with more than 2 fields have the same ABI (`Abi::Aggregate`), so this missed a *lot* of cases.

We don't currently have a clear spec for what we *want* to consider "clashing" and what is fine, so I otherwise kept the original logic. I hope to have a t-lang discussion about this at some point. But meanwhile, these changes seem like clear bugfixes.

4 files changed, 172 insertions, 89 deletions

diff --git a/compiler/rustc_lint/src/foreign_modules.rs b/compiler/rustc_lint/src/foreign_modules.rs
index a60fc0ffbbb..33b8b7c544b 100644
--- a/compiler/rustc_lint/src/foreign_modules.rs
+++ b/compiler/rustc_lint/src/foreign_modules.rs
@@ -3,8 +3,7 @@ use rustc_data_structures::unord::{UnordMap, UnordSet};
 use rustc_hir as hir;
 use rustc_hir::def::DefKind;
 use rustc_middle::query::Providers;
-use rustc_middle::ty::layout::LayoutError;
-use rustc_middle::ty::{self, Instance, Ty, TyCtxt};
+use rustc_middle::ty::{self, AdtDef, Instance, Ty, TyCtxt};
 use rustc_session::declare_lint;
 use rustc_span::{sym, Span, Symbol};
 use rustc_target::abi::FIRST_VARIANT;
@@ -212,7 +211,17 @@ fn structurally_same_type<'tcx>(
     ckind: types::CItemKind,
 ) -> bool {
     let mut seen_types = UnordSet::default();
-    structurally_same_type_impl(&mut seen_types, tcx, param_env, a, b, ckind)
+    let result = structurally_same_type_impl(&mut seen_types, tcx, param_env, a, b, ckind);
+    if cfg!(debug_assertions) && result {
+        // Sanity-check: must have same ABI, size and alignment.
+        // `extern` blocks cannot be generic, so we'll always get a layout here.
+        let a_layout = tcx.layout_of(param_env.and(a)).unwrap();
+        let b_layout = tcx.layout_of(param_env.and(b)).unwrap();
+        assert_eq!(a_layout.abi, b_layout.abi);
+        assert_eq!(a_layout.size, b_layout.size);
+        assert_eq!(a_layout.align, b_layout.align);
+    }
+    result
 }
 
 fn structurally_same_type_impl<'tcx>(
@@ -266,30 +275,21 @@ fn structurally_same_type_impl<'tcx>(
         // Do a full, depth-first comparison between the two.
         use rustc_type_ir::TyKind::*;
 
-        let compare_layouts = |a, b| -> Result<bool, &'tcx LayoutError<'tcx>> {
-            debug!("compare_layouts({:?}, {:?})", a, b);
-            let a_layout = &tcx.layout_of(param_env.and(a))?.layout.abi();
-            let b_layout = &tcx.layout_of(param_env.and(b))?.layout.abi();
-            debug!(
-                "comparing layouts: {:?} == {:?} = {}",
-                a_layout,
-                b_layout,
-                a_layout == b_layout
-            );
-            Ok(a_layout == b_layout)
-        };
-
         let is_primitive_or_pointer =
             |ty: Ty<'tcx>| ty.is_primitive() || matches!(ty.kind(), RawPtr(..) | Ref(..));
 
         ensure_sufficient_stack(|| {
             match (a.kind(), b.kind()) {
-                (Adt(a_def, _), Adt(b_def, _)) => {
-                    // We can immediately rule out these types as structurally same if
-                    // their layouts differ.
-                    match compare_layouts(a, b) {
-                        Ok(false) => return false,
-                        _ => (), // otherwise, continue onto the full, fields comparison
+                (&Adt(a_def, _), &Adt(b_def, _)) => {
+                    // Only `repr(C)` types can be compared structurally.
+                    if !(a_def.repr().c() && b_def.repr().c()) {
+                        return false;
+                    }
+                    // If the types differ in their packed-ness, align, or simd-ness they conflict.
+                    let repr_characteristica =
+                        |def: AdtDef<'tcx>| (def.repr().pack, def.repr().align, def.repr().simd());
+                    if repr_characteristica(a_def) != repr_characteristica(b_def) {
+                        return false;
                     }
 
                     // Grab a flattened representation of all fields.
@@ -311,9 +311,9 @@ fn structurally_same_type_impl<'tcx>(
                         },
                     )
                 }
-                (Array(a_ty, a_const), Array(b_ty, b_const)) => {
-                    // For arrays, we also check the constness of the type.
-                    a_const.kind() == b_const.kind()
+                (Array(a_ty, a_len), Array(b_ty, b_len)) => {
+                    // For arrays, we also check the length.
+                    a_len == b_len
                         && structurally_same_type_impl(
                             seen_types, tcx, param_env, *a_ty, *b_ty, ckind,
                         )
@@ -357,10 +357,9 @@ fn structurally_same_type_impl<'tcx>(
                             ckind,
                         )
                 }
-                (Tuple(a_args), Tuple(b_args)) => {
-                    a_args.iter().eq_by(b_args.iter(), |a_ty, b_ty| {
-                        structurally_same_type_impl(seen_types, tcx, param_env, a_ty, b_ty, ckind)
-                    })
+                (Tuple(..), Tuple(..)) => {
+                    // Tuples are not `repr(C)` so these cannot be compared structurally.
+                    false
                 }
                 // For these, it's not quite as easy to define structural-sameness quite so easily.
                 // For the purposes of this lint, take the conservative approach and mark them as
@@ -380,24 +379,21 @@ fn structurally_same_type_impl<'tcx>(
                 // An Adt and a primitive or pointer type. This can be FFI-safe if non-null
                 // enum layout optimisation is being applied.
                 (Adt(..), _) if is_primitive_or_pointer(b) => {
-                    if let Some(ty) = types::repr_nullable_ptr(tcx, param_env, a, ckind) {
-                        ty == b
+                    if let Some(a_inner) = types::repr_nullable_ptr(tcx, param_env, a, ckind) {
+                        a_inner == b
                     } else {
-                        compare_layouts(a, b).unwrap_or(false)
+                        false
                     }
                 }
                 (_, Adt(..)) if is_primitive_or_pointer(a) => {
-                    if let Some(ty) = types::repr_nullable_ptr(tcx, param_env, b, ckind) {
-                        ty == a
+                    if let Some(b_inner) = types::repr_nullable_ptr(tcx, param_env, b, ckind) {
+                        b_inner == a
                     } else {
-                        compare_layouts(a, b).unwrap_or(false)
+                        false
                     }
                 }
 
-                // Otherwise, just compare the layouts. This may fail to lint for some
-                // incompatible types, but at the very least, will stop reads into
-                // uninitialised memory.
-                _ => compare_layouts(a, b).unwrap_or(false),
+                _ => false,
             }
         })
     }
diff --git a/tests/ui/lint/clashing-extern-fn-recursion.rs b/tests/ui/lint/clashing-extern-fn-recursion.rs
index 40bef400594..ab311d398f9 100644
--- a/tests/ui/lint/clashing-extern-fn-recursion.rs
+++ b/tests/ui/lint/clashing-extern-fn-recursion.rs
@@ -92,6 +92,7 @@ mod ref_recursion_once_removed {
             reffy: &'a Reffy2<'a>,
         }
 
+        #[repr(C)]
         struct Reffy2<'a> {
             reffy: &'a Reffy1<'a>,
         }
@@ -107,6 +108,7 @@ mod ref_recursion_once_removed {
             reffy: &'a Reffy2<'a>,
         }
 
+        #[repr(C)]
         struct Reffy2<'a> {
             reffy: &'a Reffy1<'a>,
         }
diff --git a/tests/ui/lint/clashing-extern-fn.rs b/tests/ui/lint/clashing-extern-fn.rs
index 728dfabb393..a12fe81eecd 100644
--- a/tests/ui/lint/clashing-extern-fn.rs
+++ b/tests/ui/lint/clashing-extern-fn.rs
@@ -1,7 +1,6 @@
 //@ check-pass
 //@ aux-build:external_extern_fn.rs
 #![crate_type = "lib"]
-#![warn(clashing_extern_declarations)]
 
 mod redeclared_different_signature {
     mod a {
@@ -132,7 +131,7 @@ mod banana {
     mod three {
         // This _should_ trigger the lint, because repr(packed) should generate a struct that has a
         // different layout.
-        #[repr(packed)]
+        #[repr(C, packed)]
         struct Banana {
             weight: u32,
             length: u16,
@@ -143,6 +142,79 @@ mod banana {
             //~^ WARN `weigh_banana` redeclared with a different signature
         }
     }
+
+    mod four {
+        // This _should_ trigger the lint, because the type is not repr(C).
+        struct Banana {
+            weight: u32,
+            length: u16,
+        }
+        #[allow(improper_ctypes)]
+        extern "C" {
+            fn weigh_banana(count: *const Banana) -> u64;
+            //~^ WARN `weigh_banana` redeclared with a different signature
+        }
+    }
+}
+
+// 3-field structs can't be distinguished by ScalarPair, side-stepping some shortucts
+// the logic used to (incorrectly) take.
+mod banana3 {
+    mod one {
+        #[repr(C)]
+        struct Banana {
+            weight: u32,
+            length: u16,
+            color: u8,
+        }
+        extern "C" {
+            fn weigh_banana3(count: *const Banana) -> u64;
+        }
+    }
+
+    mod two {
+        #[repr(C)]
+        struct Banana {
+            weight: u32,
+            length: u16,
+            color: u8,
+        } // note: distinct type
+        // This should not trigger the lint because two::Banana is structurally equivalent to
+        // one::Banana.
+        extern "C" {
+            fn weigh_banana3(count: *const Banana) -> u64;
+        }
+    }
+
+    mod three {
+        // This _should_ trigger the lint, because repr(packed) should generate a struct that has a
+        // different layout.
+        #[repr(C, packed)]
+        struct Banana {
+            weight: u32,
+            length: u16,
+            color: u8,
+        }
+        #[allow(improper_ctypes)]
+        extern "C" {
+            fn weigh_banana3(count: *const Banana) -> u64;
+            //~^ WARN `weigh_banana3` redeclared with a different signature
+        }
+    }
+
+    mod four {
+        // This _should_ trigger the lint, because the type is not repr(C).
+        struct Banana {
+            weight: u32,
+            length: u16,
+            color: u8,
+        }
+        #[allow(improper_ctypes)]
+        extern "C" {
+            fn weigh_banana3(count: *const Banana) -> u64;
+            //~^ WARN `weigh_banana3` redeclared with a different signature
+        }
+    }
 }
 
 mod sameish_members {
@@ -223,27 +295,6 @@ mod transparent {
     }
 }
 
-#[allow(improper_ctypes)]
-mod zst {
-    mod transparent {
-        #[repr(transparent)]
-        struct TransparentZst(());
-        extern "C" {
-            fn zst() -> ();
-            fn transparent_zst() -> TransparentZst;
-        }
-    }
-
-    mod not_transparent {
-        struct NotTransparentZst(());
-        extern "C" {
-            // These shouldn't warn since all return types are zero sized
-            fn zst() -> NotTransparentZst;
-            fn transparent_zst() -> NotTransparentZst;
-        }
-    }
-}
-
 mod missing_return_type {
     mod a {
         extern "C" {
@@ -384,6 +435,7 @@ mod unknown_layout {
         extern "C" {
             pub fn generic(l: Link<u32>);
         }
+        #[repr(C)]
         pub struct Link<T> {
             pub item: T,
             pub next: *const Link<T>,
@@ -394,6 +446,7 @@ mod unknown_layout {
         extern "C" {
             pub fn generic(l: Link<u32>);
         }
+        #[repr(C)]
         pub struct Link<T> {
             pub item: T,
             pub next: *const Link<T>,
diff --git a/tests/ui/lint/clashing-extern-fn.stderr b/tests/ui/lint/clashing-extern-fn.stderr
index f75ff6d05a1..b30dd476a1d 100644
--- a/tests/ui/lint/clashing-extern-fn.stderr
+++ b/tests/ui/lint/clashing-extern-fn.stderr
@@ -1,5 +1,5 @@
 warning: `extern` block uses type `Option<TransparentNoNiche>`, which is not FFI-safe
-  --> $DIR/clashing-extern-fn.rs:429:55
+  --> $DIR/clashing-extern-fn.rs:482:55
    |
 LL |             fn hidden_niche_transparent_no_niche() -> Option<TransparentNoNiche>;
    |                                                       ^^^^^^^^^^^^^^^^^^^^^^^^^^ not FFI-safe
@@ -9,7 +9,7 @@ LL |             fn hidden_niche_transparent_no_niche() -> Option<TransparentNoN
    = note: `#[warn(improper_ctypes)]` on by default
 
 warning: `extern` block uses type `Option<UnsafeCell<NonZero<usize>>>`, which is not FFI-safe
-  --> $DIR/clashing-extern-fn.rs:433:46
+  --> $DIR/clashing-extern-fn.rs:486:46
    |
 LL |             fn hidden_niche_unsafe_cell() -> Option<UnsafeCell<NonZero<usize>>>;
    |                                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ not FFI-safe
@@ -18,7 +18,7 @@ LL |             fn hidden_niche_unsafe_cell() -> Option<UnsafeCell<NonZero<usiz
    = note: enum has no representation hint
 
 warning: `clash` redeclared with a different signature
-  --> $DIR/clashing-extern-fn.rs:14:13
+  --> $DIR/clashing-extern-fn.rs:13:13
    |
 LL |             fn clash(x: u8);
    |             ---------------- `clash` previously declared here
@@ -28,14 +28,10 @@ LL |             fn clash(x: u64);
    |
    = note: expected `unsafe extern "C" fn(u8)`
               found `unsafe extern "C" fn(u64)`
-note: the lint level is defined here
-  --> $DIR/clashing-extern-fn.rs:4:9
-   |
-LL | #![warn(clashing_extern_declarations)]
-   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   = note: `#[warn(clashing_extern_declarations)]` on by default
 
 warning: `extern_link_name` redeclared with a different signature
-  --> $DIR/clashing-extern-fn.rs:52:9
+  --> $DIR/clashing-extern-fn.rs:51:9
    |
 LL |     #[link_name = "extern_link_name"]
    |     --------------------------------- `extern_link_name` previously declared here
@@ -47,7 +43,7 @@ LL |         fn extern_link_name(x: u32);
               found `unsafe extern "C" fn(u32)`
 
 warning: `some_other_extern_link_name` redeclares `some_other_new_name` with a different signature
-  --> $DIR/clashing-extern-fn.rs:55:9
+  --> $DIR/clashing-extern-fn.rs:54:9
    |
 LL |     fn some_other_new_name(x: i16);
    |     ------------------------------- `some_other_new_name` previously declared here
@@ -59,7 +55,7 @@ LL |         #[link_name = "some_other_new_name"]
               found `unsafe extern "C" fn(u32)`
 
 warning: `other_both_names_different` redeclares `link_name_same` with a different signature
-  --> $DIR/clashing-extern-fn.rs:59:9
+  --> $DIR/clashing-extern-fn.rs:58:9
    |
 LL |     #[link_name = "link_name_same"]
    |     ------------------------------- `link_name_same` previously declared here
@@ -71,7 +67,7 @@ LL |         #[link_name = "link_name_same"]
               found `unsafe extern "C" fn(u32)`
 
 warning: `different_mod` redeclared with a different signature
-  --> $DIR/clashing-extern-fn.rs:72:9
+  --> $DIR/clashing-extern-fn.rs:71:9
    |
 LL |         fn different_mod(x: u8);
    |         ------------------------ `different_mod` previously declared here
@@ -83,7 +79,7 @@ LL |         fn different_mod(x: u64);
               found `unsafe extern "C" fn(u64)`
 
 warning: `variadic_decl` redeclared with a different signature
-  --> $DIR/clashing-extern-fn.rs:82:9
+  --> $DIR/clashing-extern-fn.rs:81:9
    |
 LL |     fn variadic_decl(x: u8, ...);
    |     ----------------------------- `variadic_decl` previously declared here
@@ -95,7 +91,19 @@ LL |         fn variadic_decl(x: u8);
               found `unsafe extern "C" fn(u8)`
 
 warning: `weigh_banana` redeclared with a different signature
-  --> $DIR/clashing-extern-fn.rs:142:13
+  --> $DIR/clashing-extern-fn.rs:141:13
+   |
+LL |             fn weigh_banana(count: *const Banana) -> u64;
+   |             --------------------------------------------- `weigh_banana` previously declared here
+...
+LL |             fn weigh_banana(count: *const Banana) -> u64;
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ this signature doesn't match the previous declaration
+   |
+   = note: expected `unsafe extern "C" fn(*const banana::one::Banana) -> u64`
+              found `unsafe extern "C" fn(*const banana::three::Banana) -> u64`
+
+warning: `weigh_banana` redeclared with a different signature
+  --> $DIR/clashing-extern-fn.rs:154:13
    |
 LL |             fn weigh_banana(count: *const Banana) -> u64;
    |             --------------------------------------------- `weigh_banana` previously declared here
@@ -103,11 +111,35 @@ LL |             fn weigh_banana(count: *const Banana) -> u64;
 LL |             fn weigh_banana(count: *const Banana) -> u64;
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ this signature doesn't match the previous declaration
    |
-   = note: expected `unsafe extern "C" fn(*const one::Banana) -> u64`
-              found `unsafe extern "C" fn(*const three::Banana) -> u64`
+   = note: expected `unsafe extern "C" fn(*const banana::one::Banana) -> u64`
+              found `unsafe extern "C" fn(*const banana::four::Banana) -> u64`
+
+warning: `weigh_banana3` redeclared with a different signature
+  --> $DIR/clashing-extern-fn.rs:200:13
+   |
+LL |             fn weigh_banana3(count: *const Banana) -> u64;
+   |             ---------------------------------------------- `weigh_banana3` previously declared here
+...
+LL |             fn weigh_banana3(count: *const Banana) -> u64;
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ this signature doesn't match the previous declaration
+   |
+   = note: expected `unsafe extern "C" fn(*const banana3::one::Banana) -> u64`
+              found `unsafe extern "C" fn(*const banana3::three::Banana) -> u64`
+
+warning: `weigh_banana3` redeclared with a different signature
+  --> $DIR/clashing-extern-fn.rs:214:13
+   |
+LL |             fn weigh_banana3(count: *const Banana) -> u64;
+   |             ---------------------------------------------- `weigh_banana3` previously declared here
+...
+LL |             fn weigh_banana3(count: *const Banana) -> u64;
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ this signature doesn't match the previous declaration
+   |
+   = note: expected `unsafe extern "C" fn(*const banana3::one::Banana) -> u64`
+              found `unsafe extern "C" fn(*const banana3::four::Banana) -> u64`
 
 warning: `draw_point` redeclared with a different signature
-  --> $DIR/clashing-extern-fn.rs:171:13
+  --> $DIR/clashing-extern-fn.rs:243:13
    |
 LL |             fn draw_point(p: Point);
    |             ------------------------ `draw_point` previously declared here
@@ -119,7 +151,7 @@ LL |             fn draw_point(p: Point);
               found `unsafe extern "C" fn(sameish_members::b::Point)`
 
 warning: `origin` redeclared with a different signature
-  --> $DIR/clashing-extern-fn.rs:197:13
+  --> $DIR/clashing-extern-fn.rs:269:13
    |
 LL |             fn origin() -> Point3;
    |             ---------------------- `origin` previously declared here
@@ -131,7 +163,7 @@ LL |             fn origin() -> Point3;
               found `unsafe extern "C" fn() -> same_sized_members_clash::b::Point3`
 
 warning: `transparent_incorrect` redeclared with a different signature
-  --> $DIR/clashing-extern-fn.rs:220:13
+  --> $DIR/clashing-extern-fn.rs:292:13
    |
 LL |             fn transparent_incorrect() -> T;
    |             -------------------------------- `transparent_incorrect` previously declared here
@@ -143,7 +175,7 @@ LL |             fn transparent_incorrect() -> isize;
               found `unsafe extern "C" fn() -> isize`
 
 warning: `missing_return_type` redeclared with a different signature
-  --> $DIR/clashing-extern-fn.rs:259:13
+  --> $DIR/clashing-extern-fn.rs:310:13
    |
 LL |             fn missing_return_type() -> usize;
    |             ---------------------------------- `missing_return_type` previously declared here
@@ -155,7 +187,7 @@ LL |             fn missing_return_type();
               found `unsafe extern "C" fn()`
 
 warning: `non_zero_usize` redeclared with a different signature
-  --> $DIR/clashing-extern-fn.rs:277:13
+  --> $DIR/clashing-extern-fn.rs:328:13
    |
 LL |             fn non_zero_usize() -> core::num::NonZero<usize>;
    |             ------------------------------------------------- `non_zero_usize` previously declared here
@@ -167,7 +199,7 @@ LL |             fn non_zero_usize() -> usize;
               found `unsafe extern "C" fn() -> usize`
 
 warning: `non_null_ptr` redeclared with a different signature
-  --> $DIR/clashing-extern-fn.rs:279:13
+  --> $DIR/clashing-extern-fn.rs:330:13
    |
 LL |             fn non_null_ptr() -> core::ptr::NonNull<usize>;
    |             ----------------------------------------------- `non_null_ptr` previously declared here
@@ -179,7 +211,7 @@ LL |             fn non_null_ptr() -> *const usize;
               found `unsafe extern "C" fn() -> *const usize`
 
 warning: `option_non_zero_usize_incorrect` redeclared with a different signature
-  --> $DIR/clashing-extern-fn.rs:373:13
+  --> $DIR/clashing-extern-fn.rs:424:13
    |
 LL |             fn option_non_zero_usize_incorrect() -> usize;
    |             ---------------------------------------------- `option_non_zero_usize_incorrect` previously declared here
@@ -191,7 +223,7 @@ LL |             fn option_non_zero_usize_incorrect() -> isize;
               found `unsafe extern "C" fn() -> isize`
 
 warning: `option_non_null_ptr_incorrect` redeclared with a different signature
-  --> $DIR/clashing-extern-fn.rs:375:13
+  --> $DIR/clashing-extern-fn.rs:426:13
    |
 LL |             fn option_non_null_ptr_incorrect() -> *const usize;
    |             --------------------------------------------------- `option_non_null_ptr_incorrect` previously declared here
@@ -203,7 +235,7 @@ LL |             fn option_non_null_ptr_incorrect() -> *const isize;
               found `unsafe extern "C" fn() -> *const isize`
 
 warning: `hidden_niche_transparent_no_niche` redeclared with a different signature
-  --> $DIR/clashing-extern-fn.rs:429:13
+  --> $DIR/clashing-extern-fn.rs:482:13
    |
 LL |             fn hidden_niche_transparent_no_niche() -> usize;
    |             ------------------------------------------------ `hidden_niche_transparent_no_niche` previously declared here
@@ -215,7 +247,7 @@ LL |             fn hidden_niche_transparent_no_niche() -> Option<TransparentNoN
               found `unsafe extern "C" fn() -> Option<TransparentNoNiche>`
 
 warning: `hidden_niche_unsafe_cell` redeclared with a different signature
-  --> $DIR/clashing-extern-fn.rs:433:13
+  --> $DIR/clashing-extern-fn.rs:486:13
    |
 LL |             fn hidden_niche_unsafe_cell() -> usize;
    |             --------------------------------------- `hidden_niche_unsafe_cell` previously declared here
@@ -226,5 +258,5 @@ LL |             fn hidden_niche_unsafe_cell() -> Option<UnsafeCell<NonZero<usiz
    = note: expected `unsafe extern "C" fn() -> usize`
               found `unsafe extern "C" fn() -> Option<UnsafeCell<NonZero<usize>>>`
 
-warning: 19 warnings emitted
+warning: 22 warnings emitted