Rollup merge of #155359 - niacdoial:improperctypes-refactor2.2, r=petrochenkov

Improperctypes refactor2.2

This is "part 2/3 of 2/3 of 1/2" of the original pull request https://github.com/rust-lang/rust/pull/134697 (refactor plus overhaul of the ImproperCTypes family of lints)
(all pulls of this series of pulls are supersets of the previous pulls.)
previous pull: https://github.com/rust-lang/rust/pull/155358
next pull: https://github.com/rust-lang/rust/pull/146273

This commit splits the lint's `visit_type` function into multiple functions that focus on specific things:
- visit_indirection (references, boxes, raw pointers)
- visit_variant_fields (the list of fields of a struct, enum variant, or union)
- visit_enum
- visit_struct_or_union
- visit_type (most "easy" decisions such as labeling `char` unsafe are here)

since, during these visits, we often move from an "outer type" to an "inner type" (structs, arrays, pointers, etc...),
two structs have been added to track the current state of a visit:
- VisitorState tracks the state related to the "original type" being checked (function argument/return, static variable)
- OuterTyData tracks the data related to the type "immediately outer to the current visited type"

r? petrochenkov (because you asked me to)
This commit is contained in:
Jonathan Brouwer
2026-04-21 16:53:38 +02:00
committed by GitHub
+300 -173
View File
@@ -192,6 +192,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 == 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
@@ -273,6 +274,17 @@ enum FfiResult<'tcx> {
/// in the `FfiResult` is final.
type PartialFfiResult<'tcx> = Option<FfiResult<'tcx>>;
/// 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! {
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
struct VisitorState: u8 {
@@ -359,6 +371,35 @@ fn can_expect_ty_params(self) -> bool {
}
}
bitflags! {
/// Data that summarises how an "outer type" surrounds its inner type(s)
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
struct OuterTyData: u8 {
/// To show that there is no outer type, the current type is directly used by a `static`
/// variable or a function/FnPtr
const NO_OUTER_TY = 0b01;
/// For NO_OUTER_TY cases, show that we are being directly used by a FnPtr specifically
/// FIXME(ctypes): this is only used for "bad behaviour" reproduced for compatibility's sake
const NO_OUTER_TY_FNPTR = 0b10;
}
}
impl OuterTyData {
/// Get the proper data for a given outer type.
fn from_ty<'tcx>(ty: Ty<'tcx>) -> Self {
match ty.kind() {
ty::FnPtr(..) => Self::NO_OUTER_TY | Self::NO_OUTER_TY_FNPTR,
ty::RawPtr(..)
| ty::Ref(..)
| ty::Adt(..)
| ty::Tuple(..)
| ty::Array(..)
| ty::Slice(_) => Self::empty(),
k @ _ => bug!("unexpected outer type {:?} of kind {:?}", ty, k),
}
}
}
/// Visitor used to recursively traverse MIR types and evaluate FFI-safety.
/// It uses ``check_*`` methods as entrypoints to be called elsewhere,
/// and ``visit_*`` methods to recurse.
@@ -378,21 +419,94 @@ fn new(cx: &'a LateContext<'tcx>, base_ty: Ty<'tcx>, base_fn_mode: CItemKind) ->
Self { cx, base_ty, base_fn_mode, cache: FxHashSet::default() }
}
/// Checks if the given `VariantDef`'s field types are "ffi-safe".
fn check_variant_for_ffi(
/// Checks if the given indirection (box,ref,pointer) is "ffi-safe".
fn visit_indirection(
&mut self,
state: VisitorState,
ty: Ty<'tcx>,
def: ty::AdtDef<'tcx>,
inner_ty: Ty<'tcx>,
indirection_kind: IndirectionKind,
) -> FfiResult<'tcx> {
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: msg!("box cannot be represented as a single pointer"),
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, OuterTyData::from_ty(ty), inner_ty)
}
}
}
}
/// Checks if the given `VariantDef`'s field types are "ffi-safe".
fn visit_variant_fields(
&mut self,
state: VisitorState,
ty: Ty<'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..
let field_ty = get_type_from_field(self.cx, field, args);
match self.visit_type(state, field_ty) {
match self.visit_type(state, OuterTyData::from_ty(ty), field_ty) {
FfiUnsafe { ty, .. } if ty.is_unit() => (),
r => return r,
}
@@ -411,7 +525,7 @@ fn check_variant_for_ffi(
let mut all_phantom = !variant.fields.is_empty();
for field in &variant.fields {
let field_ty = get_type_from_field(self.cx, field, args);
all_phantom &= match self.visit_type(state, field_ty) {
all_phantom &= match self.visit_type(state, OuterTyData::from_ty(ty), field_ty) {
FfiSafe => false,
// `()` fields are FFI-safe!
FfiUnsafe { ty, .. } if ty.is_unit() => false,
@@ -433,9 +547,125 @@ fn check_variant_for_ffi(
}
}
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() {
msg!("this struct has unspecified layout")
} else {
msg!("this union has unspecified layout")
},
help: if def.is_struct() {
Some(msg!(
"consider adding a `#[repr(C)]` or `#[repr(transparent)]` attribute to this struct"
))
} else {
// FIXME(ctypes): confirm that this makes sense for unions once #60405 / RFC2645 stabilises
Some(msg!(
"consider adding a `#[repr(C)]` or `#[repr(transparent)]` attribute to this union"
))
},
};
}
if def.non_enum_variant().field_list_has_applicable_non_exhaustive() {
return FfiUnsafe {
ty,
reason: if def.is_struct() {
msg!("this struct is non-exhaustive")
} else {
msg!("this union is non-exhaustive")
},
help: None,
};
}
if def.non_enum_variant().fields.is_empty() {
FfiUnsafe {
ty,
reason: if def.is_struct() {
msg!("this struct has no fields")
} else {
msg!("this union has no fields")
},
help: if def.is_struct() {
Some(msg!("consider adding a member to this struct"))
} else {
Some(msg!("consider adding a member to this union"))
},
}
} 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<extern fn()>` and `Result<extern fn(), ()>`
if let Some(inner_ty) = repr_nullable_ptr(self.cx.tcx, self.cx.typing_env(), ty) {
return self.visit_type(state, OuterTyData::from_ty(ty), inner_ty);
}
return FfiUnsafe {
ty,
reason: msg!("enum has no representation hint"),
help: Some(msg!(
"consider adding a `#[repr(C)]`, `#[repr(transparent)]`, or integer `#[repr(...)]` attribute to this enum"
)),
};
}
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> {
fn visit_type(
&mut self,
state: VisitorState,
outer_ty: OuterTyData,
ty: Ty<'tcx>,
) -> FfiResult<'tcx> {
use FfiResult::*;
let tcx = self.cx.tcx;
@@ -450,23 +680,8 @@ fn visit_type(&mut self, state: VisitorState, ty: Ty<'tcx>) -> FfiResult<'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: msg!("box cannot be represented as a single pointer"),
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);
@@ -485,115 +700,29 @@ fn visit_type(&mut self, state: VisitorState, ty: Ty<'tcx>) -> FfiResult<'tcx> {
)),
};
}
if !def.repr().c() && !def.repr().transparent() {
return FfiUnsafe {
ty,
reason: if def.is_struct() {
msg!("this struct has unspecified layout")
} else {
msg!("this union has unspecified layout")
},
help: if def.is_struct() {
Some(msg!(
"consider adding a `#[repr(C)]` or `#[repr(transparent)]` attribute to this struct"
))
} else {
Some(msg!(
"consider adding a `#[repr(C)]` or `#[repr(transparent)]` attribute to this union"
))
},
};
}
if def.non_enum_variant().field_list_has_applicable_non_exhaustive() {
return FfiUnsafe {
ty,
reason: if def.is_struct() {
msg!("this struct is non-exhaustive")
} else {
msg!("this union is non-exhaustive")
},
help: None,
};
}
if def.non_enum_variant().fields.is_empty() {
return FfiUnsafe {
ty,
reason: if def.is_struct() {
msg!("this struct has no fields")
} else {
msg!("this union has no fields")
},
help: if def.is_struct() {
Some(msg!("consider adding a member to this struct"))
} else {
Some(msg!("consider adding a member to this union"))
},
};
}
self.check_variant_for_ffi(state, ty, def, def.non_enum_variant(), 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<extern fn()>` and `Result<extern fn(), ()>`
if let Some(ty) =
repr_nullable_ptr(self.cx.tcx, self.cx.typing_env(), ty)
{
return self.visit_type(state, ty);
}
return FfiUnsafe {
ty,
reason: msg!("enum has no representation hint"),
help: Some(msg!(
"consider adding a `#[repr(C)]`, `#[repr(transparent)]`, or integer `#[repr(...)]` attribute to this enum"
)),
};
}
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;
}
FfiSafe
self.visit_struct_or_union(state, ty, def, args)
}
AdtKind::Enum => self.visit_enum(state, ty, def, args),
}
}
ty::Char => FfiUnsafe {
// 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)
ty::Pat(pat_ty, _) => self.visit_type(state, outer_ty, pat_ty),
// 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,
ty::Bool => FfiResult::FfiSafe,
ty::Char => FfiResult::FfiUnsafe {
ty,
reason: msg!("the `char` type has no C equivalent"),
help: Some(msg!("consider using `u32` or `libc::wchar_t` instead")),
},
// 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: msg!("slices have no C equivalent"),
@@ -610,19 +739,21 @@ fn visit_type(&mut self, state: VisitorState, ty: Ty<'tcx>) -> FfiResult<'tcx> {
help: Some(msg!("consider using `*const u8` and a length instead")),
},
ty::Tuple(..) => FfiUnsafe {
ty,
reason: msg!("tuples have unspecified layout"),
help: Some(msg!("consider using a struct instead")),
},
ty::Tuple(tuple) => {
// C functions can return void
let empty_and_safe = tuple.is_empty()
&& outer_ty.contains(OuterTyData::NO_OUTER_TY)
&& state.is_in_function_return();
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
if empty_and_safe {
FfiSafe
} else {
FfiUnsafe {
ty,
reason: msg!("tuples have unspecified layout"),
help: Some(msg!("consider using a struct instead")),
}
}
}
ty::RawPtr(ty, _)
@@ -634,9 +765,32 @@ fn visit_type(&mut self, state: VisitorState, ty: Ty<'tcx>) -> FfiResult<'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_in_function()
&& outer_ty.contains(OuterTyData::NO_OUTER_TY)
// FIXME(ctypes): VVV-this-VVV shouldn't be the case
&& !outer_ty.contains(OuterTyData::NO_OUTER_TY_FNPTR)
{
// 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: msg!("passing raw arrays by value is not FFI-safe"),
help: Some(msg!("consider passing a pointer to the array")),
}
} else {
// let's allow phantoms to go through,
// since an array of 1-ZSTs is also a 1-ZST
self.visit_type(state, OuterTyData::from_ty(ty), inner_ty)
}
}
ty::FnPtr(sig_tys, hdr) => {
let sig = sig_tys.with(hdr);
@@ -652,22 +806,25 @@ fn visit_type(&mut self, state: VisitorState, ty: Ty<'tcx>) -> FfiResult<'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(
VisitorState::ARGUMENT_TY_IN_FNPTR,
OuterTyData::from_ty(ty),
*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(VisitorState::RETURN_TY_IN_FNPTR, OuterTyData::from_ty(ty), 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::AliasTy { kind: ty::Opaque { .. }, .. }) => {
@@ -729,20 +886,6 @@ fn visit_ty(&mut self, ty: Ty<'tcx>) -> Self::Result {
})
}
/// 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: msg!("passing raw arrays by value is not FFI-safe"),
help: Some(msg!("consider passing a pointer to the array")),
})
} 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
@@ -753,23 +896,7 @@ fn check_type(&mut self, state: VisitorState, ty: Ty<'tcx>) -> FfiResult<'tcx> {
return res;
}
// 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)
self.visit_type(state, OuterTyData::NO_OUTER_TY, ty)
}
}