ImproperCTypes: change cstr linting

niacdoial · niacdoial · commit a31918845d02 · 2025-11-01T22:37:22.000+01:00
another user-visible change: change the messaging and help around
CStr/CString lints
diff --git a/compiler/rustc_lint/messages.ftl b/compiler/rustc_lint/messages.ftl
@@ -349,8 +349,10 @@ lint_improper_ctypes_char_help = consider using `u32` or `libc::wchar_t` instead
 
 lint_improper_ctypes_char_reason = the `char` type has no C equivalent
 
-lint_improper_ctypes_cstr_help =
-    consider passing a `*const std::ffi::c_char` instead, and use `CStr::as_ptr()`
+lint_improper_ctypes_cstr_help_const =
+    consider passing a `*const std::ffi::c_char` instead, converting to/from `{$ty}` as needed
+lint_improper_ctypes_cstr_help_mut =
+    consider passing a `*mut std::ffi::c_char` instead, converting to/from `{$ty}` as needed
 lint_improper_ctypes_cstr_reason = `CStr`/`CString` do not have a guaranteed layout
 
 lint_improper_ctypes_dyn = trait objects have no C equivalent
@@ -373,8 +375,8 @@ lint_improper_ctypes_opaque = opaque types have no C equivalent
 lint_improper_ctypes_slice_help = consider using a raw pointer instead
 
 lint_improper_ctypes_slice_reason = slices have no C equivalent
-lint_improper_ctypes_str_help = consider using `*const u8` and a length instead
 
+lint_improper_ctypes_str_help = consider using `*const u8` and a length instead
 lint_improper_ctypes_str_reason = string slices have no C equivalent
 lint_improper_ctypes_struct_fieldless_help = consider adding a member to this struct
 
@@ -396,6 +398,10 @@ lint_improper_ctypes_union_layout_help = consider adding a `#[repr(C)]` or `#[re
 lint_improper_ctypes_union_layout_reason = this union has unspecified layout
 lint_improper_ctypes_union_non_exhaustive = this union is non-exhaustive
 
+lint_improper_ctypes_unsized_box = this box for an unsized type contains metadata, which makes it incompatible with a C pointer
+lint_improper_ctypes_unsized_ptr = this pointer to an unsized type contains metadata, which makes it incompatible with a C pointer
+lint_improper_ctypes_unsized_ref = this reference to an unsized type contains metadata, which makes it incompatible with a C pointer
+
 lint_int_to_ptr_transmutes = transmuting an integer to a pointer creates a pointer without provenance
     .note = this is dangerous because dereferencing the resulting pointer is undefined behavior
     .note_exposed_provenance = exposed provenance semantics can be used to create a pointer based on some previously exposed provenance
diff --git a/compiler/rustc_lint/src/types/improper_ctypes.rs b/compiler/rustc_lint/src/types/improper_ctypes.rs
@@ -356,7 +356,6 @@ impl<'tcx> FfiResult<'tcx> {
     /// If the FfiUnsafe variant, 'wraps' all reasons,
     /// creating new `FfiUnsafeReason`s, putting the originals as their `inner` fields.
     /// Otherwise, keep unchanged.
-    #[expect(unused)]
     fn wrap_all(self, ty: Ty<'tcx>, note: DiagMessage, help: Option<DiagMessage>) -> Self {
         match self {
             Self::FfiUnsafe(this) => {
@@ -525,6 +524,12 @@ bitflags! {
 #[derive(Copy, Clone, Debug, PartialEq, Eq)]
 enum OuterTyKind {
     None,
+    /// Pointee through ref, raw pointer or Box
+    /// (we don't need to distinguish the ownership of Box specifically)
+    Pointee {
+        mutable: hir::Mutability,
+        raw: bool,
+    },
     /// For struct/enum/union fields
     AdtField,
     /// Placeholder for properties that will be used eventually
@@ -536,16 +541,17 @@ impl OuterTyKind {
     fn from_outer_ty<'tcx>(ty: Ty<'tcx>) -> Self {
         match ty.kind() {
             ty::FnPtr(..) => Self::None,
+            k @ (ty::Ref(_, _, mutable) | ty::RawPtr(_, mutable)) => {
+                Self::Pointee { raw: matches!(k, ty::RawPtr(..)), mutable: *mutable }
+            }
             ty::Adt(..) => {
                 if ty.boxed_ty().is_some() {
-                    Self::UnusedVariant
+                    Self::Pointee { raw: false, mutable: hir::Mutability::Mut }
                 } else {
                     Self::AdtField
                 }
             }
-            ty::RawPtr(..) | ty::Ref(..) | ty::Tuple(..) | ty::Array(..) | ty::Slice(_) => {
-                Self::UnusedVariant
-            }
+            ty::Tuple(..) | ty::Array(..) | ty::Slice(_) => Self::UnusedVariant,
             k @ _ => bug!("Unexpected outer type {:?} of kind {:?}", ty, k),
         }
     }
@@ -666,6 +672,11 @@ impl VisitorState {
     fn is_field(&self) -> bool {
         matches!(self.outer_ty_kind, OuterTyKind::AdtField)
     }
+
+    /// Whether the current type is behind a pointer that doesn't allow mutating this
+    fn is_nonmut_pointee(&self) -> bool {
+        matches!(self.outer_ty_kind, OuterTyKind::Pointee { mutable: hir::Mutability::Not, .. })
+    }
 }
 
 /// Visitor used to recursively traverse MIR types and evaluate FFI-safety.
@@ -676,14 +687,29 @@ struct ImproperCTypesVisitor<'a, 'tcx> {
     /// To prevent problems with recursive types,
     /// add a types-in-check cache.
     cache: FxHashSet<Ty<'tcx>>,
-    /// The original type being checked, before we recursed
-    /// to any other types it contains.
-    base_ty: Ty<'tcx>,
 }
 
 impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
-    fn new(cx: &'a LateContext<'tcx>, base_ty: Ty<'tcx>) -> Self {
-        Self { cx, base_ty, cache: FxHashSet::default() }
+    fn new(cx: &'a LateContext<'tcx>) -> Self {
+        Self { cx, cache: FxHashSet::default() }
+    }
+
+    /// Return the right help for Cstring and Cstr-linked unsafety.
+    fn visit_cstr(&mut self, state: VisitorState, ty: Ty<'tcx>) -> FfiResult<'tcx> {
+        debug_assert!(matches!(ty.kind(), ty::Adt(def, _)
+            if matches!(
+                self.cx.tcx.get_diagnostic_name(def.did()),
+                Some(sym::cstring_type | sym::cstr_type)
+            )
+        ));
+
+        let help = if state.is_nonmut_pointee() {
+            fluent::lint_improper_ctypes_cstr_help_const
+        } else {
+            fluent::lint_improper_ctypes_cstr_help_mut
+        };
+
+        FfiResult::new_with_reason(ty, fluent::lint_improper_ctypes_cstr_reason, Some(help))
     }
 
     /// Checks if the given indirection (box,ref,pointer) is "ffi-safe".
@@ -697,6 +723,35 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
         use FfiResult::*;
         let tcx = self.cx.tcx;
 
+        if let ty::Adt(def, _) = inner_ty.kind() {
+            if let Some(diag_name @ (sym::cstring_type | sym::cstr_type)) =
+                tcx.get_diagnostic_name(def.did())
+            {
+                // we have better error messages when checking for C-strings directly
+                let mut cstr_res = self.visit_cstr(state.get_next(ty), inner_ty); // always unsafe with one depth-one reason.
+
+                // Cstr pointer have metadata, CString is Sized
+                if diag_name == sym::cstr_type {
+                    // we need to override the "type" part of `cstr_res`'s only FfiResultReason
+                    // so it says that it's the use of the indirection that is unsafe
+                    match cstr_res {
+                        FfiResult::FfiUnsafe(ref mut reasons) => {
+                            reasons.first_mut().unwrap().reason.ty = ty;
+                        }
+                        _ => unreachable!(),
+                    }
+                    let note = match indirection_kind {
+                        IndirectionKind::RawPtr => fluent::lint_improper_ctypes_unsized_ptr,
+                        IndirectionKind::Ref => fluent::lint_improper_ctypes_unsized_ref,
+                        IndirectionKind::Box => fluent::lint_improper_ctypes_unsized_box,
+                    };
+                    return cstr_res.wrap_all(ty, note, None);
+                } else {
+                    return cstr_res;
+                }
+            }
+        }
+
         match indirection_kind {
             IndirectionKind::Box => {
                 // FIXME(ctypes): this logic is broken, but it still fits the current tests:
@@ -933,13 +988,8 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
                     AdtKind::Struct | AdtKind::Union => {
                         if let Some(sym::cstring_type | sym::cstr_type) =
                             tcx.get_diagnostic_name(def.did())
-                            && !self.base_ty.is_mutable_ptr()
                         {
-                            return FfiResult::new_with_reason(
-                                ty,
-                                fluent::lint_improper_ctypes_cstr_reason,
-                                Some(fluent::lint_improper_ctypes_cstr_help),
-                            );
+                            return self.visit_cstr(state, ty);
                         }
                         self.visit_struct_or_union(state, ty, def, args)
                     }
@@ -1220,7 +1270,7 @@ impl<'tcx> ImproperCTypesLint {
     /// Check that an extern "ABI" static variable is of a ffi-safe type.
     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);
+        let mut visitor = ImproperCTypesVisitor::new(cx);
         let ffi_res = visitor.check_type(VisitorState::static_var(), ty);
         self.process_ffi_result(cx, span, ffi_res, CItemKind::ImportedExtern);
     }
@@ -1239,15 +1289,15 @@ impl<'tcx> ImproperCTypesLint {
         for (input_ty, input_hir) in iter::zip(sig.inputs(), decl.inputs) {
             let mut state = VisitorState::entry_point_from_fnmode(fn_mode, FnPos::Arg);
             state.depth = depth;
-            let mut visitor = ImproperCTypesVisitor::new(cx, *input_ty);
+            let mut visitor = ImproperCTypesVisitor::new(cx);
             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 mut state = VisitorState::entry_point_from_fnmode(fn_mode, FnPos::Ret);
             state.depth = depth;
-            let mut visitor = ImproperCTypesVisitor::new(cx, sig.output());
+            let mut visitor = ImproperCTypesVisitor::new(cx);
             let ffi_res = visitor.check_type(state, sig.output());
             self.process_ffi_result(cx, ret_hir.span, ffi_res, fn_mode);
         }
diff --git a/tests/ui/extern/extern-C-non-FFI-safe-arg-ice-52334.stderr b/tests/ui/extern/extern-C-non-FFI-safe-arg-ice-52334.stderr
@@ -4,7 +4,7 @@ warning: `extern` callback uses type `CStr`, which is not FFI-safe
 LL | type Foo = extern "C" fn(::std::ffi::CStr);
    |                          ^^^^^^^^^^^^^^^^ not FFI-safe
    |
-   = help: consider passing a `*const std::ffi::c_char` instead, and use `CStr::as_ptr()`
+   = help: consider passing a `*mut std::ffi::c_char` instead, converting to/from `CStr` as needed
    = note: `CStr`/`CString` do not have a guaranteed layout
    = note: `#[warn(improper_ctypes)]` (part of `#[warn(improper_c_boundaries)]`) on by default
 
diff --git a/tests/ui/lint/improper-ctypes/lint-cstr.rs b/tests/ui/lint/improper-ctypes/lint-cstr.rs
@@ -6,31 +6,35 @@ use std::ffi::{CStr, CString};
 extern "C" {
     fn take_cstr(s: CStr);
     //~^ ERROR `extern` block uses type `CStr`, which is not FFI-safe
-    //~| HELP consider passing a `*const std::ffi::c_char` instead, and use `CStr::as_ptr()`
+    //~| HELP consider passing a `*mut std::ffi::c_char` instead, converting to/from `CStr` as needed
     fn take_cstr_ref(s: &CStr);
-    //~^ ERROR `extern` block uses type `CStr`, which is not FFI-safe
-    //~| HELP consider passing a `*const std::ffi::c_char` instead, and use `CStr::as_ptr()`
+    //~^ ERROR `extern` block uses type `&CStr`, which is not FFI-safe
+    //~| HELP consider passing a `*const std::ffi::c_char` instead, converting to/from `&CStr` as needed
     fn take_cstring(s: CString);
     //~^ ERROR `extern` block uses type `CString`, which is not FFI-safe
-    //~| HELP consider passing a `*const std::ffi::c_char` instead, and use `CStr::as_ptr()`
+    //~| HELP consider passing a `*mut std::ffi::c_char` instead, converting to/from `CString` as needed
     fn take_cstring_ref(s: &CString);
     //~^ ERROR `extern` block uses type `CString`, which is not FFI-safe
-    //~| HELP consider passing a `*const std::ffi::c_char` instead, and use `CStr::as_ptr()`
+    //~| HELP consider passing a `*const std::ffi::c_char` instead, converting to/from `CString` as needed
 
-    fn no_special_help_for_mut_cstring(s: *mut CString);
+    fn take_cstring_ptr_mut(s: *mut CString);
     //~^ ERROR `extern` block uses type `CString`, which is not FFI-safe
-    //~| HELP consider adding a `#[repr(C)]` or `#[repr(transparent)]` attribute to this struct
+    //~| HELP consider passing a `*mut std::ffi::c_char` instead, converting to/from `CString` as needed
 
-    fn no_special_help_for_mut_cstring_ref(s: &mut CString);
+    fn take_cstring_ref_mut(s: &mut CString);
     //~^ ERROR `extern` block uses type `CString`, which is not FFI-safe
-    //~| HELP consider adding a `#[repr(C)]` or `#[repr(transparent)]` attribute to this struct
+    //~| HELP consider passing a `*mut std::ffi::c_char` instead, converting to/from `CString` as needed
 }
 
 extern "C" fn rust_take_cstr_ref(s: &CStr) {}
-//~^ ERROR `extern` fn uses type `CStr`, which is not FFI-safe
-//~| HELP consider passing a `*const std::ffi::c_char` instead, and use `CStr::as_ptr()`
+//~^ ERROR `extern` fn uses type `&CStr`, which is not FFI-safe
+//~| HELP consider passing a `*const std::ffi::c_char` instead, converting to/from `&CStr` as needed
 extern "C" fn rust_take_cstring(s: CString) {}
 //~^ ERROR `extern` fn uses type `CString`, which is not FFI-safe
-//~| HELP consider passing a `*const std::ffi::c_char` instead, and use `CStr::as_ptr()`
-extern "C" fn rust_no_special_help_for_mut_cstring(s: *mut CString) {}
-extern "C" fn rust_no_special_help_for_mut_cstring_ref(s: &mut CString) {}
+//~| HELP consider passing a `*mut std::ffi::c_char` instead, converting to/from `CString` as needed
+extern "C" fn rust_take_cstring_ptr_mut(s: *mut CString) {}
+//~^ ERROR `extern` fn uses type `CString`, which is not FFI-safe
+//~| HELP consider passing a `*mut std::ffi::c_char` instead, converting to/from `CString` as needed
+extern "C" fn rust_take_cstring_ref_mut(s: &mut CString) {}
+//~^ ERROR `extern` fn uses type `CString`, which is not FFI-safe
+//~| HELP consider passing a `*mut std::ffi::c_char` instead, converting to/from `CString` as needed
diff --git a/tests/ui/lint/improper-ctypes/lint-cstr.stderr b/tests/ui/lint/improper-ctypes/lint-cstr.stderr
@@ -4,21 +4,22 @@ error: `extern` block uses type `CStr`, which is not FFI-safe
 LL |     fn take_cstr(s: CStr);
    |                     ^^^^ not FFI-safe
    |
-   = help: consider passing a `*const std::ffi::c_char` instead, and use `CStr::as_ptr()`
+   = help: consider passing a `*mut std::ffi::c_char` instead, converting to/from `CStr` as needed
    = note: `CStr`/`CString` do not have a guaranteed layout
 note: the lint level is defined here
   --> $DIR/lint-cstr.rs:2:9
    |
 LL | #![deny(improper_ctypes, improper_ctypes_definitions)]
    |         ^^^^^^^^^^^^^^^
 
-error: `extern` block uses type `CStr`, which is not FFI-safe
+error: `extern` block uses type `&CStr`, which is not FFI-safe
   --> $DIR/lint-cstr.rs:10:25
    |
 LL |     fn take_cstr_ref(s: &CStr);
    |                         ^^^^^ not FFI-safe
    |
-   = help: consider passing a `*const std::ffi::c_char` instead, and use `CStr::as_ptr()`
+   = note: this reference to an unsized type contains metadata, which makes it incompatible with a C pointer
+   = help: consider passing a `*const std::ffi::c_char` instead, converting to/from `&CStr` as needed
    = note: `CStr`/`CString` do not have a guaranteed layout
 
 error: `extern` block uses type `CString`, which is not FFI-safe
@@ -27,7 +28,7 @@ error: `extern` block uses type `CString`, which is not FFI-safe
 LL |     fn take_cstring(s: CString);
    |                        ^^^^^^^ not FFI-safe
    |
-   = help: consider passing a `*const std::ffi::c_char` instead, and use `CStr::as_ptr()`
+   = help: consider passing a `*mut std::ffi::c_char` instead, converting to/from `CString` as needed
    = note: `CStr`/`CString` do not have a guaranteed layout
 
 error: `extern` block uses type `CString`, which is not FFI-safe
@@ -36,34 +37,35 @@ error: `extern` block uses type `CString`, which is not FFI-safe
 LL |     fn take_cstring_ref(s: &CString);
    |                            ^^^^^^^^ not FFI-safe
    |
-   = help: consider passing a `*const std::ffi::c_char` instead, and use `CStr::as_ptr()`
+   = help: consider passing a `*const std::ffi::c_char` instead, converting to/from `CString` as needed
    = note: `CStr`/`CString` do not have a guaranteed layout
 
 error: `extern` block uses type `CString`, which is not FFI-safe
-  --> $DIR/lint-cstr.rs:20:43
+  --> $DIR/lint-cstr.rs:20:32
    |
-LL |     fn no_special_help_for_mut_cstring(s: *mut CString);
-   |                                           ^^^^^^^^^^^^ not FFI-safe
+LL |     fn take_cstring_ptr_mut(s: *mut CString);
+   |                                ^^^^^^^^^^^^ not FFI-safe
    |
-   = help: consider adding a `#[repr(C)]` or `#[repr(transparent)]` attribute to this struct
-   = note: this struct has unspecified layout
+   = help: consider passing a `*mut std::ffi::c_char` instead, converting to/from `CString` as needed
+   = note: `CStr`/`CString` do not have a guaranteed layout
 
 error: `extern` block uses type `CString`, which is not FFI-safe
-  --> $DIR/lint-cstr.rs:24:47
+  --> $DIR/lint-cstr.rs:24:32
    |
-LL |     fn no_special_help_for_mut_cstring_ref(s: &mut CString);
-   |                                               ^^^^^^^^^^^^ not FFI-safe
+LL |     fn take_cstring_ref_mut(s: &mut CString);
+   |                                ^^^^^^^^^^^^ not FFI-safe
    |
-   = help: consider adding a `#[repr(C)]` or `#[repr(transparent)]` attribute to this struct
-   = note: this struct has unspecified layout
+   = help: consider passing a `*mut std::ffi::c_char` instead, converting to/from `CString` as needed
+   = note: `CStr`/`CString` do not have a guaranteed layout
 
-error: `extern` fn uses type `CStr`, which is not FFI-safe
+error: `extern` fn uses type `&CStr`, which is not FFI-safe
   --> $DIR/lint-cstr.rs:29:37
    |
 LL | extern "C" fn rust_take_cstr_ref(s: &CStr) {}
    |                                     ^^^^^ not FFI-safe
    |
-   = help: consider passing a `*const std::ffi::c_char` instead, and use `CStr::as_ptr()`
+   = note: this reference to an unsized type contains metadata, which makes it incompatible with a C pointer
+   = help: consider passing a `*const std::ffi::c_char` instead, converting to/from `&CStr` as needed
    = note: `CStr`/`CString` do not have a guaranteed layout
 note: the lint level is defined here
   --> $DIR/lint-cstr.rs:2:26
@@ -77,8 +79,26 @@ error: `extern` fn uses type `CString`, which is not FFI-safe
 LL | extern "C" fn rust_take_cstring(s: CString) {}
    |                                    ^^^^^^^ not FFI-safe
    |
-   = help: consider passing a `*const std::ffi::c_char` instead, and use `CStr::as_ptr()`
+   = help: consider passing a `*mut std::ffi::c_char` instead, converting to/from `CString` as needed
+   = note: `CStr`/`CString` do not have a guaranteed layout
+
+error: `extern` fn uses type `CString`, which is not FFI-safe
+  --> $DIR/lint-cstr.rs:35:44
+   |
+LL | extern "C" fn rust_take_cstring_ptr_mut(s: *mut CString) {}
+   |                                            ^^^^^^^^^^^^ not FFI-safe
+   |
+   = help: consider passing a `*mut std::ffi::c_char` instead, converting to/from `CString` as needed
+   = note: `CStr`/`CString` do not have a guaranteed layout
+
+error: `extern` fn uses type `CString`, which is not FFI-safe
+  --> $DIR/lint-cstr.rs:38:44
+   |
+LL | extern "C" fn rust_take_cstring_ref_mut(s: &mut CString) {}
+   |                                            ^^^^^^^^^^^^ not FFI-safe
+   |
+   = help: consider passing a `*mut std::ffi::c_char` instead, converting to/from `CString` as needed
    = note: `CStr`/`CString` do not have a guaranteed layout
 
-error: aborting due to 8 previous errors
+error: aborting due to 10 previous errors
 

Original file line number	Diff line number	Diff line change
@@ -4,7 +4,7 @@ warning: `extern` callback uses type `CStr`, which is not FFI-safe
`4`	`4`	`LL \| type Foo = extern "C" fn(::std::ffi::CStr);`
`5`	`5`	`\| ^^^^^^^^^^^^^^^^ not FFI-safe`
`6`	`6`	`\|`
`7`		- = help: consider passing a `*const std::ffi::c_char` instead, and use `CStr::as_ptr()`
	`7`	+ = help: consider passing a `*mut std::ffi::c_char` instead, converting to/from `CStr` as needed
`8`	`8`	= note: `CStr`/`CString` do not have a guaranteed layout
`9`	`9`	= note: `#[warn(improper_ctypes)]` (part of `#[warn(improper_c_boundaries)]`) on by default
`10`	`10`