This is actually something I really like about rustfmt: if I make a line too complicated, it'll automatically get broken out across multiple lines and formatted nicely. I especially enjoy rustfmt's way of stacking things like func(|param| { or Some(StructName { on one line, formatting the internals nicely across multiple lines, and then closing with a matching }).
However,
Yeah, I wish that rustfmt didn't have any character-count-based limits. I wish that all of its limits were based on AST complexity, not characters.
Agreed. I have a few grudges with rustfmt as well, which is why I am not using it for my own projects.
What you say IMO applies particularly strongly to large matches, which tend to have complex structure paired with enough similarities between the cases that at least I tend to strongly prefer them to be consistently formatted -- using the same style for each match arm, so that I can "parse them all at once". Unfortunately, rustfmt relentlessly insists on formatting each match arm as if it was sitting alone without any context, frequently resulting in a mix of two or three different styles for match arms that are almost identical (often this hides some match arms really well so one cannot even tell apart patterns and code at a glance).
Yes, I have reported these issues on GitHub, but I feel these are design issues and I am worried they might never get fixed. I find myself trying to come up with hacks I could add in the code so that rustfmt won't go out of its way to remove symmetries from the code...
Will that always work though? i.e., if a subsequent commit touched the same lines as a prior commit, then the patch might not some cleanly. Or perhaps I am just underestimating git here.
Don't actually try to apply the subsequent patches on top of the rustfmt'd commits. Check out the first commit, rustfmt, commit. Check out the next commit, rustfmt, commit atop the first. That will always work.
This is something I've been wondering about myself: aside from everyone needing to set it up (which can indeed be automated), why not just use a git pre-commit hook?
The biggest problem with git pre-commit hooks is indeed that they aren't shared.
Even if I set up a pre-commit hook to fmt and provide cargo xtask install-hooks every single person who contributes to the repository has to remember/be told to install it.
This does reduce the work from O(PRs) to O(contributors), which is an improvement.
But better, imho, would be a setup like IntelliJ IDEA's run configurations: assuming you're sharing the "shared IDE files" that IDEA uses (i.e. your team is uniformly using IDEA so putting those IDE files into VC is acceptable), run configurations default to being developer-local. However, you can check a box to share the run configuration, and it will then be shared through VC.
I don't know the inner workings of pre-commit hooks. If it uses executables under the hood, then they definitely aren't sharable between workstations. Adding user code to git commit is also a potential security vulnerability, since you likely expect that to be a safe git-only operation.
I think the most tenable "shared pre-commit hook" would be for it to be a command line call to invoke a build system program that can do OS specific dispatch, and the call is somehow OS-independent, and on the first git commit it prompts you to ask if you trust and would like to run the repository's pre-commit hook. (I assume you can trust anybody you pull from after the initial clone to review.)
This would be a rather involved change to git upstream, however, and wouldn't benefit us for a long time, even if it were to be accepted.
I hate to say "use x.py more", but one potential solution is to push people to use e.g. ./x.py commit, which would install the pre-commit ./x.py fmt and then shell out to git.
Just a general note to everyone: Please remember that this discussion isn't solely focused around rustc. Suggestions centering on x.py, triagebot, and bors are likely not generalizable to any repository.
I don't think either of these are an issue. I think the best way for this to work is to have the bot push up a self-contained commit with only the rustfmt changes, which both makes it clear exactly what it did, and also is reproducible if something else wants to verify it was done correctly.
If you don't want that commit in perpetuity, you can "Squash and Merge" the PR.
I think @josh corrected me here, in that you could have a bot apply rustfmt to each commit in sequence.
The squash and merge idea only works if you're okay squashing everything down into one commit. Which is probably the common case though. But yes, an extra "rustfmt" commit is exactly what I meant with "poorer commit hygiene."
I mean, for non-rustc-scale projects it's simple: just don't gate the tests on style in CI. Then fmt is just another machine-automated (and machine-applicable) nit. The important part is not blocking the rest of CI on it.
Being able to ask the machine to apply rustfmt to the PR directly is useful, but not necessary. If there are other nits (such as failing tests or human review notes), the rustfmt can be folded into addressing those. If review is fine, the collaborator can push a rustfmt commit themself (PR default is to allow collaborators to edit).
The problem @kornel is pointing out is that waiting on CI checks and just getting "machine applicable tidy failed" is problematic. The solution is to say that without blocking any other CI results. That way you don't have the extra time-wasting cycle to just apply the machine's favored formatting style.
A loser formatter wouldn't even fix the underlying issue, just make it rarer to run into.
May I just ask: is this because you weren't using x.py locally or some other reason? Because if you x.py test and fmt fails it does say to use x.py fmt.
If it's because you aren't using x.py test locally, what deficiency does it have for why you aren't using it?
I think there may be a simple way to make rustfmt more user friendly.
Right now rustfmt has many thresholds that are a single number. If something is above the threshold it gets formatted one way and below it another way. The most obvious example of this is line wrap.
Instead rustfmt could have two thresholds, an upper and a lower. Things above the upper threshold would be formatted one way, things below the lower threshold would be formatted another, but things in the middle would be acceptable in either format.
So for line length it could be set to (80, 100). Meaning that if the line length is over 100 characters it will always be wrapped, but a wrap won't be removed unless the resulting line is less than 80 characters.
This avoids consecutive edits of pieces of code changing the format back and forth. It also significantly reduces the chances of rustfmt changing formatting that the developer copied from surrounding similar code. Most importantly it gives the developer some discretion over the format.
This change would be backwards compatible provided that the range overlaps the original single threshold value.
x.py is not Cargo, not even any standard build system, so my knowledge of it is zero. I only selectively copy'n'paste x.py commands from readmes or ask someone what to run. I've managed to "bless" UI tests, and that didn't run fmt.
I don't run full tests, and avoid building more than one stage, because it takes waaay to long.
But yeah, getting into how to use it is a bit tricky. But for the simple cases it should just be ./x.py test libfoo --stage 0.
Unfortunately, i don't think running cargo test in any but a special few in-tree crates (such as rustc_lexer) is officially supported. If it happens to work for a certain crate, especially if the crate is core, alloc, or std and relies on internal compiler details (such as the layout of a fat raw pointer), this is just a happy accident.
I wish we could make it just work through cargo isn't exactly tenable because of the bootstrap.
Yeah, I understand it needs to exist, and I don't expect x.py go away anytime soon. But if formatting was applied automatically on merge, then I wouldn't need to know whether it's x.py fmt, cargo fmt. And if rustfmt was more flexible, it wouldn't even need to rewrite as much.
Typically I tend to spam ./x.py check at the start just to get things compiling and then I do ./x.py test --stage 1 --bless --pass check src/test/ui the first time and tend to add --keep-stage 1 the next time (it tends to work even after rebases, unless someone has touched libstd in important ways); it works pretty well, and these days the compiler is more incr.comp friendly due to the "split up rustc crate" effort.
Formatting is rarely an issue for me when hacking on rustc, format-on-save in VSCode takes care of that just fine so ./x.py fmt is usually never needed.
