lint: rework some ImproperCTypes messages (especially around indirect…

…ions to !Sized)
rust-lang · bors · Dec 8, 2024 · Nov 5, 2024 · Nov 5, 2024 · Nov 8, 2024
commit 1d521310439ecb0e243a5056768175a51e40a9b6
diff --git a/compiler/rustc_lint/messages.ftl b/compiler/rustc_lint/messages.ftl
@@ -359,7 +359,6 @@ lint_improper_ctypes_128bit = 128-bit integers don't currently have a known stab
 lint_improper_ctypes_array_help = consider passing a pointer to the array
 
 lint_improper_ctypes_array_reason = passing raw arrays by value is not FFI-safe
-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
 
@@ -377,7 +376,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
 
@@ -388,7 +389,11 @@ lint_improper_ctypes_opaque = opaque types have no C equivalent
 lint_improper_ctypes_pat_help = consider using the base type instead
 
 lint_improper_ctypes_pat_reason = pattern types have no C equivalent
-lint_improper_ctypes_slice_help = consider using a raw pointer instead
+
+lint_improper_ctypes_sized_ptr_to_unsafe_type =
+    this reference (`{$ty}`) is can safely be translated into a C pointer, but `{$inner_ty}` itself is not FFI-safe
+
+lint_improper_ctypes_slice_help = consider using a raw pointer to the slice's first element (and a length) instead
 
 lint_improper_ctypes_slice_reason = slices have no C equivalent
 lint_improper_ctypes_str_help = consider using `*const u8` and a length instead
@@ -414,6 +419,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 rust type contains metadata, which makes it incompatible with a C pointer
+lint_improper_ctypes_unsized_ptr = this pointer to an unsized rust type contains metadata, which makes it incompatible with a C pointer
+lint_improper_ctypes_unsized_ref = this reference to an unsized rust type contains metadata, which makes it incompatible with a C pointer
+
 lint_incomplete_include =
     include macro expected single expression in source
 

diff --git a/compiler/rustc_lint/src/types.rs b/compiler/rustc_lint/src/types.rs
@@ -732,8 +732,6 @@ enum FfiResult<'tcx> {
         reason: DiagMessage,
         help: Option<DiagMessage>,
     },
-    // NOTE: this `allow` is only here for one retroactively-added commit
-    #[allow(dead_code)]
     FfiUnsafeWrapper {
         ty: Ty<'tcx>,
         reason: DiagMessage,
@@ -1044,16 +1042,47 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
 
         match *ty.kind() {
             ty::Adt(def, args) => {
-                if let Some(boxed) = ty.boxed_ty()
-                    && matches!(self.mode, CItemKind::Definition)
-                {
-                    if boxed.is_sized(tcx, self.cx.typing_env()) {
-                        return FfiSafe;
+                if let Some(inner_ty) = ty.boxed_ty() {
+                    if inner_ty.is_sized(tcx, self.cx.typing_env())
+                        || matches!(inner_ty.kind(), ty::Foreign(..))
+                    {
+                        // discussion on declaration vs definition:
+                        // see the `ty::RawPtr(inner_ty, _) | ty::Ref(_, inner_ty, _)` arm
+                        // of this `match *ty.kind()` block
+                        if matches!(self.mode, CItemKind::Definition) {
+                            return FfiSafe;
+                        } else {
+                            let inner_res = self.check_type_for_ffi(acc, inner_ty);
+                            return match inner_res {
+                                FfiUnsafe { .. } | FfiUnsafeWrapper { .. } => FfiUnsafeWrapper {
+                                    ty,
+                                    reason: fluent::lint_improper_ctypes_sized_ptr_to_unsafe_type,
+                                    wrapped: Box::new(inner_res),
+                                    help: None,
+                                },
+                                _ => inner_res,
+                            };
+                        }
                     } else {
+                        let help = match inner_ty.kind() {
+                            ty::Str => Some(fluent::lint_improper_ctypes_str_help),
+                            ty::Slice(_) => Some(fluent::lint_improper_ctypes_slice_help),
+                            ty::Adt(def, _)
+                                if matches!(def.adt_kind(), AdtKind::Struct | AdtKind::Union)
+                                    && matches!(
+                                        tcx.get_diagnostic_name(def.did()),
+                                        Some(sym::cstring_type | sym::cstr_type)
+                                    )
+                                    && !acc.base_ty.is_mutable_ptr() =>
+                            {
+                                Some(fluent::lint_improper_ctypes_cstr_help)
+                            }
+                            _ => None,
+                        };
                         return FfiUnsafe {
                             ty,
-                            reason: fluent::lint_improper_ctypes_box,
-                            help: None,
+                            reason: fluent::lint_improper_ctypes_unsized_box,
+                            help,
                         };
                     }
                 }
@@ -1209,15 +1238,6 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
                 help: Some(fluent::lint_improper_ctypes_tuple_help),
             },
 
-            ty::RawPtr(ty, _) | ty::Ref(_, ty, _)
-                if {
-                    matches!(self.mode, CItemKind::Definition)
-                        && ty.is_sized(self.cx.tcx, self.cx.typing_env())
-                } =>
-            {
-                FfiSafe
-            }
-
             ty::RawPtr(ty, _)
                 if match ty.kind() {
                     ty::Tuple(tuple) => tuple.is_empty(),
@@ -1227,7 +1247,66 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
                 FfiSafe
             }
 
-            ty::RawPtr(ty, _) | ty::Ref(_, ty, _) => self.check_type_for_ffi(acc, ty),
+            ty::RawPtr(inner_ty, _) | ty::Ref(_, inner_ty, _) => {
+                if inner_ty.is_sized(tcx, self.cx.typing_env())
+                    || matches!(inner_ty.kind(), ty::Foreign(..))
+                {
+                    // there's a nuance on what this lint should do for function definitions
+                    // (touched upon in https://github.com/rust-lang/rust/issues/66220 and https://github.com/rust-lang/rust/pull/72700)
+                    //
+                    // (`extern "C" fn fn_name(...) {...}`) versus declarations (`extern "C" {fn fn_name(...);}`).
+                    // The big question is: what does "ABI safety" mean? if you have something translated to a C pointer
+                    // (which has a stable layout) but points to FFI-unsafe type, is it safe?
+                    // on one hand, the function's ABI will match that of a similar C-declared function API,
+                    // on the other, dereferencing the pointer in not-rust will be painful.
+                    // In this code, the opinion is split between function declarations and function definitions.
+                    // For declarations, we see this as unsafe, but for definitions, we see this as safe.
+                    // This is mostly because, for extern function declarations, the actual definition of the function is written somewhere else,
+                    // so the fact that a pointer's pointee should be treated as opaque to one side or the other can be explicitely written out.
+                    // For extern function definitions, however, both callee and some callers can be written in rust,
+                    // so developers need to keep as much typing information as possible.
+                    if matches!(self.mode, CItemKind::Definition) {
+                        return FfiSafe;
+                    } else if matches!(ty.kind(), ty::RawPtr(..))
+                        && matches!(inner_ty.kind(), ty::Tuple(tuple) if tuple.is_empty())
+                    {
+                        FfiSafe
+                    } else {
+                        let inner_res = self.check_type_for_ffi(acc, inner_ty);
+                        return match inner_res {
+                            FfiSafe => inner_res,
+                            _ => FfiUnsafeWrapper {
+                                ty,
+                                reason: fluent::lint_improper_ctypes_sized_ptr_to_unsafe_type,
+                                wrapped: Box::new(inner_res),
+                                help: None,
+                            },
+                        };
+                    }
+                } else {
+                    let help = match inner_ty.kind() {
+                        ty::Str => Some(fluent::lint_improper_ctypes_str_help),
+                        ty::Slice(_) => Some(fluent::lint_improper_ctypes_slice_help),
+                        ty::Adt(def, _)
+                            if matches!(def.adt_kind(), AdtKind::Struct | AdtKind::Union)
+                                && matches!(
+                                    tcx.get_diagnostic_name(def.did()),
+                                    Some(sym::cstring_type | sym::cstr_type)
+                                )
+                                && !acc.base_ty.is_mutable_ptr() =>
+                        {
+                            Some(fluent::lint_improper_ctypes_cstr_help)
+                        }
+                        _ => None,
+                    };
+                    let reason = match ty.kind() {
+                        ty::RawPtr(..) => fluent::lint_improper_ctypes_unsized_ptr,
+                        ty::Ref(..) => fluent::lint_improper_ctypes_unsized_ref,
+                        _ => unreachable!(),
+                    };
+                    FfiUnsafe { ty, reason, help }
+                }
+            }
 
             ty::Array(inner_ty, _) => self.check_type_for_ffi(acc, inner_ty),
 
@@ -1245,7 +1324,14 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
                 for arg in sig.inputs() {
                     match self.check_type_for_ffi(acc, *arg) {
                         FfiSafe => {}
-                        r => return r,
+                        r => {
+                            return FfiUnsafeWrapper {
+                                ty,
+                                reason: fluent::lint_improper_ctypes_fnptr_indirect_reason,
+                                help: None,
+                                wrapped: Box::new(r),
+                            };
+                        }
                     }
                 }
 
@@ -1254,7 +1340,15 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
                     return FfiSafe;
                 }
 
-                self.check_type_for_ffi(acc, ret_ty)
+                match self.check_type_for_ffi(acc, ret_ty) {
+                    r @ (FfiSafe | FfiPhantom(_)) => r,
+                    r => FfiUnsafeWrapper {
+                        ty: ty.clone(),
+                        reason: fluent::lint_improper_ctypes_fnptr_indirect_reason,
+                        help: None,
+                        wrapped: Box::new(r),
+                    },
+                }
             }
 
             ty::Foreign(..) => FfiSafe,

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,6 +4,7 @@ warning: `extern` fn uses type `CStr`, which is not FFI-safe
 LL | type Foo = extern "C" fn(::std::ffi::CStr);
    |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ not FFI-safe
    |
+   = note: the function pointer to `extern "C" fn(CStr)` is FFI-unsafe due to `CStr`
    = 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
@@ -14,6 +15,7 @@ warning: `extern` block uses type `CStr`, which is not FFI-safe
 LL |     fn meh(blah: Foo);
    |                  ^^^ not FFI-safe
    |
+   = note: the function pointer to `extern "C" fn(CStr)` is FFI-unsafe due to `CStr`
    = 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

diff --git a/tests/ui/extern/extern-C-str-arg-ice-80125.stderr b/tests/ui/extern/extern-C-str-arg-ice-80125.stderr
@@ -4,6 +4,7 @@ warning: `extern` fn uses type `str`, which is not FFI-safe
 LL | type ExternCallback = extern "C" fn(*const u8, u32, str);
    |                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ not FFI-safe
    |
+   = note: the function pointer to `extern "C" fn(*const u8, u32, str)` is FFI-unsafe due to `str`
    = help: consider using `*const u8` and a length instead
    = note: string slices have no C equivalent
    = note: `#[warn(improper_ctypes_definitions)]` on by default
@@ -14,6 +15,7 @@ warning: `extern` fn uses type `str`, which is not FFI-safe
 LL | pub extern "C" fn register_something(bind: ExternCallback) -> Struct {
    |                                            ^^^^^^^^^^^^^^ not FFI-safe
    |
+   = note: the function pointer to `extern "C" fn(*const u8, u32, str)` is FFI-unsafe due to `str`
    = help: consider using `*const u8` and a length instead
    = note: string slices have no C equivalent
 

diff --git a/tests/ui/lint/extern-C-fnptr-lints-slices.rs b/tests/ui/lint/extern-C-fnptr-lints-slices.rs
@@ -3,7 +3,7 @@
 // It's an improper ctype (a slice) arg in an extern "C" fnptr.
 
 pub type F = extern "C" fn(&[u8]);
-//~^ ERROR: `extern` fn uses type `[u8]`, which is not FFI-safe
+//~^ ERROR: `extern` fn uses type `&[u8]`, which is not FFI-safe
 
 
 fn main() {}
diff --git a/tests/ui/lint/extern-C-fnptr-lints-slices.stderr b/tests/ui/lint/extern-C-fnptr-lints-slices.stderr
@@ -1,11 +1,12 @@
-error: `extern` fn uses type `[u8]`, which is not FFI-safe
+error: `extern` fn uses type `&[u8]`, which is not FFI-safe
   --> $DIR/extern-C-fnptr-lints-slices.rs:5:14
    |
 LL | pub type F = extern "C" fn(&[u8]);
    |              ^^^^^^^^^^^^^^^^^^^^ not FFI-safe
    |
-   = help: consider using a raw pointer instead
-   = note: slices have no C equivalent
+   = note: the function pointer to `for<'a> extern "C" fn(&'a [u8])` is FFI-unsafe due to `&[u8]`
+   = help: consider using a raw pointer to the slice's first element (and a length) instead
+   = note: this reference to an unsized rust type contains metadata, which makes it incompatible with a C pointer
 note: the lint level is defined here
   --> $DIR/extern-C-fnptr-lints-slices.rs:1:8
    |

diff --git a/tests/ui/lint/lint-ctypes-73249-2.stderr b/tests/ui/lint/lint-ctypes-73249-2.stderr
@@ -4,6 +4,7 @@ error: `extern` block uses type `Qux`, which is not FFI-safe
 LL |     fn lint_me() -> A<()>;
    |                     ^^^^^ not FFI-safe
    |
+   = note: this reference (`&Qux`) is can safely be translated into a C pointer, but `Qux` itself is not FFI-safe
    = note: opaque types have no C equivalent
 note: the lint level is defined here
   --> $DIR/lint-ctypes-73249-2.rs:2:9