Run rustfmt on the emitted bindings

[fitzgen]

Usually, we generate code that is fairly readable.

This is not always the case, and will often not be the case once --impl-debug lands: https://github.com/rust-lang-nursery/rust-bindgen/pull/899/files#diff-a1db39afd224d69f0732cc221a7430b8R32

We should run rustfmt on the code we emit to give it a consistent, pretty, and readable formatting.

While rustfmt does have a programmatic API, it is using libsyntax from the compiler's internals. This means it is nightly only.

So maybe we want to avoid using rustfmt as a library, and just invoke the rustfmt on the $PATH, if any? If we do this, we should pin the version of rustfmt we use with our test expectations, so that our tests don't break when new versions of rustfmt are published, or when contributors are using different versions of rustfmt.

cc @nrc

[bkchr]

Couldn't rustfmt 0.9 be used for stable and rustfmt-nightly for nightly?

[fitzgen]

Couldn't rustfmt 0.9 be used for stable and rustfmt-nightly for nightly?

If they (ever start to) format the code slightly differently, then we would have to maintain double the test expectations, which is 💩

[bkchr]

Yes that would be shitty. Do we really need formatted tests?

Yeah the would be easier to read, but I don't know if the rustfmt output is already that stable between different releases :/

[nrc]

I think rustfmt and rustfmt-nightly already have non-trivial differences in their formatting. I would expect that rustfmt formatting would not be stable enough even in the long run for all tests (because of bug fixes, etc.)

Running system rustfmt would be the best bet I think. And I guess if the user doesn't have rustfmt, you don't format, which is not awful. And if you have the facility to not run formatting in some cases, then you use that for testing too.

[fitzgen]

Do we really need formatted tests?

Certainly it isn't critical that the test expectations be formatted. I think it is more important for every day bindgen users: they should be able to easily read the bindings so that they can use them.

We could (should) make rustfmting an option. I want to say that it should be on-by-default, but maybe we should be conservative and start with it off and then turn it on-by-default later.

Running system rustfmt would be the best bet I think. And I guess if the user doesn't have rustfmt, you don't format, which is not awful.

Sounds like a plan.

[emilio]

Sorry for running late to the party, but I don't get why this is needed. What's the use case for this?

Seems like if you use bindgen as a build dependency, then you basically don't care about how the output looks. If you don't, and you check-in the generated bindings, then I can't see why can't you format the bindings yourself, and why is support on bindgen itself required.

This makes us pull a new dependency in mozilla-central, which is probably not a huge issue, but I still don't know why the support in bindgen is helpful.

[fitzgen]

What's the use case for this?

Occasionally, during development, one needs to read the generated bindings. Generally, this hasn't been problematic because the emitted bindings tended to be well formatted. This is no longer always the case after we we started emitted Debug implementations for things that couldn't derive Debug automatically.

Seems like if you use bindgen as a build dependency, then you basically don't care about how the output looks

Even when I do this, I still find myself needing to read the bindings to figure out what the exact name for some anonymous type is or whatever. It just comes up during development and debugging sometimes.

This makes us pull a new dependency in mozilla-central

This is a real problem, and I should have caught this in review. Apologies. We should put rustfmting and the new dependency behind a new feature. I think it should OK for this new feature to be on by default, and m-c can turn it off manually. Does that work for you, @emilio?

[bkchr]

Should I make a pull request for hiding it behind a feature?

[fitzgen]

Should I make a pull request for hiding it behind a feature?

Assuming that @emilio is satisfied with this plan, yes please! Thanks! :)

[upsuper]

I agree that we need to read the generated bindings occasionally, so they should have reasonable format in general, but I don't agree this holds for impl of Debug. I think we hardly want to read the impl of that, and in case someone wants, it shouldn't be terribly time consuming to format it oneself, so I don't think that justifies adding such code (even behind a feature) to bindgen.

[fitzgen]

I often read the generated bindings. The community is moving towards a world where everything is rustfmted all the time. Matching those expectations makes sense to me. It is a small feature with close to zero maintenance costs (for example, it doesn't interact with things like whitelisted sets, and options that interact with that do have a high maintenance cost, in contrast). By being behind a feature flag, I really think there isn't much to complain about here.

[upsuper]

I often read the generated bindings too, but I cannot imagine that I would want to read impl of Debug at all in general, unless I'm debugging the bindgen (and specifically the Debug impl codegen) itself. And I don't think debugging Debug codegen is a good enough reason for adding a default dependency and that bunch of code.

Adding that may not interact a lot with other code in bindgen, so there is no much overall additional complexity, but that doesn't mean it has zero maintenance costs. This "small" feature may give users a false impression that bindgen is the right place to add various pre- and post-processing phases, which should otherwise have been done independently outside bindgen. rustfmt is just one of that.

bindgen should just do the work of generating bindings. Anything else which can be done by other tools should be done by other tools orthogonally. Adding dependency and code to a library (and command line program) which serves completely nothing to its core functionality isn't a good practice in my opinion.

[fitzgen]

While it seems we just plain disagree on this issue, the whole argument is about to become moot.

In #925 we are moving off the deprecated, unmaintained, and slow to build syntex crate over to the quote crate for code generation. Once that happens, all of the generated bindings will be on a single line because quote doesn't support pretty printing like syntex does. The only way to have any readable output at all will be to run rustfmt.

Once we can no longer rely on syntex's pretty printing to get decently readable bindings, then we must start using rustfmt, because being able to read struct definitions and such from the bindings is a mandatory.