Skip to content

Dubious improper_ctypes firing on recursive non-local containment? #116831

Closed

Description

Bindgen allows you to create a "rustified non-exhaustive enum" from a C enum. This is used by pgrx-pg-sys for a specific C enum, NodeTag, because

  • NodeTag has a well-defined, finite list of variants for each major version
  • Despite this, much like a repr(Rust) enum, it regularly changes the assigned values to its variants each version, requiring an extension to be built against a specific Postgres version.
  • Each major version consistently adds at least one variant somewhere.
  • ...except for a zero-initialized value, which is always the sentinel value.
  • ...so it is much better, in terms of ergonomics, to treat it as a Rust-ish enum with an appropriate repr to make sure it is the correct size, with #[non_exhaustive], so that people can write the same code and have it be robust against future Postgres versions (though I seriously doubt anyone is going to write a 400 variant match, but hey, we've seen weirder, sooo...).

Unfortunately, it seems this can trigger the improper_ctypes lint if people add their own bindings against Postgres which mention pgrx_pg_sys's bound types, which they did not define the original bindings to, and which only recursively contains the improper ctype... i.e. they mentioned a pgrx-pg-sys-bound type, which is itself a pointer to a pgrx-pg-sys-bound type, that contains a pointer to another pgrx-pg-sys-bound type that itself contains the pgrx-pg-sys-bound enum.

Naturally, they have no idea what the hell is going on, since the error doesn't even mention which field is the offender, or how recursively deep it is.

This disappears if I make it a "rustified enum" instead, which lacks #[non_exhaustive]. It seems some reasoning is being applied along the lines of, "Non-exhaustive means that it can become FFI-incompatible in the future due to feature(arbitrary_enum_discriminant) and feature(really_tagged_unions), without breaking compat", when that has nothing to do with reality, and in fact I'm more trying to use it to encourage people to handle the C code correctly! And even if that was the case, the lint appears to be violating locality.

I tried this code:

use pgrx::pg_sys;

//  Many
//  lines
//  of
//  boiler-
//  plate 
//  code.

extern "C" {
    pub fn rdb_read_page(
        index: pg_sys::Relation,
        blkno: u32,
        lock_type: ::std::os::raw::c_int,
        pg_buffer: *mut u32,
    ) -> *mut ::std::os::raw::c_char;
}

I got this:

warning: `extern` block uses type `NodeTag`, which is not FFI-safe
  --> src/lib.rs:39:16
   |
39 |         index: pg_sys::Relation,
   |                ^^^^^^^^^^^^^^^^ not FFI-safe
   |
   = note: this enum is non-exhaustive
   = note: `#[warn(improper_ctypes)]` on by default

warning: `sample` (lib) generated 1 warning
    Finished dev [unoptimized + debuginfo] target(s) in 17.63s

I expected to not see a warning on types that weren't defined in the local crate, are de facto safe as argument types, and in any case are behind at least two pointers.

Meta

This repros on both stable and this nightly:

rustc --version --verbose:

rustc 1.75.0-nightly (fcab24817 2023-10-13)
binary: rustc
commit-hash: fcab24817c72ffbd6ffb66d92b7ddc0d3ee4d2f0
commit-date: 2023-10-13
host: x86_64-unknown-linux-gnu
release: 1.75.0-nightly
LLVM version: 17.0.2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Assignees

No one assigned

    Labels

    A-FFIArea: Foreign function interface (FFI)A-lintArea: Lints (warnings about flaws in source code) such as unused_mut.C-bugCategory: This is a bug.L-improper_ctypesLint: improper_ctypesT-compilerRelevant to the compiler team, which will review and decide on the PR/issue.T-langRelevant to the language team, which will review and decide on the PR/issue.

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions