rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
98.17k stars 12.7k forks source link

`extern "C"` functions returning ZSTs #115091

Open ChayimFriedman2 opened 1 year ago

ChayimFriedman2 commented 1 year ago

Currently, for extern "C" functions returning ZSTs the compiler returns void (as well as for other ABIs, but this is not related). However, improper_ctypes_definitions fire for such functions (if they don't return ()), e.g.:

// #[repr(C)] // Adding that does not change the outcome.
pub struct Zst;

pub extern "C" fn foo() -> Zst {
    Zst
}
warning: `extern` fn uses type `Zst`, which is not FFI-safe
 --> src/lib.rs:4:28
  |
4 | pub extern "C" fn foo() -> Zst {
  |                            ^^^ not FFI-safe
  |
  = 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
 --> src/lib.rs:2:1
  |
2 | pub struct Zst;
  | ^^^^^^^^^^^^^^
  = note: `#[warn(improper_ctypes_definitions)]` on by default

Playground.

It's pretty clear we do not want to guarantee the FFI-safety of ZST arguments. But what about the FFI safety of ZST return types? Do we want to guarantee they'll keep translating to void, or keep this not guaranteed?

Context: https://stackoverflow.com/q/76950446/7884305.

@rustbot label T-lang

Noratrieb commented 1 year ago

note that questions like these will see more activity if asked on https://internals.rust-lang.org/ on zulip

Noratrieb commented 1 year ago

But something worth pointing out is that something being a ZST is not necessarily part of its public API, so using ZSTs with private fields from other crates in return position is not okay because they could be changed to not be ZSTs in the future.

ShE3py commented 1 year ago

Some types may be optimized to ZSTs ;

pub enum Errno {
    Generic
}

pub extern "C" fn mainloop() -> Result<!, Errno> {
    Err(Errno::Generic)
}

so the generated glue will be

void mainloop();

which is kinda not good.


I think for FFI, users should use a type alias:

type IfThisReturnsItHasErrored = ();

As the type's meaning shouldn't be dropped, and the generated glue will be better:

typedef void IfThisReturnsItHasErrored;

IfThisReturnsItHasErrored mainloop();
ChayimFriedman2 commented 1 year ago

@ShE3py But sometimes as in the linked Stack Overflow question, people want a proper type with privacy and maybe generics. And I agree we should not allow all types that are ZSTs, especially types we do not guarantee are ZSTs (like the enum), but for types that we do guarantee they will be ZSTs I think it makes sense to guarantee that returning them is equal to returning void.

ShE3py commented 1 year ago

This seems to works:

use std::marker::PhantomData;

#[repr(C)]
pub struct Invariant<'a> {
    _priv: (),
    _phantom: PhantomData<fn(&'a ()) -> &'a ()>
}

#[forbid(improper_ctypes_definitions)]
pub extern "C" fn foo<'a>() -> Invariant<'a> {
    Invariant { _priv: (), _phantom: PhantomData }
}
example::foo:
        ret

I think it denies #[repr(transparent)] as it's not FFI-related, but using #[repr(C)] is asking the struct to be FFI-safe.

Noratrieb commented 1 year ago

#[repr(transparent)] is FFI-safe if the inner type is FFI-safe.

markcsaving commented 1 year ago

@ShE3py The issue with using a #[repr(C)] struct is that the extern "C" ABI might do special treatment for structs, so the corresponding return type on the C side might not be void. There don't seem to be any guarantees about this.

markcsaving commented 1 year ago

@Nilstrieb I don't think your point applies here. If we have

#[repr(transparent)]
struct TwoZSTFields((), PhantomData<()>);

which field is "the inner type"?

The documentation seems to suggest that using #[repr(transparent)] in this case isn't allowed at all, even though the compiler currently lets us do it. Checking the documentation for #[repr(transparent)], it states explicitly in the Rustonomicon that

[repr(transparent)] can only be used on a struct or single-variant enum that has a single non-zero-sized field (there may be additional zero-sized fields).

In the Rust Reference, the wording is similar; there must be a single non-zero-sized field. There is an open pull request on this topic here which seems to be in limbo.

kamirr commented 9 months ago

Some types may be optimized to ZSTs ;

pub enum Errno {
    Generic
}

pub extern "C" fn mainloop() -> Result<!, Errno> {
    Err(Errno::Generic)
}

so the generated glue will be

void mainloop();

which is kinda not good.

@ShE3py This code is invalid to begin with. Errno is not repr(C), and it has to be in order to be a valid return type of an extern "C" function. If you mark it as repr(C) or repr(u*) then the layout becomes guaranteed and the concern of optimization to ZST is void (pardon the pun).

But something worth pointing out is that something being a ZST is not necessarily part of its public API, so using ZSTs with private fields from other crates in return position is not okay because they could be changed to not be ZSTs in the future.

@Nilstrieb API and ABI stability are separate concerns. Yes, a type can stop being a ZST and it's ABI would change from that of void to that of another type. If you have a struct S { a: u32 } and change it to S { a: u32, b: u32 } then you break the ABI as well whilst maintaining the public API (note that the fields are private), starting with a ZST is not relevant.