Rollup merge of #72890 - davidtwco:issue-66202-normalize-and-transparent-improper-ctypes, r=varkor

improper ctypes: normalize return types and transparent structs

Fixes #66202.

See each commit individually (except the first which adds a test) for more detailed explanations on the changes made.

In summary, this PR ensures that return types are normalized before being checked for FFI-safety, and that transparent newtype wrappers are FFI-safe if the type being wrapped is FFI-safe (often true previously, but not if, after substitution, all types in a transparent newtype were zero sized).

Author: Dylan-DPC

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);
@@ -946,7 +952,13 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
         }
     }
 
-    fn check_type_for_ffi_and_report_errors(&mut self, sp: Span, ty: Ty<'tcx>, is_static: bool) {
+    fn check_type_for_ffi_and_report_errors(
+        &mut self,
+        sp: Span,
+        ty: Ty<'tcx>,
+        is_static: bool,
+        is_return_type: bool,
+    ) {
         // We have to check for opaque types before `normalize_erasing_regions`,
         // which will replace opaque types with their underlying concrete type.
         if self.check_for_opaque_ty(sp, ty) {
@@ -957,19 +969,29 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
         // it is only OK to use this function because extern fns cannot have
         // any generic types right now:
         let ty = self.cx.tcx.normalize_erasing_regions(ParamEnv::reveal_all(), ty);
-        // C doesn't really support passing arrays by value.
-        // The only way to pass an array by value is through a struct.
-        // So we first test that the top level isn't an array,
-        // and then recursively check the types inside.
+
+        // C doesn't really support passing arrays by value - the only way to pass an array by value
+        // is through a struct. So, first test that the top level isn't an array, and then
+        // recursively check the types inside.
         if !is_static && self.check_for_array_ty(sp, ty) {
             return;
         }
 
+        // Don't report FFI errors for unit return types. This check exists here, and not in
+        // `check_foreign_fn` (where it would make more sense) so that normalization has definitely
+        // happened.
+        if is_return_type && ty.is_unit() {
+            return;
+        }
+
         match self.check_type_for_ffi(&mut FxHashSet::default(), ty) {
             FfiResult::FfiSafe => {}
             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);
             }
@@ -982,21 +1004,19 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
         let sig = self.cx.tcx.erase_late_bound_regions(&sig);
 
         for (input_ty, input_hir) in sig.inputs().iter().zip(decl.inputs) {
-            self.check_type_for_ffi_and_report_errors(input_hir.span, input_ty, false);
+            self.check_type_for_ffi_and_report_errors(input_hir.span, input_ty, false, false);
         }
 
         if let hir::FnRetTy::Return(ref ret_hir) = decl.output {
             let ret_ty = sig.output();
-            if !ret_ty.is_unit() {
-                self.check_type_for_ffi_and_report_errors(ret_hir.span, ret_ty, false);
-            }
+            self.check_type_for_ffi_and_report_errors(ret_hir.span, ret_ty, false, true);
         }
     }
 
     fn check_foreign_static(&mut self, id: hir::HirId, span: Span) {
         let def_id = self.cx.tcx.hir().local_def_id(id);
         let ty = self.cx.tcx.type_of(def_id);
-        self.check_type_for_ffi_and_report_errors(span, ty, true);
+        self.check_type_for_ffi_and_report_errors(span, ty, true, false);
     }
 }
 

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 {

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.

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

@@ -0,0 +1,17 @@
+// check-pass
+
+#![deny(improper_ctypes)]
+
+// This test checks that return types are normalized before being checked for FFI-safety, and that
+// transparent newtype wrappers are FFI-safe if the type being wrapped is FFI-safe.
+
+#[repr(transparent)]
+pub struct W<T>(T);
+
+extern "C" {
+    pub fn bare() -> ();
+    pub fn normalize() -> <() as ToOwned>::Owned;
+    pub fn transparent() -> W<()>;
+}
+
+fn main() {}