-
Notifications
You must be signed in to change notification settings - Fork 12.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Format the world #67540
Format the world #67540
Conversation
@bors r+ |
📌 Commit 4fe589e1aaa82c0b3fbf2f359f0bca0612d5a84b has been approved by |
@bors r- |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
RustcEncodable, | ||
RustcDecodable, | ||
HashStable, | ||
)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was "merge_derives: false" discussed?
region_infer::values::LivenessValues, | ||
places_conflict, | ||
borrow_set::BorrowSet, facts::AllFacts, location::LocationTable, nll::ToRegionVid, | ||
places_conflict, region_infer::values::LivenessValues, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't great.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I consider this a failing of the import style -- I would prefer that we not have multi-line imports at all, as they make e.g. grepping for uses much harder (and lead to potential problems like this).
4fe589e
to
eeb0de3
Compare
@matthewjasper I agree that both of those that you noted aren't great, but I think if we block this until we're happy with the specific output it's not going to land. However, with the consideration that reverting merge_derives later is hard, but not applying it now is easy, I've enabled that option. I suspect that we'll want to shy towards defaults in the long run, but I would say that it's a rustfmt bug to merge them when it causes you to go really multiline. Force-pushed tidy fixes and the above. |
Also moves formatting to use edition 2018, and to be done in parallel. This brings near-linear speed ups (at least with a small amount of cores).
☔ The latest upstream changes (presumably #67532) made this pull request unmergeable. Please resolve the merge conflicts. |
eeb0de3
to
a06baa5
Compare
@bors r=Centril |
📌 Commit a06baa5 has been approved by |
⌛ Testing commit a06baa5 with merge 1280432f948b01b14df8ed6d00b89a3509be5489... |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
💔 Test failed - checks-azure |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
And that's cause a bunch of merge conflicts :) |
I'm really glad this happened, but a bit of warning would have been nice... |
The blog has an announcement about this PR merging, but it was published once it was fait accompli. @Mark-Simulacrum recommends using this snippet: # This should be the name of the remote for rust-lang/rust
git fetch upstream
# This rebases up to the bors merge right before formatting landed;
# it needs to be done manually.
git rebase -i 9b98af84c4aa66392236fff59c86da2130d46d46
# This rebases onto the formatting PR (given the previous command, only that).
# We tell git to resolve all conflicts in favor of your code (`-Xtheirs`),
# and the `--exec` command specifies that after each commit lands, it will be formatted.
# This command will fail if your PR has intermediary commits with syntax conflicts.
git rebase -i a916ac22b9f7f1f0f7aba0a41a789b3ecd765018 \
--exec './x.py fmt && git add -u && git commit --amend` \
# This exec is optional, and won't work if your intermediate commits don't build,
# but it helps make sure that the formatting resolution didn't introduce any errors.
# It's recommended to run it afterwards before pushing at least.
--exec './x.py check' \
-Xtheirs |
This was brought up a couple times at compiler team meetings and the long term plan has been to do it around the holidays; I agree that it would've been a good idea to publish something on internals or so a bit earlier. |
What this PR did do the Miri error printing code is quite the disaster. Previously, we consistently had a |
|
This basically means I have to add IMO this is a serious deficiency in rustfmt. (In fact I think almost all of my frustration about rustfmt stems from how it handles matches.) Cc rust-lang/rustfmt#3995 |
This PR modifies the formatting infrastructure a bit in the first commit (to enable the forgotten 2018 edition), as well as removes most directories from the ignore list in rustfmt.toml. It then follows that up with the second commit which runs
x.py fmt
and formats the entire repository.We continue to not format the test directory (
src/test
) because of interactions with pretty printing and, in part, because re-blessing all of those files is somewhat harder to review, so is best suited for a follow up PR in my opinion.Update: Instructions for resolving merge conflicts can be found in the blog post on Inside Rust. —mbrubeck