Rollup merge of #128735 - jieyouxu:pr-120176-revive, r=cjgillot

Add a special case for `CStr`/`CString` in the `improper_ctypes` lint

Revives #120176. Just needed to bless a test and fix an argument, but seemed reasonable to me otherwise.

Instead of saying to "consider adding a `#[repr(C)]` or `#[repr(transparent)]` attribute to this struct", we now tell users to "Use `*const ffi::c_char` instead, and pass the value from `CStr::as_ptr()`" when the type involved is a `CStr` or a `CString`.

The suggestion is not made for `&mut CString` or `*mut CString`.

r? ``````@cjgillot`````` (since you were the reviewer of the original PR #120176, but feel free to reroll)

Author: tgross35

Diff for: compiler/rustc_lint/messages.ftl

@@ -361,6 +361,11 @@ lint_improper_ctypes_box = box cannot be represented as a single pointer
 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_reason = `CStr`/`CString` do not have a guaranteed layout
+
 lint_improper_ctypes_dyn = trait objects have no C equivalent
 
 lint_improper_ctypes_enum_repr_help =

Diff for: compiler/rustc_lint/src/types.rs

@@ -985,6 +985,14 @@ struct ImproperCTypesVisitor<'a, 'tcx> {
     mode: CItemKind,
 }
 
+/// Accumulator for recursive ffi type checking
+struct CTypesVisitorState<'tcx> {
+    cache: FxHashSet<Ty<'tcx>>,
+    /// The original type being checked, before we recursed
+    /// to any other types it contains.
+    base_ty: Ty<'tcx>,
+}
+
 enum FfiResult<'tcx> {
     FfiSafe,
     FfiPhantom(Ty<'tcx>),
@@ -1213,7 +1221,7 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
     /// Checks if the given field's type is "ffi-safe".
     fn check_field_type_for_ffi(
         &self,
-        cache: &mut FxHashSet<Ty<'tcx>>,
+        acc: &mut CTypesVisitorState<'tcx>,
         field: &ty::FieldDef,
         args: GenericArgsRef<'tcx>,
     ) -> FfiResult<'tcx> {
@@ -1223,13 +1231,13 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
             .tcx
             .try_normalize_erasing_regions(self.cx.param_env, field_ty)
             .unwrap_or(field_ty);
-        self.check_type_for_ffi(cache, field_ty)
+        self.check_type_for_ffi(acc, field_ty)
     }
 
     /// Checks if the given `VariantDef`'s field types are "ffi-safe".
     fn check_variant_for_ffi(
         &self,
-        cache: &mut FxHashSet<Ty<'tcx>>,
+        acc: &mut CTypesVisitorState<'tcx>,
         ty: Ty<'tcx>,
         def: ty::AdtDef<'tcx>,
         variant: &ty::VariantDef,
@@ -1239,7 +1247,7 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
         let transparent_with_all_zst_fields = if def.repr().transparent() {
             if let Some(field) = 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(cache, field, args) {
+                match self.check_field_type_for_ffi(acc, field, args) {
                     FfiUnsafe { ty, .. } if ty.is_unit() => (),
                     r => return r,
                 }
@@ -1257,7 +1265,7 @@ 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(cache, field, args) {
+            all_phantom &= match self.check_field_type_for_ffi(acc, field, args) {
                 FfiSafe => false,
                 // `()` fields are FFI-safe!
                 FfiUnsafe { ty, .. } if ty.is_unit() => false,
@@ -1277,7 +1285,11 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
 
     /// Checks if the given type is "ffi-safe" (has a stable, well-defined
     /// representation which can be exported to C code).
-    fn check_type_for_ffi(&self, cache: &mut FxHashSet<Ty<'tcx>>, ty: Ty<'tcx>) -> FfiResult<'tcx> {
+    fn check_type_for_ffi(
+        &self,
+        acc: &mut CTypesVisitorState<'tcx>,
+        ty: Ty<'tcx>,
+    ) -> FfiResult<'tcx> {
         use FfiResult::*;
 
         let tcx = self.cx.tcx;
@@ -1286,7 +1298,7 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
         // `struct S(*mut S);`.
         // FIXME: A recursion limit is necessary as well, for irregular
         // recursive types.
-        if !cache.insert(ty) {
+        if !acc.cache.insert(ty) {
             return FfiSafe;
         }
 
@@ -1308,6 +1320,17 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
                 }
                 match def.adt_kind() {
                     AdtKind::Struct | AdtKind::Union => {
+                        if let Some(sym::cstring_type | sym::cstr_type) =
+                            tcx.get_diagnostic_name(def.did())
+                            && !acc.base_ty.is_mutable_ptr()
+                        {
+                            return FfiUnsafe {
+                                ty,
+                                reason: fluent::lint_improper_ctypes_cstr_reason,
+                                help: Some(fluent::lint_improper_ctypes_cstr_help),
+                            };
+                        }
+
                         if !def.repr().c() && !def.repr().transparent() {
                             return FfiUnsafe {
                                 ty,
@@ -1354,7 +1377,7 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
                             };
                         }
 
-                        self.check_variant_for_ffi(cache, ty, def, def.non_enum_variant(), args)
+                        self.check_variant_for_ffi(acc, ty, def, def.non_enum_variant(), args)
                     }
                     AdtKind::Enum => {
                         if def.variants().is_empty() {
@@ -1378,7 +1401,7 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
                             if let Some(ty) =
                                 repr_nullable_ptr(self.cx.tcx, self.cx.param_env, ty, self.mode)
                             {
-                                return self.check_type_for_ffi(cache, ty);
+                                return self.check_type_for_ffi(acc, ty);
                             }
 
                             return FfiUnsafe {
@@ -1399,7 +1422,7 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
                                 };
                             }
 
-                            match self.check_variant_for_ffi(cache, ty, def, variant, args) {
+                            match self.check_variant_for_ffi(acc, ty, def, variant, args) {
                                 FfiSafe => (),
                                 r => return r,
                             }
@@ -1469,9 +1492,9 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
                 FfiSafe
             }
 
-            ty::RawPtr(ty, _) | ty::Ref(_, ty, _) => self.check_type_for_ffi(cache, ty),
+            ty::RawPtr(ty, _) | ty::Ref(_, ty, _) => self.check_type_for_ffi(acc, ty),
 
-            ty::Array(inner_ty, _) => self.check_type_for_ffi(cache, inner_ty),
+            ty::Array(inner_ty, _) => self.check_type_for_ffi(acc, inner_ty),
 
             ty::FnPtr(sig_tys, hdr) => {
                 let sig = sig_tys.with(hdr);
@@ -1485,7 +1508,7 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
 
                 let sig = tcx.instantiate_bound_regions_with_erased(sig);
                 for arg in sig.inputs() {
-                    match self.check_type_for_ffi(cache, *arg) {
+                    match self.check_type_for_ffi(acc, *arg) {
                         FfiSafe => {}
                         r => return r,
                     }
@@ -1496,7 +1519,7 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
                     return FfiSafe;
                 }
 
-                self.check_type_for_ffi(cache, ret_ty)
+                self.check_type_for_ffi(acc, ret_ty)
             }
 
             ty::Foreign(..) => FfiSafe,
@@ -1619,7 +1642,8 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
             return;
         }
 
-        match self.check_type_for_ffi(&mut FxHashSet::default(), ty) {
+        let mut acc = CTypesVisitorState { cache: FxHashSet::default(), base_ty: ty };
+        match self.check_type_for_ffi(&mut acc, ty) {
             FfiResult::FfiSafe => {}
             FfiResult::FfiPhantom(ty) => {
                 self.emit_ffi_unsafe_type_lint(

Diff for: compiler/rustc_span/src/symbol.rs

@@ -673,6 +673,7 @@ symbols! {
         crate_visibility_modifier,
         crt_dash_static: "crt-static",
         csky_target_feature,
+        cstr_type,
         cstring_type,
         ctlz,
         ctlz_nonzero,

Diff for: library/core/src/ffi/c_str.rs

@@ -91,6 +91,7 @@ use crate::{fmt, intrinsics, ops, slice, str};
 /// [str]: prim@str "str"
 #[derive(PartialEq, Eq, Hash)]
 #[stable(feature = "core_c_str", since = "1.64.0")]
+#[rustc_diagnostic_item = "cstr_type"]
 #[rustc_has_incoherent_inherent_impls]
 #[lang = "CStr"]
 // `fn from` in `impl From<&CStr> for Box<CStr>` current implementation relies

Diff for: tests/ui/extern/extern-C-non-FFI-safe-arg-ice-52334.stderr

@@ -1,21 +1,21 @@
-warning: `extern` fn uses type `[i8 or u8 (arch dependant)]`, which is not FFI-safe
+warning: `extern` fn uses type `CStr`, which is not FFI-safe
   --> $DIR/extern-C-non-FFI-safe-arg-ice-52334.rs:7:12
    |
 LL | type Foo = extern "C" fn(::std::ffi::CStr);
    |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ not FFI-safe
    |
-   = help: consider using a raw pointer instead
-   = note: slices have no C equivalent
+   = help: consider passing a `*const std::ffi::c_char` instead, and use `CStr::as_ptr()`
+   = note: `CStr`/`CString` do not have a guaranteed layout
    = note: `#[warn(improper_ctypes_definitions)]` on by default
 
-warning: `extern` block uses type `[i8 or u8 (arch dependant)]`, which is not FFI-safe
+warning: `extern` block uses type `CStr`, which is not FFI-safe
   --> $DIR/extern-C-non-FFI-safe-arg-ice-52334.rs:10:18
    |
 LL |     fn meh(blah: Foo);
    |                  ^^^ not FFI-safe
    |
-   = help: consider using a raw pointer instead
-   = note: slices have no C equivalent
+   = help: consider passing a `*const std::ffi::c_char` instead, and use `CStr::as_ptr()`
+   = note: `CStr`/`CString` do not have a guaranteed layout
    = note: `#[warn(improper_ctypes)]` on by default
 
 warning: 2 warnings emitted

Diff for: tests/ui/lint/lint-ctypes-cstr.rs

@@ -0,0 +1,36 @@
+#![crate_type = "lib"]
+#![deny(improper_ctypes, improper_ctypes_definitions)]
+
+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()`
+    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()`
+    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()`
+    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()`
+
+    fn no_special_help_for_mut_cstring(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
+
+    fn no_special_help_for_mut_cstring_ref(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
+}
+
+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()`
+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) {}

Diff for: tests/ui/lint/lint-ctypes-cstr.stderr

@@ -0,0 +1,84 @@
+error: `extern` block uses type `CStr`, which is not FFI-safe
+  --> $DIR/lint-ctypes-cstr.rs:7:21
+   |
+LL |     fn take_cstr(s: CStr);
+   |                     ^^^^ not FFI-safe
+   |
+   = help: consider passing a `*const std::ffi::c_char` instead, and use `CStr::as_ptr()`
+   = note: `CStr`/`CString` do not have a guaranteed layout
+note: the lint level is defined here
+  --> $DIR/lint-ctypes-cstr.rs:2:9
+   |
+LL | #![deny(improper_ctypes, improper_ctypes_definitions)]
+   |         ^^^^^^^^^^^^^^^
+
+error: `extern` block uses type `CStr`, which is not FFI-safe
+  --> $DIR/lint-ctypes-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: `CStr`/`CString` do not have a guaranteed layout
+
+error: `extern` block uses type `CString`, which is not FFI-safe
+  --> $DIR/lint-ctypes-cstr.rs:13:24
+   |
+LL |     fn take_cstring(s: CString);
+   |                        ^^^^^^^ not FFI-safe
+   |
+   = help: consider passing a `*const std::ffi::c_char` instead, and use `CStr::as_ptr()`
+   = note: `CStr`/`CString` do not have a guaranteed layout
+
+error: `extern` block uses type `CString`, which is not FFI-safe
+  --> $DIR/lint-ctypes-cstr.rs:16:28
+   |
+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()`
+   = note: `CStr`/`CString` do not have a guaranteed layout
+
+error: `extern` block uses type `CString`, which is not FFI-safe
+  --> $DIR/lint-ctypes-cstr.rs:20:43
+   |
+LL |     fn no_special_help_for_mut_cstring(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
+
+error: `extern` block uses type `CString`, which is not FFI-safe
+  --> $DIR/lint-ctypes-cstr.rs:24:47
+   |
+LL |     fn no_special_help_for_mut_cstring_ref(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
+
+error: `extern` fn uses type `CStr`, which is not FFI-safe
+  --> $DIR/lint-ctypes-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: `CStr`/`CString` do not have a guaranteed layout
+note: the lint level is defined here
+  --> $DIR/lint-ctypes-cstr.rs:2:26
+   |
+LL | #![deny(improper_ctypes, improper_ctypes_definitions)]
+   |                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: `extern` fn uses type `CString`, which is not FFI-safe
+  --> $DIR/lint-ctypes-cstr.rs:32:36
+   |
+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()`
+   = note: `CStr`/`CString` do not have a guaranteed layout
+
+error: aborting due to 8 previous errors
+