Make `--rustfmt-bindings`/`Builder::rustfmt_bindings` on by default

[dimbleby]

Edited by @fitzgen

Ever since the switch to quote instead of syntex, the bindings get emitted in a decidedly not-pretty-printed fashion.

We have an option to run rustfmt on the bindings: --rustfmt-bindings/Builder::rustfmt_bindings. It is off by default, but it should be on by default. Instead, users should turn it off if they don't need it or want it.

See src/options.rs and src/lib.rs for the command line flags and Builder definition respectively.

---

Original comment:

Bindgen could really do with sticking in some more newlines.

Eg at the moment I'm getting this 4000 character monstrosity, which I reckon should be about a hundred lines of code.

This is a recent regression. I assume, but have not verified, that it's a result of merging no-syntex.

Perhaps the intention is that I should always manually rustfmt the output? That does give an OK-ish solution - rustfmt does a pretty good job with the line that I linked above, though it fails later in the file with lines that are too long for it.

If the intention is that rustfmt is the solution then the docs should say something about this. Eg the README example doggo.h actually gives output that's not much like what is promised:

/* automatically generated by rust-bindgen */

# [ repr ( C ) ] # [ derive ( Debug , Copy ) ] pub struct Doggo { pub many : :: std :: os :: raw :: c_int , pub wow : :: std :: os :: raw :: c_char , } # [ test ] fn bindgen_test_layout_Doggo ( ) { assert_eq ! ( :: std :: mem :: size_of :: < Doggo > ( ) , 8usize , concat ! ( "Size of: " , stringify ! ( Doggo ) ) ) ; assert_eq ! ( :: std :: mem :: align_of :: < Doggo > ( ) , 4usize , concat ! ( "Alignment of " , stringify ! ( Doggo ) ) ) ; assert_eq ! ( unsafe { & ( * ( 0 as * const Doggo ) ) . many as * const _ as usize } , 0usize , concat ! ( "Alignment of field: " , stringify ! ( Doggo ) , "::" , stringify ! ( many ) ) ) ; assert_eq ! ( unsafe { & ( * ( 0 as * const Doggo ) ) . wow as * const _ as usize } , 4usize , concat ! ( "Alignment of field: " , stringify ! ( Doggo ) , "::" , stringify ! ( wow ) ) ) ; } impl Clone for Doggo { fn clone ( & self ) -> Self { * self } } extern "C" {
 pub fn eleven_out_of_ten_majestic_af ( pupper : * mut Doggo , ) ;

[fitzgen]

Yes, this is expected after the move off of syntex.

There is Builder::rustfmt_bindings and --rustfmt-bindings to run rustfmt on the emitted bindings.

Re: the doggo.h example: with --rustfmt-bindings and --no-layout-tests, I get this:

/* automatically generated by rust-bindgen */

#[repr(C)]
#[derive(Debug, Copy)]
pub struct Doggo {
    pub many: ::std::os::raw::c_int,
    pub wow: ::std::os::raw::c_char,
}
impl Clone for Doggo {
    fn clone(&self) -> Self { *self }
}
extern "C" {
    pub fn eleven_out_of_ten_majestic_af(pupper: *mut Doggo);
}

Which I think is fair. The README doesn't mention flags, it is just there to help people understand what bindgen does and why you might want to use it.

---

That said, yes I recognize this situation is not ideal.

Things I think we can do about it:

• Add documentation somewhere (which place is best?) that the Builder::rustfmt_bindings/--rustfmt-bindings options exist

• Make rustfmt_bindings/--rustfmt-bindings enabled by default

Things I think we can't do:

• Go back to syntex

Any other ideas?

[dimbleby]

Yeah, I think that what you say above is right: rustfmt should be enabled by default, and the docs should mention this.

(IMO rustfmt-nightly is so much better than rustfmt that people should be guided towards that if possible; but this is certainly a second order consideration).

The current output is likely more of an issue for people like me who are long-term bindgen users and accustomed to the autogenerated code being already tolerably well formatted. It came as quite the shock when I saw what it is now producing!

If the autogenerated code is either automatically passed through rustfmt, or there's some message making clear that this is what's expected to happen, then there's probably no real issue here at all.

[fitzgen]

(IMO rustfmt-nightly is so much better than rustfmt that people should be guided towards that if possible; but this is certainly a second order consideration).

We just use the system rustfmt, if it exists, because it is impossible to use rustfmt as a library except on nightly. That means we use whatever rustfmt the user has installed (hopefully rustfmt-nightly). If they don't have it installed, we don't format the bindings.

[highfive]

Hi! If you have any questions regarding this issue, feel free to make a comment here, or ask it in the #servo channel in IRC.

If you intend to work on this issue, then add @highfive: assign me to your comment, and I'll assign this to you. 😄

[fitzgen]

I think we need to make rustfmting the default before the next release. Too many people will get bitten by this.

[harlanhaskins]

I'd like to work on this, if possible!

@highfive: assign me