Don't emit clashing decl lint for FFI-safe enums.

jumbatm · jumbatm · commit 0e61890f529c · 2020-07-13T23:06:13.000+10:00
An example of an FFI-safe enum conversion is when converting
Option&lt;NonZeroUsize&gt; 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).
diff --git a/src/librustc_lint/builtin.rs b/src/librustc_lint/builtin.rs
@@ -2141,14 +2141,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
@@ -2165,10 +2167,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();
 
@@ -2196,7 +2198,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
@@ -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> {
@@ -559,15 +559,16 @@ 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, references, `core::num::NonZero*`,
-    /// `core::ptr::NonNull`, and `#[repr(transparent)]` newtypes.
-    fn is_repr_nullable_ptr(
+    /// `core::ptr::NonNull`, and `#[repr(transparent)]` newtypes. If it is, return the known
+    /// non-null field type, else None.
+    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;
@@ -577,16 +578,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
@@ -598,7 +599,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.
@@ -742,7 +743,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
@@ -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
 
