ImproperCTypes: change handling of ADTs

A change in how type checking goes through structs/enums/unions,
mostly to be able to yield multiple lints if multiple fields are unsafe

Author: niacdoial

compiler/rustc_lint/messages.ftl

@@ -399,24 +399,25 @@ lint_improper_ctypes_slice_reason = slices have no C equivalent
 
 lint_improper_ctypes_str_help = consider using `*const u8` and a length instead
 lint_improper_ctypes_str_reason = string slices have no C equivalent
-lint_improper_ctypes_struct_fieldless_help = consider adding a member to this struct
 
-lint_improper_ctypes_struct_fieldless_reason = this struct has no fields
+lint_improper_ctypes_struct_consider_transparent = `{$ty}` has exactly one non-zero-sized field, consider making it `#[repr(transparent)]` instead
 lint_improper_ctypes_struct_dueto = this struct/enum/union (`{$ty}`) is FFI-unsafe due to a `{$inner_ty}` field
 
-lint_improper_ctypes_struct_layout_help = consider adding a `#[repr(C)]` or `#[repr(transparent)]` attribute to this struct
+lint_improper_ctypes_struct_fieldless_help = consider adding a member to this struct
+lint_improper_ctypes_struct_fieldless_reason = `{$ty}` has no fields
 
-lint_improper_ctypes_struct_layout_reason = this struct has unspecified layout
-lint_improper_ctypes_struct_non_exhaustive = this struct is non-exhaustive
-lint_improper_ctypes_struct_zst = this struct contains only zero-sized fields
+lint_improper_ctypes_struct_layout_help = consider adding a `#[repr(C)]` (not `#[repr(C,packed)]`) or `#[repr(transparent)]` attribute to `{$ty}`
+lint_improper_ctypes_struct_layout_reason = `{$ty}` has unspecified layout
+lint_improper_ctypes_struct_non_exhaustive = `{$ty}` is non-exhaustive
+lint_improper_ctypes_struct_zst = `{$ty}` contains only zero-sized fields
 
 lint_improper_ctypes_tuple_help = consider using a struct instead
-
 lint_improper_ctypes_tuple_reason = tuples have unspecified layout
-lint_improper_ctypes_union_fieldless_help = consider adding a member to this union
 
+lint_improper_ctypes_union_consider_transparent = `{$ty}` has exactly one non-zero-sized field, consider making it `#[repr(transparent)]` instead
+lint_improper_ctypes_union_fieldless_help = consider adding a member to this union
 lint_improper_ctypes_union_fieldless_reason = this union has no fields
-lint_improper_ctypes_union_layout_help = consider adding a `#[repr(C)]` or `#[repr(transparent)]` attribute to this union
+lint_improper_ctypes_union_layout_help = consider adding a `#[repr(C)]` attribute to this union
 
 lint_improper_ctypes_union_layout_reason = this union has unspecified layout
 lint_improper_ctypes_union_non_exhaustive = this union is non-exhaustive

compiler/rustc_lint/src/types.rs

@@ -707,6 +707,26 @@ pub(crate) fn transparent_newtype_field<'a, 'tcx>(
     })
 }
 
+/// for a given ADT variant, list which fields are non-1ZST
+/// (`repr(transparent)` guarantees that there is at most one)
+pub(crate) fn map_non_1zst_fields<'a, 'tcx>(
+    tcx: TyCtxt<'tcx>,
+    variant: &'a ty::VariantDef,
+) -> Vec<bool> {
+    let typing_env = ty::TypingEnv::non_body_analysis(tcx, variant.def_id);
+    variant
+        .fields
+        .iter()
+        .map(|field| {
+            let field_ty = tcx.type_of(field.did).instantiate_identity();
+            let is_1zst = tcx
+                .layout_of(typing_env.as_query_input(field_ty))
+                .is_ok_and(|layout| layout.is_1zst());
+            !is_1zst
+        })
+        .collect()
+}
+
 /// Is type known to be non-null?
 fn ty_is_known_nonnull<'tcx>(
     tcx: TyCtxt<'tcx>,

compiler/rustc_lint/src/types/improper_ctypes.rs

@@ -36,18 +36,20 @@ fn get_type_from_field<'tcx>(
     cx.tcx.try_normalize_erasing_regions(cx.typing_env(), field_ty).unwrap_or(field_ty)
 }
 
-/// Check a variant of a non-exhaustive enum for improper ctypes
+/// Check a variant of a non-exhaustive enum for improper ctypes.
+/// Returns two bools: "we have FFI-unsafety due to non-exhaustive enum" and
+/// "we have FFI-unsafety due to a non-exhaustive enum variant".
 ///
 /// We treat `#[non_exhaustive] enum` as "ensure that code will compile if new variants are added".
 /// This includes linting, on a best-effort basis. There are valid additions that are unlikely.
 ///
 /// Adding a data-carrying variant to an existing C-like enum that is passed to C is "unlikely",
 /// so we don't need the lint to account for it.
 /// e.g. going from enum Foo { A, B, C } to enum Foo { A, B, C, D(u32) }.
-pub(crate) fn check_non_exhaustive_variant(
+pub(crate) fn flag_non_exhaustive_variant(
     non_exhaustive_variant_list: bool,
     variant: &ty::VariantDef,
-) -> ControlFlow<DiagMessage, ()> {
+) -> (bool, bool) {
     // non_exhaustive suggests it is possible that someone might break ABI
     // see: https://github.com/rust-lang/rust/issues/44109#issuecomment-537583344
     // so warn on complex enums being used outside their crate
@@ -56,15 +58,15 @@ pub(crate) fn check_non_exhaustive_variant(
         // with an enum like `#[repr(u8)] enum Enum { A(DataA), B(DataB), }`
         // but exempt enums with unit ctors like C's (e.g. from rust-bindgen)
         if variant_has_complex_ctor(variant) {
-            return ControlFlow::Break(fluent::lint_improper_ctypes_non_exhaustive);
+            return (true, false);
         }
     }
 
     if variant.field_list_has_applicable_non_exhaustive() {
-        return ControlFlow::Break(fluent::lint_improper_ctypes_non_exhaustive_variant);
+        return (false, true);
     }
 
-    ControlFlow::Continue(())
+    (false, false)
 }
 
 fn variant_has_complex_ctor(variant: &ty::VariantDef) -> bool {
@@ -707,46 +709,118 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
     ) -> FfiResult<'tcx> {
         use FfiResult::*;
 
-        let transparent_with_all_zst_fields = if def.repr().transparent() {
-            if let Some(field) = super::transparent_newtype_field(self.cx.tcx, variant) {
-                // Transparent newtypes have at most one non-ZST field which needs to be checked..
-                let field_ty = get_type_from_field(self.cx, field, args);
-                return self.visit_type(state, Some(ty), field_ty);
-                //match self.visit_type(state, Some(ty), field_ty) {
-                //    FfiUnsafe { ty, .. } if ty.is_unit() => (), // TODO lol
-                //    r => return r,
-                //}
-                //
-                // false
+        let mut ffires_accumulator = FfiSafe;
+
+        let (transparent_with_all_zst_fields, field_list) =
+            if !matches!(def.adt_kind(), AdtKind::Enum) && def.repr().transparent() {
+                // determine if there is 0 or 1 non-1ZST field, and which it is.
+                // (note: for enums, "transparent" means 1-variant)
+                if let Some(field) = super::transparent_newtype_field(self.cx.tcx, variant) {
+                    // Transparent newtypes have at most one non-ZST field which needs to be checked later
+                    (false, vec![field])
+                } else {
+                    // ..or have only ZST fields, which is FFI-unsafe (unless those fields are all
+                    // `PhantomData`).
+                    (true, variant.fields.iter().collect::<Vec<_>>())
+                }
             } else {
-                // ..or have only ZST fields, which is FFI-unsafe (unless those fields are all
-                // `PhantomData`).
-                true
-            }
-        } else {
-            false
-        };
+                (false, variant.fields.iter().collect::<Vec<_>>())
+            };
 
         // We can't completely trust `repr(C)` markings, so make sure the fields are actually safe.
         let mut all_phantom = !variant.fields.is_empty();
-        for field in &variant.fields {
+        let mut fields_ok_list = vec![true; field_list.len()];
+
+        for (field_i, field) in field_list.into_iter().enumerate() {
             let field_ty = get_type_from_field(self.cx, field, args);
-            all_phantom &= match self.visit_type(state, Some(ty), field_ty) {
-                FfiSafe => false,
-                // `()` fields are FFI-safe!
-                //FfiUnsafe { ty, .. } if ty.is_unit() => false, // TODO get back here
+            let ffi_res = self.visit_type(state, Some(ty), field_ty);
+
+            // checking that this is not an FfiUnsafe due to an unit type:
+            // visit_type should be smart enough to not consider it unsafe if called from another ADT
+            #[cfg(debug_assertions)]
+            if let FfiUnsafe(ref reasons) = ffi_res {
+                if let (1, Some(FfiUnsafeExplanation { reason, .. })) =
+                    (reasons.len(), reasons.first())
+                {
+                    let FfiUnsafeReason { ty, .. } = reason.as_ref();
+                    debug_assert!(!ty.is_unit());
+                }
+            }
+
+            all_phantom &= match ffi_res {
                 FfiPhantom(..) => true,
+                FfiSafe => false,
                 r @ FfiUnsafe { .. } => {
-                    return r.wrap_all(
-                        ty,
-                        fluent::lint_improper_ctypes_struct_dueto,
-                        None,
-                    );
+                    fields_ok_list[field_i] = false;
+                    ffires_accumulator += r;
+                    false
                 }
             }
         }
 
-        if all_phantom {
+        // if we have bad fields, also report a possible transparent_with_all_zst_fields
+        // (if this combination is somehow possible)
+        // otherwise, having all fields be phantoms
+        // takes priority over transparent_with_all_zst_fields
+        if let FfiUnsafe(explanations) = ffires_accumulator {
+            // we assume the repr() of this ADT is either non-packed C or transparent.
+            debug_assert!(
+                def.repr().c()
+                    || def.repr().transparent()
+                    || def.repr().int.is_some()
+            );
+
+            if def.repr().transparent() || matches!(def.adt_kind(), AdtKind::Enum) {
+                let field_ffires = FfiUnsafe(explanations).wrap_all(
+                    ty,
+                    fluent::lint_improper_ctypes_struct_dueto,
+                    None,
+                );
+                if transparent_with_all_zst_fields {
+                    field_ffires
+                        + FfiResult::new_with_reason(
+                            ty,
+                            fluent::lint_improper_ctypes_struct_zst,
+                            None,
+                        )
+                } else {
+                    field_ffires
+                }
+            } else {
+                // since we have a repr(C) struct/union, there's a chance that we have some unsafe fields,
+                // but also exactly one non-1ZST field that is FFI-safe:
+                // we want to suggest repr(transparent) here.
+                // (FIXME(ctypes): confirm that this makes sense for unions once #60405 / RFC2645 stabilises)
+                let non_1zst_fields = super::map_non_1zst_fields(self.cx.tcx, variant);
+                let (last_non_1zst, non_1zst_count) = non_1zst_fields.into_iter().enumerate().fold(
+                    (None, 0_usize),
+                    |(prev_nz, count), (field_i, is_nz)| {
+                        if is_nz { (Some(field_i), count + 1) } else { (prev_nz, count) }
+                    },
+                );
+                let help = if non_1zst_count == 1
+                    && last_non_1zst.map(|field_i| fields_ok_list[field_i]) == Some(true)
+                {
+                    match def.adt_kind() {
+                        AdtKind::Struct => {
+                            Some(fluent::lint_improper_ctypes_struct_consider_transparent)
+                        }
+                        AdtKind::Union => {
+                            Some(fluent::lint_improper_ctypes_union_consider_transparent)
+                        }
+                        AdtKind::Enum => bug!("cannot suggest an enum to be repr(transparent)"),
+                    }
+                } else {
+                    None
+                };
+
+                FfiUnsafe(explanations).wrap_all(
+                    ty,
+                    fluent::lint_improper_ctypes_struct_dueto,
+                    help,
+                )
+            }
+        } else if all_phantom {
             FfiPhantom(ty)
         } else if transparent_with_all_zst_fields {
             FfiResult::new_with_reason(ty, fluent::lint_improper_ctypes_struct_zst, None)
@@ -858,22 +932,42 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
 
         let non_exhaustive = def.variant_list_has_applicable_non_exhaustive();
         // Check the contained variants.
-        let ret = def.variants().iter().try_for_each(|variant| {
-            check_non_exhaustive_variant(non_exhaustive, variant)
-                .map_break(|reason| FfiResult::new_with_reason(ty, reason, None))?;
 
-            match self.visit_variant_fields(state, ty, def, variant, args) {
-                FfiSafe => ControlFlow::Continue(()),
-                r => ControlFlow::Break(r),
-            }
+        let (mut nonexhaustive_flag, mut nonexhaustive_variant_flag) = (false, false);
+        def.variants().iter().for_each(|variant| {
+            let (nonex_enum, nonex_var) = flag_non_exhaustive_variant(non_exhaustive, variant);
+            nonexhaustive_flag |= nonex_enum;
+            nonexhaustive_variant_flag |= nonex_var;
         });
-        if let ControlFlow::Break(result) = ret {
+
+        // "nonexhaustive" lints only happen outside of the crate defining the enum, so no CItemKind override
+        // (meaning: the fault lies in the function call, not the enum)
+        if nonexhaustive_flag {
+            FfiResult::new_with_reason(ty, fluent::lint_improper_ctypes_non_exhaustive, None)
+        } else if nonexhaustive_variant_flag {
+            FfiResult::new_with_reason(
+                ty,
+                fluent::lint_improper_ctypes_non_exhaustive_variant,
+                None,
+            )
+        } else {
+
+            let ffires = def
+                .variants()
+                .iter()
+                .map(|variant| {
+                    let variant_res = self.visit_variant_fields(state, ty, def, variant, args);
+                    // FIXME(ctypes): check that enums allow any (up to all) variants to be phantoms?
+                    // (previous code says no, but I don't know why? the problem with phantoms is that they're ZSTs, right?)
+                    variant_res.forbid_phantom()
+                })
+                .reduce(|r1, r2| r1 + r2)
+                .unwrap(); // always at least one variant if we hit this branch
+
             // this enum is visited in the middle of another lint,
             // so we override the "cause type" of the lint
-            // (for more detail, see comment in ``visit_struct_union`` before its call to ``result.with_overrides``)
-            result.with_overrides(Some(ty))
-        } else {
-            FfiSafe
+            // (for more detail, see comment in ``visit_struct_union`` before its call to ``ffires.with_overrides``)
+            ffires.with_overrides(Some(ty))
         }
     }
 

tests/ui/extern/extern-C-str-arg-ice-80125.stderr

@@ -15,8 +15,8 @@ warning: `extern` fn uses type `Struct`, which is not FFI-safe
 LL | pub extern "C" fn register_something(bind: ExternCallback) -> Struct {
    |                                                               ^^^^^^ not FFI-safe
    |
-   = help: consider adding a `#[repr(C)]` or `#[repr(transparent)]` attribute to this struct
-   = note: this struct has unspecified layout
+   = help: consider adding a `#[repr(C)]` (not `#[repr(C,packed)]`) or `#[repr(transparent)]` attribute to `Struct`
+   = note: `Struct` has unspecified layout
 note: the type is defined here
   --> $DIR/extern-C-str-arg-ice-80125.rs:6:1
    |

tests/ui/extern/issue-16250.stderr

@@ -4,8 +4,8 @@ error: `extern` block uses type `Foo`, which is not FFI-safe
 LL |     pub fn foo(x: (Foo));
    |                    ^^^ not FFI-safe
    |
-   = help: consider adding a `#[repr(C)]` or `#[repr(transparent)]` attribute to this struct
-   = note: this struct has unspecified layout
+   = help: consider adding a `#[repr(C)]` (not `#[repr(C,packed)]`) or `#[repr(transparent)]` attribute to `Foo`
+   = note: `Foo` has unspecified layout
 note: the type is defined here
   --> $DIR/issue-16250.rs:3:1
    |

tests/ui/lint/improper_ctypes/lint-113436-1.stderr

@@ -10,8 +10,8 @@ note: the type is defined here
    |
 LL | pub struct Bar {
    | ^^^^^^^^^^^^^^
-   = help: consider adding a `#[repr(C)]` or `#[repr(transparent)]` attribute to this struct
-   = note: this struct has unspecified layout
+   = help: consider adding a `#[repr(C)]` (not `#[repr(C,packed)]`) or `#[repr(transparent)]` attribute to `NotSafe`
+   = note: `NotSafe` has unspecified layout
 note: the type is defined here
   --> $DIR/lint-113436-1.rs:13:1
    |
@@ -35,8 +35,8 @@ note: the type is defined here
    |
 LL | pub struct Bar {
    | ^^^^^^^^^^^^^^
-   = help: consider adding a `#[repr(C)]` or `#[repr(transparent)]` attribute to this struct
-   = note: this struct has unspecified layout
+   = help: consider adding a `#[repr(C)]` (not `#[repr(C,packed)]`) or `#[repr(transparent)]` attribute to `NotSafe`
+   = note: `NotSafe` has unspecified layout
 note: the type is defined here
   --> $DIR/lint-113436-1.rs:13:1
    |

tests/ui/lint/improper_ctypes/lint-73249-5.stderr

@@ -4,6 +4,12 @@ error: `extern` block uses type `A`, which is not FFI-safe
 LL |     pub fn lint_me() -> A;
    |                         ^ not FFI-safe
    |
+   = note: this struct/enum/union (`A`) is FFI-unsafe due to a `Qux` field
+note: the type is defined here
+  --> $DIR/lint-73249-5.rs:16:1
+   |
+LL | pub struct A {
+   | ^^^^^^^^^^^^
    = note: opaque types have no C equivalent
 note: the lint level is defined here
   --> $DIR/lint-73249-5.rs:2:9