ImproperCTypes: change handling of FnPtrs

niacdoial · niacdoial · commit 3d4ce20202ee · 2025-08-30T21:50:51.000+02:00
Notably, those FnPtrs are treated as "the item impacted by the error",
instead of the functions/structs making use of them.
diff --git a/compiler/rustc_lint/messages.ftl b/compiler/rustc_lint/messages.ftl
@@ -383,7 +383,9 @@ lint_improper_ctypes_enum_repr_help =
 lint_improper_ctypes_enum_repr_reason = enum has no representation hint
 lint_improper_ctypes_fnptr_help = consider using an `extern fn(...) -> ...` function pointer instead
 
+lint_improper_ctypes_fnptr_indirect_reason = the function pointer to `{$ty}` is FFI-unsafe due to `{$inner_ty}`
 lint_improper_ctypes_fnptr_reason = this function pointer has Rust-specific calling convention
+
 lint_improper_ctypes_non_exhaustive = this enum is non-exhaustive
 lint_improper_ctypes_non_exhaustive_variant = this enum has non-exhaustive variants
 
diff --git a/compiler/rustc_lint/src/types/improper_ctypes.rs b/compiler/rustc_lint/src/types/improper_ctypes.rs
@@ -11,8 +11,8 @@ use rustc_hir::intravisit::VisitorExt;
 use rustc_hir::{self as hir, AmbigArg};
 use rustc_middle::bug;
 use rustc_middle::ty::{
-    self, Adt, AdtDef, AdtKind, GenericArgsRef, Ty, TyCtxt, TypeSuperVisitable, TypeVisitable,
-    TypeVisitableExt,
+    self, Adt, AdtDef, AdtKind, Binder, FnSig, GenericArgsRef, Ty, TyCtxt, TypeSuperVisitable,
+    TypeVisitable, TypeVisitableExt,
 };
 use rustc_session::{declare_lint, declare_lint_pass};
 use rustc_span::def_id::LocalDefId;
@@ -23,6 +23,8 @@ use super::repr_nullable_ptr;
 use crate::lints::{ImproperCTypes, ImproperCTypesLayer, UsesPowerAlignment};
 use crate::{LateContext, LateLintPass, LintContext, fluent_generated as fluent};
 
+type Sig<'tcx> = Binder<'tcx, FnSig<'tcx>>;
+
 /// Getting the (normalized) type out of a field (for, e.g., an enum variant or a tuple).
 #[inline]
 fn get_type_from_field<'tcx>(
@@ -156,7 +158,6 @@ impl<'tcx> FfiResult<'tcx> {
     }
     /// If the FfiPhantom variant, turns it into a FfiUnsafe version.
     /// Otherwise, keep unchanged.
-    #[expect(unused)]
     fn forbid_phantom(self) -> Self {
         match self {
             Self::FfiPhantom(ty) => {
@@ -528,6 +529,41 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
         }
     }
 
+    /// Checks whether an `extern "ABI" fn` function pointer is indeed FFI-safe to call.
+    fn visit_fnptr(
+        &self,
+        _state: VisitorState,
+        _outer_ty: Option<Ty<'tcx>>,
+        ty: Ty<'tcx>,
+        sig: Sig<'tcx>,
+    ) -> FfiResult<'tcx> {
+        use FfiResult::*;
+        debug_assert!(!sig.abi().is_rustic_abi());
+
+        let sig = self.cx.tcx.instantiate_bound_regions_with_erased(sig);
+
+        let mut all_ffires = FfiSafe;
+
+        for arg in sig.inputs() {
+            let ffi_res = self.visit_type(VisitorState::ArgumentTyInFnPtr, Some(ty), *arg);
+            all_ffires += ffi_res.forbid_phantom().wrap_all(
+                ty,
+                fluent::lint_improper_ctypes_fnptr_indirect_reason,
+                None,
+            );
+        }
+
+        let ret_ty = sig.output();
+
+        let ffi_res = self.visit_type(VisitorState::ReturnTyInFnPtr, Some(ty), ret_ty);
+        all_ffires += ffi_res.forbid_phantom().wrap_all(
+            ty,
+            fluent::lint_improper_ctypes_fnptr_indirect_reason,
+            None,
+        );
+        all_ffires
+    }
+
     /// Checks if a simple numeric (int, float) type has an actual portable definition
     /// for the compile target.
     fn visit_numeric(&self, ty: Ty<'tcx>) -> FfiResult<'tcx> {
@@ -955,27 +991,21 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
                 }
             }
 
+            // fnptrs are a special case, they always need to be treated as
+            // "the element rendered unsafe" because their unsafety doesn't affect
+            // their surroundings, and their type is often declared inline
+            // as a result, don't go into them when scanning for the safety of something else
             ty::FnPtr(sig_tys, hdr) => {
                 let sig = sig_tys.with(hdr);
                 if sig.abi().is_rustic_abi() {
-                    return FfiResult::new_with_reason(
+                    FfiResult::new_with_reason(
                         ty,
                         fluent::lint_improper_ctypes_fnptr_reason,
                         Some(fluent::lint_improper_ctypes_fnptr_help),
-                    );
-                }
-
-                let sig = tcx.instantiate_bound_regions_with_erased(sig);
-                for arg in sig.inputs() {
-                    match self.visit_type(VisitorState::ArgumentTyInFnPtr, Some(ty), *arg) {
-                        FfiSafe => {}
-                        r => return r,
-                    }
+                    )
+                } else {
+                    FfiSafe
                 }
-
-                let ret_ty = sig.output();
-
-                self.visit_type(VisitorState::ReturnTyInFnPtr, Some(ty), ret_ty)
             }
 
             ty::Foreign(..) => FfiSafe,
@@ -1046,10 +1076,29 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
                 return res;
             }
         }
-
         self.visit_type(state, None, ty)
     }
 
+    fn check_for_fnptr(&self, ty: Ty<'tcx>) -> FfiResult<'tcx> {
+        let ty =  self.cx.tcx.try_normalize_erasing_regions(self.cx.typing_env(), ty).unwrap_or(ty);
+
+        match *ty.kind() {
+            ty::FnPtr(sig_tys, hdr) => {
+                let sig = sig_tys.with(hdr);
+                if sig.abi().is_rustic_abi() {
+                    bug!(
+                        "expected to inspect the type of an `extern \"ABI\"` FnPtr, not an internal-ABI one"
+                    )
+                } else {
+                    self.visit_fnptr(VisitorState::None, None, ty, sig)
+                }
+            }
+            r @ _ => {
+                bug!("expected to inspect the type of an `extern \"ABI\"` FnPtr, not {:?}", r,)
+            }
+        }
+    }
+
     fn check_arg_for_power_alignment(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
         let tcx = cx.tcx;
         assert!(tcx.sess.target.os == "aix");
@@ -1117,7 +1166,6 @@ impl<'tcx> ImproperCTypesLint {
     fn check_type_for_external_abi_fnptr(
         &mut self,
         cx: &LateContext<'tcx>,
-        state: VisitorState,
         hir_ty: &hir::Ty<'tcx>,
         ty: Ty<'tcx>,
     ) {
@@ -1160,8 +1208,7 @@ impl<'tcx> ImproperCTypesLint {
         let all_types = iter::zip(visitor.tys.drain(..), visitor.spans.drain(..));
         all_types.for_each(|(fn_ptr_ty, span)| {
             let visitor = ImproperCTypesVisitor::new(cx);
-            // TODO: make a check_for_fnptr
-            let ffi_res = visitor.check_for_type(state, fn_ptr_ty);
+            let ffi_res = visitor.check_for_fnptr(fn_ptr_ty);
 
             self.process_ffi_result(cx, span, ffi_res, CItemKind::Callback)
         });
@@ -1172,21 +1219,18 @@ impl<'tcx> ImproperCTypesLint {
     fn check_fn_for_external_abi_fnptr(
         &mut self,
         cx: &LateContext<'tcx>,
-        fn_mode: CItemKind,
         def_id: LocalDefId,
         decl: &'tcx hir::FnDecl<'_>,
     ) {
         let sig = cx.tcx.fn_sig(def_id).instantiate_identity();
         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);
-            self.check_type_for_external_abi_fnptr(cx, state, input_hir, *input_ty);
+            self.check_type_for_external_abi_fnptr(cx, input_hir, *input_ty);
         }
 
         if let hir::FnRetTy::Return(ret_hir) = decl.output {
-            let state = VisitorState::return_from_fnmode(fn_mode);
-            self.check_type_for_external_abi_fnptr(cx, state, ret_hir, sig.output());
+            self.check_type_for_external_abi_fnptr(cx, ret_hir, sig.output());
         }
     }
 
@@ -1207,6 +1251,7 @@ impl<'tcx> ImproperCTypesLint {
         ImproperCTypesVisitor::check_struct_for_power_alignment(cx, item, adt_def);
     }
 
+    /// Check that an extern "ABI" static variable is of a ffi-safe type.
     fn check_foreign_static(&self, cx: &LateContext<'tcx>, id: hir::OwnerId, span: Span) {
         let ty = cx.tcx.type_of(id).instantiate_identity();
         let visitor = ImproperCTypesVisitor::new(cx);
@@ -1359,20 +1404,14 @@ impl<'tcx> LateLintPass<'tcx> for ImproperCTypesLint {
                 // fnptrs are a special case, they always need to be treated as
                 // "the element rendered unsafe" because their unsafety doesn't affect
                 // their surroundings, and their type is often declared inline
+                self.check_fn_for_external_abi_fnptr(cx, it.owner_id.def_id, sig.decl);
                 if !abi.is_rustic_abi() {
                     self.check_foreign_fn(
                         cx,
                         CItemKind::ImportedExtern,
                         it.owner_id.def_id,
                         sig.decl,
                     );
-                } else {
-                    self.check_fn_for_external_abi_fnptr(
-                        cx,
-                        CItemKind::ImportedExtern,
-                        it.owner_id.def_id,
-                        sig.decl,
-                    );
                 }
             }
             hir::ForeignItemKind::Static(ty, _, _) if !abi.is_rustic_abi() => {
@@ -1389,7 +1428,6 @@ impl<'tcx> LateLintPass<'tcx> for ImproperCTypesLint {
             | hir::ItemKind::TyAlias(_, _, ty) => {
                 self.check_type_for_external_abi_fnptr(
                     cx,
-                    VisitorState::StaticTy,
                     ty,
                     cx.tcx.type_of(item.owner_id).instantiate_identity(),
                 );
@@ -1422,7 +1460,6 @@ 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::StaticTy,
             field.ty,
             cx.tcx.type_of(field.def_id).instantiate_identity(),
         );
@@ -1448,10 +1485,9 @@ impl<'tcx> LateLintPass<'tcx> for ImproperCTypesLint {
         // fnptrs are a special case, they always need to be treated as
         // "the element rendered unsafe" because their unsafety doesn't affect
         // their surroundings, and their type is often declared inline
+        self.check_fn_for_external_abi_fnptr(cx, id, decl);
         if !abi.is_rustic_abi() {
             self.check_foreign_fn(cx, CItemKind::ExportedFunction, id, decl);
-        } else {
-            self.check_fn_for_external_abi_fnptr(cx, CItemKind::ExportedFunction, id, decl);
         }
     }
 }
diff --git a/tests/ui/lint/improper_ctypes/lint-94223.stderr b/tests/ui/lint/improper_ctypes/lint-94223.stderr
@@ -4,6 +4,7 @@ error: `extern` callback uses type `[u8]`, which is not FFI-safe
 LL | pub fn bad(f: extern "C" fn([u8])) {}
    |               ^^^^^^^^^^^^^^^^^^^ not FFI-safe
    |
+   = note: the function pointer to `extern "C" fn([u8])` is FFI-unsafe due to `[u8]`
    = help: consider using a raw pointer instead
    = note: slices have no C equivalent
 note: the lint level is defined here
@@ -18,6 +19,7 @@ error: `extern` callback uses type `[u8]`, which is not FFI-safe
 LL | pub fn bad_twice(f: Result<extern "C" fn([u8]), extern "C" fn([u8])>) {}
    |                            ^^^^^^^^^^^^^^^^^^^ not FFI-safe
    |
+   = note: the function pointer to `extern "C" fn([u8])` is FFI-unsafe due to `[u8]`
    = help: consider using a raw pointer instead
    = note: slices have no C equivalent
 
@@ -27,6 +29,7 @@ error: `extern` callback uses type `[u8]`, which is not FFI-safe
 LL | pub fn bad_twice(f: Result<extern "C" fn([u8]), extern "C" fn([u8])>) {}
    |                                                 ^^^^^^^^^^^^^^^^^^^ not FFI-safe
    |
+   = note: the function pointer to `extern "C" fn([u8])` is FFI-unsafe due to `[u8]`
    = help: consider using a raw pointer instead
    = note: slices have no C equivalent
 
@@ -36,6 +39,7 @@ error: `extern` callback uses type `[u8]`, which is not FFI-safe
 LL | struct BadStruct(extern "C" fn([u8]));
    |                  ^^^^^^^^^^^^^^^^^^^ not FFI-safe
    |
+   = note: the function pointer to `extern "C" fn([u8])` is FFI-unsafe due to `[u8]`
    = help: consider using a raw pointer instead
    = note: slices have no C equivalent
 
@@ -45,6 +49,7 @@ error: `extern` callback uses type `[u8]`, which is not FFI-safe
 LL |     A(extern "C" fn([u8])),
    |       ^^^^^^^^^^^^^^^^^^^ not FFI-safe
    |
+   = note: the function pointer to `extern "C" fn([u8])` is FFI-unsafe due to `[u8]`
    = help: consider using a raw pointer instead
    = note: slices have no C equivalent
 
@@ -54,6 +59,7 @@ error: `extern` callback uses type `[u8]`, which is not FFI-safe
 LL |     A(extern "C" fn([u8])),
    |       ^^^^^^^^^^^^^^^^^^^ not FFI-safe
    |
+   = note: the function pointer to `extern "C" fn([u8])` is FFI-unsafe due to `[u8]`
    = help: consider using a raw pointer instead
    = note: slices have no C equivalent
 
@@ -63,6 +69,7 @@ error: `extern` callback uses type `[u8]`, which is not FFI-safe
 LL | type Foo = extern "C" fn([u8]);
    |            ^^^^^^^^^^^^^^^^^^^ not FFI-safe
    |
+   = note: the function pointer to `extern "C" fn([u8])` is FFI-unsafe due to `[u8]`
    = help: consider using a raw pointer instead
    = note: slices have no C equivalent
 
@@ -72,6 +79,7 @@ error: `extern` callback uses type `Option<&<T as FooTrait>::FooType>`, which is
 LL | pub type Foo2<T> = extern "C" fn(Option<&<T as FooTrait>::FooType>);
    |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ not FFI-safe
    |
+   = note: the function pointer to `for<'a> extern "C" fn(Option<&'a <T as FooTrait>::FooType>)` is FFI-unsafe due to `Option<&<T as FooTrait>::FooType>`
    = help: consider adding a `#[repr(C)]`, `#[repr(transparent)]`, or integer `#[repr(...)]` attribute to this enum
    = note: enum has no representation hint
 
@@ -81,6 +89,7 @@ error: `extern` callback uses type `FfiUnsafe`, which is not FFI-safe
 LL | pub static BAD: extern "C" fn(FfiUnsafe) = f;
    |                 ^^^^^^^^^^^^^^^^^^^^^^^^ not FFI-safe
    |
+   = note: the function pointer to `extern "C" fn(FfiUnsafe)` is FFI-unsafe due to `FfiUnsafe`
    = help: consider adding a `#[repr(C)]` or `#[repr(transparent)]` attribute to this struct
    = note: this struct has unspecified layout
 note: the type is defined here
@@ -95,6 +104,7 @@ error: `extern` callback uses type `FfiUnsafe`, which is not FFI-safe
 LL | pub static BAD_TWICE: Result<extern "C" fn(FfiUnsafe), extern "C" fn(FfiUnsafe)> = Ok(f);
    |                              ^^^^^^^^^^^^^^^^^^^^^^^^ not FFI-safe
    |
+   = note: the function pointer to `extern "C" fn(FfiUnsafe)` is FFI-unsafe due to `FfiUnsafe`
    = help: consider adding a `#[repr(C)]` or `#[repr(transparent)]` attribute to this struct
    = note: this struct has unspecified layout
 note: the type is defined here
@@ -109,6 +119,7 @@ error: `extern` callback uses type `FfiUnsafe`, which is not FFI-safe
 LL | pub static BAD_TWICE: Result<extern "C" fn(FfiUnsafe), extern "C" fn(FfiUnsafe)> = Ok(f);
    |                                                        ^^^^^^^^^^^^^^^^^^^^^^^^ not FFI-safe
    |
+   = note: the function pointer to `extern "C" fn(FfiUnsafe)` is FFI-unsafe due to `FfiUnsafe`
    = help: consider adding a `#[repr(C)]` or `#[repr(transparent)]` attribute to this struct
    = note: this struct has unspecified layout
 note: the type is defined here
@@ -123,6 +134,7 @@ error: `extern` callback uses type `FfiUnsafe`, which is not FFI-safe
 LL | pub const BAD_CONST: extern "C" fn(FfiUnsafe) = f;
    |                      ^^^^^^^^^^^^^^^^^^^^^^^^ not FFI-safe
    |
+   = note: the function pointer to `extern "C" fn(FfiUnsafe)` is FFI-unsafe due to `FfiUnsafe`
    = help: consider adding a `#[repr(C)]` or `#[repr(transparent)]` attribute to this struct
    = note: this struct has unspecified layout
 note: the type is defined here
diff --git a/tests/ui/lint/improper_ctypes/lint-fn.rs b/tests/ui/lint/improper_ctypes/lint-fn.rs
@@ -1,5 +1,5 @@
 #![allow(private_interfaces)]
-#![deny(improper_c_fn_definitions, improper_c_callbacks)]
+#![deny(improper_c_callbacks, improper_c_fn_definitions)]
 
 use std::default::Default;
 use std::marker::PhantomData;
@@ -114,21 +114,22 @@ pub extern "C" fn fn_type2(p: fn()) { }
 //~^ ERROR uses type `fn()`
 
 pub extern "C" fn fn_contained(p: RustBadRet) { }
-//~^ ERROR: uses type `(u32, u64)`
+// ^ FIXME it doesn't see the error... but at least it reports it elsewhere?
 
 pub extern "C" fn transparent_str(p: TransparentStr) { }
 //~^ ERROR: uses type `&str`
 
 pub extern "C" fn transparent_fn(p: TransparentBadFn) { }
-//~^ ERROR: uses type `(u32, u64)`
+// ^ possible FIXME: it doesn't see the actual FnPtr's error...
+//   but at least it reports it elsewhere?
 
 pub extern "C" fn good3(fptr: Option<extern "C" fn()>) { }
 
-pub extern "C" fn good4(aptr: &[u8; 4 as usize]) { }
+pub extern "C" fn argument_with_assumptions_4(aptr: &[u8; 4 as usize]) { }
 
 pub extern "C" fn good5(s: StructWithProjection) { }
 
-pub extern "C" fn good6(s: StructWithProjectionAndLifetime) { }
+pub extern "C" fn argument_with_assumptions_6(s: StructWithProjectionAndLifetime) { }
 
 pub extern "C" fn good7(fptr: extern "C" fn() -> ()) { }
 
@@ -144,7 +145,7 @@ pub extern "C" fn good12(size: usize) { }
 
 pub extern "C" fn good13(n: TransparentInt) { }
 
-pub extern "C" fn good14(p: TransparentRef) { }
+pub extern "C" fn argument_with_assumptions_14(p: TransparentRef) { }
 
 pub extern "C" fn good15(p: TransparentLifetime) { }
 
diff --git a/tests/ui/lint/improper_ctypes/lint-fn.stderr b/tests/ui/lint/improper_ctypes/lint-fn.stderr
