lint: unify enum variant, union and struct logic

This commit applies the changes introduced in #72890 to both enum
variants and unions - where the logic prior to #72890 was duplicated.

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

2 files changed, 107 insertions, 152 deletions

diff --git a/src/librustc_lint/types.rs b/src/librustc_lint/types.rs
index f429eef90c8..b20bf432614 100644
--- a/src/librustc_lint/types.rs
+++ b/src/librustc_lint/types.rs
@@ -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 {
@@ -613,6 +613,50 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
         }
     }
 
+    /// 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.cx.param_env) {
+                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> {
@@ -634,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",
-                                help: Some(
+                                reason: format!("this {} has unspecified layout", kind),
+                                help: Some(format!(
                                     "consider adding a `#[repr(C)]` or \
-                                            `#[repr(transparent)]` attribute to this struct",
-                                ),
+                                             `#[repr(transparent)]` attribute to this {}",
+                                    kind
+                                )),
                             };
                         }
 
@@ -651,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,
                             };
                         }
@@ -659,85 +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",
-                                help: Some("consider adding a member to this struct"),
+                                reason: format!("this {} has no fields", kind),
+                                help: Some(format!("consider adding a member to this {}", kind)),
                             };
                         }
 
-                        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)
-                            {
-                                self.check_field_type_for_ffi(cache, field, substs)
-                            } 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 r = self.check_field_type_for_ffi(cache, field, substs);
-                                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 }
+                        self.check_variant_for_ffi(cache, ty, def, def.non_enum_variant(), substs)
                     }
                     AdtKind::Enum => {
                         if def.variants.is_empty() {
@@ -752,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(),
                                     ),
                                 };
                             }
@@ -765,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,
                             };
                         }
@@ -776,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 {
-                                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 => {}
-                                    FfiUnsafe { .. } => {
-                                        return r;
-                                    }
-                                    FfiPhantom(..) => {
-                                        return FfiUnsafe {
-                                            ty,
-                                            reason: "this enum contains a PhantomData field",
-                                            help: None,
-                                        };
-                                    }
-                                }
+                            match self.check_variant_for_ffi(cache, ty, def, variant, substs) {
+                                FfiSafe => (),
+                                r => return r,
                             }
                         }
+
                         FfiSafe
                     }
                 }
@@ -814,13 +769,13 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
 
             ty::Char => FfiUnsafe {
                 ty,
-                reason: "the `char` type has no C equivalent",
-                help: Some("consider using `u32` or `libc::wchar_t` instead"),
+                reason: "the `char` type has no C equivalent".into(),
+                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,
             },
 
@@ -829,24 +784,24 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
 
             ty::Slice(_) => FfiUnsafe {
                 ty,
-                reason: "slices have no C equivalent",
-                help: Some("consider using a raw pointer instead"),
+                reason: "slices have no C equivalent".into(),
+                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",
-                help: Some("consider using `*const u8` and a length instead"),
+                reason: "string slices have no C equivalent".into(),
+                help: Some("consider using `*const u8` and a length instead".into()),
             },
 
             ty::Tuple(..) => FfiUnsafe {
                 ty,
-                reason: "tuples have unspecified layout",
-                help: Some("consider using a struct instead"),
+                reason: "tuples have unspecified layout".into(),
+                help: Some("consider using a struct instead".into()),
             },
 
             ty::RawPtr(ty::TypeAndMut { ty, .. }) | ty::Ref(_, ty, _) => {
@@ -860,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(),
                             ),
                         };
                     }
@@ -897,7 +854,7 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
             // 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", help: None }
+                FfiUnsafe { ty, reason: "opaque types have no C equivalent".into(), help: None }
             }
 
             ty::Param(..)
@@ -1004,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());
             }
         }
     }
diff --git a/src/librustc_middle/ty/mod.rs b/src/librustc_middle/ty/mod.rs
index 1f94c28b357..30643165ce9 100644
--- a/src/librustc_middle/ty/mod.rs
+++ b/src/librustc_middle/ty/mod.rs
@@ -1807,6 +1807,31 @@ 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>,
+        param_env: ParamEnv<'tcx>,
+    ) -> Option<&FieldDef> {
+        for field in &self.fields {
+            let field_ty = field.ty(tcx, InternalSubsts::identity_for_item(tcx, self.def_id));
+
+            // `normalize_erasing_regions` will fail for projections that contain generic
+            // parameters, so check these before normalizing.
+            if field_ty.has_projections() && field_ty.needs_subst() {
+                return Some(field);
+            }
+
+            let field_ty = tcx.normalize_erasing_regions(param_env, field_ty);
+            if !field_ty.is_zst(tcx, self.def_id) {
+                return Some(field);
+            }
+        }
+
+        None
+    }
 }
 
 #[derive(Copy, Clone, Debug, PartialEq, Eq, RustcEncodable, RustcDecodable, HashStable)]
@@ -2376,33 +2401,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 = field.ty(tcx, InternalSubsts::identity_for_item(tcx, self.did));
-
-            // `normalize_erasing_regions` will fail for projections that contain generic
-            // parameters, so check these before normalizing.
-            if field_ty.has_projections() && field_ty.needs_subst() {
-                return Some(field);
-            }
-
-            let field_ty = tcx.normalize_erasing_regions(param_env, field_ty);
-            if !field_ty.is_zst(tcx, self.did) {
-                return Some(field);
-            }
-        }
-
-        None
-    }
 }
 
 impl<'tcx> FieldDef {