Forced rustfmt is a roadblock to contributing

[Mark_Simulacrum]

I personally am not yet sure if I am willing to have such "behind the back" changes, though I lean towards agreeing that we should automatically rustfmt in rust-lang/rust merges (whether as a part of the actual merge commit, or by incorporating the changes on the previous commits, or some other way, I don't know. I think this doesn't matter much).

However, to my knowledge, the tooling to do so does not exist, and I do believe that the benefits rustfmt brought and continues to bring outweigh the (marginal, I hope, though it sounds like some people disagree) pain of failing CI.

We have an open PR that should at least move the failing format check to a separate builder so we don't block the "test" PR builder on it passing; I plan to review it today.

[burntsushi]

Yeah, to be honest, I was thinking of my own repos, where I really try to do my best to keep a clean history. I don't think rust-lang/rust prioritizes that as much, so there's definitely a different calculus there.

[mark-i-m]

Personally, I would not like things to happen automatically in the background. I often break PRs into chains multiple smaller PRs master->A->B->C, and it's already a pain to rebase A, B, and C on top of the latest master every time something causes a conflict. Auto-formatting makes this worse because it means that I won't know ahead of time that my formatting is failing. Instead, I will just be confronted by yet another rebase conflict...

EDIT: to elaborate a bit more, I think reducing git conflicts is one of the biggest things in favor of rustfmt. Like others, I use fmt-on-save. I personally don't really care that much what the code looks like; I just want it consistent so that any git conflicts I get are from actual changes.

[CAD97]

Does git actually allow you to make arbitrary changes in a merge commit? Even if so, if this is the commit you'd introduce the formatting fixes in, it seems problematic to have a merge commit with changes other than the merge.

[scottmcm]

@CAD97

If it's because you aren't using x.py test locally, what deficiency does it have for why you aren't using it?

FWIW, I have never successfully run a full test. The few times I've tried it's failed on the make tests, and it takes so long that I just don't bother. (I do run it for parts, but never a full one.)

[rpjohnst]

Git does allow arbitrary changes in merge commits, and can show them correctly in diff views.

[skade]

@Mark_Simulacrum

I personally am not yet sure if I am willing to have such "behind the back" changes, though I lean towards agreeing that we should automatically rustfmt in rust-lang/rust merges (whether as a part of the actual merge commit, or by incorporating the changes on the previous commits, or some other way, I don't know. I think this doesn't matter much).

I'm wondering how many of those changes will end up being an additional patch similar to "make rustfmt happy", which are arguably more noise then anything else.

I see 2 paths that a user may reasonably take:

• A single, throwaway commit, running rustfmt
• Amending the last commit, merging the fmt changes into a semantic change

I don't see any reason why the process can't be automated and needs user intervention.

Automation may add one and a half more way:

• formatting automatically, as part of a merge commit
  • potentially allowing to format multiple commits at once, as part of a rollup

Our bot can leave a marker ("automatic adjustments for formatting", which is fine).

We automate the process of formatting the code to a standard, so I see a reasonable case for also applying this automation without user intervention. In the most dominant case, we ask someone to lend their hands and their computer to type "cargo fmt" and try again.

[kornel]

But demonstrably, gofmt does not work like this. There is no canonical form of Go AST.

https://play.golang.org/p/N8aAHiaNjYB

This is the output of gofmt:

func main() {
	fmt.Println(person{"Bob", 20})

	fmt.Println(person{
		"Bob", 20})

	fmt.Println(person{"Bob",
		20})

	fmt.Println(person{
		"Bob",
		20})

	fmt.Println(person{
		"Bob",
		20,
	})
}

It forced indentation, brace style, and normalized spaces between tokens, but it never dared to move values across the lines.

I'd also like to point out that "style" (and its configurability that Russ Cox mentioned) and forced "canonicalization" are separate problems.

There's a lot of emphasis on using formatters to eliminate "squabbling" about subjective and endlessly-debatable details of the formatting style (tabs vs spaces, 1TBS vs allman, etc.). It's fine to choose one option, and help apply it consistently. That's not my issue with rustfmt. I happen to personally agree with almost all of these style choices, and style details can be adjusted in the current rustfmt anyway.

It's the "canonical AST" approach that is the problem, which alters layout of the code, and is fundamental to rustfmt. Rustfmt does not allow the programmer to control which things stand out, which are tucked away as boilerplate. It prevents grouping of logically related method calls together, because to rustfmt they're all the same — that's not a style choice, that's a loss of semantic information! It adds unnecessary code churn (and risk of merge conflicts) when lines cross various thresholds. These problems stay the same no matter what style one prefers, so it's not shallow complaint about particular style, but a deeper workflow issue that affects all code styles.

[mcy]

@kornel

It's the "canonical AST" approach that is the problem, which alters layout of the code, and is fundamental to rustfmt. Rustfmt does not allow the programmer to control which things stand out, which are tucked away as boilerplate. It prevents grouping of logically related method calls together, because to rustfmt they're all the same — that's not a style choice, that's a loss of semantic information! It adds unnecessary code churn (and risk of merge conflicts) when lines cross various thresholds. These problems stay the same no matter what style one prefers, so it's not shallow complaint about particular style, but a deeper workflow issue that affects all code styles.

This is clearly an issue with rustfmt that I think should be addressed through patches to rustfmt, then. This layout information you describe should be part of the AST, even if it irrelevant to semantic analysis of that AST. I think we both agree on this point.

[Centril]

@kornel

It's the "canonical AST" approach that is the problem, which alters layout of the code, and is fundamental to rustfmt. Rustfmt does not allow the programmer to control which things stand out, which are tucked away as boilerplate. It prevents grouping of logically related method calls together, because to rustfmt they're all the same — that's not a style choice, that's a loss of semantic information! It adds unnecessary code churn (and risk of merge conflicts) when lines cross various thresholds. These problems stay the same no matter what style one prefers, so it's not shallow complaint about particular style, but a deeper workflow issue that affects all code styles.

You can still force rustfmt to avoid smashing together groupings by using comments of various forms, or various abstraction mechanisms that the language provides (modules, separate impls, more functions, etc.). Not only does that preserve semantic information, but makes the code overall more understandable.

Also, using rustfmt to break lines when they cross some threshold (which is reasonably picked at e.g. 100 characters, a good heuristic for split screen editor workflows) allows you to write code in a horrible fashion and then have rustfmt clean it up on save, which is a time saver overall.

So far I'm very happy with us having moved to enforcing rustfmt on CI. Setting up format-on-save was easy, dealing with the initial wave of merge conflicts was relatively painless, and now it improves my workflow as a regular contributor quite a bit.

[petrochenkov]

From a reviewer's point of view I'd prefer all commits to be formatted before I read them.

Even without format-on-save, running x.py test tidy --bless takes only few seconds and saves everyone's time, so I still don't understand why people don't run it before submitting any changes (unless they are first-time contributors, of course).

[mark-i-m]

@skade

I see 2 paths that a user may reasonably take:

• A single, throwaway commit, running rustfmt
• Amending the last commit, merging the fmt changes into a semantic change

I do neither of these. I run ./x.py fmt before every commit. That way, all the commits contain properly formatted code, and there is no noisy history.

A middle ground might be to add a rustbot command to add a rustfmt commit to your PR. It does not run by default, so those who don't want autoformatting don't have to deal with it. But it is available for anyone who wants it.

[SlightlyOutOfPhase]

But demonstrably, gofmt does not work like this. There is no canonical form of Go AST.

That may be true, but I feel like it's worth something that gofmt has literally zero configuration options while rustfmt has quite a large number of them, which to me makes it significantly more useful in the long run.

Like, not that I have much experience writing Go code as it is, but the thought of people actively expecting me to use a formatter that forced not only tabs, but specifically eight-space-worth tabs at all times even for personal projects makes me want to keep it that way.

[CAD97]

Since I just ran accross it, and it may be interesting to people:

Restyled.io provides a GitHub App to provide style enforcement, and supports Rust. If I'm reading their documentation correctly, it works by opening a PR against your PR branch that provides commits applying the automated format, and only does so if the format is necessary.

While I'm still split on whether/how rust-lang/rust could/should provide auto-formatting hooks, this may prove useful to other repositories who would like similar functionality.

[RalfJung]

As a datapoint for rustfmt getting in the way, in a PR I was working on today it turned a <10-line change into more than 100 lines of diff because some heuristic flipped and it re-flowed everything. Sure, immediately-applied closures are not terribly common in Rust, but I would not call them an antipattern either, and even if so that's no excuse to make reviewer's life so bad. Plus, this of course caused conflicts during rebasing, and I almost dropped another change that was made to that code in the mean time.

I am not sure when the time saved in no longer having to manually re-flow function signatures that become too wide will make up for the time lost in incidents like this, but I think that will take a while for me.

@Centril

You can still force rustfmt to avoid smashing together groupings by using comments of various forms, or various abstraction mechanisms that the language provides (modules, separate impl s, more functions, etc.). Not only does that preserve semantic information, but makes the code overall more understandable.

Whitespace is also a good means to structure visuals, including text and code. Sure, one can do without, but readability suffers. IOW, we are giving up on one of just a few channels we have to convey intent.

[CAD97]

@anon2808951

Making sure that rustfmt's formatting doesn't break compilation on older Rust versions is a real pain, even if the project itself doesn't use rustfmt.

Do you mind providing an example? I don't think rustfmt should (by default) be reformatting anything in a way that requires any new compiler support. It's supposed to just be reflowing whitespace.

@anon2808951

Having to tell first-time contributors that their changes are welcome, but they need to undo the rustfmt run they "helpfully" included in their commit? Not great.

Rustfmt really should have a "don't allow" config dead switch other than just #![rustfmt::skip] for projects that explicitly don't want to rustfmt the world. Then contributors won't have to worry about disabling format-on-save.

And a format-just-the-vcs-diff mode is also a great potential feature fighting just this problem.

@anon2808951

When contributing to other people's projects, I was more than once close to say "this is the PR, you can take it or leave it" because reformatting it "properly" would have meant cherry-picking commit #1, running cargo fmt on it, cherry-picking commit #2 on top, fixing the conflicts introduced by the reformat, for each and every commit.

That may be the easiest way to come up with doing it, but specifically because rustfmt is deterministic, that is definitely not the best way of doing it. Instead, the operations are

• checkout commit before the PR split git reset --hard <commit>
• for each commit in the PR
  • apply the commit's state to your working directory git diff <commit> | git apply --index --reverse
  • run formatting cargo fmt (or ./x.py fmt for rust-lang tree)
  • commit the formatted commit git commit --reuse-message=<commit>

Disclaimer: I assembled these commands via googling for them and have not tested them to make sure they work. Yes, this is nonobvious. But that's git's fault, not rustfmt's.

[mjbshaw]

@RalfJung

a PR I was working on today it turned a <10-line change into more than 100 lines of diff because some heuristic flipped and it re-flowed everything

FWIW I think a decent part of the blame is on GitHub's terrible code review/diff UI. Having used Google's (internal) Critique code reviewer, I can say there are much better ways this diff could be rendered (and review UI in general).

The code and rustc's style are also partly to blame. The code is heavily nested and rustc's 4-space indentations mean you run out of horizontal real-estate quickly (which leads to reflowing).

It's a useful datapoint but I feel like rustfmt is the metaphorical "straw that broke the camel's back" in this situation.

@anon2808951

Making sure that rustfmt's formatting doesn't break compilation on older Rust versions is a real pain, even if the project itself doesn't use rustfmt.

WTF? Is that a thing? I would expect a formatter to never change compilation results or program output (unless there's a bug, obviously).

[cuviper]

The only change I know besides whitespace is that it may introduce a trailing comma, e.g. when parameters are split across lines. I think it might also add a semicolon after return statements and the like, if you didn't have it. I've never seen any of that be an issue for compatibility though.

[skade]

@mark-i-m

I do neither of these. I run ./x.py fmt before every commit. That way, all the commits contain properly formatted code, and there is no noisy history.

This fits perfectly in what I describe, it makes it a null-op. The question is what to do when rustfmt has not been run.

[skade]

I feel like this discussion is moving away from whether rustfmt should be applied to all PRs and how it should happen towards general issues with rustfmt's design. This discussion is useful, but I think it would merit a second thread. Otherwise, the core of this discussion will be lost.