lint: transitive FFI-safety for transparent types

This commit ensures that if a `repr(transparent)` newtype's only
non-zero-sized field is FFI-safe then the newtype is also FFI-safe.

Previously, ZSTs were ignored for the purposes of linting FFI-safety
in transparent structs - thus, only the single non-ZST would be checked
for FFI-safety. However, if the non-zero-sized field is a generic
parameter, and is substituted for a ZST, then the type would be
considered FFI-unsafe (as when every field is thought to be zero-sized,
the type is considered to be "composed only of `PhantomData`" which is
FFI-unsafe).

In this commit, for transparent structs, the non-zero-sized field is
identified (before any substitutions are applied, necessarily) and then
that field's type (now with substitutions) is checked for FFI-safety
(where previously it would have been skipped for being zero-sized in
this case).

To handle the case where the non-zero-sized field is a generic
parameter, which is substituted for `()` (a ZST), and is being used
as a return type - the `FfiUnsafe` result (previously `FfiPhantom`) is
caught and silenced.

Signed-off-by: David Wood <david@davidtw.co>

5 files changed, 69 insertions, 51 deletions

diff --git a/src/librustc_lint/types.rs b/src/librustc_lint/types.rs
index b60867b3d2d..cdb0eda645a 100644
--- a/src/librustc_lint/types.rs
+++ b/src/librustc_lint/types.rs
@@ -6,7 +6,6 @@ use rustc_attr as attr;
 use rustc_data_structures::fx::FxHashSet;
 use rustc_errors::Applicability;
 use rustc_hir as hir;
-use rustc_hir::def_id::DefId;
 use rustc_hir::{is_range_literal, ExprKind, Node};
 use rustc_index::vec::Idx;
 use rustc_middle::mir::interpret::{sign_extend, truncate};
@@ -511,10 +510,6 @@ enum FfiResult<'tcx> {
     FfiUnsafe { ty: Ty<'tcx>, reason: &'static str, help: Option<&'static str> },
 }
 
-fn is_zst<'tcx>(tcx: TyCtxt<'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<'tcx>(tcx: TyCtxt<'tcx>, ty: Ty<'tcx>) -> bool {
     match ty.kind {
         ty::FnPtr(_) => true,
@@ -523,7 +518,7 @@ fn ty_is_known_nonnull<'tcx>(tcx: TyCtxt<'tcx>, ty: Ty<'tcx>) -> bool {
             for field in field_def.all_fields() {
                 let field_ty =
                     tcx.normalize_erasing_regions(ParamEnv::reveal_all(), field.ty(tcx, substs));
-                if is_zst(tcx, field.did, field_ty) {
+                if field_ty.is_zst(tcx, field.did) {
                     continue;
                 }
 
@@ -653,32 +648,43 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
                             };
                         }
 
-                        // We can't completely trust repr(C) and repr(transparent) markings;
-                        // make sure the fields are actually safe.
-                        let mut all_phantom = true;
-                        for field in &def.non_enum_variant().fields {
-                            let field_ty = cx.normalize_erasing_regions(
-                                ParamEnv::reveal_all(),
-                                field.ty(cx, substs),
-                            );
-                            // repr(transparent) types are allowed to have arbitrary ZSTs, not just
-                            // PhantomData -- skip checking all ZST fields
-                            if def.repr.transparent() && is_zst(cx, field.did, field_ty) {
-                                continue;
+                        if def.repr.transparent() {
+                            // Can assume that only one field is not a ZST, so only check
+                            // that field's type for FFI-safety.
+                            if let Some(field) =
+                                def.transparent_newtype_field(cx, self.cx.param_env)
+                            {
+                                let field_ty = cx.normalize_erasing_regions(
+                                    self.cx.param_env,
+                                    field.ty(cx, substs),
+                                );
+                                self.check_type_for_ffi(cache, field_ty)
+                            } else {
+                                FfiSafe
                             }
-                            let r = self.check_type_for_ffi(cache, field_ty);
-                            match r {
-                                FfiSafe => {
-                                    all_phantom = false;
-                                }
-                                FfiPhantom(..) => {}
-                                FfiUnsafe { .. } => {
-                                    return r;
+                        } else {
+                            // We can't completely trust repr(C) markings; make sure the fields are
+                            // actually safe.
+                            let mut all_phantom = true;
+                            for field in &def.non_enum_variant().fields {
+                                let field_ty = cx.normalize_erasing_regions(
+                                    self.cx.param_env,
+                                    field.ty(cx, substs),
+                                );
+                                let r = self.check_type_for_ffi(cache, field_ty);
+                                match r {
+                                    FfiSafe => {
+                                        all_phantom = false;
+                                    }
+                                    FfiPhantom(..) => {}
+                                    FfiUnsafe { .. } => {
+                                        return r;
+                                    }
                                 }
                             }
-                        }
 
-                        if all_phantom { FfiPhantom(ty) } else { FfiSafe }
+                            if all_phantom { FfiPhantom(ty) } else { FfiSafe }
+                        }
                     }
                     AdtKind::Union => {
                         if !def.repr.c() && !def.repr.transparent() {
@@ -708,7 +714,7 @@ 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() && is_zst(cx, field.did, field_ty) {
+                            if def.repr.transparent() && field_ty.is_zst(cx, field.did) {
                                 continue;
                             }
                             let r = self.check_type_for_ffi(cache, field_ty);
@@ -774,7 +780,7 @@ 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() && is_zst(cx, field.did, field_ty) {
+                                if def.repr.transparent() && field_ty.is_zst(cx, field.did) {
                                     continue;
                                 }
                                 let r = self.check_type_for_ffi(cache, field_ty);
@@ -983,6 +989,9 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
             FfiResult::FfiPhantom(ty) => {
                 self.emit_ffi_unsafe_type_lint(ty, sp, "composed only of `PhantomData`", None);
             }
+            // If `ty` is a `repr(transparent)` newtype, and the non-zero-sized type is a generic
+            // argument, which after substitution, is `()`, then this branch can be hit.
+            FfiResult::FfiUnsafe { ty, .. } if is_return_type && ty.is_unit() => return,
             FfiResult::FfiUnsafe { ty, reason, help } => {
                 self.emit_ffi_unsafe_type_lint(ty, sp, reason, help);
             }
diff --git a/src/librustc_middle/ty/mod.rs b/src/librustc_middle/ty/mod.rs
index ffbe3a40297..caa1b4cb375 100644
--- a/src/librustc_middle/ty/mod.rs
+++ b/src/librustc_middle/ty/mod.rs
@@ -2390,6 +2390,29 @@ impl<'tcx> AdtDef {
     pub fn sized_constraint(&self, tcx: TyCtxt<'tcx>) -> &'tcx [Ty<'tcx>] {
         tcx.adt_sized_constraint(self.did).0
     }
+
+    /// `repr(transparent)` structs can have a single non-ZST field, this function returns that
+    /// field.
+    pub fn transparent_newtype_field(
+        &self,
+        tcx: TyCtxt<'tcx>,
+        param_env: ParamEnv<'tcx>,
+    ) -> Option<&FieldDef> {
+        assert!(self.is_struct() && self.repr.transparent());
+
+        for field in &self.non_enum_variant().fields {
+            let field_ty = tcx.normalize_erasing_regions(
+                param_env,
+                field.ty(tcx, InternalSubsts::identity_for_item(tcx, self.did)),
+            );
+
+            if !field_ty.is_zst(tcx, self.did) {
+                return Some(field);
+            }
+        }
+
+        None
+    }
 }
 
 impl<'tcx> FieldDef {
diff --git a/src/librustc_middle/ty/sty.rs b/src/librustc_middle/ty/sty.rs
index 5d4c2a54267..7550be39d4a 100644
--- a/src/librustc_middle/ty/sty.rs
+++ b/src/librustc_middle/ty/sty.rs
@@ -2186,6 +2186,11 @@ impl<'tcx> TyS<'tcx> {
             }
         }
     }
+
+    /// Is this a zero-sized type?
+    pub fn is_zst(&'tcx self, tcx: TyCtxt<'tcx>, did: DefId) -> bool {
+        tcx.layout_of(tcx.param_env(did).and(self)).map(|layout| layout.is_zst()).unwrap_or(false)
+    }
 }
 
 /// Typed constant value.
diff --git a/src/test/ui/lint/lint-ctypes-66202.rs b/src/test/ui/lint/lint-ctypes-66202.rs
index 3fe4560f44b..ebab41d143e 100644
--- a/src/test/ui/lint/lint-ctypes-66202.rs
+++ b/src/test/ui/lint/lint-ctypes-66202.rs
@@ -1,3 +1,5 @@
+// check-pass
+
 #![deny(improper_ctypes)]
 
 // This test checks that return types are normalized before being checked for FFI-safety, and that
@@ -10,7 +12,6 @@ extern "C" {
     pub fn bare() -> ();
     pub fn normalize() -> <() as ToOwned>::Owned;
     pub fn transparent() -> W<()>;
-    //~^ ERROR uses type `W<()>`
 }
 
 fn main() {}
diff --git a/src/test/ui/lint/lint-ctypes-66202.stderr b/src/test/ui/lint/lint-ctypes-66202.stderr
deleted file mode 100644
index 759c77deadc..00000000000
--- a/src/test/ui/lint/lint-ctypes-66202.stderr
+++ /dev/null
@@ -1,20 +0,0 @@
-error: `extern` block uses type `W<()>`, which is not FFI-safe
-  --> $DIR/lint-ctypes-66202.rs:12:29
-   |
-LL |     pub fn transparent() -> W<()>;
-   |                             ^^^^^ not FFI-safe
-   |
-note: the lint level is defined here
-  --> $DIR/lint-ctypes-66202.rs:1:9
-   |
-LL | #![deny(improper_ctypes)]
-   |         ^^^^^^^^^^^^^^^
-   = note: composed only of `PhantomData`
-note: the type is defined here
-  --> $DIR/lint-ctypes-66202.rs:7:1
-   |
-LL | pub struct W<T>(T);
-   | ^^^^^^^^^^^^^^^^^^^
-
-error: aborting due to previous error
-