nightly/beta regression: fnptrs with types containing () is warned to be not FFI-safe, while it is before

[oxalica]

Code

#![deny(improper_ctypes_definitions)]

#[repr(C)]
pub struct Wrap<T>(u8, T);

pub extern "C" fn f() -> Wrap<()> { todo!() } // This does not warn.

const _: extern "C" fn() -> Wrap<()> = f; // This warns.

Current output

error: `extern` fn uses type `()`, which is not FFI-safe
 --> src/lib.rs:8:10
  |
8 | const _: extern "C" fn() -> Wrap<()> = f; // This warns.
  |          ^^^^^^^^^^^^^^^^^^^^^^^^^^^ not FFI-safe
  |
  = help: consider using a struct instead
  = note: tuples have unspecified layout
note: the lint level is defined here
 --> src/lib.rs:1:9
  |
1 | #![deny(improper_ctypes_definitions)]
  |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^

Desired output

<no errors or warnings>

Rationale and extra context

The function itself extern "C" fn f() -> Wrap<()> is considered FFI-safe and does not trigger the warning (#72890). Its function pointer should also be allowed, to be consistent.

Other cases

Discovered on CI https://github.com/oxalica/async-ffi/actions/runs/5450150255/jobs/9915116819#step:4:37

The original code has several more levels of indirection, but is also considered FFI-safe before.

Extracted original code

#![deny(improper_ctypes_definitions)]
use std::marker::PhantomData;

pub type FfiFuture<T> = BorrowingFfiFuture<'static, T>;

#[repr(transparent)]
pub struct BorrowingFfiFuture<'a, T>(LocalBorrowingFfiFuture<'a, T>);

// Does not matter here, but is repr(C).
#[repr(C)]
struct FfiContext(u8);

#[repr(C)]
pub struct LocalBorrowingFfiFuture<'a, T> {
    fut_ptr: *mut (),
    poll_fn: unsafe extern "C" fn(fut_ptr: *mut (), context_ptr: *mut FfiContext) -> FfiPoll<T>,
    drop_fn: unsafe extern "C" fn(*mut ()),
    _marker: PhantomData<&'a ()>,
}

#[repr(C, u8)]
pub enum FfiPoll<T> {
    Ready(T),
    Pending,
    Panicked,
}

pub extern "C" fn f() -> FfiFuture<()> { todo!() }
const _: extern "C" fn() -> FfiFuture<()> = f;

Anything else?

According to CI logs, the warning occurs since rust 1.72.0-nightly (0ab38e9 2023-07-03). Probably caused by #108611. cc @davidtwco

[davidtwco]

Okay, so, I had a fix for this that just got rid of the regression, but I'm not sure that's the correct solution here - I think this is correct to be linting in the fn-ptr case and should be linting for the function case.

() is FFI-unsafe, but we allow a function to return () because that's the default, but that doesn't mean that a () within a type is okay (with the exception of a repr(transparent) wrapper around the () - because that's just returning (), which we've already decided is okay).

In the function case that works right now, that's being allowed because of this code..

rust/compiler/rustc_lint/src/types.rs

Lines 1373 to 1375 in fd68a6d

	// If `ty` is a `repr(transparent)` newtype, and the non-zero-sized type is a generic
	// argument, which after substitution, is `()`, then this branch can be hit.
	FfiResult::FfiUnsafe { ty, .. } if is_return_type && ty.is_unit() => {}

..which is there to support the repr(transparent) case. However, it's not quite the right check. It doesn't actually check for repr(transparent)! It checks that the type - however nested - which was deemed FFI-unsafe is () and that type is part of a return type. So any FFI-unsafe () in a return type makes the entire return type FFI-safe - which is clearly incorrect. That's making the example below compile, which absolutely shouldn't.

#![deny(improper_ctypes_definitions)]

struct Foo; // <-- FFI-unsafe!

#[repr(C)]
struct Bar {
    // This field's `()` would be the first FFI-unsafety error, but will be silenced 
    // because `Bar` is used as a return type, and then the rest of the errors
    // will be missed. Comment out this field and it will error.
    x: (),
    bad: Foo, // <-- should error!
}

extern "C" fn foo() -> Bar { todo!(); }

I've submitted #113457, which makes this even more of a regression by making the function case fail too. We already had a simple "is it a unit" check, so I extended that to check for transparent newtype wrappers too, and removed the incorrect check.

[oxalica]

() is FFI-unsafe, but we allow a function to return () because that's the default, but that doesn't mean that a () within a type is okay (with the exception of a repr(transparent) wrapper around the () - because that's just returning (), which we've already decided is okay).

...That's making the example below compile, which absolutely shouldn't.

#![deny(improper_ctypes_definitions)]

struct Foo; // <-- FFI-unsafe!

#[repr(C)]
struct Bar {
    // This field's `()` would be the first FFI-unsafety error, but will be silenced 
    // because `Bar` is used as a return type, and then the rest of the errors
    // will be missed. Comment out this field and it will error.
    x: (),
    bad: Foo, // <-- should error!
}

extern "C" fn foo() -> Bar { todo!(); }

Oh, so () in fields should be FFI-unsafe. I didn't know that before. I found #106675 which allows PhantomData in structs by ignoring them and I thought the same happens for all ZST including (). But actually PhantomData is the only special case to be allowed?

This is unfortunate since I'm trying to return a #[repr(u8)] enum FfiPoll<T> { Ready(T), Pending, Panicked } where T is likely to be (). What should I do instead if I want FfiPoll<()> to be FFI-safe with layout #[repr(u8)] enum { Ready /*no data*/, Pending, Panicked } ?

[davidtwco]

Oh, so () in fields should be FFI-unsafe. I didn't know that before. I found #106675 which allows PhantomData in structs by ignoring them and I thought the same happens for all ZST including (). But actually PhantomData is the only special case to be allowed?

I'm not so sure anymore after reading @lukas-code's comment (#113457 (comment)).

It's clear that some changes are required regardless - our current implementation wrong as demonstrated in the final example from my previous comment, but it's also inconsistent, in that this example...

#![deny(improper_ctypes_definitions)]

#[repr(C)]
pub struct Foo {
    a: u8,
    b: (),
}

extern "C" fn foo(x: Foo) -> Foo { todo!() }

...will lint for the argument use of Foo, but not the return use of Foo, when really it should be both or neither. After my pull request, it's both. With and without my pull request, when you change this to PhantomData<()>, it doesn't lint.

[davidtwco]

I'll nominate this issue so that it sees some discussion - see also Zulip thread and #113457.

[oxalica]

I posted some more info on zulip but it may also worth mentioning here.

Nomicon said ZST is still zero-sized under repr(C), rather than FFI-unsafe.

ZSTs are still zero-sized, even though this is not a standard behavior in C, and is explicitly contrary to the behavior of an empty type in C++, which says they should still consume a byte of space.

[oxalica]

The issue now also exists in beta.

I also found uitests which seems to expect ZST to be FFI-safe, but ends in #[allow(improper_ctypes)] to suppress the warning and tests nothing.

rust/tests/ui/lint/clashing-extern-fn.rs

Lines 237 to 243 in ffb9b61

	mod not_transparent {
	struct NotTransparentZst(());
	extern "C" {
	// These shouldn't warn since all return types are zero sized
	fn zst() -> NotTransparentZst;
	fn transparent_zst() -> NotTransparentZst;
	}

[joshtriplett]

We discussed this in today's @rust-lang/lang meeting. We didn't have consensus that this should warn; at least some people thought the code in the top comment should continue to compile without warnings. We did have consensus that changing that (to warn) would need an FCP.

So, we felt that we'd like to see a fix removing the warning applied to beta and nightly, and then separately if someone wants to propose that we change this to warn, we could evaluate that and do an FCP on it. But at the moment the temperature of the team seems to be that we don't have consensus for such an FCP.

[apiraino]

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-medium

[nikomatsakis]

@rustbot labels -I-lang-nominated

We rendered our opinion here