diff --git a/compiler/rustc_lint/src/types/improper_ctypes.rs b/compiler/rustc_lint/src/types/improper_ctypes.rs index 2c88397086699..23b997ddaebd6 100644 --- a/compiler/rustc_lint/src/types/improper_ctypes.rs +++ b/compiler/rustc_lint/src/types/improper_ctypes.rs @@ -137,6 +137,17 @@ declare_lint_pass!(ImproperCTypesLint => [ USES_POWER_ALIGNMENT ]); +/// Getting the (normalized) type out of a field (for, e.g., an enum variant or a tuple). +#[inline] +fn get_type_from_field<'tcx>( + cx: &LateContext<'tcx>, + field: &ty::FieldDef, + args: GenericArgsRef<'tcx>, +) -> Ty<'tcx> { + let field_ty = field.ty(cx.tcx, args); + 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 /// /// We treat `#[non_exhaustive] enum` as "ensure that code will compile if new variants are added". @@ -178,6 +189,7 @@ fn variant_has_complex_ctor(variant: &ty::VariantDef) -> bool { fn check_arg_for_power_alignment<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool { let tcx = cx.tcx; assert!(tcx.sess.target.os == "aix"); + // Structs (under repr(C)) follow the power alignment rule if: // - the first field of the struct is a floating-point type that // is greater than 4-bytes, or @@ -242,12 +254,22 @@ fn check_struct_for_power_alignment<'tcx>( } } +/// Annotates whether we are in the context of an item *defined* in rust +/// and exposed to an FFI boundary, +/// or the context of an item from elsewhere, whose interface is re-*declared* in rust. #[derive(Clone, Copy)] enum CItemKind { Declaration, Definition, } +/// Annotates whether we are in the context of a function's argument types or return type. +#[derive(Clone, Copy)] +enum FnPos { + Arg, + Ret, +} + enum FfiResult<'tcx> { FfiSafe, FfiPhantom(Ty<'tcx>), @@ -259,9 +281,22 @@ enum FfiResult<'tcx> { /// in the `FfiResult` is final. type PartialFfiResult<'tcx> = Option>; +/// What type indirection points to a given type. +#[derive(Clone, Copy)] +enum IndirectionKind { + /// Box (valid non-null pointer, owns pointee). + Box, + /// Ref (valid non-null pointer, borrows pointee). + Ref, + /// Raw pointer (not necessarily non-null or valid. no info on ownership). + RawPtr, +} + bitflags! { + /// VisitorState flags that are linked with the root type's use. + /// (These are the permanent part of the state, kept when visiting new mir::Ty.) #[derive(Clone, Copy, Debug, PartialEq, Eq)] - struct VisitorState: u8 { + struct RootUseFlags: u16 { /// For use in (externally-linked) static variables. const STATIC = 0b000001; /// For use in functions in general. @@ -276,7 +311,45 @@ bitflags! { } } -impl VisitorState { +/// Description of the relationship between current mir::Ty and +/// the type (or lack thereof) immediately containing it +#[derive(Copy, Clone, Debug, PartialEq, Eq)] +enum OuterTyKind { + None, + /// A variant that should not exist, + /// but is needed because we don't change the lint's behavior yet + NoneThroughFnPtr, + /// Placeholder for properties that will be used eventually + UnusedVariant, +} + +impl OuterTyKind { + /// Computes the relationship by providing the containing mir::Ty itself + fn from_outer_ty<'tcx>(ty: Ty<'tcx>) -> Self { + match ty.kind() { + ty::FnPtr(..) => Self::NoneThroughFnPtr, + ty::RawPtr(..) + | ty::Ref(..) + | ty::Adt(..) + | ty::Tuple(..) + | ty::Array(..) + | ty::Slice(_) => Self::UnusedVariant, + k @ _ => bug!("Unexpected outer type {:?} of kind {:?}", ty, k), + } + } +} + +#[derive(Copy, Clone, Debug, PartialEq, Eq)] +struct VisitorState { + /// Flags describing both the overall context in which the current mir::Ty is, + /// linked to how the Visitor's original mir::Ty was used. + root_use_flags: RootUseFlags, + /// Flags describing both the immediate context in which the current mir::Ty is, + /// linked to how it relates to its parent mir::Ty (or lack thereof). + outer_ty_kind: OuterTyKind, +} + +impl RootUseFlags { // The values that can be set. const STATIC_TY: Self = Self::STATIC; const ARGUMENT_TY_IN_DEFINITION: Self = @@ -291,57 +364,100 @@ impl VisitorState { const RETURN_TY_IN_FNPTR: Self = Self::from_bits(Self::FUNC.bits() | Self::THEORETICAL.bits() | Self::FN_RETURN.bits()) .unwrap(); +} - /// Get the proper visitor state for a given function's arguments. - fn argument_from_fnmode(fn_mode: CItemKind) -> Self { - match fn_mode { - CItemKind::Definition => VisitorState::ARGUMENT_TY_IN_DEFINITION, - CItemKind::Declaration => VisitorState::ARGUMENT_TY_IN_DECLARATION, +impl VisitorState { + /// From an existing state, compute the state of any subtype of the current type. + /// (General case.) + fn get_next<'tcx>(&self, current_ty: Ty<'tcx>) -> Self { + assert!(!matches!(current_ty.kind(), ty::FnPtr(..))); + Self { + root_use_flags: self.root_use_flags, + outer_ty_kind: OuterTyKind::from_outer_ty(current_ty), } } - - /// Get the proper visitor state for a given function's return type. - fn return_from_fnmode(fn_mode: CItemKind) -> Self { - match fn_mode { - CItemKind::Definition => VisitorState::RETURN_TY_IN_DEFINITION, - CItemKind::Declaration => VisitorState::RETURN_TY_IN_DECLARATION, + /// From an existing state, compute the state of any subtype of the current type. + /// (Case where the current type is a function pointer, + /// meaning we need to specify if the subtype is an argument or the return.) + fn get_next_in_fnptr<'tcx>(&self, current_ty: Ty<'tcx>, fn_pos: FnPos) -> Self { + assert!(matches!(current_ty.kind(), ty::FnPtr(..))); + Self { + root_use_flags: match fn_pos { + FnPos::Ret => RootUseFlags::RETURN_TY_IN_FNPTR, + FnPos::Arg => RootUseFlags::ARGUMENT_TY_IN_FNPTR, + }, + outer_ty_kind: OuterTyKind::from_outer_ty(current_ty), } } + /// Generate the state for an "outermost" type that needs to be checked + fn entry_point(root_use_flags: RootUseFlags) -> Self { + Self { root_use_flags, outer_ty_kind: OuterTyKind::None } + } + + /// Get the proper visitor state for a given function's arguments or return type. + fn entry_point_from_fnmode(fn_mode: CItemKind, fn_pos: FnPos) -> Self { + let p_flags = match (fn_mode, fn_pos) { + (CItemKind::Definition, FnPos::Ret) => RootUseFlags::RETURN_TY_IN_DEFINITION, + (CItemKind::Declaration, FnPos::Ret) => RootUseFlags::RETURN_TY_IN_DECLARATION, + (CItemKind::Definition, FnPos::Arg) => RootUseFlags::ARGUMENT_TY_IN_DEFINITION, + (CItemKind::Declaration, FnPos::Arg) => RootUseFlags::ARGUMENT_TY_IN_DECLARATION, + }; + Self::entry_point(p_flags) + } + + /// Get the proper visitor state for a static variable's type + fn static_var() -> Self { + Self::entry_point(RootUseFlags::STATIC_TY) + } + /// Whether the type is used in a function. - fn is_in_function(self) -> bool { - let ret = self.contains(Self::FUNC); + fn is_in_function(&self) -> bool { + let ret = self.root_use_flags.contains(RootUseFlags::FUNC); if ret { - debug_assert!(!self.contains(Self::STATIC)); + debug_assert!(!self.root_use_flags.contains(RootUseFlags::STATIC)); } ret } /// Whether the type is used (directly or not) in a function, in return position. - fn is_in_function_return(self) -> bool { - let ret = self.contains(Self::FN_RETURN); + fn is_in_function_return(&self) -> bool { + let ret = self.root_use_flags.contains(RootUseFlags::FN_RETURN); if ret { debug_assert!(self.is_in_function()); } ret } + + /// Whether the type is directly used in a function, in return position. + fn is_direct_function_return(&self) -> bool { + matches!(self.outer_ty_kind, OuterTyKind::None | OuterTyKind::NoneThroughFnPtr) + && self.is_in_function_return() + } + + /// Whether the type itself is the type of a function argument or return type. + fn is_direct_in_function(&self) -> bool { + matches!(self.outer_ty_kind, OuterTyKind::None | OuterTyKind::NoneThroughFnPtr) + && self.is_in_function() + } + /// Whether the type is used (directly or not) in a defined function. /// In other words, whether or not we allow non-FFI-safe types behind a C pointer, /// to be treated as an opaque type on the other side of the FFI boundary. - fn is_in_defined_function(self) -> bool { - self.contains(Self::DEFINED) && self.is_in_function() + fn is_in_defined_function(&self) -> bool { + self.root_use_flags.contains(RootUseFlags::DEFINED) && self.is_in_function() } /// Whether the type is used (directly or not) in a function pointer type. /// Here, we also allow non-FFI-safe types behind a C pointer, /// to be treated as an opaque type on the other side of the FFI boundary. - fn is_in_fnptr(self) -> bool { - self.contains(Self::THEORETICAL) && self.is_in_function() + fn is_in_fnptr(&self) -> bool { + self.root_use_flags.contains(RootUseFlags::THEORETICAL) && self.is_in_function() } /// Whether we can expect type parameters and co in a given type. - fn can_expect_ty_params(self) -> bool { + fn can_expect_ty_params(&self) -> bool { // rust-defined functions, as well as FnPtrs - self.contains(Self::THEORETICAL) || self.is_in_defined_function() + self.root_use_flags.contains(RootUseFlags::THEORETICAL) || self.is_in_defined_function() } } @@ -364,36 +480,94 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> { Self { cx, base_ty, base_fn_mode, cache: FxHashSet::default() } } - /// Checks if the given field's type is "ffi-safe". - fn check_field_type_for_ffi( + /// Checks if the given indirection (box,ref,pointer) is "ffi-safe". + fn visit_indirection( &mut self, state: VisitorState, - field: &ty::FieldDef, - args: GenericArgsRef<'tcx>, + ty: Ty<'tcx>, + inner_ty: Ty<'tcx>, + indirection_kind: IndirectionKind, ) -> FfiResult<'tcx> { - let field_ty = field.ty(self.cx.tcx, args); - let field_ty = self - .cx - .tcx - .try_normalize_erasing_regions(self.cx.typing_env(), field_ty) - .unwrap_or(field_ty); - self.visit_type(state, field_ty) + use FfiResult::*; + let tcx = self.cx.tcx; + + match indirection_kind { + IndirectionKind::Box => { + // FIXME(ctypes): this logic is broken, but it still fits the current tests: + // - for some reason `Box<_>`es in `extern "ABI" {}` blocks + // (including within FnPtr:s) + // are not treated as pointers but as FFI-unsafe structs + // - otherwise, treat the box itself correctly, and follow pointee safety logic + // as described in the other `indirection_type` match branch. + if state.is_in_defined_function() + || (state.is_in_fnptr() && matches!(self.base_fn_mode, CItemKind::Definition)) + { + if inner_ty.is_sized(tcx, self.cx.typing_env()) { + return FfiSafe; + } else { + return FfiUnsafe { + ty, + reason: fluent::lint_improper_ctypes_box, + help: None, + }; + } + } else { + // (mid-retcon-commit-chain comment:) + // this is the original fallback behavior, which is wrong + if let ty::Adt(def, args) = ty.kind() { + self.visit_struct_or_union(state, ty, *def, args) + } else if cfg!(debug_assertions) { + bug!("ImproperCTypes: this retcon commit was badly written") + } else { + FfiSafe + } + } + } + IndirectionKind::Ref | IndirectionKind::RawPtr => { + // Weird behaviour for pointee safety. the big question here is + // "if you have a FFI-unsafe pointee behind a FFI-safe pointer type, is it ok?" + // The answer until now is: + // "It's OK for rust-defined functions and callbacks, we'll assume those are + // meant to be opaque types on the other side of the FFI boundary". + // + // Reasoning: + // For extern function declarations, the actual definition of the function is + // written somewhere else, meaning the declaration is free to express this + // opaqueness with an extern type (opaque caller-side) or a std::ffi::c_void + // (opaque callee-side). For extern function definitions, however, in the case + // where the type is opaque caller-side, it is not opaque callee-side, + // and having the full type information is necessary to compile the function. + // + // It might be better to rething this, or even ignore pointee safety for a first + // batch of behaviour changes. See the discussion that ends with + // https://github.com/rust-lang/rust/pull/134697#issuecomment-2692610258 + if (state.is_in_defined_function() || state.is_in_fnptr()) + && inner_ty.is_sized(self.cx.tcx, self.cx.typing_env()) + { + FfiSafe + } else { + self.visit_type(state.get_next(ty), inner_ty) + } + } + } } /// Checks if the given `VariantDef`'s field types are "ffi-safe". - fn check_variant_for_ffi( + fn visit_variant_fields( &mut self, state: VisitorState, ty: Ty<'tcx>, - def: ty::AdtDef<'tcx>, + def: AdtDef<'tcx>, variant: &ty::VariantDef, args: GenericArgsRef<'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.. - match self.check_field_type_for_ffi(state, field, args) { + let field_ty = get_type_from_field(self.cx, field, args); + match self.visit_type(state.get_next(ty), field_ty) { FfiUnsafe { ty, .. } if ty.is_unit() => (), r => return r, } @@ -411,7 +585,8 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> { // 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 { - all_phantom &= match self.check_field_type_for_ffi(state, field, args) { + let field_ty = get_type_from_field(self.cx, field, args); + all_phantom &= match self.visit_type(state.get_next(ty), field_ty) { FfiSafe => false, // `()` fields are FFI-safe! FfiUnsafe { ty, .. } if ty.is_unit() => false, @@ -429,6 +604,111 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> { } } + fn visit_struct_or_union( + &mut self, + state: VisitorState, + ty: Ty<'tcx>, + def: AdtDef<'tcx>, + args: GenericArgsRef<'tcx>, + ) -> FfiResult<'tcx> { + debug_assert!(matches!(def.adt_kind(), AdtKind::Struct | AdtKind::Union)); + use FfiResult::*; + + if !def.repr().c() && !def.repr().transparent() { + return FfiUnsafe { + ty, + reason: if def.is_struct() { + fluent::lint_improper_ctypes_struct_layout_reason + } else { + fluent::lint_improper_ctypes_union_layout_reason + }, + help: if def.is_struct() { + Some(fluent::lint_improper_ctypes_struct_layout_help) + } else { + // FIXME(#60405): confirm that this makes sense for unions once #60405 / RFC2645 stabilises + Some(fluent::lint_improper_ctypes_union_layout_help) + }, + }; + } + + if def.non_enum_variant().field_list_has_applicable_non_exhaustive() { + return FfiUnsafe { + ty, + reason: if def.is_struct() { + fluent::lint_improper_ctypes_struct_non_exhaustive + } else { + fluent::lint_improper_ctypes_union_non_exhaustive + }, + help: None, + }; + } + + if def.non_enum_variant().fields.is_empty() { + return FfiUnsafe { + ty, + reason: if def.is_struct() { + fluent::lint_improper_ctypes_struct_fieldless_reason + } else { + fluent::lint_improper_ctypes_union_fieldless_reason + }, + help: if def.is_struct() { + Some(fluent::lint_improper_ctypes_struct_fieldless_help) + } else { + Some(fluent::lint_improper_ctypes_union_fieldless_help) + }, + }; + } else { + self.visit_variant_fields(state, ty, def, def.non_enum_variant(), args) + } + } + + fn visit_enum( + &mut self, + state: VisitorState, + ty: Ty<'tcx>, + def: AdtDef<'tcx>, + args: GenericArgsRef<'tcx>, + ) -> FfiResult<'tcx> { + debug_assert!(matches!(def.adt_kind(), AdtKind::Enum)); + use FfiResult::*; + + if def.variants().is_empty() { + // Empty enums are okay... although sort of useless. + return FfiSafe; + } + // Check for a repr() attribute to specify the size of the + // discriminant. + if !def.repr().c() && !def.repr().transparent() && def.repr().int.is_none() { + // Special-case types like `Option` and `Result` + if let Some(inner_ty) = repr_nullable_ptr(self.cx.tcx, self.cx.typing_env(), ty) { + return self.visit_type(state.get_next(ty), inner_ty); + } + + return FfiUnsafe { + ty, + reason: fluent::lint_improper_ctypes_enum_repr_reason, + help: Some(fluent::lint_improper_ctypes_enum_repr_help), + }; + } + + 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| FfiUnsafe { ty, reason, help: None })?; + + match self.visit_variant_fields(state, ty, def, variant, args) { + FfiSafe => ControlFlow::Continue(()), + r => ControlFlow::Break(r), + } + }); + if let ControlFlow::Break(result) = ret { + return result; + } + + FfiSafe + } + /// Checks if the given type is "ffi-safe" (has a stable, well-defined /// representation which can be exported to C code). fn visit_type(&mut self, state: VisitorState, ty: Ty<'tcx>) -> FfiResult<'tcx> { @@ -446,23 +726,8 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> { match *ty.kind() { ty::Adt(def, args) => { - if let Some(boxed) = ty.boxed_ty() - && ( - // FIXME(ctypes): this logic is broken, but it still fits the current tests - state.is_in_defined_function() - || (state.is_in_fnptr() - && matches!(self.base_fn_mode, CItemKind::Definition)) - ) - { - if boxed.is_sized(tcx, self.cx.typing_env()) { - return FfiSafe; - } else { - return FfiUnsafe { - ty, - reason: fluent::lint_improper_ctypes_box, - help: None, - }; - } + if let Some(inner_ty) = ty.boxed_ty() { + return self.visit_indirection(state, ty, inner_ty, IndirectionKind::Box); } if def.is_phantom_data() { return FfiPhantom(ty); @@ -479,109 +744,30 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> { help: Some(fluent::lint_improper_ctypes_cstr_help), }; } - - if !def.repr().c() && !def.repr().transparent() { - return FfiUnsafe { - ty, - reason: if def.is_struct() { - fluent::lint_improper_ctypes_struct_layout_reason - } else { - fluent::lint_improper_ctypes_union_layout_reason - }, - help: if def.is_struct() { - Some(fluent::lint_improper_ctypes_struct_layout_help) - } else { - Some(fluent::lint_improper_ctypes_union_layout_help) - }, - }; - } - - if def.non_enum_variant().field_list_has_applicable_non_exhaustive() { - return FfiUnsafe { - ty, - reason: if def.is_struct() { - fluent::lint_improper_ctypes_struct_non_exhaustive - } else { - fluent::lint_improper_ctypes_union_non_exhaustive - }, - help: None, - }; - } - - if def.non_enum_variant().fields.is_empty() { - return FfiUnsafe { - ty, - reason: if def.is_struct() { - fluent::lint_improper_ctypes_struct_fieldless_reason - } else { - fluent::lint_improper_ctypes_union_fieldless_reason - }, - help: if def.is_struct() { - Some(fluent::lint_improper_ctypes_struct_fieldless_help) - } else { - Some(fluent::lint_improper_ctypes_union_fieldless_help) - }, - }; - } - - self.check_variant_for_ffi(state, ty, def, def.non_enum_variant(), args) + self.visit_struct_or_union(state, ty, def, args) } - AdtKind::Enum => { - if def.variants().is_empty() { - // Empty enums are okay... although sort of useless. - return FfiSafe; - } - // Check for a repr() attribute to specify the size of the - // discriminant. - if !def.repr().c() && !def.repr().transparent() && def.repr().int.is_none() - { - // Special-case types like `Option` and `Result` - if let Some(ty) = - repr_nullable_ptr(self.cx.tcx, self.cx.typing_env(), ty) - { - return self.visit_type(state, ty); - } + AdtKind::Enum => self.visit_enum(state, ty, def, args), + } + } - return FfiUnsafe { - ty, - reason: fluent::lint_improper_ctypes_enum_repr_reason, - help: Some(fluent::lint_improper_ctypes_enum_repr_help), - }; - } + // Pattern types are just extra invariants on the type that you need to uphold, + // but only the base type is relevant for being representable in FFI. + // (note: this lint was written when pattern types could only be integers constrained to ranges) + // (also note: the lack of ".get_next(ty)" on the state is on purpose) + ty::Pat(pat_ty, _) => self.visit_type(state, pat_ty), - 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| FfiUnsafe { ty, reason, help: None })?; - - match self.check_variant_for_ffi(state, ty, def, variant, args) { - FfiSafe => ControlFlow::Continue(()), - r => ControlFlow::Break(r), - } - }); - if let ControlFlow::Break(result) = ret { - return result; - } + // types which likely have a stable representation, if the target architecture defines those + // note: before rust 1.77, 128-bit ints were not FFI-safe on x86_64 + ty::Int(..) | ty::Uint(..) | ty::Float(..) => FfiResult::FfiSafe, - FfiSafe - } - } - } + ty::Bool => FfiResult::FfiSafe, - ty::Char => FfiUnsafe { + ty::Char => FfiResult::FfiUnsafe { ty, reason: fluent::lint_improper_ctypes_char_reason, help: Some(fluent::lint_improper_ctypes_char_help), }, - // It's just extra invariants on the type that you need to uphold, - // but only the base type is relevant for being representable in FFI. - ty::Pat(base, ..) => self.visit_type(state, base), - - // Primitive types with a stable representation. - ty::Bool | ty::Int(..) | ty::Uint(..) | ty::Float(..) | ty::Never => FfiSafe, - ty::Slice(_) => FfiUnsafe { ty, reason: fluent::lint_improper_ctypes_slice_reason, @@ -598,19 +784,17 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> { help: Some(fluent::lint_improper_ctypes_str_help), }, - ty::Tuple(..) => FfiUnsafe { - ty, - reason: fluent::lint_improper_ctypes_tuple_reason, - help: Some(fluent::lint_improper_ctypes_tuple_help), - }, - - ty::RawPtr(ty, _) | ty::Ref(_, ty, _) - if { - (state.is_in_defined_function() || state.is_in_fnptr()) - && ty.is_sized(self.cx.tcx, self.cx.typing_env()) - } => - { - FfiSafe + ty::Tuple(tuple) => { + if tuple.is_empty() && state.is_direct_function_return() { + // C functions can return void + FfiSafe + } else { + FfiUnsafe { + ty, + reason: fluent::lint_improper_ctypes_tuple_reason, + help: Some(fluent::lint_improper_ctypes_tuple_help), + } + } } ty::RawPtr(ty, _) @@ -622,9 +806,31 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> { FfiSafe } - ty::RawPtr(ty, _) | ty::Ref(_, ty, _) => self.visit_type(state, ty), + ty::RawPtr(inner_ty, _) => { + return self.visit_indirection(state, ty, inner_ty, IndirectionKind::RawPtr); + } + ty::Ref(_, inner_ty, _) => { + return self.visit_indirection(state, ty, inner_ty, IndirectionKind::Ref); + } - ty::Array(inner_ty, _) => self.visit_type(state, inner_ty), + ty::Array(inner_ty, _) => { + if state.is_direct_in_function() + // FIXME(ctypes): VVV-this-VVV shouldn't be part of the check + && !matches!(state.outer_ty_kind, OuterTyKind::NoneThroughFnPtr) + { + // C doesn't really support passing arrays by value - the only way to pass an array by value + // is through a struct. + FfiResult::FfiUnsafe { + ty, + reason: fluent::lint_improper_ctypes_array_reason, + help: Some(fluent::lint_improper_ctypes_array_help), + } + } else { + // let's allow phantoms to go through, + // since an array of 1-ZSTs is also a 1-ZST + self.visit_type(state.get_next(ty), inner_ty) + } + } ty::FnPtr(sig_tys, hdr) => { let sig = sig_tys.with(hdr); @@ -638,22 +844,20 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> { let sig = tcx.instantiate_bound_regions_with_erased(sig); for arg in sig.inputs() { - match self.visit_type(VisitorState::ARGUMENT_TY_IN_FNPTR, *arg) { + match self.visit_type(state.get_next_in_fnptr(ty, FnPos::Arg), *arg) { FfiSafe => {} r => return r, } } let ret_ty = sig.output(); - if ret_ty.is_unit() { - return FfiSafe; - } - - self.visit_type(VisitorState::RETURN_TY_IN_FNPTR, ret_ty) + self.visit_type(state.get_next_in_fnptr(ty, FnPos::Ret), ret_ty) } ty::Foreign(..) => FfiSafe, + ty::Never => FfiSafe, + // 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::Alias(ty::Opaque, ..) => { @@ -702,61 +906,19 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> { } } - if let Some(ty) = self - .cx - .tcx - .try_normalize_erasing_regions(self.cx.typing_env(), ty) - .unwrap_or(ty) - .visit_with(&mut ProhibitOpaqueTypes) - .break_value() - { - Some(FfiResult::FfiUnsafe { - ty, - reason: fluent::lint_improper_ctypes_opaque, - help: None, - }) - } else { - None - } + ty.visit_with(&mut ProhibitOpaqueTypes).break_value().map(|ty| FfiResult::FfiUnsafe { + ty, + reason: fluent::lint_improper_ctypes_opaque, + help: None, + }) } - /// Check if the type is array and emit an unsafe type lint. - fn check_for_array_ty(&mut self, ty: Ty<'tcx>) -> PartialFfiResult<'tcx> { - if let ty::Array(..) = ty.kind() { - Some(FfiResult::FfiUnsafe { - ty, - reason: fluent::lint_improper_ctypes_array_reason, - help: Some(fluent::lint_improper_ctypes_array_help), - }) - } else { - None - } - } - - /// Determine the FFI-safety of a single (MIR) type, given the context of how it is used. fn check_type(&mut self, state: VisitorState, ty: Ty<'tcx>) -> FfiResult<'tcx> { + let ty = self.cx.tcx.try_normalize_erasing_regions(self.cx.typing_env(), ty).unwrap_or(ty); if let Some(res) = self.visit_for_opaque_ty(ty) { return res; } - let ty = self.cx.tcx.try_normalize_erasing_regions(self.cx.typing_env(), ty).unwrap_or(ty); - - // C doesn't really support passing arrays by value - the only way to pass an array by value - // is through a struct. So, first test that the top level isn't an array, and then - // recursively check the types inside. - if state.is_in_function() { - if let Some(res) = self.check_for_array_ty(ty) { - return res; - } - } - - // Don't report FFI errors for unit return types. This check exists here, and not in - // the caller (where it would make more sense) so that normalization has definitely - // happened. - if state.is_in_function_return() && ty.is_unit() { - return FfiResult::FfiSafe; - } - self.visit_type(state, ty) } } @@ -786,7 +948,7 @@ impl<'tcx> ImproperCTypesLint { self.spans.push(ty.span); } - hir::intravisit::walk_ty(self, ty) + hir::intravisit::walk_ty(self, ty); } } @@ -831,12 +993,12 @@ impl<'tcx> ImproperCTypesLint { let sig = cx.tcx.instantiate_bound_regions_with_erased(sig); for (input_ty, input_hir) in iter::zip(sig.inputs(), decl.inputs) { - let state = VisitorState::argument_from_fnmode(fn_mode); + let state = VisitorState::entry_point_from_fnmode(fn_mode, FnPos::Arg); self.check_type_for_external_abi_fnptr(cx, state, input_hir, *input_ty, fn_mode); } if let hir::FnRetTy::Return(ret_hir) = decl.output { - let state = VisitorState::return_from_fnmode(fn_mode); + let state = VisitorState::entry_point_from_fnmode(fn_mode, FnPos::Ret); self.check_type_for_external_abi_fnptr(cx, state, ret_hir, sig.output(), fn_mode); } } @@ -861,7 +1023,7 @@ impl<'tcx> ImproperCTypesLint { fn check_foreign_static(&mut self, cx: &LateContext<'tcx>, id: hir::OwnerId, span: Span) { let ty = cx.tcx.type_of(id).instantiate_identity(); let mut visitor = ImproperCTypesVisitor::new(cx, ty, CItemKind::Declaration); - let ffi_res = visitor.check_type(VisitorState::STATIC_TY, ty); + let ffi_res = visitor.check_type(VisitorState::static_var(), ty); self.process_ffi_result(cx, span, ffi_res, CItemKind::Declaration); } @@ -877,14 +1039,14 @@ impl<'tcx> ImproperCTypesLint { let sig = cx.tcx.instantiate_bound_regions_with_erased(sig); for (input_ty, input_hir) in iter::zip(sig.inputs(), decl.inputs) { - let state = VisitorState::argument_from_fnmode(fn_mode); + let state = VisitorState::entry_point_from_fnmode(fn_mode, FnPos::Arg); let mut visitor = ImproperCTypesVisitor::new(cx, *input_ty, fn_mode); let ffi_res = visitor.check_type(state, *input_ty); self.process_ffi_result(cx, input_hir.span, ffi_res, fn_mode); } if let hir::FnRetTy::Return(ret_hir) = decl.output { - let state = VisitorState::return_from_fnmode(fn_mode); + let state = VisitorState::entry_point_from_fnmode(fn_mode, FnPos::Ret); let mut visitor = ImproperCTypesVisitor::new(cx, sig.output(), fn_mode); let ffi_res = visitor.check_type(state, sig.output()); self.process_ffi_result(cx, ret_hir.span, ffi_res, fn_mode); @@ -985,7 +1147,7 @@ impl<'tcx> LateLintPass<'tcx> for ImproperCTypesLint { | hir::ItemKind::TyAlias(_, _, ty) => { self.check_type_for_external_abi_fnptr( cx, - VisitorState::STATIC_TY, + VisitorState::static_var(), ty, cx.tcx.type_of(item.owner_id).instantiate_identity(), CItemKind::Definition, @@ -1019,7 +1181,7 @@ impl<'tcx> LateLintPass<'tcx> for ImproperCTypesLint { fn check_field_def(&mut self, cx: &LateContext<'tcx>, field: &'tcx hir::FieldDef<'tcx>) { self.check_type_for_external_abi_fnptr( cx, - VisitorState::STATIC_TY, + VisitorState::static_var(), field.ty, cx.tcx.type_of(field.def_id).instantiate_identity(), CItemKind::Definition, diff --git a/tests/ui/lint/improper-ctypes/lint-non-recursion-limit.rs b/tests/ui/lint/improper-ctypes/lint-non-recursion-limit.rs index 61e95dc5a464c..1d75a7ca9ab9f 100644 --- a/tests/ui/lint/improper-ctypes/lint-non-recursion-limit.rs +++ b/tests/ui/lint/improper-ctypes/lint-non-recursion-limit.rs @@ -1,5 +1,9 @@ //@ check-pass +//! This test checks that the depth limit of the ImproperCTypes lints counts the depth +//! of a type properly. +//! Issue: https://github.com/rust-lang/rust/issues/130757 + #![recursion_limit = "5"] #![allow(unused)] #![deny(improper_ctypes)]