Rollup merge of #116863 - workingjubilee:non-exhaustive-is-not-ffi-unsafe, r=jieyouxu

warn less about non-exhaustive in ffi

Bindgen allows generating `#[non_exhaustive] #[repr(u32)]` enums. This results in nonintuitive nonlocal `improper_ctypes` warnings, even when the types are otherwise perfectly valid in C.

Adjust for actual tooling expectations by avoiding warning on simple enums with only unit variants.

Closes https://github.com/rust-lang/rust/issues/116831

compiler/rustc_lint/src/types.rs

@@ -18,6 +18,8 @@ use rustc_target::spec::abi::Abi as SpecAbi;
 use tracing::debug;
 use {rustc_ast as ast, rustc_hir as hir};
 
+mod improper_ctypes;
+
 use crate::lints::{
     AmbiguousWidePointerComparisons, AmbiguousWidePointerComparisonsAddrMetadataSuggestion,
     AmbiguousWidePointerComparisonsAddrSuggestion, AtomicOrderingFence, AtomicOrderingLoad,
@@ -983,15 +985,6 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
                             // Empty enums are okay... although sort of useless.
                             return FfiSafe;
                         }
-
-                        if def.is_variant_list_non_exhaustive() && !def.did().is_local() {
-                            return FfiUnsafe {
-                                ty,
-                                reason: fluent::lint_improper_ctypes_non_exhaustive,
-                                help: None,
-                            };
-                        }
-
                         // Check for a repr() attribute to specify the size of the
                         // discriminant.
                         if !def.repr().c() && !def.repr().transparent() && def.repr().int.is_none()
@@ -1010,21 +1003,23 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
                             };
                         }
 
+                        use improper_ctypes::{
+                            check_non_exhaustive_variant, non_local_and_non_exhaustive,
+                        };
+
+                        let non_local_def = non_local_and_non_exhaustive(def);
                         // Check the contained variants.
-                        for variant in def.variants() {
-                            let is_non_exhaustive = variant.is_field_list_non_exhaustive();
-                            if is_non_exhaustive && !variant.def_id.is_local() {
-                                return FfiUnsafe {
-                                    ty,
-                                    reason: fluent::lint_improper_ctypes_non_exhaustive_variant,
-                                    help: None,
-                                };
-                            }
+                        let ret = def.variants().iter().try_for_each(|variant| {
+                            check_non_exhaustive_variant(non_local_def, variant)
+                                .map_break(|reason| FfiUnsafe { ty, reason, help: None })?;
 
                             match self.check_variant_for_ffi(acc, ty, def, variant, args) {
-                                FfiSafe => (),
-                                r => return r,
+                                FfiSafe => ControlFlow::Continue(()),
+                                r => ControlFlow::Break(r),
                             }
+                        });
+                        if let ControlFlow::Break(result) = ret {
+                            return result;
                         }
 
                         FfiSafe

compiler/rustc_lint/src/types/improper_ctypes.rs

@@ -0,0 +1,51 @@
+use std::ops::ControlFlow;
+
+use rustc_errors::DiagMessage;
+use rustc_hir::def::CtorKind;
+use rustc_middle::ty;
+
+use crate::fluent_generated as fluent;
+
+/// Check a variant of a non-exhaustive enum for improper ctypes
+///
+/// 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(
+    non_local_def: bool,
+    variant: &ty::VariantDef,
+) -> ControlFlow<DiagMessage, ()> {
+    // 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
+    if non_local_def {
+        // which is why we only warn about really_tagged_union reprs from https://rust.tf/rfc2195
+        // 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);
+        }
+    }
+
+    let non_exhaustive_variant_fields = variant.is_field_list_non_exhaustive();
+    if non_exhaustive_variant_fields && !variant.def_id.is_local() {
+        return ControlFlow::Break(fluent::lint_improper_ctypes_non_exhaustive_variant);
+    }
+
+    ControlFlow::Continue(())
+}
+
+fn variant_has_complex_ctor(variant: &ty::VariantDef) -> bool {
+    // CtorKind::Const means a "unit" ctor
+    !matches!(variant.ctor_kind(), Some(CtorKind::Const))
+}
+
+// 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
+pub(crate) fn non_local_and_non_exhaustive(def: ty::AdtDef<'_>) -> bool {
+    def.is_variant_list_non_exhaustive() && !def.did().is_local()
+}

tests/ui/rfcs/rfc-2008-non-exhaustive/improper_ctypes/auxiliary/types.rs

@@ -27,3 +27,14 @@ pub enum NonExhaustiveVariants {
     #[non_exhaustive] Tuple(u32),
     #[non_exhaustive] Struct { field: u32 }
 }
+
+// Note the absence of repr(C): it's not necessary, and recent C code can now use repr hints too.
+#[repr(u32)]
+#[non_exhaustive]
+pub enum NonExhaustiveCLikeEnum {
+    One = 1,
+    Two = 2,
+    Three = 3,
+    Four = 4,
+    Five = 5,
+}

tests/ui/rfcs/rfc-2008-non-exhaustive/improper_ctypes/extern_crate_improper.rs

@@ -6,7 +6,10 @@ extern crate types;
 // This test checks that non-exhaustive types with `#[repr(C)]` from an extern crate are considered
 // improper.
 
-use types::{NonExhaustiveEnum, NonExhaustiveVariants, NormalStruct, TupleStruct, UnitStruct};
+use types::{
+    NonExhaustiveCLikeEnum, NonExhaustiveEnum, NonExhaustiveVariants,
+    NormalStruct, TupleStruct, UnitStruct,
+};
 
 extern "C" {
     pub fn non_exhaustive_enum(_: NonExhaustiveEnum);
@@ -21,4 +24,9 @@ extern "C" {
     //~^ ERROR `extern` block uses type `NonExhaustiveVariants`, which is not FFI-safe
 }
 
+// These should pass without remark, as they're C-compatible, despite being "non-exhaustive".
+extern "C" {
+    pub fn non_exhaustive_c_compat_enum(_: NonExhaustiveCLikeEnum);
+}
+
 fn main() {}

tests/ui/rfcs/rfc-2008-non-exhaustive/improper_ctypes/extern_crate_improper.stderr

@@ -1,5 +1,5 @@
 error: `extern` block uses type `NonExhaustiveEnum`, which is not FFI-safe
-  --> $DIR/extern_crate_improper.rs:12:35
+  --> $DIR/extern_crate_improper.rs:15:35
    |
 LL |     pub fn non_exhaustive_enum(_: NonExhaustiveEnum);
    |                                   ^^^^^^^^^^^^^^^^^ not FFI-safe
@@ -12,7 +12,7 @@ LL | #![deny(improper_ctypes)]
    |         ^^^^^^^^^^^^^^^
 
 error: `extern` block uses type `NormalStruct`, which is not FFI-safe
-  --> $DIR/extern_crate_improper.rs:14:44
+  --> $DIR/extern_crate_improper.rs:17:44
    |
 LL |     pub fn non_exhaustive_normal_struct(_: NormalStruct);
    |                                            ^^^^^^^^^^^^ not FFI-safe
@@ -20,7 +20,7 @@ LL |     pub fn non_exhaustive_normal_struct(_: NormalStruct);
    = note: this struct is non-exhaustive
 
 error: `extern` block uses type `UnitStruct`, which is not FFI-safe
-  --> $DIR/extern_crate_improper.rs:16:42
+  --> $DIR/extern_crate_improper.rs:19:42
    |
 LL |     pub fn non_exhaustive_unit_struct(_: UnitStruct);
    |                                          ^^^^^^^^^^ not FFI-safe
@@ -28,7 +28,7 @@ LL |     pub fn non_exhaustive_unit_struct(_: UnitStruct);
    = note: this struct is non-exhaustive
 
 error: `extern` block uses type `TupleStruct`, which is not FFI-safe
-  --> $DIR/extern_crate_improper.rs:18:43
+  --> $DIR/extern_crate_improper.rs:21:43
    |
 LL |     pub fn non_exhaustive_tuple_struct(_: TupleStruct);
    |                                           ^^^^^^^^^^^ not FFI-safe
@@ -36,7 +36,7 @@ LL |     pub fn non_exhaustive_tuple_struct(_: TupleStruct);
    = note: this struct is non-exhaustive
 
 error: `extern` block uses type `NonExhaustiveVariants`, which is not FFI-safe
-  --> $DIR/extern_crate_improper.rs:20:38
+  --> $DIR/extern_crate_improper.rs:23:38
    |
 LL |     pub fn non_exhaustive_variant(_: NonExhaustiveVariants);
    |                                      ^^^^^^^^^^^^^^^^^^^^^ not FFI-safe