From 4da72f538752aad2ab4b77dad115ef5889365baf Mon Sep 17 00:00:00 2001
From: jumbatm <30644300+jumbatm@users.noreply.github.com>
Date: Mon, 29 Jun 2020 07:33:15 +1000
Subject: [PATCH 1/6] Add additional clashing_extern_decl cases.

---
 .../ui/lint/auxiliary/external_extern_fn.rs   |   2 +-
 src/test/ui/lint/clashing-extern-fn.rs        | 146 +++++++++++++-----
 src/test/ui/lint/clashing-extern-fn.stderr    |  38 ++---
 3 files changed, 124 insertions(+), 62 deletions(-)

diff --git a/src/test/ui/lint/auxiliary/external_extern_fn.rs b/src/test/ui/lint/auxiliary/external_extern_fn.rs
index b2caebc6fee0e..c2a8cadc6b5f8 100644
--- a/src/test/ui/lint/auxiliary/external_extern_fn.rs
+++ b/src/test/ui/lint/auxiliary/external_extern_fn.rs
@@ -1,3 +1,3 @@
-extern {
+extern "C" {
     pub fn extern_fn(x: u8);
 }
diff --git a/src/test/ui/lint/clashing-extern-fn.rs b/src/test/ui/lint/clashing-extern-fn.rs
index 544614100ba28..72c5d3da4b003 100644
--- a/src/test/ui/lint/clashing-extern-fn.rs
+++ b/src/test/ui/lint/clashing-extern-fn.rs
@@ -3,52 +3,40 @@
 #![crate_type = "lib"]
 #![warn(clashing_extern_declarations)]
 
-extern crate external_extern_fn;
-
-extern "C" {
-    fn clash(x: u8);
-    fn no_clash(x: u8);
-}
-
-fn redeclared_different_signature() {
-    extern "C" {
-        fn clash(x: u64); //~ WARN `clash` redeclared with a different signature
+mod redeclared_different_signature {
+    mod a {
+        extern "C" {
+            fn clash(x: u8);
+        }
     }
-
-    unsafe {
-        clash(123);
-        no_clash(123);
+    mod b {
+        extern "C" {
+            fn clash(x: u64); //~ WARN `clash` redeclared with a different signature
+        }
     }
 }
 
-fn redeclared_same_signature() {
-    extern "C" {
-        fn no_clash(x: u8);
+mod redeclared_same_signature {
+    mod a {
+        extern "C" {
+            fn no_clash(x: u8);
+        }
     }
-    unsafe {
-        no_clash(123);
+    mod b {
+        extern "C" {
+            fn no_clash(x: u8);
+        }
     }
 }
 
-extern "C" {
-    fn extern_fn(x: u64);
-}
-
-fn extern_clash() {
+extern crate external_extern_fn;
+mod extern_no_clash {
+    // Should not clash with external_extern_fn::extern_fn.
     extern "C" {
-        fn extern_fn(x: u32); //~ WARN `extern_fn` redeclared with a different signature
-    }
-    unsafe {
-        extern_fn(123);
+        fn extern_fn(x: u8);
     }
 }
 
-fn extern_no_clash() {
-    unsafe {
-        external_extern_fn::extern_fn(123);
-        crate::extern_fn(123);
-    }
-}
 extern "C" {
     fn some_other_new_name(x: i16);
 
@@ -134,9 +122,9 @@ mod banana {
             weight: u32,
             length: u16,
         } // note: distinct type
-        extern "C" {
           // This should not trigger the lint because two::Banana is structurally equivalent to
           // one::Banana.
+        extern "C" {
             fn weigh_banana(count: *const Banana) -> u64;
         }
     }
@@ -180,7 +168,93 @@ mod sameish_members {
         // always be the case, for every architecture and situation. This is also a really odd
         // thing to do anyway.
         extern "C" {
-            fn draw_point(p: Point); //~ WARN `draw_point` redeclared with a different
+            fn draw_point(p: Point);
+            //~^ WARN `draw_point` redeclared with a different signature
+        }
+    }
+}
+
+mod transparent {
+    #[repr(transparent)]
+    struct T(usize);
+    mod a {
+        use super::T;
+        extern "C" {
+            fn transparent() -> T;
+        }
+    }
+
+    mod b {
+        extern "C" {
+            // Shouldn't warn here, because repr(transparent) guarantees that T's layout is the
+            // same as just the usize.
+            fn transparent() -> usize;
+        }
+    }
+}
+
+mod missing_return_type {
+    mod a {
+        extern "C" {
+            fn missing_return_type() -> usize;
+        }
+    }
+
+    mod b {
+        extern "C" {
+            // This should warn, because we can't assume that the first declaration is the correct
+            // one -- if this one is the correct one, then calling the usize-returning version
+            // would allow reads into uninitialised memory.
+            fn missing_return_type();
+            //~^ WARN `missing_return_type` redeclared with a different signature
+        }
+    }
+}
+
+mod non_zero_and_non_null {
+    mod a {
+        extern "C" {
+            fn non_zero_usize() -> core::num::NonZeroUsize;
+            fn non_null_ptr() -> core::ptr::NonNull<usize>;
+        }
+    }
+    mod b {
+        extern "C" {
+            // For both of these cases, if there's a clash, you're either gaining an incorrect
+            // invariant that the value is non-zero, or you're missing out on that invariant. Both
+            // cases are warning for, from both a caller-convenience and optimisation perspective.
+            fn non_zero_usize() -> usize;
+            //~^ WARN `non_zero_usize` redeclared with a different signature
+            fn non_null_ptr() -> *const usize;
+            //~^ WARN `non_null_ptr` redeclared with a different signature
+        }
+    }
+}
+
+mod null_optimised_enums {
+    mod a {
+        extern "C" {
+            fn option_non_zero_usize() -> usize;
+            fn option_non_zero_isize() -> isize;
+            fn option_non_null_ptr() -> *const usize;
+
+            fn option_non_zero_usize_incorrect() -> usize;
+            fn option_non_null_ptr_incorrect() -> *const usize;
+        }
+    }
+    mod b {
+        extern "C" {
+            // This should be allowed, because these conversions are guaranteed to be FFI-safe (see
+            // #60300)
+            fn option_non_zero_usize() -> Option<core::num::NonZeroUsize>;
+            fn option_non_zero_isize() -> Option<core::num::NonZeroIsize>;
+            fn option_non_null_ptr() -> Option<core::ptr::NonNull<usize>>;
+
+            // However, these should be incorrect (note isize instead of usize)
+            fn option_non_zero_usize_incorrect() -> isize;
+            //~^ WARN `option_non_zero_usize_incorrect` redeclared with a different signature
+            fn option_non_null_ptr_incorrect() -> *const isize;
+            //~^ WARN `option_non_null_ptr_incorrect` redeclared with a different signature
         }
     }
 }
diff --git a/src/test/ui/lint/clashing-extern-fn.stderr b/src/test/ui/lint/clashing-extern-fn.stderr
index 96e51ab5a3ec7..a2aaaba717370 100644
--- a/src/test/ui/lint/clashing-extern-fn.stderr
+++ b/src/test/ui/lint/clashing-extern-fn.stderr
@@ -1,11 +1,11 @@
 warning: `clash` redeclared with a different signature
-  --> $DIR/clashing-extern-fn.rs:15:9
+  --> $DIR/clashing-extern-fn.rs:14:13
    |
-LL |     fn clash(x: u8);
-   |     ---------------- `clash` previously declared here
+LL |             fn clash(x: u8);
+   |             ---------------- `clash` previously declared here
 ...
-LL |         fn clash(x: u64);
-   |         ^^^^^^^^^^^^^^^^^ this signature doesn't match the previous declaration
+LL |             fn clash(x: u64);
+   |             ^^^^^^^^^^^^^^^^^ this signature doesn't match the previous declaration
    |
 note: the lint level is defined here
   --> $DIR/clashing-extern-fn.rs:4:9
@@ -15,20 +15,8 @@ LL | #![warn(clashing_extern_declarations)]
    = note: expected `unsafe extern "C" fn(u8)`
               found `unsafe extern "C" fn(u64)`
 
-warning: `extern_fn` redeclared with a different signature
-  --> $DIR/clashing-extern-fn.rs:39:9
-   |
-LL |     fn extern_fn(x: u64);
-   |     --------------------- `extern_fn` previously declared here
-...
-LL |         fn extern_fn(x: u32);
-   |         ^^^^^^^^^^^^^^^^^^^^^ this signature doesn't match the previous declaration
-   |
-   = note: expected `unsafe extern "C" fn(u64)`
-              found `unsafe extern "C" fn(u32)`
-
 warning: `extern_link_name` redeclared with a different signature
-  --> $DIR/clashing-extern-fn.rs:64:9
+  --> $DIR/clashing-extern-fn.rs:52:9
    |
 LL | /     #[link_name = "extern_link_name"]
 LL | |     fn some_new_name(x: i16);
@@ -41,7 +29,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:67:9
+  --> $DIR/clashing-extern-fn.rs:55:9
    |
 LL |       fn some_other_new_name(x: i16);
    |       ------------------------------- `some_other_new_name` previously declared here
@@ -55,7 +43,7 @@ LL | |         fn some_other_extern_link_name(x: u32);
               found `unsafe extern "C" fn(u32)`
 
 warning: `other_both_names_different` redeclares `link_name_same` with a different signature
-  --> $DIR/clashing-extern-fn.rs:71:9
+  --> $DIR/clashing-extern-fn.rs:59:9
    |
 LL | /     #[link_name = "link_name_same"]
 LL | |     fn both_names_different(x: i16);
@@ -70,7 +58,7 @@ LL | |         fn other_both_names_different(x: u32);
               found `unsafe extern "C" fn(u32)`
 
 warning: `different_mod` redeclared with a different signature
-  --> $DIR/clashing-extern-fn.rs:84:9
+  --> $DIR/clashing-extern-fn.rs:72:9
    |
 LL |         fn different_mod(x: u8);
    |         ------------------------ `different_mod` previously declared here
@@ -82,7 +70,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:94:9
+  --> $DIR/clashing-extern-fn.rs:82:9
    |
 LL |     fn variadic_decl(x: u8, ...);
    |     ----------------------------- `variadic_decl` previously declared here
@@ -94,7 +82,7 @@ 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:154:13
+  --> $DIR/clashing-extern-fn.rs:142:13
    |
 LL |             fn weigh_banana(count: *const Banana) -> u64;
    |             --------------------------------------------- `weigh_banana` previously declared here
@@ -106,7 +94,7 @@ LL |             fn weigh_banana(count: *const Banana) -> u64;
               found `unsafe extern "C" fn(*const banana::three::Banana) -> u64`
 
 warning: `draw_point` redeclared with a different signature
-  --> $DIR/clashing-extern-fn.rs:183:13
+  --> $DIR/clashing-extern-fn.rs:171:13
    |
 LL |             fn draw_point(p: Point);
    |             ------------------------ `draw_point` previously declared here
@@ -117,5 +105,5 @@ LL |             fn draw_point(p: Point);
    = note: expected `unsafe extern "C" fn(sameish_members::a::Point)`
               found `unsafe extern "C" fn(sameish_members::b::Point)`
 
-warning: 9 warnings emitted
+warning: 8 warnings emitted
 

From 3eaead7d51f3d680bdae596e9c4ef00bdc03403e Mon Sep 17 00:00:00 2001
From: jumbatm <30644300+jumbatm@users.noreply.github.com>
Date: Fri, 3 Jul 2020 20:14:05 +1000
Subject: [PATCH 2/6] Don't emit clashing decl lint for FFI-safe enums.

An example of an FFI-safe enum conversion is when converting
Option<NonZeroUsize> to usize. Because the Some value must be non-zero,
rustc can use 0 to represent the None variant, making this conversion is
safe. Furthermore, it can be relied on (and removing this optimisation
already would be a breaking change).
---
 src/librustc_lint/builtin.rs               | 69 +++++++++++++++++++---
 src/librustc_lint/types.rs                 | 31 +++++-----
 src/test/ui/lint/clashing-extern-fn.stderr | 62 ++++++++++++++++++-
 3 files changed, 137 insertions(+), 25 deletions(-)

diff --git a/src/librustc_lint/builtin.rs b/src/librustc_lint/builtin.rs
index a45817beea164..47d1c7ba6f1f4 100644
--- a/src/librustc_lint/builtin.rs
+++ b/src/librustc_lint/builtin.rs
@@ -2155,14 +2155,16 @@ impl ClashingExternDeclarations {
             let a_kind = &a.kind;
             let b_kind = &b.kind;
 
+            use rustc_target::abi::LayoutOf;
+            let compare_layouts = |a, b| {
+                let a_layout = &cx.layout_of(a).unwrap().layout.abi;
+                let b_layout = &cx.layout_of(b).unwrap().layout.abi;
+                let result = a_layout == b_layout;
+                result
+            };
+
             match (a_kind, b_kind) {
-                (Adt(..), Adt(..)) => {
-                    // Adts are pretty straightforward: just compare the layouts.
-                    use rustc_target::abi::LayoutOf;
-                    let a_layout = cx.layout_of(a).unwrap().layout;
-                    let b_layout = cx.layout_of(b).unwrap().layout;
-                    a_layout == b_layout
-                }
+                (Adt(..), Adt(..)) => compare_layouts(a, b),
                 (Array(a_ty, a_const), Array(b_ty, b_const)) => {
                     // For arrays, we also check the constness of the type.
                     a_const.val == b_const.val
@@ -2179,10 +2181,10 @@ impl ClashingExternDeclarations {
                     a_mut == b_mut && Self::structurally_same_type(cx, a_ty, b_ty)
                 }
                 (FnDef(..), FnDef(..)) => {
-                    // As we don't compare regions, skip_binder is fine.
                     let a_poly_sig = a.fn_sig(tcx);
                     let b_poly_sig = b.fn_sig(tcx);
 
+                    // As we don't compare regions, skip_binder is fine.
                     let a_sig = a_poly_sig.skip_binder();
                     let b_sig = b_poly_sig.skip_binder();
 
@@ -2210,7 +2212,56 @@ impl ClashingExternDeclarations {
                 | (Opaque(..), Opaque(..)) => false,
                 // These definitely should have been caught above.
                 (Bool, Bool) | (Char, Char) | (Never, Never) | (Str, Str) => unreachable!(),
-                _ => false,
+
+                // Disjoint kinds.
+                (_, _) => {
+                    // First, check if the conversion is FFI-safe. This can be so if the type is an
+                    // enum with a non-null field (see improper_ctypes).
+                    let is_primitive_or_pointer =
+                        |ty: Ty<'tcx>| ty.is_primitive() || matches!(ty.kind, RawPtr(..));
+                    if (is_primitive_or_pointer(a) || is_primitive_or_pointer(b))
+                        && !(is_primitive_or_pointer(a) && is_primitive_or_pointer(b))
+                        && (matches!(a_kind, Adt(..)) || matches!(b_kind, Adt(..)))
+                    /* ie, 1 adt and 1 primitive */
+                    {
+                        let (primitive_ty, adt_ty) =
+                            if is_primitive_or_pointer(a) { (a, b) } else { (b, a) };
+                        // First, check that the Adt is FFI-safe to use.
+                        use crate::types::{ImproperCTypesMode, ImproperCTypesVisitor};
+                        let vis =
+                            ImproperCTypesVisitor { cx, mode: ImproperCTypesMode::Declarations };
+
+                        if let Adt(def, substs) = adt_ty.kind {
+                            let repr_nullable = vis.is_repr_nullable_ptr(adt_ty, def, substs);
+                            if let Some(safe_ty) = repr_nullable {
+                                let safe_ty_layout = &cx.layout_of(safe_ty).unwrap();
+                                let primitive_ty_layout = &cx.layout_of(primitive_ty).unwrap();
+
+                                use rustc_target::abi::Abi::*;
+                                match (&safe_ty_layout.abi, &primitive_ty_layout.abi) {
+                                    (Scalar(safe), Scalar(primitive)) => {
+                                        // The two types are safe to convert between if `safe` is
+                                        // the non-zero version of `primitive`.
+                                        use std::ops::RangeInclusive;
+
+                                        let safe_range: &RangeInclusive<_> = &safe.valid_range;
+                                        let primitive_range: &RangeInclusive<_> =
+                                            &primitive.valid_range;
+
+                                        return primitive_range.end() == safe_range.end()
+                                            // This check works for both signed and unsigned types due to wraparound.
+                                            && *safe_range.start() == 1
+                                            && *primitive_range.start() == 0;
+                                    }
+                                    _ => {}
+                                }
+                            }
+                        }
+                    }
+                    // Otherwise, just compare the layouts. This may be underapproximate, but at
+                    // the very least, will stop reads into uninitialised memory.
+                    compare_layouts(a, b)
+                }
             }
         }
     }
diff --git a/src/librustc_lint/types.rs b/src/librustc_lint/types.rs
index 1affc9457b89e..3d1080bea4c9c 100644
--- a/src/librustc_lint/types.rs
+++ b/src/librustc_lint/types.rs
@@ -509,14 +509,14 @@ declare_lint! {
 
 declare_lint_pass!(ImproperCTypesDefinitions => [IMPROPER_CTYPES_DEFINITIONS]);
 
-enum ImproperCTypesMode {
+crate enum ImproperCTypesMode {
     Declarations,
     Definitions,
 }
 
-struct ImproperCTypesVisitor<'a, 'tcx> {
-    cx: &'a LateContext<'tcx>,
-    mode: ImproperCTypesMode,
+crate struct ImproperCTypesVisitor<'a, 'tcx> {
+    crate cx: &'a LateContext<'tcx>,
+    crate mode: ImproperCTypesMode,
 }
 
 enum FfiResult<'tcx> {
@@ -562,17 +562,18 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
         }
     }
 
-    /// Check if this enum can be safely exported based on the "nullable pointer optimization".
-    /// Currently restricted to function pointers, boxes, references, `core::num::NonZero*`,
-    /// `core::ptr::NonNull`, and `#[repr(transparent)]` newtypes.
-    fn is_repr_nullable_ptr(
+    /// Check if this enum can be safely exported based on the "nullable pointer optimization". If
+    /// the type is it is, return the known non-null field type, else None.  Currently restricted
+    /// to function pointers, boxes, references, `core::num::NonZero*`, `core::ptr::NonNull`, and
+    /// `#[repr(transparent)]` newtypes.
+    crate fn is_repr_nullable_ptr(
         &self,
         ty: Ty<'tcx>,
         ty_def: &'tcx ty::AdtDef,
         substs: SubstsRef<'tcx>,
-    ) -> bool {
+    ) -> Option<Ty<'tcx>> {
         if ty_def.variants.len() != 2 {
-            return false;
+            return None;
         }
 
         let get_variant_fields = |index| &ty_def.variants[VariantIdx::new(index)].fields;
@@ -582,16 +583,16 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
         } else if variant_fields[1].is_empty() {
             &variant_fields[0]
         } else {
-            return false;
+            return None;
         };
 
         if fields.len() != 1 {
-            return false;
+            return None;
         }
 
         let field_ty = fields[0].ty(self.cx.tcx, substs);
         if !self.ty_is_known_nonnull(field_ty) {
-            return false;
+            return None;
         }
 
         // At this point, the field's type is known to be nonnull and the parent enum is
@@ -603,7 +604,7 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
             bug!("improper_ctypes: Option nonnull optimization not applied?");
         }
 
-        true
+        Some(field_ty)
     }
 
     /// Check if the type is array and emit an unsafe type lint.
@@ -753,7 +754,7 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
                         // discriminant.
                         if !def.repr.c() && !def.repr.transparent() && def.repr.int.is_none() {
                             // Special-case types like `Option<extern fn()>`.
-                            if !self.is_repr_nullable_ptr(ty, def, substs) {
+                            if self.is_repr_nullable_ptr(ty, def, substs).is_none() {
                                 return FfiUnsafe {
                                     ty,
                                     reason: "enum has no representation hint".into(),
diff --git a/src/test/ui/lint/clashing-extern-fn.stderr b/src/test/ui/lint/clashing-extern-fn.stderr
index a2aaaba717370..49a9d044a2fae 100644
--- a/src/test/ui/lint/clashing-extern-fn.stderr
+++ b/src/test/ui/lint/clashing-extern-fn.stderr
@@ -105,5 +105,65 @@ LL |             fn draw_point(p: Point);
    = note: expected `unsafe extern "C" fn(sameish_members::a::Point)`
               found `unsafe extern "C" fn(sameish_members::b::Point)`
 
-warning: 8 warnings emitted
+warning: `missing_return_type` redeclared with a different signature
+  --> $DIR/clashing-extern-fn.rs:208:13
+   |
+LL |             fn missing_return_type() -> usize;
+   |             ---------------------------------- `missing_return_type` previously declared here
+...
+LL |             fn missing_return_type();
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^ this signature doesn't match the previous declaration
+   |
+   = note: expected `unsafe extern "C" fn() -> usize`
+              found `unsafe extern "C" fn()`
+
+warning: `non_zero_usize` redeclared with a different signature
+  --> $DIR/clashing-extern-fn.rs:226:13
+   |
+LL |             fn non_zero_usize() -> core::num::NonZeroUsize;
+   |             ----------------------------------------------- `non_zero_usize` previously declared here
+...
+LL |             fn non_zero_usize() -> usize;
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ this signature doesn't match the previous declaration
+   |
+   = note: expected `unsafe extern "C" fn() -> std::num::NonZeroUsize`
+              found `unsafe extern "C" fn() -> usize`
+
+warning: `non_null_ptr` redeclared with a different signature
+  --> $DIR/clashing-extern-fn.rs:228:13
+   |
+LL |             fn non_null_ptr() -> core::ptr::NonNull<usize>;
+   |             ----------------------------------------------- `non_null_ptr` previously declared here
+...
+LL |             fn non_null_ptr() -> *const usize;
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ this signature doesn't match the previous declaration
+   |
+   = note: expected `unsafe extern "C" fn() -> std::ptr::NonNull<usize>`
+              found `unsafe extern "C" fn() -> *const usize`
+
+warning: `option_non_zero_usize_incorrect` redeclared with a different signature
+  --> $DIR/clashing-extern-fn.rs:254:13
+   |
+LL |             fn option_non_zero_usize_incorrect() -> usize;
+   |             ---------------------------------------------- `option_non_zero_usize_incorrect` previously declared here
+...
+LL |             fn option_non_zero_usize_incorrect() -> isize;
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ this signature doesn't match the previous declaration
+   |
+   = note: expected `unsafe extern "C" fn() -> usize`
+              found `unsafe extern "C" fn() -> isize`
+
+warning: `option_non_null_ptr_incorrect` redeclared with a different signature
+  --> $DIR/clashing-extern-fn.rs:256:13
+   |
+LL |             fn option_non_null_ptr_incorrect() -> *const usize;
+   |             --------------------------------------------------- `option_non_null_ptr_incorrect` previously declared here
+...
+LL |             fn option_non_null_ptr_incorrect() -> *const isize;
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ this signature doesn't match the previous declaration
+   |
+   = note: expected `unsafe extern "C" fn() -> *const usize`
+              found `unsafe extern "C" fn() -> *const isize`
+
+warning: 13 warnings emitted
 

From 5e52edca529ba5051d0bb752c751159d3057ab15 Mon Sep 17 00:00:00 2001
From: jumbatm <fungjys@gmail.com>
Date: Sat, 4 Jul 2020 12:07:43 +1000
Subject: [PATCH 3/6] Apply suggested wording changes from code review.

Co-authored-by: Teymour Aldridge <42674621+teymour-aldridge@users.noreply.github.com>
---
 src/librustc_lint/builtin.rs           | 2 +-
 src/librustc_lint/types.rs             | 2 +-
 src/test/ui/lint/clashing-extern-fn.rs | 8 ++++----
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/librustc_lint/builtin.rs b/src/librustc_lint/builtin.rs
index 47d1c7ba6f1f4..7b4c1a3f4bdc6 100644
--- a/src/librustc_lint/builtin.rs
+++ b/src/librustc_lint/builtin.rs
@@ -2215,7 +2215,7 @@ impl ClashingExternDeclarations {
 
                 // Disjoint kinds.
                 (_, _) => {
-                    // First, check if the conversion is FFI-safe. This can be so if the type is an
+                    // First, check if the conversion is FFI-safe. This can happen if the type is an
                     // enum with a non-null field (see improper_ctypes).
                     let is_primitive_or_pointer =
                         |ty: Ty<'tcx>| ty.is_primitive() || matches!(ty.kind, RawPtr(..));
diff --git a/src/librustc_lint/types.rs b/src/librustc_lint/types.rs
index 3d1080bea4c9c..a147e05e10dac 100644
--- a/src/librustc_lint/types.rs
+++ b/src/librustc_lint/types.rs
@@ -563,7 +563,7 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
     }
 
     /// Check if this enum can be safely exported based on the "nullable pointer optimization". If
-    /// the type is it is, return the known non-null field type, else None.  Currently restricted
+    /// it can, return the known non-null field type, otherwise return `None`. Currently restricted
     /// to function pointers, boxes, references, `core::num::NonZero*`, `core::ptr::NonNull`, and
     /// `#[repr(transparent)]` newtypes.
     crate fn is_repr_nullable_ptr(
diff --git a/src/test/ui/lint/clashing-extern-fn.rs b/src/test/ui/lint/clashing-extern-fn.rs
index 72c5d3da4b003..44c2067edc037 100644
--- a/src/test/ui/lint/clashing-extern-fn.rs
+++ b/src/test/ui/lint/clashing-extern-fn.rs
@@ -202,9 +202,9 @@ mod missing_return_type {
 
     mod b {
         extern "C" {
-            // This should warn, because we can't assume that the first declaration is the correct
-            // one -- if this one is the correct one, then calling the usize-returning version
-            // would allow reads into uninitialised memory.
+            // This should output a warning because we can't assume that the first declaration is
+            // the correct one -- if this one is the correct one, then calling the usize-returning
+            // version would allow reads into uninitialised memory.
             fn missing_return_type();
             //~^ WARN `missing_return_type` redeclared with a different signature
         }
@@ -220,7 +220,7 @@ mod non_zero_and_non_null {
     }
     mod b {
         extern "C" {
-            // For both of these cases, if there's a clash, you're either gaining an incorrect
+            // If there's a clash in either of these cases you're either gaining an incorrect
             // invariant that the value is non-zero, or you're missing out on that invariant. Both
             // cases are warning for, from both a caller-convenience and optimisation perspective.
             fn non_zero_usize() -> usize;

From 060666d0a4cb814328990d271ae9741f2d20cb0d Mon Sep 17 00:00:00 2001
From: jumbatm <30644300+jumbatm@users.noreply.github.com>
Date: Mon, 13 Jul 2020 23:06:35 +1000
Subject: [PATCH 4/6] Address code review comments.

- Make `is_repr_nullable_ptr` freestanding again to avoid usage of
ImproperCTypesVisitor in ClashingExternDeclarations (and don't
accidentally revert the ParamEnv::reveal_all() fix from a week earlier)
- Revise match condition for 1 Adt, 1 primitive
- Generalise check for non-null type so that it would also work for
ranges which exclude any single value (all bits set, for example)
- Make is_repr_nullable_ptr return the representable type instead of
just a boolean, to avoid adding an additional, independent "source of
truth" about the FFI-compatibility of Option-like enums. Also, rename to
`repr_nullable_ptr`.
---
 src/librustc_lint/builtin.rs               | 109 +++++-------
 src/librustc_lint/types.rs                 | 196 +++++++++++++--------
 src/librustc_middle/ty/sty.rs              |  15 +-
 src/test/ui/lint/clashing-extern-fn.rs     |   5 +
 src/test/ui/lint/clashing-extern-fn.stderr |  24 ++-
 5 files changed, 202 insertions(+), 147 deletions(-)

diff --git a/src/librustc_lint/builtin.rs b/src/librustc_lint/builtin.rs
index 7b4c1a3f4bdc6..7c4f893ce1d42 100644
--- a/src/librustc_lint/builtin.rs
+++ b/src/librustc_lint/builtin.rs
@@ -20,7 +20,9 @@
 //! If you define a new `LateLintPass`, you will also need to add it to the
 //! `late_lint_methods!` invocation in `lib.rs`.
 
-use crate::{EarlyContext, EarlyLintPass, LateContext, LateLintPass, LintContext};
+use crate::{
+    types::CItemKind, EarlyContext, EarlyLintPass, LateContext, LateLintPass, LintContext,
+};
 use rustc_ast::ast::{self, Expr};
 use rustc_ast::attr::{self, HasAttrs};
 use rustc_ast::tokenstream::{TokenStream, TokenTree};
@@ -2144,7 +2146,13 @@ impl ClashingExternDeclarations {
     /// Checks whether two types are structurally the same enough that the declarations shouldn't
     /// clash. We need this so we don't emit a lint when two modules both declare an extern struct,
     /// with the same members (as the declarations shouldn't clash).
-    fn structurally_same_type<'tcx>(cx: &LateContext<'tcx>, a: Ty<'tcx>, b: Ty<'tcx>) -> bool {
+    fn structurally_same_type<'tcx>(
+        cx: &LateContext<'tcx>,
+        a: Ty<'tcx>,
+        b: Ty<'tcx>,
+        ckind: CItemKind,
+    ) -> bool {
+        debug!("structurally_same_type(cx, a = {:?}, b = {:?})", a, b);
         let tcx = cx.tcx;
         if a == b || rustc_middle::ty::TyS::same_type(a, b) {
             // All nominally-same types are structurally same, too.
@@ -2156,29 +2164,30 @@ impl ClashingExternDeclarations {
             let b_kind = &b.kind;
 
             use rustc_target::abi::LayoutOf;
-            let compare_layouts = |a, b| {
-                let a_layout = &cx.layout_of(a).unwrap().layout.abi;
-                let b_layout = &cx.layout_of(b).unwrap().layout.abi;
-                let result = a_layout == b_layout;
-                result
+            let compare_layouts = |a, b| -> bool {
+                &cx.layout_of(a).unwrap().layout.abi == &cx.layout_of(b).unwrap().layout.abi
             };
 
+            #[allow(rustc::usage_of_ty_tykind)]
+            let is_primitive_or_pointer =
+                |kind: &ty::TyKind<'_>| kind.is_primitive() || matches!(kind, RawPtr(..));
+
             match (a_kind, b_kind) {
                 (Adt(..), Adt(..)) => compare_layouts(a, b),
                 (Array(a_ty, a_const), Array(b_ty, b_const)) => {
                     // For arrays, we also check the constness of the type.
                     a_const.val == b_const.val
-                        && Self::structurally_same_type(cx, a_const.ty, b_const.ty)
-                        && Self::structurally_same_type(cx, a_ty, b_ty)
+                        && Self::structurally_same_type(cx, a_const.ty, b_const.ty, ckind)
+                        && Self::structurally_same_type(cx, a_ty, b_ty, ckind)
                 }
-                (Slice(a_ty), Slice(b_ty)) => Self::structurally_same_type(cx, a_ty, b_ty),
+                (Slice(a_ty), Slice(b_ty)) => Self::structurally_same_type(cx, a_ty, b_ty, ckind),
                 (RawPtr(a_tymut), RawPtr(b_tymut)) => {
                     a_tymut.mutbl == a_tymut.mutbl
-                        && Self::structurally_same_type(cx, &a_tymut.ty, &b_tymut.ty)
+                        && Self::structurally_same_type(cx, &a_tymut.ty, &b_tymut.ty, ckind)
                 }
                 (Ref(_a_region, a_ty, a_mut), Ref(_b_region, b_ty, b_mut)) => {
                     // For structural sameness, we don't need the region to be same.
-                    a_mut == b_mut && Self::structurally_same_type(cx, a_ty, b_ty)
+                    a_mut == b_mut && Self::structurally_same_type(cx, a_ty, b_ty, ckind)
                 }
                 (FnDef(..), FnDef(..)) => {
                     let a_poly_sig = a.fn_sig(tcx);
@@ -2191,13 +2200,13 @@ impl ClashingExternDeclarations {
                     (a_sig.abi, a_sig.unsafety, a_sig.c_variadic)
                         == (b_sig.abi, b_sig.unsafety, b_sig.c_variadic)
                         && a_sig.inputs().iter().eq_by(b_sig.inputs().iter(), |a, b| {
-                            Self::structurally_same_type(cx, a, b)
+                            Self::structurally_same_type(cx, a, b, ckind)
                         })
-                        && Self::structurally_same_type(cx, a_sig.output(), b_sig.output())
+                        && Self::structurally_same_type(cx, a_sig.output(), b_sig.output(), ckind)
                 }
                 (Tuple(a_substs), Tuple(b_substs)) => {
                     a_substs.types().eq_by(b_substs.types(), |a_ty, b_ty| {
-                        Self::structurally_same_type(cx, a_ty, b_ty)
+                        Self::structurally_same_type(cx, a_ty, b_ty, ckind)
                     })
                 }
                 // For these, it's not quite as easy to define structural-sameness quite so easily.
@@ -2210,58 +2219,27 @@ impl ClashingExternDeclarations {
                 | (GeneratorWitness(..), GeneratorWitness(..))
                 | (Projection(..), Projection(..))
                 | (Opaque(..), Opaque(..)) => false,
+
                 // These definitely should have been caught above.
                 (Bool, Bool) | (Char, Char) | (Never, Never) | (Str, Str) => unreachable!(),
 
-                // Disjoint kinds.
-                (_, _) => {
-                    // First, check if the conversion is FFI-safe. This can happen if the type is an
-                    // enum with a non-null field (see improper_ctypes).
-                    let is_primitive_or_pointer =
-                        |ty: Ty<'tcx>| ty.is_primitive() || matches!(ty.kind, RawPtr(..));
-                    if (is_primitive_or_pointer(a) || is_primitive_or_pointer(b))
-                        && !(is_primitive_or_pointer(a) && is_primitive_or_pointer(b))
-                        && (matches!(a_kind, Adt(..)) || matches!(b_kind, Adt(..)))
-                    /* ie, 1 adt and 1 primitive */
-                    {
-                        let (primitive_ty, adt_ty) =
-                            if is_primitive_or_pointer(a) { (a, b) } else { (b, a) };
-                        // First, check that the Adt is FFI-safe to use.
-                        use crate::types::{ImproperCTypesMode, ImproperCTypesVisitor};
-                        let vis =
-                            ImproperCTypesVisitor { cx, mode: ImproperCTypesMode::Declarations };
-
-                        if let Adt(def, substs) = adt_ty.kind {
-                            let repr_nullable = vis.is_repr_nullable_ptr(adt_ty, def, substs);
-                            if let Some(safe_ty) = repr_nullable {
-                                let safe_ty_layout = &cx.layout_of(safe_ty).unwrap();
-                                let primitive_ty_layout = &cx.layout_of(primitive_ty).unwrap();
-
-                                use rustc_target::abi::Abi::*;
-                                match (&safe_ty_layout.abi, &primitive_ty_layout.abi) {
-                                    (Scalar(safe), Scalar(primitive)) => {
-                                        // The two types are safe to convert between if `safe` is
-                                        // the non-zero version of `primitive`.
-                                        use std::ops::RangeInclusive;
-
-                                        let safe_range: &RangeInclusive<_> = &safe.valid_range;
-                                        let primitive_range: &RangeInclusive<_> =
-                                            &primitive.valid_range;
-
-                                        return primitive_range.end() == safe_range.end()
-                                            // This check works for both signed and unsigned types due to wraparound.
-                                            && *safe_range.start() == 1
-                                            && *primitive_range.start() == 0;
-                                    }
-                                    _ => {}
-                                }
-                            }
-                        }
+                // An Adt and a primitive type. This can be FFI-safe is the ADT is an enum with a
+                // non-null field.
+                (Adt(..), other_kind) | (other_kind, Adt(..))
+                    if is_primitive_or_pointer(other_kind) =>
+                {
+                    let (primitive, adt) =
+                        if is_primitive_or_pointer(&a.kind) { (a, b) } else { (b, a) };
+                    if let Some(ty) = crate::types::repr_nullable_ptr(cx, adt, ckind) {
+                        ty == primitive
+                    } else {
+                        compare_layouts(a, b)
                     }
-                    // Otherwise, just compare the layouts. This may be underapproximate, but at
-                    // the very least, will stop reads into uninitialised memory.
-                    compare_layouts(a, b)
                 }
+                // 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),
             }
         }
     }
@@ -2282,7 +2260,12 @@ impl<'tcx> LateLintPass<'tcx> for ClashingExternDeclarations {
                     existing_hid, existing_decl_ty, this_fi.hir_id, this_decl_ty
                 );
                 // Check that the declarations match.
-                if !Self::structurally_same_type(cx, existing_decl_ty, this_decl_ty) {
+                if !Self::structurally_same_type(
+                    cx,
+                    existing_decl_ty,
+                    this_decl_ty,
+                    CItemKind::Declaration,
+                ) {
                     let orig_fi = tcx.hir().expect_foreign_item(existing_hid);
                     let orig = Self::name_of_extern_decl(tcx, orig_fi);
 
diff --git a/src/librustc_lint/types.rs b/src/librustc_lint/types.rs
index a147e05e10dac..a35c313d0b643 100644
--- a/src/librustc_lint/types.rs
+++ b/src/librustc_lint/types.rs
@@ -11,12 +11,13 @@ use rustc_index::vec::Idx;
 use rustc_middle::mir::interpret::{sign_extend, truncate};
 use rustc_middle::ty::layout::{IntegerExt, SizeSkeleton};
 use rustc_middle::ty::subst::SubstsRef;
-use rustc_middle::ty::{self, AdtKind, Ty, TypeFoldable};
+use rustc_middle::ty::{self, AdtKind, Ty, TyCtxt, TypeFoldable};
 use rustc_span::source_map;
 use rustc_span::symbol::sym;
 use rustc_span::{Span, DUMMY_SP};
+use rustc_target::abi::Abi;
 use rustc_target::abi::{Integer, LayoutOf, TagEncoding, VariantIdx, Variants};
-use rustc_target::spec::abi::Abi;
+use rustc_target::spec::abi::Abi as SpecAbi;
 
 use log::debug;
 use std::cmp;
@@ -509,14 +510,15 @@ declare_lint! {
 
 declare_lint_pass!(ImproperCTypesDefinitions => [IMPROPER_CTYPES_DEFINITIONS]);
 
-crate enum ImproperCTypesMode {
-    Declarations,
-    Definitions,
+#[derive(Clone, Copy)]
+crate enum CItemKind {
+    Declaration,
+    Definition,
 }
 
-crate struct ImproperCTypesVisitor<'a, 'tcx> {
-    crate cx: &'a LateContext<'tcx>,
-    crate mode: ImproperCTypesMode,
+struct ImproperCTypesVisitor<'a, 'tcx> {
+    cx: &'a LateContext<'tcx>,
+    mode: CItemKind,
 }
 
 enum FfiResult<'tcx> {
@@ -525,53 +527,87 @@ enum FfiResult<'tcx> {
     FfiUnsafe { ty: Ty<'tcx>, reason: String, help: Option<String> },
 }
 
-impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
-    /// Is type known to be non-null?
-    fn ty_is_known_nonnull(&self, ty: Ty<'tcx>) -> bool {
-        match ty.kind {
-            ty::FnPtr(_) => true,
-            ty::Ref(..) => true,
-            ty::Adt(def, _)
-                if def.is_box() && matches!(self.mode, ImproperCTypesMode::Definitions) =>
-            {
-                true
-            }
-            ty::Adt(def, substs) if def.repr.transparent() && !def.is_union() => {
-                let guaranteed_nonnull_optimization = self
-                    .cx
-                    .tcx
-                    .get_attrs(def.did)
-                    .iter()
-                    .any(|a| a.check_name(sym::rustc_nonnull_optimization_guaranteed));
-
-                if guaranteed_nonnull_optimization {
-                    return true;
-                }
+/// Is type known to be non-null?
+fn ty_is_known_nonnull<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>, mode: CItemKind) -> bool {
+    let tcx = cx.tcx;
+    match ty.kind {
+        ty::FnPtr(_) => true,
+        ty::Ref(..) => true,
+        ty::Adt(def, _) if def.is_box() && matches!(mode, CItemKind::Definition) => true,
+        ty::Adt(def, substs) if def.repr.transparent() && !def.is_union() => {
+            let guaranteed_nonnull_optimization = tcx
+                .get_attrs(def.did)
+                .iter()
+                .any(|a| a.check_name(sym::rustc_nonnull_optimization_guaranteed));
 
-                for variant in &def.variants {
-                    if let Some(field) = variant.transparent_newtype_field(self.cx.tcx) {
-                        if self.ty_is_known_nonnull(field.ty(self.cx.tcx, substs)) {
-                            return true;
-                        }
+            if guaranteed_nonnull_optimization {
+                return true;
+            }
+            for variant in &def.variants {
+                if let Some(field) = variant.transparent_newtype_field(tcx) {
+                    if ty_is_known_nonnull(cx, field.ty(tcx, substs), mode) {
+                        return true;
                     }
                 }
-
-                false
             }
-            _ => false,
+
+            false
         }
+        _ => false,
     }
+}
+/// Given a potentially non-null type `ty`, return its default, nullable type.
+fn get_nullable_type<'tcx>(tcx: TyCtxt<'tcx>, ty: Ty<'tcx>) -> Ty<'tcx> {
+    match ty.kind {
+        ty::Adt(field_def, field_substs) => {
+            let field_variants = &field_def.variants;
+            // We hit this case for #[repr(transparent)] structs with a single
+            // field.
+            debug_assert!(
+                field_variants.len() == 1 && field_variants[VariantIdx::new(0)].fields.len() == 1,
+                "inner ty not a newtype struct"
+            );
+            debug_assert!(field_def.repr.transparent(), "inner ty not transparent");
+            // As it's easy to get this wrong, it's worth noting that
+            // `inner_field_ty` is not the same as `field_ty`: Given Option<S>,
+            // where S is a transparent newtype of some type T, `field_ty`
+            // gives us S, while `inner_field_ty` is T.
+            let inner_field_ty =
+                field_def.variants[VariantIdx::new(0)].fields[0].ty(tcx, field_substs);
+            get_nullable_type(tcx, inner_field_ty)
+        }
+        ty::Int(ty) => tcx.mk_mach_int(ty),
+        ty::Uint(ty) => tcx.mk_mach_uint(ty),
+        ty::RawPtr(ty_mut) => tcx.mk_ptr(ty_mut),
+        // As these types are always non-null, the nullable equivalent of
+        // Option<T> of these types are their raw pointer counterparts.
+        ty::Ref(_region, ty, mutbl) => tcx.mk_ptr(ty::TypeAndMut { ty, mutbl }),
+        ty::FnPtr(..) => {
+            // There is no nullable equivalent for Rust's function pointers -- you
+            // must use an Option<fn(..) -> _> to represent it.
+            ty
+        }
 
-    /// Check if this enum can be safely exported based on the "nullable pointer optimization". If
-    /// it can, return the known non-null field type, otherwise return `None`. Currently restricted
-    /// to function pointers, boxes, references, `core::num::NonZero*`, `core::ptr::NonNull`, and
-    /// `#[repr(transparent)]` newtypes.
-    crate fn is_repr_nullable_ptr(
-        &self,
-        ty: Ty<'tcx>,
-        ty_def: &'tcx ty::AdtDef,
-        substs: SubstsRef<'tcx>,
-    ) -> Option<Ty<'tcx>> {
+        // We should only ever reach this case if ty_is_known_nonnull is extended
+        // to other types.
+        ref unhandled => {
+            unreachable!("Unhandled scalar kind: {:?} while checking {:?}", unhandled, ty)
+        }
+    }
+}
+
+/// Check if this enum can be safely exported based on the "nullable pointer optimization". If it
+/// can, return the the type that `ty` can be safely converted to, otherwise return `None`.
+/// Currently restricted to function pointers, boxes, references, `core::num::NonZero*`,
+/// `core::ptr::NonNull`, and `#[repr(transparent)]` newtypes.
+/// FIXME: This duplicates code in codegen.
+crate fn repr_nullable_ptr<'tcx>(
+    cx: &LateContext<'tcx>,
+    ty: Ty<'tcx>,
+    ckind: CItemKind,
+) -> Option<Ty<'tcx>> {
+    debug!("is_repr_nullable_ptr(cx, ty = {:?})", ty);
+    if let ty::Adt(ty_def, substs) = ty.kind {
         if ty_def.variants.len() != 2 {
             return None;
         }
@@ -590,23 +626,35 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
             return None;
         }
 
-        let field_ty = fields[0].ty(self.cx.tcx, substs);
-        if !self.ty_is_known_nonnull(field_ty) {
+        let field_ty = fields[0].ty(cx.tcx, substs);
+        if !ty_is_known_nonnull(cx, field_ty, ckind) {
             return None;
         }
 
-        // At this point, the field's type is known to be nonnull and the parent enum is
-        // Option-like. If the computed size for the field and the enum are different, the non-null
-        // optimization isn't being applied (and we've got a problem somewhere).
-        let compute_size_skeleton =
-            |t| SizeSkeleton::compute(t, self.cx.tcx, self.cx.param_env).unwrap();
+        // At this point, the field's type is known to be nonnull and the parent enum is Option-like.
+        // If the computed size for the field and the enum are different, the nonnull optimization isn't
+        // being applied (and we've got a problem somewhere).
+        let compute_size_skeleton = |t| SizeSkeleton::compute(t, cx.tcx, cx.param_env).unwrap();
         if !compute_size_skeleton(ty).same_size(compute_size_skeleton(field_ty)) {
             bug!("improper_ctypes: Option nonnull optimization not applied?");
         }
 
-        Some(field_ty)
+        // Return the nullable type this Option-like enum can be safely represented with.
+        let field_ty_abi = &cx.layout_of(field_ty).unwrap().abi;
+        if let Abi::Scalar(field_ty_scalar) = field_ty_abi {
+            match (field_ty_scalar.valid_range.start(), field_ty_scalar.valid_range.end()) {
+                (0, _) => bug!("Non-null optimisation extended to a non-zero value."),
+                (1, _) => {
+                    return Some(get_nullable_type(cx.tcx, field_ty));
+                }
+                (start, end) => unreachable!("Unhandled start and end range: ({}, {})", start, end),
+            };
+        }
     }
+    None
+}
 
+impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
     /// Check if the type is array and emit an unsafe type lint.
     fn check_for_array_ty(&mut self, sp: Span, ty: Ty<'tcx>) -> bool {
         if let ty::Array(..) = ty.kind {
@@ -687,7 +735,7 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
     fn check_type_for_ffi(&self, cache: &mut FxHashSet<Ty<'tcx>>, ty: Ty<'tcx>) -> FfiResult<'tcx> {
         use FfiResult::*;
 
-        let cx = self.cx.tcx;
+        let tcx = self.cx.tcx;
 
         // Protect against infinite recursion, for example
         // `struct S(*mut S);`.
@@ -698,9 +746,7 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
         }
 
         match ty.kind {
-            ty::Adt(def, _)
-                if def.is_box() && matches!(self.mode, ImproperCTypesMode::Definitions) =>
-            {
+            ty::Adt(def, _) if def.is_box() && matches!(self.mode, CItemKind::Definition) => {
                 FfiSafe
             }
 
@@ -754,7 +800,7 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
                         // discriminant.
                         if !def.repr.c() && !def.repr.transparent() && def.repr.int.is_none() {
                             // Special-case types like `Option<extern fn()>`.
-                            if self.is_repr_nullable_ptr(ty, def, substs).is_none() {
+                            if repr_nullable_ptr(self.cx, ty, self.mode).is_none() {
                                 return FfiUnsafe {
                                     ty,
                                     reason: "enum has no representation hint".into(),
@@ -837,7 +883,7 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
 
             ty::RawPtr(ty::TypeAndMut { ty, .. }) | ty::Ref(_, ty, _)
                 if {
-                    matches!(self.mode, ImproperCTypesMode::Definitions)
+                    matches!(self.mode, CItemKind::Definition)
                         && ty.is_sized(self.cx.tcx.at(DUMMY_SP), self.cx.param_env)
                 } =>
             {
@@ -863,7 +909,7 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
                     };
                 }
 
-                let sig = cx.erase_late_bound_regions(&sig);
+                let sig = tcx.erase_late_bound_regions(&sig);
                 if !sig.output().is_unit() {
                     let r = self.check_type_for_ffi(cache, sig.output());
                     match r {
@@ -895,9 +941,7 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
 
             // `extern "C" fn` functions can have type parameters, which may or may not be FFI-safe,
             //  so they are currently ignored for the purposes of this lint.
-            ty::Param(..) | ty::Projection(..)
-                if matches!(self.mode, ImproperCTypesMode::Definitions) =>
-            {
+            ty::Param(..) | ty::Projection(..) if matches!(self.mode, CItemKind::Definition) => {
                 FfiSafe
             }
 
@@ -922,14 +966,14 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
         help: Option<&str>,
     ) {
         let lint = match self.mode {
-            ImproperCTypesMode::Declarations => IMPROPER_CTYPES,
-            ImproperCTypesMode::Definitions => IMPROPER_CTYPES_DEFINITIONS,
+            CItemKind::Declaration => IMPROPER_CTYPES,
+            CItemKind::Definition => IMPROPER_CTYPES_DEFINITIONS,
         };
 
         self.cx.struct_span_lint(lint, sp, |lint| {
             let item_description = match self.mode {
-                ImproperCTypesMode::Declarations => "block",
-                ImproperCTypesMode::Definitions => "fn",
+                CItemKind::Declaration => "block",
+                CItemKind::Definition => "fn",
             };
             let mut diag = lint.build(&format!(
                 "`extern` {} uses type `{}`, which is not FFI-safe",
@@ -1053,8 +1097,12 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
         self.check_type_for_ffi_and_report_errors(span, ty, true, false);
     }
 
-    fn is_internal_abi(&self, abi: Abi) -> bool {
-        if let Abi::Rust | Abi::RustCall | Abi::RustIntrinsic | Abi::PlatformIntrinsic = abi {
+    fn is_internal_abi(&self, abi: SpecAbi) -> bool {
+        if let SpecAbi::Rust
+        | SpecAbi::RustCall
+        | SpecAbi::RustIntrinsic
+        | SpecAbi::PlatformIntrinsic = abi
+        {
             true
         } else {
             false
@@ -1064,7 +1112,7 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
 
 impl<'tcx> LateLintPass<'tcx> for ImproperCTypesDeclarations {
     fn check_foreign_item(&mut self, cx: &LateContext<'_>, it: &hir::ForeignItem<'_>) {
-        let mut vis = ImproperCTypesVisitor { cx, mode: ImproperCTypesMode::Declarations };
+        let mut vis = ImproperCTypesVisitor { cx, mode: CItemKind::Declaration };
         let abi = cx.tcx.hir().get_foreign_abi(it.hir_id);
 
         if !vis.is_internal_abi(abi) {
@@ -1099,7 +1147,7 @@ impl<'tcx> LateLintPass<'tcx> for ImproperCTypesDefinitions {
             _ => return,
         };
 
-        let mut vis = ImproperCTypesVisitor { cx, mode: ImproperCTypesMode::Definitions };
+        let mut vis = ImproperCTypesVisitor { cx, mode: CItemKind::Definition };
         if !vis.is_internal_abi(abi) {
             vis.check_foreign_fn(hir_id, decl);
         }
diff --git a/src/librustc_middle/ty/sty.rs b/src/librustc_middle/ty/sty.rs
index df8fa4d73ddf1..310ab4f7235eb 100644
--- a/src/librustc_middle/ty/sty.rs
+++ b/src/librustc_middle/ty/sty.rs
@@ -202,6 +202,16 @@ pub enum TyKind<'tcx> {
     Error(DelaySpanBugEmitted),
 }
 
+impl TyKind<'tcx> {
+    #[inline]
+    pub fn is_primitive(&self) -> bool {
+        match self {
+            Bool | Char | Int(_) | Uint(_) | Float(_) => true,
+            _ => false,
+        }
+    }
+}
+
 /// A type that is not publicly constructable. This prevents people from making `TyKind::Error`
 /// except through `tcx.err*()`.
 #[derive(Copy, Clone, Debug, Eq, Hash, PartialEq, PartialOrd, Ord)]
@@ -1766,10 +1776,7 @@ impl<'tcx> TyS<'tcx> {
 
     #[inline]
     pub fn is_primitive(&self) -> bool {
-        match self.kind {
-            Bool | Char | Int(_) | Uint(_) | Float(_) => true,
-            _ => false,
-        }
+        self.kind.is_primitive()
     }
 
     #[inline]
diff --git a/src/test/ui/lint/clashing-extern-fn.rs b/src/test/ui/lint/clashing-extern-fn.rs
index 44c2067edc037..e165a4f4658d9 100644
--- a/src/test/ui/lint/clashing-extern-fn.rs
+++ b/src/test/ui/lint/clashing-extern-fn.rs
@@ -181,6 +181,7 @@ mod transparent {
         use super::T;
         extern "C" {
             fn transparent() -> T;
+            fn transparent_incorrect() -> T;
         }
     }
 
@@ -189,6 +190,10 @@ mod transparent {
             // Shouldn't warn here, because repr(transparent) guarantees that T's layout is the
             // same as just the usize.
             fn transparent() -> usize;
+
+            // Should warn, because there's a signedness conversion here:
+            fn transparent_incorrect() -> isize;
+            //~^ WARN `transparent_incorrect` redeclared with a different signature
         }
     }
 }
diff --git a/src/test/ui/lint/clashing-extern-fn.stderr b/src/test/ui/lint/clashing-extern-fn.stderr
index 49a9d044a2fae..31810a2998ac8 100644
--- a/src/test/ui/lint/clashing-extern-fn.stderr
+++ b/src/test/ui/lint/clashing-extern-fn.stderr
@@ -105,8 +105,20 @@ LL |             fn draw_point(p: Point);
    = note: expected `unsafe extern "C" fn(sameish_members::a::Point)`
               found `unsafe extern "C" fn(sameish_members::b::Point)`
 
+warning: `transparent_incorrect` redeclared with a different signature
+  --> $DIR/clashing-extern-fn.rs:195:13
+   |
+LL |             fn transparent_incorrect() -> T;
+   |             -------------------------------- `transparent_incorrect` previously declared here
+...
+LL |             fn transparent_incorrect() -> isize;
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ this signature doesn't match the previous declaration
+   |
+   = note: expected `unsafe extern "C" fn() -> transparent::T`
+              found `unsafe extern "C" fn() -> isize`
+
 warning: `missing_return_type` redeclared with a different signature
-  --> $DIR/clashing-extern-fn.rs:208:13
+  --> $DIR/clashing-extern-fn.rs:213:13
    |
 LL |             fn missing_return_type() -> usize;
    |             ---------------------------------- `missing_return_type` previously declared here
@@ -118,7 +130,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:226:13
+  --> $DIR/clashing-extern-fn.rs:231:13
    |
 LL |             fn non_zero_usize() -> core::num::NonZeroUsize;
    |             ----------------------------------------------- `non_zero_usize` previously declared here
@@ -130,7 +142,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:228:13
+  --> $DIR/clashing-extern-fn.rs:233:13
    |
 LL |             fn non_null_ptr() -> core::ptr::NonNull<usize>;
    |             ----------------------------------------------- `non_null_ptr` previously declared here
@@ -142,7 +154,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:254:13
+  --> $DIR/clashing-extern-fn.rs:259:13
    |
 LL |             fn option_non_zero_usize_incorrect() -> usize;
    |             ---------------------------------------------- `option_non_zero_usize_incorrect` previously declared here
@@ -154,7 +166,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:256:13
+  --> $DIR/clashing-extern-fn.rs:261:13
    |
 LL |             fn option_non_null_ptr_incorrect() -> *const usize;
    |             --------------------------------------------------- `option_non_null_ptr_incorrect` previously declared here
@@ -165,5 +177,5 @@ LL |             fn option_non_null_ptr_incorrect() -> *const isize;
    = note: expected `unsafe extern "C" fn() -> *const usize`
               found `unsafe extern "C" fn() -> *const isize`
 
-warning: 13 warnings emitted
+warning: 14 warnings emitted
 

From 30a6f57308bf850891a2f4f47b9f1c325e2ac887 Mon Sep 17 00:00:00 2001
From: jumbatm <30644300+jumbatm@users.noreply.github.com>
Date: Wed, 15 Jul 2020 17:03:53 +1000
Subject: [PATCH 5/6] Handle structs with zst members.

---
 src/librustc_lint/types.rs | 51 +++++++++++++++++++++-----------------
 1 file changed, 28 insertions(+), 23 deletions(-)

diff --git a/src/librustc_lint/types.rs b/src/librustc_lint/types.rs
index a35c313d0b643..de750010ed1e6 100644
--- a/src/librustc_lint/types.rs
+++ b/src/librustc_lint/types.rs
@@ -11,7 +11,7 @@ use rustc_index::vec::Idx;
 use rustc_middle::mir::interpret::{sign_extend, truncate};
 use rustc_middle::ty::layout::{IntegerExt, SizeSkeleton};
 use rustc_middle::ty::subst::SubstsRef;
-use rustc_middle::ty::{self, AdtKind, Ty, TyCtxt, TypeFoldable};
+use rustc_middle::ty::{self, AdtKind, Ty, TypeFoldable};
 use rustc_span::source_map;
 use rustc_span::symbol::sym;
 use rustc_span::{Span, DUMMY_SP};
@@ -556,25 +556,26 @@ fn ty_is_known_nonnull<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>, mode: CItemKi
         _ => false,
     }
 }
-/// Given a potentially non-null type `ty`, return its default, nullable type.
-fn get_nullable_type<'tcx>(tcx: TyCtxt<'tcx>, ty: Ty<'tcx>) -> Ty<'tcx> {
-    match ty.kind {
+/// Given a non-null scalar (or transparent) type `ty`, return the nullable version of that type.
+/// If the type passed in was not scalar, returns None.
+fn get_nullable_type<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Option<Ty<'tcx>> {
+    let tcx = cx.tcx;
+    Some(match ty.kind {
         ty::Adt(field_def, field_substs) => {
-            let field_variants = &field_def.variants;
-            // We hit this case for #[repr(transparent)] structs with a single
-            // field.
-            debug_assert!(
-                field_variants.len() == 1 && field_variants[VariantIdx::new(0)].fields.len() == 1,
-                "inner ty not a newtype struct"
-            );
-            debug_assert!(field_def.repr.transparent(), "inner ty not transparent");
-            // As it's easy to get this wrong, it's worth noting that
-            // `inner_field_ty` is not the same as `field_ty`: Given Option<S>,
-            // where S is a transparent newtype of some type T, `field_ty`
-            // gives us S, while `inner_field_ty` is T.
-            let inner_field_ty =
-                field_def.variants[VariantIdx::new(0)].fields[0].ty(tcx, field_substs);
-            get_nullable_type(tcx, inner_field_ty)
+            let inner_field_ty = {
+                let first_non_zst_ty =
+                    field_def.variants.iter().filter_map(|v| v.transparent_newtype_field(tcx));
+                debug_assert_eq!(
+                    first_non_zst_ty.clone().count(),
+                    1,
+                    "Wrong number of fields for transparent type"
+                );
+                first_non_zst_ty
+                    .last()
+                    .expect("No non-zst fields in transparent type.")
+                    .ty(tcx, field_substs)
+            };
+            return get_nullable_type(cx, inner_field_ty);
         }
         ty::Int(ty) => tcx.mk_mach_int(ty),
         ty::Uint(ty) => tcx.mk_mach_uint(ty),
@@ -591,9 +592,13 @@ fn get_nullable_type<'tcx>(tcx: TyCtxt<'tcx>, ty: Ty<'tcx>) -> Ty<'tcx> {
         // We should only ever reach this case if ty_is_known_nonnull is extended
         // to other types.
         ref unhandled => {
-            unreachable!("Unhandled scalar kind: {:?} while checking {:?}", unhandled, ty)
+            debug!(
+                "get_nullable_type: Unhandled scalar kind: {:?} while checking {:?}",
+                unhandled, ty
+            );
+            return None;
         }
-    }
+    })
 }
 
 /// Check if this enum can be safely exported based on the "nullable pointer optimization". If it
@@ -643,9 +648,9 @@ crate fn repr_nullable_ptr<'tcx>(
         let field_ty_abi = &cx.layout_of(field_ty).unwrap().abi;
         if let Abi::Scalar(field_ty_scalar) = field_ty_abi {
             match (field_ty_scalar.valid_range.start(), field_ty_scalar.valid_range.end()) {
-                (0, _) => bug!("Non-null optimisation extended to a non-zero value."),
+                (0, _) => unreachable!("Non-null optimisation extended to a non-zero value."),
                 (1, _) => {
-                    return Some(get_nullable_type(cx.tcx, field_ty));
+                    return Some(get_nullable_type(cx, field_ty).unwrap());
                 }
                 (start, end) => unreachable!("Unhandled start and end range: ({}, {})", start, end),
             };

From 0bd292dea1a8f6e60ac8957ac0fcd0d237034918 Mon Sep 17 00:00:00 2001
From: jumbatm <30644300+jumbatm@users.noreply.github.com>
Date: Tue, 7 Jul 2020 21:08:14 +1000
Subject: [PATCH 6/6] Fix missed same-sized member clash in
 ClashingExternDeclarations.

---
 src/librustc_lint/builtin.rs               | 37 +++++++++++++++++++---
 src/test/ui/lint/clashing-extern-fn.rs     | 22 +++++++++++++
 src/test/ui/lint/clashing-extern-fn.stderr | 26 +++++++++++----
 3 files changed, 73 insertions(+), 12 deletions(-)

diff --git a/src/librustc_lint/builtin.rs b/src/librustc_lint/builtin.rs
index 7c4f893ce1d42..06e7c2b6f3625 100644
--- a/src/librustc_lint/builtin.rs
+++ b/src/librustc_lint/builtin.rs
@@ -38,14 +38,14 @@ use rustc_hir::def_id::DefId;
 use rustc_hir::{ForeignItemKind, GenericParamKind, PatKind};
 use rustc_hir::{HirId, HirIdSet, Node};
 use rustc_middle::lint::LintDiagnosticBuilder;
-use rustc_middle::ty::subst::GenericArgKind;
+use rustc_middle::ty::subst::{GenericArgKind, Subst};
 use rustc_middle::ty::{self, Ty, TyCtxt};
 use rustc_session::lint::FutureIncompatibleInfo;
 use rustc_span::edition::Edition;
 use rustc_span::source_map::Spanned;
 use rustc_span::symbol::{kw, sym, Ident, Symbol};
 use rustc_span::{BytePos, Span};
-use rustc_target::abi::VariantIdx;
+use rustc_target::abi::{LayoutOf, VariantIdx};
 use rustc_trait_selection::traits::misc::can_type_implement_copy;
 
 use crate::nonstandard_style::{method_context, MethodLateContext};
@@ -2163,9 +2163,11 @@ impl ClashingExternDeclarations {
             let a_kind = &a.kind;
             let b_kind = &b.kind;
 
-            use rustc_target::abi::LayoutOf;
             let compare_layouts = |a, b| -> bool {
-                &cx.layout_of(a).unwrap().layout.abi == &cx.layout_of(b).unwrap().layout.abi
+                let a_layout = &cx.layout_of(a).unwrap().layout.abi;
+                let b_layout = &cx.layout_of(b).unwrap().layout.abi;
+                debug!("{:?} == {:?} = {}", a_layout, b_layout, a_layout == b_layout);
+                a_layout == b_layout
             };
 
             #[allow(rustc::usage_of_ty_tykind)]
@@ -2173,7 +2175,32 @@ impl ClashingExternDeclarations {
                 |kind: &ty::TyKind<'_>| kind.is_primitive() || matches!(kind, RawPtr(..));
 
             match (a_kind, b_kind) {
-                (Adt(..), Adt(..)) => compare_layouts(a, b),
+                (Adt(_, a_substs), Adt(_, b_substs)) => {
+                    let a = a.subst(cx.tcx, a_substs);
+                    let b = b.subst(cx.tcx, b_substs);
+                    debug!("Comparing {:?} and {:?}", a, b);
+
+                    if let (Adt(a_def, ..), Adt(b_def, ..)) = (&a.kind, &b.kind) {
+                        // Grab a flattened representation of all fields.
+                        let a_fields = a_def.variants.iter().flat_map(|v| v.fields.iter());
+                        let b_fields = b_def.variants.iter().flat_map(|v| v.fields.iter());
+                        compare_layouts(a, b)
+                            && a_fields.eq_by(
+                                b_fields,
+                                |&ty::FieldDef { did: a_did, .. },
+                                 &ty::FieldDef { did: b_did, .. }| {
+                                    Self::structurally_same_type(
+                                        cx,
+                                        tcx.type_of(a_did),
+                                        tcx.type_of(b_did),
+                                        ckind,
+                                    )
+                                },
+                            )
+                    } else {
+                        unreachable!()
+                    }
+                }
                 (Array(a_ty, a_const), Array(b_ty, b_const)) => {
                     // For arrays, we also check the constness of the type.
                     a_const.val == b_const.val
diff --git a/src/test/ui/lint/clashing-extern-fn.rs b/src/test/ui/lint/clashing-extern-fn.rs
index e165a4f4658d9..d6ac7ccccc77b 100644
--- a/src/test/ui/lint/clashing-extern-fn.rs
+++ b/src/test/ui/lint/clashing-extern-fn.rs
@@ -174,6 +174,28 @@ mod sameish_members {
     }
 }
 
+mod same_sized_members_clash {
+    mod a {
+        #[repr(C)]
+        struct Point3 {
+            x: f32,
+            y: f32,
+            z: f32,
+        }
+        extern "C" { fn origin() -> Point3; }
+    }
+    mod b {
+        #[repr(C)]
+        struct Point3 {
+            x: i32,
+            y: i32,
+            z: i32, // NOTE: Incorrectly redeclared as i32
+        }
+        extern "C" { fn origin() -> Point3; }
+        //~^ WARN `origin` redeclared with a different signature
+    }
+}
+
 mod transparent {
     #[repr(transparent)]
     struct T(usize);
diff --git a/src/test/ui/lint/clashing-extern-fn.stderr b/src/test/ui/lint/clashing-extern-fn.stderr
index 31810a2998ac8..cca0c4c59eb19 100644
--- a/src/test/ui/lint/clashing-extern-fn.stderr
+++ b/src/test/ui/lint/clashing-extern-fn.stderr
@@ -105,8 +105,20 @@ LL |             fn draw_point(p: Point);
    = note: expected `unsafe extern "C" fn(sameish_members::a::Point)`
               found `unsafe extern "C" fn(sameish_members::b::Point)`
 
+warning: `origin` redeclared with a different signature
+  --> $DIR/clashing-extern-fn.rs:194:22
+   |
+LL |         extern "C" { fn origin() -> Point3; }
+   |                      ---------------------- `origin` previously declared here
+...
+LL |         extern "C" { fn origin() -> Point3; }
+   |                      ^^^^^^^^^^^^^^^^^^^^^^ this signature doesn't match the previous declaration
+   |
+   = note: expected `unsafe extern "C" fn() -> same_sized_members_clash::a::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:195:13
+  --> $DIR/clashing-extern-fn.rs:217:13
    |
 LL |             fn transparent_incorrect() -> T;
    |             -------------------------------- `transparent_incorrect` previously declared here
@@ -118,7 +130,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:213:13
+  --> $DIR/clashing-extern-fn.rs:235:13
    |
 LL |             fn missing_return_type() -> usize;
    |             ---------------------------------- `missing_return_type` previously declared here
@@ -130,7 +142,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:231:13
+  --> $DIR/clashing-extern-fn.rs:253:13
    |
 LL |             fn non_zero_usize() -> core::num::NonZeroUsize;
    |             ----------------------------------------------- `non_zero_usize` previously declared here
@@ -142,7 +154,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:233:13
+  --> $DIR/clashing-extern-fn.rs:255:13
    |
 LL |             fn non_null_ptr() -> core::ptr::NonNull<usize>;
    |             ----------------------------------------------- `non_null_ptr` previously declared here
@@ -154,7 +166,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:259:13
+  --> $DIR/clashing-extern-fn.rs:281:13
    |
 LL |             fn option_non_zero_usize_incorrect() -> usize;
    |             ---------------------------------------------- `option_non_zero_usize_incorrect` previously declared here
@@ -166,7 +178,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:261:13
+  --> $DIR/clashing-extern-fn.rs:283:13
    |
 LL |             fn option_non_null_ptr_incorrect() -> *const usize;
    |             --------------------------------------------------- `option_non_null_ptr_incorrect` previously declared here
@@ -177,5 +189,5 @@ LL |             fn option_non_null_ptr_incorrect() -> *const isize;
    = note: expected `unsafe extern "C" fn() -> *const usize`
               found `unsafe extern "C" fn() -> *const isize`
 
-warning: 14 warnings emitted
+warning: 15 warnings emitted