rust-lang / rust-lang/rust-bindgen

`Builder` methods receiving `bool` parameters considered harmful

Open
#2,474 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Rust
Stars
5.3k
Forks
829
Avg merge
1d 1h
Merged PRs (30d)
15

Description

Following the "X considered harmful" tradition. I think it would be best to avoid having methods for the Builder type that take boolean parameters unless they are strictly necessary. Here are some reasons why:

  • It makes the bindgen and bindgen-cli APIs less homogeneous: In some cases, the method corresponding to a flag has a slightly different name. Like layout_tests and --no-layout-tests. This happens with several of the --no-.* flags.
  • Makes documentation difficult: Some methods like block_extern_crate don't have a clear truth value, so we have to spend some more lines explaining what happens if you call the method with true, then with false and then when it is not called. If the method did not take any parameters we would only have to explain what happens if you call the method or not.
  • Makes harder to understand the default behavior: This goes in the same direction as the previous item. For some methods, like recursive_allowlist, the default behavior is true and for others, like generate_block, it is false. This means that documenting the default behavior is a bit harder because you have to explain which one between true and false is equivalent to not calling the method.
  • It is not intuitive for methods that aren't named after a verb: If a method is called generate_something, you'd assume that passing true tells bindgen to generate something and that passing false tells to not generate something. But for some methods, like objc_extern_crate, this is not as clear.

I'd suggest doing one or more of the following:

  • Remove the boolean parameters from all methods whose default behavior is obtained by passing false: For example, fit_macro_constants(true) would become fit_macro_constants() and fit_macro_constants(false) will stop existing because it is the default behavior.
  • Do the same for the methods with a true default behavior but add a negative prefix to their names: For example, layout_tests(false) will become no_layout_tests() and layout_tests(true) will stop existing because it is the default behavior.
  • Generate enums for methods without a clear truth value: For example, objc_extern_crate(true) would become objc_import_style(ImportStyle::ExternCrate) and objc_extern_crate(false) would become objc_import_style(ImportStyle::Use).

I also acknowledge that there are some disadvantages to doing these changes:

  • It would be a huge breaking change and would put a lot of strain in the users of bindgen.
  • I suppose that passing booleans makes easier to parametrize bindgen invocations:
// So this kind of code 
fn call_bindgen(layout_tests: bool) -> Bindings {
    Builder::default().layout_tests(layout_tests).generate().unwrap()
} 
// Would have to be rewritten as
fn call_bindgen(layout_tests: bool) -> Bindings {
    let mut builder = Builder::default();
    if !layout_tests {
        builder = builder.no_layout_tests();
    }
    builder.generate().unwrap()
} 

But I suspect that the second disadvantage will affect very few users as most people will fix the options they use. On the other hand, I think that this simplification would make bindgen easier to use and avoid writing preventive bindgen invocations where a bunch of methods are being called with their default values just to be sure.

cc @emilio, @kulp, @amanjeev.

Edit:

I had an AFK conversation with someone who mentioned that one advantage of having bool parameters is that makes your code less prone to breaking changes. As an example, if we decided to stop generating layout tests by default then people using layout_test(bool) would not be affected. However, people using no_layout_tests() would get a warning/error telling them that the method is deprecated/does not exist anymore.

However, in both scenarios users that never used the layout_test method would see their code change with a new version, so in that sense both options are the same.

So yeah, there are disadvantages when defaults change but I'd say that this does not happen that often. If anything the default of an option will likely change once in the lifetime of the option. Like size_t_is_usize where the option was added but disabled by default, and afterwards it became a default option so we had to deprecate --size_t-is-usize and add --no-size_t-is-usize.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by auditing the Builder methods, bindgen API, bindgen-cli flags, and the boolean options named in the issue, including layout_tests, block_extern_crate, recursive_allowlist, generate_block, and objc_extern_crate. Map their defaults, CLI spellings, and compatibility implications; the work is done only when a specific, agreed migration scope and validation plan exists.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
api, cli, tooling
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.