The improper_ctypes lint is too noisy to be useful

[alexcrichton]

Recent changes appear to generate new warnings for the improper_ctypes lint. Code such as this:

struct Internals {
    // ...
}

#[repr(C)]
pub struct internals_t {
    internals: Internals,
}

pub extern "C" fn internals_new() -> *mut internals_t {
    loop {}
}

now generates the warning:

warning: `extern` fn uses type `Internals`, which is not FFI-safe
  --> src/lib.rs:10:38
   |
10 | pub extern "C" fn internals_new() -> *mut internals_t {
   |                                      ^^^^^^^^^^^^^^^^ not FFI-safe
   |
   = note: `#[warn(improper_ctypes)]` on by default
   = help: consider adding a `#[repr(C)]` or `#[repr(transparent)]` attribute to this struct
   = note: this struct has unspecified layout
note: type defined here
  --> src/lib.rs:1:1
   |
1  | / struct Internals {
2  | |     
3  | | }
   | |_^

This pattern can be quite common if a Rust library is exposing a C interface. Many pointer types moving across the boundary are intentionally opaque to the C side of the interface, so there's no need for #[repr(C)]

The lint, however, now generates thousands of warnings on these crates and has to basically be shut off entirely, removing the usefulness of the lint in the first place.

[pnkfelix]

(Was this injected by PR #65134 ?)

[Centril]

It was, yes, but let's focus discussion in #66220.

[chorman0773]

I'm having a problem with this in a low-level system interface for my OS's syscall api, which has interfaces like this:

// Handle is just `#[repr(transparent)]` over `()`
// HandlePtr<T> is just `#[repr(transparent)] *mut T` that communicates that it lives outside of the user-space usable address space (it lives in the thread-local handle address space)
use super::handle::{Handle,HandlePtr};

// This is just a type alias of c_long
use super::result::SysResult;

pub struct FileHandle(Handle); 

extern "C"{
   pub fn OpenFile(st: *const FileOpenOptions, hdl: *mut HandlePtr<FileHandle>) -> SysResult;
   pub fn CloseFile(hdl: HandlePtr<FileHandle>) -> SysResult;
}

There are also several more handle types and interfaces (the approach of the kernel is an OO handle pattern, much like the NT kernel, though it passes things arround as pointers and the expressed api is strongly typed). This was to provide some semblance of type safety when using the primitive api (which, in some edge cases, may be more useful than the safe wrapper api). In addition, C itself allows void* and struct Handle* to have different ABI - and rust hasn't resolved whether it allows C platforms that have them be different. In this case, the apis just syscall, which do access... something in the kernel's thread-local address space.
The actual *Handle values are never handled in userspace and are opaque to the kernel (if it was a non-zst and you tried to deref it and read/write, you'd be greeted with a suprise SIGSEGV from the kernel if you're lucky... if not the kernel has just handed you an integer value that was turned into a pointer for the interface, so you could be reading arbitrary data from userspace).

[thomcc]

It's particularly unfortunate that this lint:

1. Has a very high false-positive rate.
2. Fires in the crate that uses the type rather than the one that defines it.

This means in cases where the type may be defined in one crate and used in others, there's no reasonable way for me to review the complaint, judge that "yeah actually this type is fine, it's a false positive", and then add an #[allow(improper_ctypes)] to my crate somewhere, and have downstream users of my crate not see the warning when they use that type in FFI. That is -- as it is, the downstream crate would have to to add the #[allow] to their code as well (in this kind of situation).

This is a pretty scary lint for a downstream user to see (even one working with FFI code), which means that in practice crates in this situation have to follow what the lint says, even if it's wrong.

(It would be nice if we could support #[allow(improper_ctypes)] on the type somehow, although it would be pretty inconsistent with how allow works anywhere else, so I imagine it's completely impractical. A better fix would just be to make the lint not have these false positives anyway...)

[danielhenrymantilla]

I've just found a (silly) workaround, to make repr(transparent) not ignore the PhantomData field because ZST (which is what it otherwise does), by making it generic over that one field 😅:

#[repr(transparent)]
#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
pub struct PhantomNonExhaustive<PD = ::core::marker::PhantomData<u8>>(
    pub(crate) PD,
);

This does make PhantomNonExhaustive be a 1-ZST which is FFI-safe, and yet unconstructible within safe Rust.

[Dylan-DPC]

With the revert and the later push, the warning is no longer generated for this code.