rust-lang / rust-lang/rust-bindgen
`Builder` methods receiving `bool` parameters considered harmful
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
bindgenandbindgen-cliAPIs less homogeneous: In some cases, the method corresponding to a flag has a slightly different name. Likelayout_testsand--no-layout-tests. This happens with several of the--no-.*flags. - Makes documentation difficult: Some methods like
block_extern_cratedon't have a clear truth value, so we have to spend some more lines explaining what happens if you call the method withtrue, then withfalseand 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 istrueand for others, likegenerate_block, it isfalse. This means that documenting the default behavior is a bit harder because you have to explain which one betweentrueandfalseis 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 passingtruetellsbindgento generate something and that passingfalsetells to not generate something. But for some methods, likeobjc_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 becomefit_macro_constants()andfit_macro_constants(false)will stop existing because it is the default behavior. - Do the same for the methods with a
truedefault behavior but add a negative prefix to their names: For example,layout_tests(false)will becomeno_layout_tests()andlayout_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 becomeobjc_import_style(ImportStyle::ExternCrate)andobjc_extern_crate(false)would becomeobjc_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
bindgeninvocations:
// 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
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- 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