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

Avoid assigning (wrong) types to constants using #define #2120

Open
JanBeh opened this issue Nov 20, 2021 · 4 comments
Open

Avoid assigning (wrong) types to constants using #define #2120

JanBeh opened this issue Nov 20, 2021 · 4 comments

Comments

@JanBeh
Copy link

JanBeh commented Nov 20, 2021

Input C/C++ Header

#define MYCONST 2

Bindgen Invocation

$ bindgen input.h

Actual Results

/* automatically generated by rust-bindgen 0.59.1 */

pub const MYCONST: u32 = 2;

Expected Results

It would be better to generate something like

macro_rules! MYCONST { () => { 2 }; }

and to expect users to use MYCONST!() instead of MYCONST.

Reasoning

Constants using #define in a C header file do not have a type. They are inserted by the preprocessor as a literal where needed. Translating them to a Rust const with a type (u32, i32, or others) may lead to unexpected results. For example, i32 can be interchaged with std::os::raw::c_int on most platforms. But code that relies on that will not be portable. See discussion on Rust Users Forum "Interfacing C code with bindgen: #define and types".

Migration

Maybe it can be made possible to enable this new described behavior with an option that is not enabled by default; or the generated macro_rules!s could be created in addition to the consts (unless being switched off by an option).

@JanBeh
Copy link
Author

JanBeh commented Nov 20, 2021

@emilio
Copy link
Contributor

emilio commented Nov 26, 2021

Yeah this is sorta expected. You can set the type via ParseCallbacks::int_macro. The main problem with doing the above is that integer promotion rules in C and Rust are quite different, so it might not behave so intuitively as you might think...

@JanBeh
Copy link
Author

JanBeh commented Nov 30, 2021

There exists an easy workaround when comparing (small) constants such as 1, 2, 3, or -1, -2 with a c_int (or any other C type returned by a function, which might be different from c_int). You can write:

if some_ffi_module::some_func() == some_ffi_module::SOME_CONST as _ {
    /* … */
}

This cast should be okay if some_func's return type is big enough to contain the expected constant return value (which seems to be a reasonable assumption in most cases). But using as _ is at least confusing to whoever reads the code, and you won't get a warning if the constant doesn't fit the type returned by the function. Writing .try_into().unwrap() seems safer, but it is more verbose and would could cause runtime errors (where it should be a compile-time error if the constant really doesn't fit).

When matching constants, things get even more ugly, because it's impossible to use as _ or try_into().unwrap() directly in a pattern (it requires a match guard). See this post for some (ugly) examples.

I would thus say that assigning particular types to constants using #define should be avoided, and Rust macros should be used instead (at least as an option that can be enabled).

@ojeda
Copy link
Contributor

ojeda commented Sep 28, 2023

Constants using #define in a C header file do not have a type. They are inserted by the preprocessor as a literal where needed.

It is true they are just macros, but integer constants do have a type in C.

I'm asking because in C, I can pass MYCONST to both functions expecting an u8 or an int. It's not specified/defined by the #define which type the 1 actually has.

In C that works because the argument gets converted, not because the integer constant (i.e. after replacement) did not have a type, i.e. the 1 has type int.

Now, yes, in Rust there is type inference for integer literals without a suffix, which is what you want to use here to get some convenience back. But note that you are forced to pick a type and cast when using const instead. It seems like you would like to have rust-lang/rfcs#1349.

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

No branches or pull requests

3 participants