Rollup merge of rust-lang#60300 - mjbshaw:ffi_types, r=rkruppe

Allow null-pointer-optimized enums in FFI if their underlying representation is FFI safe

I'm not sure if this requires an RFC. I attempted to start [a discussion on internals.rust-lang.org](https://internals.rust-lang.org/t/options-ffi-safety-and-guarantees-for-abi-compatibility-with-nonnull-optimizations/9784) and when no one really objected I figured I'd go ahead and try implementing this.

This allows types like `Option<NonZeroU8>` to be used in FFI without triggering the `improper_ctypes` lint. This works by changing the `is_repr_nullable_ptr` function to consider an enum `E` to be FFI-safe if:

- `E` has no explicit `#[repr(...)]`.
- It only has two variants.
- One of those variants is empty (meaning it has no fields).
- The other variant has only one field.
- That field is one of the following:
  - `&T`
  - `&mut T`
  - `extern "C" fn`
  - `core::num::NonZero*`
  - `core::ptr::NonNull<T>`
  - `#[repr(transparent)] struct` wrapper around one of the types in this list.
- The size of `E` and its field are both known and are both the same size (implying `E` is participating in the nonnull optimization).

This logic seems consistent with [the Rust nomicon](https://doc.rust-lang.org/nomicon/repr-rust.html).

src/libcore/num/mod.rs

@@ -50,6 +50,7 @@ assert_eq!(size_of::<Option<core::num::", stringify!($Ty), ">>(), size_of::<", s
                 #[derive(Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash)]
                 #[repr(transparent)]
                 #[rustc_layout_scalar_valid_range_start(1)]
+                #[cfg_attr(not(stage0), rustc_nonnull_optimization_guaranteed)]
                 pub struct $Ty($Int);
             }
 

src/libcore/ptr.rs

@@ -2938,6 +2938,7 @@ impl<'a, T: ?Sized> From<NonNull<T>> for Unique<T> {
 #[stable(feature = "nonnull", since = "1.25.0")]
 #[repr(transparent)]
 #[rustc_layout_scalar_valid_range_start(1)]
+#[cfg_attr(not(stage0), rustc_nonnull_optimization_guaranteed)]
 pub struct NonNull<T: ?Sized> {
     pointer: *const T,
 }

src/librustc_lint/types.rs

@@ -1,10 +1,11 @@
 #![allow(non_snake_case)]
 
 use rustc::hir::{ExprKind, Node};
+use crate::hir::def_id::DefId;
 use rustc::hir::lowering::is_range_literal;
 use rustc::ty::subst::SubstsRef;
 use rustc::ty::{self, AdtKind, ParamEnv, Ty, TyCtxt};
-use rustc::ty::layout::{self, IntegerExt, LayoutOf, VariantIdx};
+use rustc::ty::layout::{self, IntegerExt, LayoutOf, VariantIdx, SizeSkeleton};
 use rustc::{lint, util};
 use rustc_data_structures::indexed_vec::Idx;
 use util::nodemap::FxHashSet;
@@ -14,11 +15,11 @@ use lint::{LintPass, LateLintPass};
 use std::cmp;
 use std::{i8, i16, i32, i64, u8, u16, u32, u64, f32, f64};
 
-use syntax::{ast, attr};
+use syntax::{ast, attr, source_map};
 use syntax::errors::Applicability;
+use syntax::symbol::sym;
 use rustc_target::spec::abi::Abi;
 use syntax_pos::Span;
-use syntax::source_map;
 
 use rustc::hir;
 
@@ -522,42 +523,79 @@ enum FfiResult<'tcx> {
     },
 }
 
+fn is_zst<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, did: DefId, ty: Ty<'tcx>) -> bool {
+    tcx.layout_of(tcx.param_env(did).and(ty)).map(|layout| layout.is_zst()).unwrap_or(false)
+}
+
+fn ty_is_known_nonnull<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, ty: Ty<'tcx>) -> bool {
+    match ty.sty {
+        ty::FnPtr(_) => true,
+        ty::Ref(..) => true,
+        ty::Adt(field_def, substs) if field_def.repr.transparent() && field_def.is_struct() => {
+            for field in &field_def.non_enum_variant().fields {
+                let field_ty = tcx.normalize_erasing_regions(
+                    ParamEnv::reveal_all(),
+                    field.ty(tcx, substs),
+                );
+                if is_zst(tcx, field.did, field_ty) {
+                    continue;
+                }
+
+                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(tcx, field_ty) {
+                    return true;
+                }
+            }
+
+            false
+        }
+        _ => false,
+    }
+}
+
 /// Check if this enum can be safely exported based on the
 /// "nullable pointer optimization". Currently restricted
-/// to function pointers and references, but could be
-/// expanded to cover NonZero raw pointers and newtypes.
+/// to function pointers, references, core::num::NonZero*,
+/// core::ptr::NonNull, and #[repr(transparent)] newtypes.
 /// FIXME: This duplicates code in codegen.
 fn is_repr_nullable_ptr<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
-                                  def: &'tcx ty::AdtDef,
+                                  ty: Ty<'tcx>,
+                                  ty_def: &'tcx ty::AdtDef,
                                   substs: SubstsRef<'tcx>)
                                   -> bool {
-    if def.variants.len() == 2 {
-        let data_idx;
+    if ty_def.variants.len() != 2 {
+        return false;
+    }
 
-        let zero = VariantIdx::new(0);
-        let one = VariantIdx::new(1);
+    let get_variant_fields = |index| &ty_def.variants[VariantIdx::new(index)].fields;
+    let variant_fields = [get_variant_fields(0), get_variant_fields(1)];
+    let fields = if variant_fields[0].is_empty() {
+        &variant_fields[1]
+    } else if variant_fields[1].is_empty() {
+        &variant_fields[0]
+    } else {
+        return false;
+    };
 
-        if def.variants[zero].fields.is_empty() {
-            data_idx = one;
-        } else if def.variants[one].fields.is_empty() {
-            data_idx = zero;
-        } else {
-            return false;
-        }
+    if fields.len() != 1 {
+        return false;
+    }
 
-        if def.variants[data_idx].fields.len() == 1 {
-            match def.variants[data_idx].fields[0].ty(tcx, substs).sty {
-                ty::FnPtr(_) => {
-                    return true;
-                }
-                ty::Ref(..) => {
-                    return true;
-                }
-                _ => {}
-            }
-        }
+    let field_ty = fields[0].ty(tcx, substs);
+    if !ty_is_known_nonnull(tcx, field_ty) {
+        return false;
     }
-    false
+
+    // 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, tcx, ParamEnv::reveal_all()).unwrap();
+    if !compute_size_skeleton(ty).same_size(compute_size_skeleton(field_ty)) {
+        bug!("improper_ctypes: Option nonnull optimization not applied?");
+    }
+
+    true
 }
 
 impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
@@ -612,14 +650,8 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
                             );
                             // repr(transparent) types are allowed to have arbitrary ZSTs, not just
                             // PhantomData -- skip checking all ZST fields
-                            if def.repr.transparent() {
-                                let is_zst = cx
-                                    .layout_of(cx.param_env(field.did).and(field_ty))
-                                    .map(|layout| layout.is_zst())
-                                    .unwrap_or(false);
-                                if is_zst {
-                                    continue;
-                                }
+                            if def.repr.transparent() && is_zst(cx, field.did, field_ty) {
+                                continue;
                             }
                             let r = self.check_type_for_ffi(cache, field_ty);
                             match r {
@@ -682,7 +714,7 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
                         // discriminant.
                         if !def.repr.c() && def.repr.int.is_none() {
                             // Special-case types like `Option<extern fn()>`.
-                            if !is_repr_nullable_ptr(cx, def, substs) {
+                            if !is_repr_nullable_ptr(cx, ty, def, substs) {
                                 return FfiUnsafe {
                                     ty: ty,
                                     reason: "enum has no representation hint",

src/libsyntax/feature_gate.rs

@@ -1134,6 +1134,13 @@ pub const BUILTIN_ATTRIBUTES: &[BuiltinAttribute] = &[
             is just used to enable niche optimizations in libcore \
             and will never be stable",
         cfg_fn!(rustc_attrs))),
+    (sym::rustc_nonnull_optimization_guaranteed, Whitelisted, template!(Word),
+    Gated(Stability::Unstable,
+        sym::rustc_attrs,
+        "the `#[rustc_nonnull_optimization_guaranteed]` attribute \
+            is just used to enable niche optimizations in libcore \
+            and will never be stable",
+        cfg_fn!(rustc_attrs))),
     (sym::rustc_regions, Normal, template!(Word), Gated(Stability::Unstable,
                                     sym::rustc_attrs,
                                     "the `#[rustc_regions]` attribute \

src/libsyntax_pos/symbol.rs

@@ -491,6 +491,7 @@ symbols! {
         rustc_layout_scalar_valid_range_end,
         rustc_layout_scalar_valid_range_start,
         rustc_mir,
+        rustc_nonnull_optimization_guaranteed,
         rustc_object_lifetime_default,
         rustc_on_unimplemented,
         rustc_outlives,

src/test/ui/feature-gates/feature-gate-rustc-attrs-1.rs

@@ -4,5 +4,6 @@
 
 #[rustc_variance] //~ ERROR the `#[rustc_variance]` attribute is just used for rustc unit tests and will never be stable
 #[rustc_error] //~ ERROR the `#[rustc_error]` attribute is just used for rustc unit tests and will never be stable
+#[rustc_nonnull_optimization_guaranteed] //~ ERROR the `#[rustc_nonnull_optimization_guaranteed]` attribute is just used to enable niche optimizations in libcore and will never be stable
 
 fn main() {}

src/test/ui/feature-gates/feature-gate-rustc-attrs-1.stderr

@@ -16,6 +16,15 @@ LL | #[rustc_error]
    = note: for more information, see https://github.com/rust-lang/rust/issues/29642
    = help: add #![feature(rustc_attrs)] to the crate attributes to enable
 
-error: aborting due to 2 previous errors
+error[E0658]: the `#[rustc_nonnull_optimization_guaranteed]` attribute is just used to enable niche optimizations in libcore and will never be stable
+  --> $DIR/feature-gate-rustc-attrs-1.rs:7:1
+   |
+LL | #[rustc_nonnull_optimization_guaranteed]
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: for more information, see https://github.com/rust-lang/rust/issues/29642
+   = help: add #![feature(rustc_attrs)] to the crate attributes to enable
+
+error: aborting due to 3 previous errors
 
 For more information about this error, try `rustc --explain E0658`.

src/test/ui/lint/lint-ctypes-enum.rs

@@ -1,6 +1,8 @@
 #![deny(improper_ctypes)]
 #![allow(dead_code)]
 
+use std::num;
+
 enum Z { }
 enum U { A }
 enum B { C, D }
@@ -15,14 +17,39 @@ enum U8 { A, B, C }
 #[repr(isize)]
 enum Isize { A, B, C }
 
+#[repr(transparent)]
+struct Transparent<T>(T, std::marker::PhantomData<Z>);
+
+struct Rust<T>(T);
+
 extern {
    fn zf(x: Z);
    fn uf(x: U); //~ ERROR enum has no representation hint
    fn bf(x: B); //~ ERROR enum has no representation hint
    fn tf(x: T); //~ ERROR enum has no representation hint
-   fn reprc(x: ReprC);
-   fn u8(x: U8);
-   fn isize(x: Isize);
+   fn repr_c(x: ReprC);
+   fn repr_u8(x: U8);
+   fn repr_isize(x: Isize);
+   fn option_ref(x: Option<&'static u8>);
+   fn option_fn(x: Option<extern "C" fn()>);
+   fn nonnull(x: Option<std::ptr::NonNull<u8>>);
+   fn nonzero_u8(x: Option<num::NonZeroU8>);
+   fn nonzero_u16(x: Option<num::NonZeroU16>);
+   fn nonzero_u32(x: Option<num::NonZeroU32>);
+   fn nonzero_u64(x: Option<num::NonZeroU64>);
+   fn nonzero_u128(x: Option<num::NonZeroU128>);
+   //~^ ERROR 128-bit integers don't currently have a known stable ABI
+   fn nonzero_usize(x: Option<num::NonZeroUsize>);
+   fn nonzero_i8(x: Option<num::NonZeroI8>);
+   fn nonzero_i16(x: Option<num::NonZeroI16>);
+   fn nonzero_i32(x: Option<num::NonZeroI32>);
+   fn nonzero_i64(x: Option<num::NonZeroI64>);
+   fn nonzero_i128(x: Option<num::NonZeroI128>);
+   //~^ ERROR 128-bit integers don't currently have a known stable ABI
+   fn nonzero_isize(x: Option<num::NonZeroIsize>);
+   fn repr_transparent(x: Option<Transparent<num::NonZeroU8>>);
+   fn repr_rust(x: Option<Rust<num::NonZeroU8>>); //~ ERROR enum has no representation hint
+   fn no_result(x: Result<(), num::NonZeroI32>); //~ ERROR enum has no representation hint
 }
 
 pub fn main() { }

src/test/ui/lint/lint-ctypes-enum.stderr

@@ -1,5 +1,5 @@
 error: `extern` block uses type `U` which is not FFI-safe: enum has no representation hint
-  --> $DIR/lint-ctypes-enum.rs:20:13
+  --> $DIR/lint-ctypes-enum.rs:27:13
    |
 LL |    fn uf(x: U);
    |             ^
@@ -11,36 +11,64 @@ LL | #![deny(improper_ctypes)]
    |         ^^^^^^^^^^^^^^^
    = help: consider adding a #[repr(...)] attribute to this enum
 note: type defined here
-  --> $DIR/lint-ctypes-enum.rs:5:1
+  --> $DIR/lint-ctypes-enum.rs:7:1
    |
 LL | enum U { A }
    | ^^^^^^^^^^^^
 
 error: `extern` block uses type `B` which is not FFI-safe: enum has no representation hint
-  --> $DIR/lint-ctypes-enum.rs:21:13
+  --> $DIR/lint-ctypes-enum.rs:28:13
    |
 LL |    fn bf(x: B);
    |             ^
    |
    = help: consider adding a #[repr(...)] attribute to this enum
 note: type defined here
-  --> $DIR/lint-ctypes-enum.rs:6:1
+  --> $DIR/lint-ctypes-enum.rs:8:1
    |
 LL | enum B { C, D }
    | ^^^^^^^^^^^^^^^
 
 error: `extern` block uses type `T` which is not FFI-safe: enum has no representation hint
-  --> $DIR/lint-ctypes-enum.rs:22:13
+  --> $DIR/lint-ctypes-enum.rs:29:13
    |
 LL |    fn tf(x: T);
    |             ^
    |
    = help: consider adding a #[repr(...)] attribute to this enum
 note: type defined here
-  --> $DIR/lint-ctypes-enum.rs:7:1
+  --> $DIR/lint-ctypes-enum.rs:9:1
    |
 LL | enum T { E, F, G }
    | ^^^^^^^^^^^^^^^^^^
 
-error: aborting due to 3 previous errors
+error: `extern` block uses type `u128` which is not FFI-safe: 128-bit integers don't currently have a known stable ABI
+  --> $DIR/lint-ctypes-enum.rs:40:23
+   |
+LL |    fn nonzero_u128(x: Option<num::NonZeroU128>);
+   |                       ^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: `extern` block uses type `i128` which is not FFI-safe: 128-bit integers don't currently have a known stable ABI
+  --> $DIR/lint-ctypes-enum.rs:47:23
+   |
+LL |    fn nonzero_i128(x: Option<num::NonZeroI128>);
+   |                       ^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: `extern` block uses type `std::option::Option<Rust<std::num::NonZeroU8>>` which is not FFI-safe: enum has no representation hint
+  --> $DIR/lint-ctypes-enum.rs:51:20
+   |
+LL |    fn repr_rust(x: Option<Rust<num::NonZeroU8>>);
+   |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: consider adding a #[repr(...)] attribute to this enum
+
+error: `extern` block uses type `std::result::Result<(), std::num::NonZeroI32>` which is not FFI-safe: enum has no representation hint
+  --> $DIR/lint-ctypes-enum.rs:52:20
+   |
+LL |    fn no_result(x: Result<(), num::NonZeroI32>);
+   |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = help: consider adding a #[repr(...)] attribute to this enum
+
+error: aborting due to 7 previous errors