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`.

Author: jumbatm

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);