From e9cf280ef2ea4550d2305484873a63dfd133abb7 Mon Sep 17 00:00:00 2001 From: Jubilee Young Date: Mon, 16 Oct 2023 23:56:22 -0700 Subject: [PATCH 1/2] 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. --- compiler/rustc_lint/src/types.rs | 27 ++++++++++++------- .../improper_ctypes/auxiliary/types.rs | 11 ++++++++ .../improper_ctypes/extern_crate_improper.rs | 10 ++++++- .../extern_crate_improper.stderr | 10 +++---- 4 files changed, 43 insertions(+), 15 deletions(-) diff --git a/compiler/rustc_lint/src/types.rs b/compiler/rustc_lint/src/types.rs index 900c496e0330d..eef1f0d133ae4 100644 --- a/compiler/rustc_lint/src/types.rs +++ b/compiler/rustc_lint/src/types.rs @@ -3,6 +3,7 @@ use std::ops::ControlFlow; use rustc_data_structures::fx::FxHashSet; use rustc_errors::DiagMessage; +use rustc_hir::def::CtorKind; use rustc_hir::{is_range_literal, Expr, ExprKind, Node}; use rustc_middle::bug; use rustc_middle::ty::layout::{IntegerExt, LayoutOf, SizeSkeleton}; @@ -1386,15 +1387,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() @@ -1413,8 +1405,25 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> { }; } + // 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 + let nonexhaustive_nonlocal_ffi = + def.is_variant_list_non_exhaustive() && !def.did().is_local(); + // Check the contained variants. for variant in def.variants() { + // but only warn about really_tagged_union reprs, + // exempt enums with unit ctors like C's (like rust-bindgen) + if nonexhaustive_nonlocal_ffi + && !matches!(variant.ctor_kind(), Some(CtorKind::Const)) + { + return FfiUnsafe { + ty, + reason: fluent::lint_improper_ctypes_non_exhaustive, + help: None, + }; + }; let is_non_exhaustive = variant.is_field_list_non_exhaustive(); if is_non_exhaustive && !variant.def_id.is_local() { return FfiUnsafe { diff --git a/tests/ui/rfcs/rfc-2008-non-exhaustive/improper_ctypes/auxiliary/types.rs b/tests/ui/rfcs/rfc-2008-non-exhaustive/improper_ctypes/auxiliary/types.rs index d6251fcb768f4..4dc5932feab40 100644 --- a/tests/ui/rfcs/rfc-2008-non-exhaustive/improper_ctypes/auxiliary/types.rs +++ b/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, +} diff --git a/tests/ui/rfcs/rfc-2008-non-exhaustive/improper_ctypes/extern_crate_improper.rs b/tests/ui/rfcs/rfc-2008-non-exhaustive/improper_ctypes/extern_crate_improper.rs index 7a9b465bb56f6..c7f470fb787a7 100644 --- a/tests/ui/rfcs/rfc-2008-non-exhaustive/improper_ctypes/extern_crate_improper.rs +++ b/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() {} diff --git a/tests/ui/rfcs/rfc-2008-non-exhaustive/improper_ctypes/extern_crate_improper.stderr b/tests/ui/rfcs/rfc-2008-non-exhaustive/improper_ctypes/extern_crate_improper.stderr index 43c8e1015e674..afc3d3838ad38 100644 --- a/tests/ui/rfcs/rfc-2008-non-exhaustive/improper_ctypes/extern_crate_improper.stderr +++ b/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 From 62aa8f0740a5a847482df50ae457f22113630719 Mon Sep 17 00:00:00 2001 From: Jubilee Young Date: Thu, 19 Sep 2024 13:56:24 -0700 Subject: [PATCH 2/2] compiler: Embed consensus in `lint::types::improper_ctypes` Extracting this logic into a module makes it easier to write down, and more importantly, later find, the actual decisions we've made. --- compiler/rustc_lint/src/types.rs | 42 +++++---------- .../rustc_lint/src/types/improper_ctypes.rs | 51 +++++++++++++++++++ 2 files changed, 65 insertions(+), 28 deletions(-) create mode 100644 compiler/rustc_lint/src/types/improper_ctypes.rs diff --git a/compiler/rustc_lint/src/types.rs b/compiler/rustc_lint/src/types.rs index eef1f0d133ae4..48e9e78db1e6f 100644 --- a/compiler/rustc_lint/src/types.rs +++ b/compiler/rustc_lint/src/types.rs @@ -3,7 +3,6 @@ use std::ops::ControlFlow; use rustc_data_structures::fx::FxHashSet; use rustc_errors::DiagMessage; -use rustc_hir::def::CtorKind; use rustc_hir::{is_range_literal, Expr, ExprKind, Node}; use rustc_middle::bug; use rustc_middle::ty::layout::{IntegerExt, LayoutOf, SizeSkeleton}; @@ -19,6 +18,8 @@ use rustc_target::spec::abi::Abi as SpecAbi; use tracing::debug; use {rustc_ast as ast, rustc_attr as attr, rustc_hir as hir}; +mod improper_ctypes; + use crate::lints::{ AmbiguousWidePointerComparisons, AmbiguousWidePointerComparisonsAddrMetadataSuggestion, AmbiguousWidePointerComparisonsAddrSuggestion, AtomicOrderingFence, AtomicOrderingLoad, @@ -1405,38 +1406,23 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> { }; } - // 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 - let nonexhaustive_nonlocal_ffi = - def.is_variant_list_non_exhaustive() && !def.did().is_local(); + 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() { - // but only warn about really_tagged_union reprs, - // exempt enums with unit ctors like C's (like rust-bindgen) - if nonexhaustive_nonlocal_ffi - && !matches!(variant.ctor_kind(), Some(CtorKind::Const)) - { - return FfiUnsafe { - ty, - reason: fluent::lint_improper_ctypes_non_exhaustive, - help: None, - }; - }; - 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 diff --git a/compiler/rustc_lint/src/types/improper_ctypes.rs b/compiler/rustc_lint/src/types/improper_ctypes.rs new file mode 100644 index 0000000000000..1030101c545da --- /dev/null +++ b/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 { + // 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() +}