Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cannot override integer type of constified enum module. #2711

Open
ju1ius opened this issue Jan 6, 2024 · 4 comments
Open

Cannot override integer type of constified enum module. #2711

ju1ius opened this issue Jan 6, 2024 · 4 comments

Comments

@ju1ius
Copy link

ju1ius commented Jan 6, 2024

Hi,

Sorry if I'm missing something but there doesn't seem to be a way to override the integer type of a constified enum module.

I tried the naive approach of using ParseCallbacks::int_macro, but to no avail.

Input C Header

typedef uint16_t node_kind;

#define NODE_KIND_LIST_SHIFT 6

enum node_kinds {
  NODE_KIND_FOO = 1,
  NODE_KIND_BAR,
  // ...
  NODE_KIND_FOO_LIST = 1 << NODE_KIND_LIST_SHIFT,
  NODE_KIND_BAR_LIST,
};

Bindgen Invocation

use bindgen::{callbacks::{IntKind, ParseCallbacks}, Builder, EnumVariation};

struct MyCallbacks;
impl ParseCallbacks for MyCallBacks {
    // This works for macros, but not for enum members
    fn int_macro(&self, name: &str, value: i64) -> Option<IntKind> {
        match name {
            s if s.starts_with("NODE_KIND_") => {
                assert!(value >= 0 && value <= u16::MAX as _, "{name} ({value}) overflows u16 bounds.");
                Some(IntKind::U16)
            }
            _ => None,
        }
    }
}

Builder::default()
    .header("input.h")
    .default_enum_style(EnumVariation::ModuleConsts)
    .parse_callbacks(Box::new(MyCallbacks))
    .generate()
    .unwrap()

Actual Results

pub type node_kind = u16;
pub const NODE_KIND_LIST_SHIFT: u16 = 6;
pub mod node_kinds {
    pub type Type = ::std::os::raw::c_uint;
    pub const NODE_KIND_FOO: Type = 1;
    pub const NODE_KIND_BAR: Type = 2;
    // ...
    pub const NODE_KIND_FOO_LIST: Type = 64;
    pub const NODE_KIND_BAR_LIST: Type = 65;
}

Expected Results

There should be a way to obtain the following result:

pub type node_kind = u16;
pub const NODE_KIND_LIST_SHIFT: u16 = 6;
pub mod node_kinds {
    pub type Type = u16;
    pub const NODE_KIND_FOO: Type = 1;
    pub const NODE_KIND_BAR: Type = 2;
    // ...
    pub const NODE_KIND_FOO_LIST: Type = 64;
    pub const NODE_KIND_BAR_LIST: Type = 65;
}

As a side note, this is another use case for #1916.

@emilio
Copy link
Contributor

emilio commented Jan 12, 2024

That's kind of intentional, because functions taking a node_kinds would need to accept a c-int (would break the ABI otherwise).

@emilio
Copy link
Contributor

emilio commented Jan 12, 2024

Same for structs containing it or what not. Thus, not clear it's a bug, would be rather dangerous to do as proposed. But please reopen if you disagree or I've gotten something wrong.

@emilio emilio closed this as completed Jan 12, 2024
@ju1ius
Copy link
Author

ju1ius commented Jan 13, 2024

@emilio Sorry, my example had too little context.

I get your point but in this case, the real public type is node_kind (u16), not node_kinds (c_int), i.e.:

typedef uint16_t node_kind; 

// node_kinds exposes the valid values as a convenience,
// but the type itself is never used in the API
enum node_kinds {
  NODE_KIND_FOO = 1,
  // ... lotta kinds
};

typedef struct _node {
  node_kind kind; // uint16_t
  // ...
} node;

node* node_new_empty(node_kind kind);  // takes an uint16_t

There are many other places like this in the library, where:

  • the API uses a specific integer type T (not c_int)
  • an enum exposes the valid values for T as a convenience but the enum is never used as a type in the API

It gets even more annoying when #defined constants are involved since you have to juggle between c_int, u32 and u16 when it really should be just u16 everywhere.

Thus, not clear it's a bug,

Let's say it's a feature request then. 😉
I can open another issue (or update the OP) with a more accurate description of the use-case if you want.

would be rather dangerous to do as proposed.

Of course, if implemented it should definitely be opt-in.
There could also be a safeguard, i.e. fail to compile if the enum type is used as a function argument or return type?

But please reopen if you disagree or I've gotten something wrong.

Please note that regular users cannot reopen issues on this tracker.

@emilio emilio reopened this Jan 13, 2024
@emilio
Copy link
Contributor

emilio commented Jan 13, 2024

reopening as per the above though I still think it'd be rather footgunny.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants