Rollup merge of rust-lang#155299 - RalfJung:repr_transparent_non_zst_fields, r=jdonszelmann

make repr_transparent_non_zst_fields a hard error

This lint is about which fields we consider "trivial" for `repr(transparent)`. For `repr(transparent)` to be valid, there can be at most one non-trivial field. In other words, trivial fields are those that we promise do not affect the layout or ABI of the `repr(transparent)` type. Historically we considered all types with size 0 and alignment 1 (i.e., all 1-ZST) to be trivial. However we'd like to take some of that back:
- Types might be 1-ZST today but that's not actually meant to be a semver guarantee, so it's bad for downstream code to rely on it. Therefore we should not accept types that have private fields or that are marked `#[non_exhaustive]` (except when we are in the same crate as that type).
- Types might be 1-ZST but still be relevant for the ABI because they are `repr(C)` and who knows what the C ABI does. In particular on MSVC a struct whose only field is a 0-length array has size 1. Rust incorrectly gives it size 0. With rust-lang/rfcs#3845 we can hopefully fix the layout, which would make "is that type a 1-ZST" target-dependent, and we ideally should reject such code on all targets.

This was a deny-by-default FCW since rust-lang#147185, which landed almost 6 months ago and shipped with Rust 1.93. (If this PR lands now it will ship with 1.97.) Already back then we found hardly any crater impact. The [tracking issue](rust-lang#78586) has had no new relevant backrefs since that PR landed.

So, I think it is time to make this a hard error.
Fixes rust-lang#78586 (tracking issue)
Fixes rust-lang/unsafe-code-guidelines#552 because this means the `repr(transparent)` ABI compatibility rule no longer ever "ignores" `repr(C)` fields.

@rust-lang/lang What do you think? See [here](rust-lang#155299 (comment)) for the crater analysis; the summary is that there's no relevant regressions found in the wild. But some points have been raised by people:
- The [`ghost`](https://github.com/dtolnay/ghost) crate offers a macro to define `PhantomData`-like types, and those types involve a `repr(C)`. If someone uses such a type as a marker inside a `repr(transparent)`, that will no longer work. Apparently nobody does that in the code checked by crater. A new version of the crate has been released that fixes this.
- rust-lang#155925 is unresolved: there is currently no way for a crate to say "yes this type has private field but I promise it will remain a 1-ZST" (except via the unstable `#[rustc_pub_transparent]`). That means it is not possible for a crate to expose a semantically relevant maker type (like `GhostToken`) that is "trivial" for `repr(transparent)` purposes. Apparently currently nobody does this in the ecosystem (the parts crater can see, anyway), but it seems like a sensible thing to do. If we are concerned about this, we could limit this PR to only make the `repr(C)` and `#[non_exhaustive]` part of the check a hard error, and leave the "has private fields" part as a warning.

Also note that the private field check is technically a bit odd: we literally check "is the type defined in this crate or are all fields public". We do *not* check if the current module can access those fields. So if a type has private fields then one can rely on it being "trivial" everywhere in the current crate, even outside the module that defined the type. This is not how field privacy usually works. If we want to restrict this to "only modules that can 'see' the fields are allowed to rely on the type being a 1-ZST", that's technically a breaking change. I don't know if this was a deliberate choice or just the easiest thing to implement. @scottmcm do you remember?

Author: JonathanBrouwer

compiler/rustc_hir_analysis/src/check/check.rs

@@ -11,7 +11,7 @@ use rustc_hir::def::{CtorKind, DefKind};
 use rustc_hir::{LangItem, Node, find_attr, intravisit};
 use rustc_infer::infer::{RegionVariableOrigin, TyCtxtInferExt};
 use rustc_infer::traits::{Obligation, ObligationCauseCode, WellFormedLoc};
-use rustc_lint_defs::builtin::{REPR_TRANSPARENT_NON_ZST_FIELDS, UNSUPPORTED_CALLING_CONVENTIONS};
+use rustc_lint_defs::builtin::UNSUPPORTED_CALLING_CONVENTIONS;
 use rustc_macros::Diagnostic;
 use rustc_middle::hir::nested_filter;
 use rustc_middle::middle::resolve_bound_vars::ResolvedArg;
@@ -1735,39 +1735,6 @@ pub(super) fn check_packed_inner(
 }
 
 pub(super) fn check_transparent<'tcx>(tcx: TyCtxt<'tcx>, adt: ty::AdtDef<'tcx>) {
-    struct ZeroSizedFieldReprTransparentIncompatibility<'tcx> {
-        unsuited: UnsuitedInfo<'tcx>,
-    }
-
-    impl<'a, 'tcx> Diagnostic<'a, ()> for ZeroSizedFieldReprTransparentIncompatibility<'tcx> {
-        fn into_diag(self, dcx: DiagCtxtHandle<'a>, level: Level) -> Diag<'a, ()> {
-            let Self { unsuited } = self;
-            let (title, note) = match unsuited.reason {
-                UnsuitedReason::NonExhaustive => (
-                    "external non-exhaustive types",
-                    "is marked with `#[non_exhaustive]`, so it could become non-zero-sized in the future.",
-                ),
-                UnsuitedReason::PrivateField => (
-                    "external types with private fields",
-                    "contains private fields, so it could become non-zero-sized in the future.",
-                ),
-                UnsuitedReason::ReprC => (
-                    "`repr(C)` types",
-                    "is a `#[repr(C)]` type, so it is not guaranteed to be zero-sized on all targets.",
-                ),
-            };
-            Diag::new(
-                dcx,
-                level,
-                format!("zero-sized fields in `repr(transparent)` cannot contain {title}"),
-            )
-            .with_note(format!(
-                "this field contains `{field_ty}`, which {note}",
-                field_ty = unsuited.ty,
-            ))
-        }
-    }
-
     if !adt.repr().transparent() {
         return;
     }
@@ -1787,108 +1754,137 @@ pub(super) fn check_transparent<'tcx>(tcx: TyCtxt<'tcx>, adt: ty::AdtDef<'tcx>)
         // Don't bother checking the fields.
         return;
     }
+    let variant = adt.variant(VariantIdx::ZERO);
 
-    let typing_env = ty::TypingEnv::non_body_analysis(tcx, adt.did());
-    // For each field, figure out if it has "trivial" layout (i.e., is a 1-ZST).
-    struct FieldInfo<'tcx> {
-        span: Span,
-        trivial: bool,
-        ty: Ty<'tcx>,
-    }
-
-    let field_infos = adt.all_fields().map(|field| {
-        let ty = field.ty(tcx, GenericArgs::identity_for_item(tcx, field.did)).skip_norm_wip();
-        let layout = tcx.layout_of(typing_env.as_query_input(ty));
-        // We are currently checking the type this field came from, so it must be local
-        let span = tcx.hir_span_if_local(field.did).unwrap();
-        let trivial = layout.is_ok_and(|layout| layout.is_1zst());
-        FieldInfo { span, trivial, ty }
-    });
-
-    let non_trivial_fields = field_infos
-        .clone()
-        .filter_map(|field| if !field.trivial { Some(field.span) } else { None });
-    let non_trivial_count = non_trivial_fields.clone().count();
-    if non_trivial_count >= 2 {
-        bad_non_zero_sized_fields(
-            tcx,
-            adt,
-            non_trivial_count,
-            non_trivial_fields,
-            tcx.def_span(adt.did()),
-        );
+    if variant.fields.len() <= 1 {
+        // No need to check when there's at most one field.
         return;
     }
 
-    // Even some 1-ZST fields are not allowed though, if they have `non_exhaustive` or private
-    // fields or `repr(C)`. We call those fields "unsuited".
-    struct UnsuitedInfo<'tcx> {
-        /// The source of the problem, a type that is found somewhere within the field type.
-        ty: Ty<'tcx>,
-        reason: UnsuitedReason,
-    }
-    enum UnsuitedReason {
-        NonExhaustive,
-        PrivateField,
-        ReprC,
+    let typing_env = ty::TypingEnv::non_body_analysis(tcx, adt.did());
+
+    /// We call a field "trivial" for `repr(transparent)` purposes if it can be ignored.
+    /// IOW, `repr(transparent)` is allowed if there is at most one non-trivial field.
+    /// This enum captures all the reasons why a field might not be "trivial".
+    enum NonTrivialReason<'tcx> {
+        UnknownLayout,
+        NonZeroSized,
+        NonTrivialAlignment,
+        PrivateField { inside: Ty<'tcx> },
+        NonExhaustive { ty: Ty<'tcx> },
+        ReprC { ty: Ty<'tcx> },
+    }
+    struct NonTrivialFieldInfo<'tcx> {
+        span: Span,
+        reason: NonTrivialReason<'tcx>,
     }
 
-    fn check_unsuited<'tcx>(
+    /// Check if this type is "trivial" for `repr(transparent)`. If not, return the reason why
+    /// and the problematic type.
+    fn is_trivial<'tcx>(
         tcx: TyCtxt<'tcx>,
         typing_env: ty::TypingEnv<'tcx>,
         ty: Ty<'tcx>,
-    ) -> ControlFlow<UnsuitedInfo<'tcx>> {
+    ) -> ControlFlow<NonTrivialReason<'tcx>> {
         // We can encounter projections during traversal, so ensure the type is normalized.
         let ty =
             tcx.try_normalize_erasing_regions(typing_env, Unnormalized::new_wip(ty)).unwrap_or(ty);
         match ty.kind() {
-            ty::Tuple(list) => list.iter().try_for_each(|t| check_unsuited(tcx, typing_env, t)),
-            ty::Array(ty, _) => check_unsuited(tcx, typing_env, *ty),
+            ty::Tuple(list) => list.iter().try_for_each(|t| is_trivial(tcx, typing_env, t)),
+            ty::Array(ty, _) => is_trivial(tcx, typing_env, *ty),
             ty::Adt(def, args) => {
                 if !def.did().is_local() && !find_attr!(tcx, def.did(), RustcPubTransparent(_)) {
                     let non_exhaustive = def.is_variant_list_non_exhaustive()
                         || def.variants().iter().any(ty::VariantDef::is_field_list_non_exhaustive);
+                    if non_exhaustive {
+                        return ControlFlow::Break(NonTrivialReason::NonExhaustive { ty });
+                    }
                     let has_priv = def.all_fields().any(|f| !f.vis.is_public());
-                    if non_exhaustive || has_priv {
-                        return ControlFlow::Break(UnsuitedInfo {
-                            ty,
-                            reason: if non_exhaustive {
-                                UnsuitedReason::NonExhaustive
-                            } else {
-                                UnsuitedReason::PrivateField
-                            },
-                        });
+                    if has_priv {
+                        return ControlFlow::Break(NonTrivialReason::PrivateField { inside: ty });
                     }
                 }
                 if def.repr().c() {
-                    return ControlFlow::Break(UnsuitedInfo { ty, reason: UnsuitedReason::ReprC });
+                    return ControlFlow::Break(NonTrivialReason::ReprC { ty });
                 }
                 def.all_fields()
                     .map(|field| field.ty(tcx, args).skip_norm_wip())
-                    .try_for_each(|t| check_unsuited(tcx, typing_env, t))
+                    .try_for_each(|t| is_trivial(tcx, typing_env, t))
             }
             _ => ControlFlow::Continue(()),
         }
     }
 
-    let mut prev_unsuited_1zst = false;
-    for field in field_infos {
-        if field.trivial
-            && let Some(unsuited) = check_unsuited(tcx, typing_env, field.ty).break_value()
-        {
-            // If there are any non-trivial fields, then there can be no non-exhaustive 1-zsts.
-            // Otherwise, it's only an issue if there's >1 non-exhaustive 1-zst.
-            if non_trivial_count > 0 || prev_unsuited_1zst {
-                tcx.emit_node_span_lint(
-                    REPR_TRANSPARENT_NON_ZST_FIELDS,
-                    tcx.local_def_id_to_hir_id(adt.did().expect_local()),
-                    field.span,
-                    ZeroSizedFieldReprTransparentIncompatibility { unsuited },
-                );
-            } else {
-                prev_unsuited_1zst = true;
+    let non_trivial_fields = variant
+        .fields
+        .iter()
+        .filter_map(|field| {
+            let ty = field.ty(tcx, GenericArgs::identity_for_item(tcx, field.did)).skip_norm_wip();
+            let layout = tcx.layout_of(typing_env.as_query_input(ty));
+            // We are currently checking the type this field came from, so it must be local
+            let span = tcx.hir_span_if_local(field.did).unwrap();
+            // Rule out non-1ZST
+            if !layout.is_ok_and(|layout| layout.is_1zst()) {
+                let reason = match layout {
+                    Err(_) => NonTrivialReason::UnknownLayout,
+                    Ok(layout) => {
+                        if !(layout.is_sized() && layout.size.bytes() == 0) {
+                            NonTrivialReason::NonZeroSized
+                        } else {
+                            NonTrivialReason::NonTrivialAlignment
+                        }
+                    }
+                };
+                return Some(NonTrivialFieldInfo { span, reason });
+            }
+            // Recursively check for other things that have to be ruled out.
+            if let Some(reason) = is_trivial(tcx, typing_env, ty).break_value() {
+                return Some(NonTrivialFieldInfo { span, reason });
             }
+            // Otherwise,
+            None
+        })
+        .collect::<Vec<_>>();
+
+    if non_trivial_fields.len() > 1 {
+        let count = non_trivial_fields.len();
+        let desc = if adt.is_enum() {
+            format_args!("the variant of a transparent {}", adt.descr())
+        } else {
+            format_args!("transparent {}", adt.descr())
+        };
+        let ty_span = tcx.def_span(adt.did());
+        let mut diag = tcx.dcx().struct_span_err(
+            ty_span,
+            format!("{desc} needs at most one non-trivial field, but has {count}"),
+        );
+        diag.code(E0690);
+
+        // Label for the type.
+        diag.span_label(ty_span, format!("needs at most one non-trivial field, but has {count}"));
+        // Label for each non-trivial field.
+        for field in non_trivial_fields {
+            let msg = match field.reason {
+                NonTrivialReason::UnknownLayout => {
+                    format!("this field is generic and hence may have non-zero size")
+                }
+                NonTrivialReason::NonZeroSized => format!("this field has non-zero size"),
+                NonTrivialReason::NonTrivialAlignment => format!("this field requires alignment"),
+                NonTrivialReason::PrivateField { inside } => format!(
+                    "this field contains `{inside}`, which has private fields, so it could become non-zero-sized in the future"
+                ),
+                NonTrivialReason::NonExhaustive { ty } => format!(
+                    "this field contains `{ty}`, which is marked with `#[non_exhaustive]`, so it could become non-zero-sized in the future"
+                ),
+                NonTrivialReason::ReprC { ty } => format!(
+                    "this field contains `{ty}`, which is a `#[repr(C)]` type, so it is not guaranteed to be zero-sized on all targets"
+                ),
+            };
+            diag.span_label(field.span, msg);
         }
+
+        diag.emit();
+        return;
     }
 }
 

compiler/rustc_hir_analysis/src/check/mod.rs

@@ -590,32 +590,6 @@ fn bad_variant_count<'tcx>(tcx: TyCtxt<'tcx>, adt: ty::AdtDef<'tcx>, sp: Span, d
     });
 }
 
-/// Emit an error when encountering two or more non-zero-sized fields in a transparent
-/// enum.
-fn bad_non_zero_sized_fields<'tcx>(
-    tcx: TyCtxt<'tcx>,
-    adt: ty::AdtDef<'tcx>,
-    field_count: usize,
-    field_spans: impl Iterator<Item = Span>,
-    sp: Span,
-) {
-    if adt.is_enum() {
-        tcx.dcx().emit_err(errors::TransparentNonZeroSizedEnum {
-            span: sp,
-            spans: field_spans.collect(),
-            field_count,
-            desc: adt.descr(),
-        });
-    } else {
-        tcx.dcx().emit_err(errors::TransparentNonZeroSized {
-            span: sp,
-            spans: field_spans.collect(),
-            field_count,
-            desc: adt.descr(),
-        });
-    }
-}
-
 // FIXME: Consider moving this method to a more fitting place.
 pub fn potentially_plural_count(count: usize, word: &str) -> String {
     format!("{} {}{}", count, word, pluralize!(count))

compiler/rustc_hir_analysis/src/errors.rs

@@ -1008,30 +1008,6 @@ pub(crate) struct TransparentEnumVariant {
     pub path: String,
 }
 
-#[derive(Diagnostic)]
-#[diag("the variant of a transparent {$desc} needs at most one field with non-trivial size or alignment, but has {$field_count}", code = E0690)]
-pub(crate) struct TransparentNonZeroSizedEnum<'a> {
-    #[primary_span]
-    #[label("needs at most one field with non-trivial size or alignment, but has {$field_count}")]
-    pub span: Span,
-    #[label("this field has non-zero size or requires alignment")]
-    pub spans: Vec<Span>,
-    pub field_count: usize,
-    pub desc: &'a str,
-}
-
-#[derive(Diagnostic)]
-#[diag("transparent {$desc} needs at most one field with non-trivial size or alignment, but has {$field_count}", code = E0690)]
-pub(crate) struct TransparentNonZeroSized<'a> {
-    #[primary_span]
-    #[label("needs at most one field with non-trivial size or alignment, but has {$field_count}")]
-    pub span: Span,
-    #[label("this field has non-zero size or requires alignment")]
-    pub spans: Vec<Span>,
-    pub field_count: usize,
-    pub desc: &'a str,
-}
-
 #[derive(Diagnostic)]
 #[diag("extern static is too large for the target architecture")]
 pub(crate) struct TooLargeStatic {

compiler/rustc_lint/src/lib.rs

@@ -376,10 +376,6 @@ fn register_builtins(store: &mut LintStore) {
     store.register_renamed("static_mut_ref", "static_mut_refs");
     store.register_renamed("temporary_cstring_as_ptr", "dangling_pointers_from_temporaries");
     store.register_renamed("elided_named_lifetimes", "mismatched_lifetime_syntaxes");
-    store.register_renamed(
-        "repr_transparent_external_private_fields",
-        "repr_transparent_non_zst_fields",
-    );
 
     // These were moved to tool lints, but rustc still sees them when compiling normally, before
     // tool lints are registered, so `check_tool_name_for_backwards_compat` doesn't work. Use
@@ -654,6 +650,16 @@ fn register_builtins(store: &mut LintStore) {
         "inline_always_mismatching_target_features",
         "replaced by a hard error for `#[inline(always)]` with `#[target_feature]`",
     );
+    store.register_removed(
+        "repr_transparent_external_private_fields",
+        "converted into hard error, \
+         see <https://github.com/rust-lang/rust/issues/78586> for more information",
+    );
+    store.register_removed(
+        "repr_transparent_non_zst_fields",
+        "converted into hard error, \
+         see <https://github.com/rust-lang/rust/issues/78586> for more information",
+    );
 }
 
 fn register_internals(store: &mut LintStore) {