Auto merge of #73257 - davidtwco:issue-73249-improper-ctypes-projection, r=lcnr,varkor

ty: projections in `transparent_newtype_field`

Fixes #73249.

This PR modifies `transparent_newtype_field` so that it handles
projections with generic parameters, where `normalize_erasing_regions`
would ICE.

src/librustc_lint/types.rs

@@ -11,7 +11,7 @@ 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, ParamEnv, Ty, TyCtxt};
+use rustc_middle::ty::{self, AdtKind, ParamEnv, Ty, TyCtxt, TypeFoldable};
 use rustc_span::source_map;
 use rustc_span::symbol::sym;
 use rustc_span::Span;
@@ -507,7 +507,7 @@ struct ImproperCTypesVisitor<'a, 'tcx> {
 enum FfiResult<'tcx> {
     FfiSafe,
     FfiPhantom(Ty<'tcx>),
-    FfiUnsafe { ty: Ty<'tcx>, reason: &'static str, help: Option<&'static str> },
+    FfiUnsafe { ty: Ty<'tcx>, reason: String, help: Option<String> },
 }
 
 fn ty_is_known_nonnull<'tcx>(tcx: TyCtxt<'tcx>, ty: Ty<'tcx>) -> bool {
@@ -597,6 +597,66 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
         }
     }
 
+    /// Checks if the given field's type is "ffi-safe".
+    fn check_field_type_for_ffi(
+        &self,
+        cache: &mut FxHashSet<Ty<'tcx>>,
+        field: &ty::FieldDef,
+        substs: SubstsRef<'tcx>,
+    ) -> FfiResult<'tcx> {
+        let field_ty = field.ty(self.cx.tcx, substs);
+        if field_ty.has_opaque_types() {
+            self.check_type_for_ffi(cache, field_ty)
+        } else {
+            let field_ty = self.cx.tcx.normalize_erasing_regions(self.cx.param_env, field_ty);
+            self.check_type_for_ffi(cache, field_ty)
+        }
+    }
+
+    /// Checks if the given `VariantDef`'s field types are "ffi-safe".
+    fn check_variant_for_ffi(
+        &self,
+        cache: &mut FxHashSet<Ty<'tcx>>,
+        ty: Ty<'tcx>,
+        def: &ty::AdtDef,
+        variant: &ty::VariantDef,
+        substs: SubstsRef<'tcx>,
+    ) -> FfiResult<'tcx> {
+        use FfiResult::*;
+
+        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) = variant.transparent_newtype_field(self.cx.tcx) {
+                self.check_field_type_for_ffi(cache, field, substs)
+            } else {
+                bug!("malformed transparent type");
+            }
+        } else {
+            // We can't completely trust repr(C) markings; make sure the fields are
+            // actually safe.
+            let mut all_phantom = !variant.fields.is_empty();
+            for field in &variant.fields {
+                match self.check_field_type_for_ffi(cache, &field, substs) {
+                    FfiSafe => {
+                        all_phantom = false;
+                    }
+                    FfiPhantom(..) if def.is_enum() => {
+                        return FfiUnsafe {
+                            ty,
+                            reason: "this enum contains a PhantomData field".into(),
+                            help: None,
+                        };
+                    }
+                    FfiPhantom(..) => {}
+                    r => return r,
+                }
+            }
+
+            if all_phantom { FfiPhantom(ty) } else { FfiSafe }
+        }
+    }
+
     /// Checks if the given type is "ffi-safe" (has a stable, well-defined
     /// representation which can be exported to C code).
     fn check_type_for_ffi(&self, cache: &mut FxHashSet<Ty<'tcx>>, ty: Ty<'tcx>) -> FfiResult<'tcx> {
@@ -618,15 +678,18 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
                     return FfiPhantom(ty);
                 }
                 match def.adt_kind() {
-                    AdtKind::Struct => {
+                    AdtKind::Struct | AdtKind::Union => {
+                        let kind = if def.is_struct() { "struct" } else { "union" };
+
                         if !def.repr.c() && !def.repr.transparent() {
                             return FfiUnsafe {
                                 ty,
-                                reason: "this struct has unspecified layout",
+                                reason: format!("this {} has unspecified layout", kind),
-                                help: Some(
+                                help: Some(format!(
                                     "consider adding a `#[repr(C)]` or \
-                                            `#[repr(transparent)]` attribute to this struct",
+                                             `#[repr(transparent)]` attribute to this {}",
-                                ),
+                                    kind
+                                )),
                             };
                         }
 
@@ -635,7 +698,7 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
                         if is_non_exhaustive && !def.did.is_local() {
                             return FfiUnsafe {
                                 ty,
-                                reason: "this struct is non-exhaustive",
+                                reason: format!("this {} is non-exhaustive", kind),
                                 help: None,
                             };
                         }
@@ -643,93 +706,12 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
                         if def.non_enum_variant().fields.is_empty() {
                             return FfiUnsafe {
                                 ty,
-                                reason: "this struct has no fields",
+                                reason: format!("this {} has no fields", kind),
-                                help: Some("consider adding a member to this struct"),
+                                help: Some(format!("consider adding a member to this {}", kind)),
                             };
                         }
 
-                        if def.repr.transparent() {
+                        self.check_variant_for_ffi(cache, ty, def, def.non_enum_variant(), substs)
-                            // 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
-                            }
-                        } 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 }
-                        }
-                    }
-                    AdtKind::Union => {
-                        if !def.repr.c() && !def.repr.transparent() {
-                            return FfiUnsafe {
-                                ty,
-                                reason: "this union has unspecified layout",
-                                help: Some(
-                                    "consider adding a `#[repr(C)]` or \
-                                            `#[repr(transparent)]` attribute to this union",
-                                ),
-                            };
-                        }
-
-                        if def.non_enum_variant().fields.is_empty() {
-                            return FfiUnsafe {
-                                ty,
-                                reason: "this union has no fields",
-                                help: Some("consider adding a field to this union"),
-                            };
-                        }
-
-                        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() && field_ty.is_zst(cx, field.did) {
-                                continue;
-                            }
-                            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 }
                     }
                     AdtKind::Enum => {
                         if def.variants.is_empty() {
@@ -744,11 +726,12 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
                             if !is_repr_nullable_ptr(cx, ty, def, substs) {
                                 return FfiUnsafe {
                                     ty,
-                                    reason: "enum has no representation hint",
+                                    reason: "enum has no representation hint".into(),
                                     help: Some(
                                         "consider adding a `#[repr(C)]`, \
                                                 `#[repr(transparent)]`, or integer `#[repr(...)]` \
-                                                attribute to this enum",
+                                                attribute to this enum"
+                                            .into(),
                                     ),
                                 };
                             }
@@ -757,7 +740,7 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
                         if def.is_variant_list_non_exhaustive() && !def.did.is_local() {
                             return FfiUnsafe {
                                 ty,
-                                reason: "this enum is non-exhaustive",
+                                reason: "this enum is non-exhaustive".into(),
                                 help: None,
                             };
                         }
@@ -768,37 +751,17 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
                             if is_non_exhaustive && !variant.def_id.is_local() {
                                 return FfiUnsafe {
                                     ty,
-                                    reason: "this enum has non-exhaustive variants",
+                                    reason: "this enum has non-exhaustive variants".into(),
                                     help: None,
                                 };
                             }
 
-                            for field in &variant.fields {
+                            match self.check_variant_for_ffi(cache, ty, def, variant, substs) {
-                                let field_ty = cx.normalize_erasing_regions(
+                                FfiSafe => (),
-                                    ParamEnv::reveal_all(),
+                                r => return r,
-                                    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() && field_ty.is_zst(cx, field.did) {
-                                    continue;
-                                }
-                                let r = self.check_type_for_ffi(cache, field_ty);
-                                match r {
-                                    FfiSafe => {}
-                                    FfiUnsafe { .. } => {
-                                        return r;
-                                    }
-                                    FfiPhantom(..) => {
-                                        return FfiUnsafe {
-                                            ty,
-                                            reason: "this enum contains a PhantomData field",
-                                            help: None,
-                                        };
-                                    }
-                                }
                             }
                         }
+
                         FfiSafe
                     }
                 }
@@ -806,13 +769,13 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
 
             ty::Char => FfiUnsafe {
                 ty,
-                reason: "the `char` type has no C equivalent",
+                reason: "the `char` type has no C equivalent".into(),
-                help: Some("consider using `u32` or `libc::wchar_t` instead"),
+                help: Some("consider using `u32` or `libc::wchar_t` instead".into()),
             },
 
             ty::Int(ast::IntTy::I128) | ty::Uint(ast::UintTy::U128) => FfiUnsafe {
                 ty,
-                reason: "128-bit integers don't currently have a known stable ABI",
+                reason: "128-bit integers don't currently have a known stable ABI".into(),
                 help: None,
             },
 
@@ -821,24 +784,24 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
 
             ty::Slice(_) => FfiUnsafe {
                 ty,
-                reason: "slices have no C equivalent",
+                reason: "slices have no C equivalent".into(),
-                help: Some("consider using a raw pointer instead"),
+                help: Some("consider using a raw pointer instead".into()),
             },
 
             ty::Dynamic(..) => {
-                FfiUnsafe { ty, reason: "trait objects have no C equivalent", help: None }
+                FfiUnsafe { ty, reason: "trait objects have no C equivalent".into(), help: None }
             }
 
             ty::Str => FfiUnsafe {
                 ty,
-                reason: "string slices have no C equivalent",
+                reason: "string slices have no C equivalent".into(),
-                help: Some("consider using `*const u8` and a length instead"),
+                help: Some("consider using `*const u8` and a length instead".into()),
             },
 
             ty::Tuple(..) => FfiUnsafe {
                 ty,
-                reason: "tuples have unspecified layout",
+                reason: "tuples have unspecified layout".into(),
-                help: Some("consider using a struct instead"),
+                help: Some("consider using a struct instead".into()),
             },
 
             ty::RawPtr(ty::TypeAndMut { ty, .. }) | ty::Ref(_, ty, _) => {
@@ -852,10 +815,12 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
                     Abi::Rust | Abi::RustIntrinsic | Abi::PlatformIntrinsic | Abi::RustCall => {
                         return FfiUnsafe {
                             ty,
-                            reason: "this function pointer has Rust-specific calling convention",
+                            reason: "this function pointer has Rust-specific calling convention"
+                                .into(),
                             help: Some(
                                 "consider using an `extern fn(...) -> ...` \
-                                        function pointer instead",
+                                        function pointer instead"
+                                    .into(),
                             ),
                         };
                     }
@@ -886,6 +851,12 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
 
             ty::Foreign(..) => FfiSafe,
 
+            // While opaque types are checked for earlier, if a projection in a struct field
+            // normalizes to an opaque type, then it will reach this branch.
+            ty::Opaque(..) => {
+                FfiUnsafe { ty, reason: "opaque types have no C equivalent".into(), help: None }
+            }
+
             ty::Param(..)
             | ty::Infer(..)
             | ty::Bound(..)
@@ -895,7 +866,6 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
             | ty::GeneratorWitness(..)
             | ty::Placeholder(..)
             | ty::Projection(..)
-            | ty::Opaque(..)
             | ty::FnDef(..) => bug!("unexpected type in foreign function: {:?}", ty),
         }
     }
@@ -925,8 +895,6 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
     }
 
     fn check_for_opaque_ty(&mut self, sp: Span, ty: Ty<'tcx>) -> bool {
-        use rustc_middle::ty::TypeFoldable;
-
         struct ProhibitOpaqueTypes<'tcx> {
             ty: Option<Ty<'tcx>>,
         };
@@ -993,7 +961,7 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
             // 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);
+                self.emit_ffi_unsafe_type_lint(ty, sp, &reason, help.as_deref());
             }
         }
     }

src/librustc_middle/ty/mod.rs

@@ -1807,6 +1807,19 @@ impl<'tcx> VariantDef {
     pub fn is_field_list_non_exhaustive(&self) -> bool {
         self.flags.intersects(VariantFlags::IS_FIELD_LIST_NON_EXHAUSTIVE)
     }
+
+    /// `repr(transparent)` structs can have a single non-ZST field, this function returns that
+    /// field.
+    pub fn transparent_newtype_field(&self, tcx: TyCtxt<'tcx>) -> Option<&FieldDef> {
+        for field in &self.fields {
+            let field_ty = field.ty(tcx, InternalSubsts::identity_for_item(tcx, self.def_id));
+            if !field_ty.is_zst(tcx, self.def_id) {
+                return Some(field);
+            }
+        }
+
+        None
+    }
 }
 
 #[derive(Copy, Clone, Debug, PartialEq, Eq, RustcEncodable, RustcDecodable, HashStable)]
@@ -2376,29 +2389,6 @@ 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/test/ui/lint/lint-ctypes-73249-1.rs

@@ -0,0 +1,21 @@
+// check-pass
+#![deny(improper_ctypes)]
+
+pub trait Foo {
+    type Assoc: 'static;
+}
+
+impl Foo for () {
+    type Assoc = u32;
+}
+
+extern "C" {
+    pub fn lint_me(x: Bar<()>);
+}
+
+#[repr(transparent)]
+pub struct Bar<T: Foo> {
+    value: &'static <T as Foo>::Assoc,
+}
+
+fn main() {}

src/test/ui/lint/lint-ctypes-73249-2.rs

@@ -0,0 +1,29 @@
+#![feature(type_alias_impl_trait)]
+#![deny(improper_ctypes)]
+
+pub trait Baz { }
+
+impl Baz for () { }
+
+type Qux = impl Baz;
+
+fn assign() -> Qux {}
+
+pub trait Foo {
+    type Assoc: 'static;
+}
+
+impl Foo for () {
+    type Assoc = Qux;
+}
+
+#[repr(transparent)]
+pub struct A<T: Foo> {
+    x: &'static <T as Foo>::Assoc,
+}
+
+extern "C" {
+    pub fn lint_me() -> A<()>; //~ ERROR: uses type `impl Baz`
+}
+
+fn main() {}

src/test/ui/lint/lint-ctypes-73249-2.stderr

@@ -0,0 +1,15 @@
+error: `extern` block uses type `impl Baz`, which is not FFI-safe
+  --> $DIR/lint-ctypes-73249-2.rs:26:25
+   |
+LL |     pub fn lint_me() -> A<()>;
+   |                         ^^^^^ not FFI-safe
+   |
+note: the lint level is defined here
+  --> $DIR/lint-ctypes-73249-2.rs:2:9
+   |
+LL | #![deny(improper_ctypes)]
+   |         ^^^^^^^^^^^^^^^
+   = note: opaque types have no C equivalent
+
+error: aborting due to previous error
+

src/test/ui/lint/lint-ctypes-73249-3.rs

@@ -0,0 +1,21 @@
+#![feature(type_alias_impl_trait)]
+#![deny(improper_ctypes)]
+
+pub trait Baz { }
+
+impl Baz for u32 { }
+
+type Qux = impl Baz;
+
+fn assign() -> Qux { 3 }
+
+#[repr(C)]
+pub struct A {
+    x: Qux,
+}
+
+extern "C" {
+    pub fn lint_me() -> A; //~ ERROR: uses type `impl Baz`
+}
+
+fn main() {}

src/test/ui/lint/lint-ctypes-73249-3.stderr

@@ -0,0 +1,15 @@
+error: `extern` block uses type `impl Baz`, which is not FFI-safe
+  --> $DIR/lint-ctypes-73249-3.rs:18:25
+   |
+LL |     pub fn lint_me() -> A;
+   |                         ^ not FFI-safe
+   |
+note: the lint level is defined here
+  --> $DIR/lint-ctypes-73249-3.rs:2:9
+   |
+LL | #![deny(improper_ctypes)]
+   |         ^^^^^^^^^^^^^^^
+   = note: opaque types have no C equivalent
+
+error: aborting due to previous error
+

src/test/ui/lint/lint-ctypes-73249-4.rs

@@ -0,0 +1,24 @@
+// check-pass
+#![deny(improper_ctypes)]
+
+use std::marker::PhantomData;
+
+trait Foo {
+    type Assoc;
+}
+
+impl Foo for () {
+    type Assoc = PhantomData<()>;
+}
+
+#[repr(transparent)]
+struct Wow<T> where T: Foo<Assoc = PhantomData<T>> {
+    x: <T as Foo>::Assoc,
+    v: u32,
+}
+
+extern "C" {
+    fn test(v: Wow<()>);
+}
+
+fn main() {}

src/test/ui/lint/lint-ctypes-73249-5.rs

@@ -0,0 +1,21 @@
+#![feature(type_alias_impl_trait)]
+#![deny(improper_ctypes)]
+
+pub trait Baz { }
+
+impl Baz for u32 { }
+
+type Qux = impl Baz;
+
+fn assign() -> Qux { 3 }
+
+#[repr(transparent)]
+pub struct A {
+    x: Qux,
+}
+
+extern "C" {
+    pub fn lint_me() -> A; //~ ERROR: uses type `impl Baz`
+}
+
+fn main() {}

src/test/ui/lint/lint-ctypes-73249-5.stderr

@@ -0,0 +1,15 @@
+error: `extern` block uses type `impl Baz`, which is not FFI-safe
+  --> $DIR/lint-ctypes-73249-5.rs:18:25
+   |
+LL |     pub fn lint_me() -> A;
+   |                         ^ not FFI-safe
+   |
+note: the lint level is defined here
+  --> $DIR/lint-ctypes-73249-5.rs:2:9
+   |
+LL | #![deny(improper_ctypes)]
+   |         ^^^^^^^^^^^^^^^
+   = note: opaque types have no C equivalent
+
+error: aborting due to previous error
+

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

@@ -0,0 +1,21 @@
+// check-pass
+#![deny(improper_ctypes)]
+
+pub trait Foo {
+    type Assoc;
+}
+
+impl Foo for () {
+    type Assoc = u32;
+}
+
+extern "C" {
+    pub fn lint_me(x: Bar<()>);
+}
+
+#[repr(transparent)]
+pub struct Bar<T: Foo> {
+    value: <T as Foo>::Assoc,
+}
+
+fn main() {}