Running rustfmt on rust-lang/rust and other rust-lang repositories

tools and infrastructure
61

I think targeting December 28th would be good – it’s pretty much right in between Christmas and New Year’s so probably one of the lowest points in terms of contributions starting.

Largely I don’t think a time matters that much since once the tooling is ready we’ll pull down the compiler, run it, and then as per normal land a PR that switches it on.

62

Hmm… and that would presumably be the same for https://github.com/rust-lang/rust/pull/53654 and possibly switching to rust 2018 within the compiler? Do you forsee doing those things at the same time?

63

@ljedrz has a work-in-progress PR for rustfix support: https://github.com/rust-lang/rust/pull/56595

1 Like
64

Is this still planned for 12/28?

65

One sentiment I hadn’t seen mentioned is that running rustfmt on the entire repository is going to be a real pain for any large in-progress pull requests (I’m a little biased here). The code in the compiler currently follows a variety of conventions, so there are going to be a lot of changes, which are not going to be convenient to rebase on top of. I’m not sure what the solution to this is (maybe a longer adjustment period to give authors a change to push such PRs over the line?), but it’s worth pointing out.

66

I think it shouldn’t be a problem. If you run rustfmt on your PR at the end, we should still end up with a clean diff, right?

1 Like
67

Depends on what version of rustfmt you have locally installed :-|

68

I thought rustfmt has stability guarantees? Or maybe I’m just misunderstanding the stability guarantees…

2 Likes
69

After looking at Format Rust code by Mark-Simulacrum · Pull Request #57318 · rust-lang/rust · GitHub

+125,472 −87,080

Has anybody ever seen negative diffs from rustfmt?
It eats so much space :frowning:

Line lengths are not respected - even if the line fits into 100 characters perfectly it's still broken.
Many formatting choices are very different from what was used in rustc previously.

Perhaps rustfmt config needs to be tweaked to use "One True Line Length" instead of "one length for wrapping this, another shorter length for wrapping that"?
I'd also change the config to make formatting closer to what was used previously.

I guess it's a necessary evil to keep the formatting consistent, but (despite maybe sounding silly) I'm very unhappy that code I'm working on has to go through this, and I'll enjoy working on code less and will have less motivation if the code looks that much different from what I'd personally write.

4 Likes
70

Without regard to the specifics here, I have similar problems with auto-formatters that use different measures of “appropriateness” than those that I do personally. It’s probably a necessary burden for projects that have many concurrent maintainers, but it is painful for those of us who have preferred code rendering styles that differ from the community consensus.

71

My preferred style differs in many respects to the community consensus, but I still think community consensus should rule.

3 Likes
72

I don’t think this is “community consensus”.

-Ok(s) => if s == "true" { "y" } else { "n" },
+Ok(s) => {
+	if s == "true" {
+		"y"
+	} else {
+		"n"
+	}
+}

It’s just rustfmt not respecting line lengths.

Remember how the conditional operator “cond ? if_yes : if_no” was always rejected because "if is already an expression"? With formatting like this that just sounds like mockery in retrospect.

12 Likes
73

Yes I saw that one and I agree it looks really bloated.

74

Perhaps the right course of action is a style RFC?

1 Like
75

The style team already had that discussion and agreed that that kind of if should be on one line; I think that’s interacting badly with the rules regarding what kind of expression can comprise a one-line match arm.

We never did settle on a complete solution for when rules overlap.

2 Likes
76

I have a similar dislike of the lack of vertical density that rustfmt will create. Unfortunately most of the configuration options that can make this better are unstable.

3 Likes
77

I had similar issues with the default rustfmt config.

To make it usable, I only added a single config line: use_small_heuristics = "max"

With that, it avoids turning 40 characters into 4 lines and makes the diff much more reasonable.

3 Likes
78

The option is not user-facing (WidthHeuristics::single_line_if_else_max_width), but the default scales with max_width, so tweaking max_width might be the way to go?

EDIT:

Yeah, that makes more sense to use directly.

79

Fortunately, this is not an issue for rustc codebase.
EDIT: Actually, I'm not sure in this. What version of rustfmt should be used exactly? Does rustfmt have an analogue of RUSTC_BOOTSTRAP=1?

If it's indeed solvable with one line in config, that would be great!
I'll check what difference it makes on rustc code.

80

Similarly, this (and other similar examples) seems arbitrarily wrapped, as it’s not exceeding the line length:

- cmd.arg("-C").arg(format!("debug-assertions={}", debug_assertions));
+ cmd.arg("-C")
+     .arg(format!("debug-assertions={}", debug_assertions));

It seems any nested call is wrapped (e.g. https://github.com/rust-lang/rust/pull/57318/files#diff-e2a5ddcb8c7e32ab05929b0e42ff4719L93), which seems over-the-top.

Have the rustfmt configurations for rustc been discussed somewhere?

5 Likes