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

@@ -2142,13 +2142,14 @@ 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)) => {
@@ -2196,58 +2197,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) {
+                        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),
             }
         }
     }

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,14 @@ declare_lint! {
 
 declare_lint_pass!(ImproperCTypesDefinitions => [IMPROPER_CTYPES_DEFINITIONS]);
 
-crate enum ImproperCTypesMode {
+enum ImproperCTypesMode {
     Declarations,
     Definitions,
 }
 
-crate struct ImproperCTypesVisitor<'a, 'tcx> {
-    crate cx: &'a LateContext<'tcx>,
-    crate mode: ImproperCTypesMode,
+struct ImproperCTypesVisitor<'a, 'tcx> {
+    cx: &'a LateContext<'tcx>,
+    mode: ImproperCTypesMode,
 }
 
 enum FfiResult<'tcx> {
@@ -525,48 +526,78 @@ 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(field_def, substs) if field_def.repr.transparent() && !field_def.is_union() => {
-                for field in field_def.all_fields() {
-                    let field_ty = self.cx.tcx.normalize_erasing_regions(
-                        self.cx.param_env,
-                        field.ty(self.cx.tcx, substs),
-                    );
-                    if field_ty.is_zst(self.cx.tcx, field.did) {
-                        continue;
-                    }
-
-                    let attrs = self.cx.tcx.get_attrs(field_def.did);
-                    if attrs
-                        .iter()
-                        .any(|a| a.check_name(sym::rustc_nonnull_optimization_guaranteed))
-                        || self.ty_is_known_nonnull(field_ty)
-                    {
-                        return true;
-                    }
+fn ty_is_known_nonnull<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
+    let tcx = cx.tcx;
+    match ty.kind {
+        ty::FnPtr(_) => true,
+        ty::Ref(..) => true,
+        ty::Adt(field_def, substs) if field_def.repr.transparent() && !field_def.is_union() => {
+            for field in field_def.all_fields() {
+                let field_ty = tcx.normalize_erasing_regions(cx.param_env, field.ty(tcx, substs));
+                if field_ty.is_zst(tcx, field.did) {
+                    continue;
                 }
 
-                false
+                let attrs = tcx.get_attrs(field_def.did);
+                if attrs.iter().any(|a| a.check_name(sym::rustc_nonnull_optimization_guaranteed))
+                    || ty_is_known_nonnull(cx, field_ty)
+                {
+                    return true;
+                }
             }
-            _ => 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".
-    /// Currently restricted to function pointers, references, `core::num::NonZero*`,
-    /// `core::ptr::NonNull`, and `#[repr(transparent)]` newtypes. If it can, return the known
-    /// non-null field type, otherwise return `None`.
-    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 `ty` can be safely exported based on the "nullable pointer optimization".
+/// Currently restricted to function pointers, references, `core::num::NonZero*`,
+/// `core::ptr::NonNull`, and `#[repr(transparent)]` newtypes. If it can, return the nullable type
+/// that `ty` can be converted to, else None.
+/// FIXME: This duplicates code in codegen.
+crate fn repr_nullable_ptr<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> 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;
         }
@@ -585,23 +616,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) {
             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 {
@@ -682,7 +725,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);`.
@@ -743,7 +786,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).is_none() {
                                 return FfiUnsafe {
                                     ty,
                                     reason: "enum has no representation hint".into(),
@@ -852,7 +895,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 {
@@ -1042,8 +1085,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

src/librustc_middle/ty/sty.rs

@@ -206,6 +206,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)]
@@ -1710,10 +1720,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]

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
         }
     }
 }