warn less about non-exhaustive in ffi

[workingjubilee]

Bindgen allows generating #[non_exhaustive] #[repr(u32)] enums. This results in nonintuitive nonlocal improper_ctypes warnings, even when the types are otherwise perfectly valid in C.

Adjust for actual tooling expectations by avoiding warning on simple enums with only unit variants.

Closes#116831

[rustbot]

r? @petrochenkov

(rustbot has picked a reviewer for you, use r? to override)

[Mark-Simulacrum]

The docs don't make explicit that the ABI matches C's - https://doc.rust-lang.org/reference/type-layout.html#primitive-representation-of-field-less-enums - my sense is that's probably just an oversight?

But as-is it's not clear to me that this is accurate.

[workingjubilee]

typedefenumName: uint8_t {
/* fields */
} Name;

is legal C code as-of C23 and must, by brute force of logic, match

#[repr(u8)]enumName{/* fields */}

or fail to compile, according to my understanding. (There simply isn't any other valid representation for the two types in either language's rules, if the Rust variants are data-free: they must match.)

[workingjubilee]

I am happy to revise the wording but

• it would be inconsistent with improper_ctypes as-is to warn on u32 in FFI
• the inadequacy of the docs is a preexisting issue it seems
• if I had my druthers I would yeet the entire idea of linting on another crate's types in a crate that isn't the declaring crate from the lint, because it seems absurd, especially when in an extern "C" block that is only declaring a binding against C functions, not declaring a new Rust function (thus the bound function implicitly exists anyway), and this is very much the most-lamented lint in terms of false positives.

[workingjubilee]

I suppose some tests might depend on not having their imports formatted, but this is silly...

[petrochenkov]

When I started reading #116831 my first reaction was "if NodeTag is so special, then it's fine to use allow", but the problem is that you cannot actually apply allow to the single location where it would be most appropriate - enum NodeTag definition.

From the issue it looks like what you want to express is "this enum is non_exhaustive, but I guarantee that no future variant additions will change its ABI, so it's fine to use it in C interfaces and improper_ctypes should not be reported".

Maybe non_exhaustive can be enhanced with some markers describing what we promise not to add, and it would be useful in other situations too, not just in improper_ctypes.
In the meantime the lint could be relaxed.

[petrochenkov]

Warning on #[repr(C)] #[non_exhaustive] was an explicit lang team decision, so I'll send this back to lang team for reconsidering.

[workingjubilee]

When I started reading #116831 my first reaction was "if NodeTag is so special, then it's fine to use allow", but the problem is that you cannot actually apply allow to the single location where it would be most appropriate - enum NodeTag definition.

Correct, if the... approximately entire linting system was reworked, as I understand it, I guess the allow-site could be in the upstream crate?

[petrochenkov]

Well, not allow specifically, something else.
allow is supposed to be placed on the location where something suspicious happens, e.g. a FFI-unsafe type is used in FFI (i.e. fn rdb_read_page in the linked issue).
What we want here is to mark NodeTag as FFI-safe, so it's never suspicious when used in any location.

[workingjubilee]

Right, an arbitrary #[no_really_let_me_promise_a_semistable_abi_yet_nonexhaustive_for_reasons_that_are_really_long_to_explain] technically also works. That solution does not "spark joy", I think, but I can accept it.

[bors]

☔ The latest upstream changes (presumably #124558) made this pull request unmergeable. Please resolve the merge conflicts.

[workingjubilee]

uhhhh
r? lang

[traviscross]

@rustbot labels -I-lang-nominated

We discussed this issue in the meeting on 2024-06-12. Our consensus was as follows:

• The intent of "non-exhaustive" (on enums) is to say "ensure that code will continue to compile if new variants are added".
  • For lints, we generally try to ensure that you also won't get new lints.
  • But lints are lints, we can say it's ok if "there exists extensions that would be legal" and "the ones that would get you a new lint are unlikely".
• In the case:
  • We judge that adding a new data-carrying variant to an existing C-like enum that is passed to C is "unlikely", so we don't need the lint to account for it.
    • E.g. going from enum Foo { A, B, C } to enum Foo { A, B, C, D(u32) }.
• Additional consensus items:
  • Non-exhaustive currently means code must be prepared for:
    • On enums: new variants with any set of fields, public or private, etc.
    • On structs or enum variants themselves: can add new fields with any type, public or private, etc.
  • You cannot have values of an enum outside of the declared variants regardless of #[non_exhaustive], that would require a fresh feature.
• Possible future work:
  • Extensions or revamped non-exhaustive that lets you be more specific about the range of future changes.
  • A way to say that an enum may have variants not listed (e.g., due to C code).

[workingjubilee]

Thank you, I will rebase this shortly and write about-as-much down into rustc_lint.

[jieyouxu]

Apart from the minor nit, and based on the T-lang decision prior to this, this looks reasonable to me. Feel free to r=me after slightly expanding on what "really tagged union" means in the comment.

[jieyouxu]

@bors r+

[bors]

📌 Commit 64f5736 has been approved by jieyouxu

It is now in the queue for this repository.

[workingjubilee]

er!
@bors r-

[workingjubilee]

@bors r=jieyouxu

[bors]

📌 Commit 62aa8f0 has been approved by jieyouxu

It is now in the queue for this repository.