Skip to content
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

Update Rustfmt (add let-else support) #113225

Merged
merged 17 commits into from
Jul 1, 2023
Merged

Conversation

calebcartwright
Copy link
Member

Adds let-else formatting support

Bit more detail in: https://github.com/rust-lang/rustfmt/blob/master/CHANGELOG.md#160-2023-07-02
Accompanying blog post: rust-lang/blog.rust-lang.org#1117

I know we're getting close to tool week, however, there's been extensive discussion and testing of the changes in this between both t-style and t-rustfmt. Our confidence level is extremely high, and even if it's only on nightly for a few days, I'd still much prefer that and being able to get this out with 1.72 vs having to push to 1.73

Closes rust-lang/rustfmt#4914
cc @rust-lang/style for awareness

ytmimi and others added 17 commits June 20, 2023 08:26
The function properly handles recovering comments before and after the
`else` keyword, and properly handles how to write the else when users
configure `control_brace_style`.
This will make it easier to format the divergent blocks of `let-else`
statements since it'll be easier to prevent the block from being
formatted on a single line if the preconditions aren't met.
`rewrite_let_else_block` gives us more control over allowing the `else`
block to be formatted on a single line than
`<ast::Block as Rewrite>::rewrite`.
These test cases try to cover various edge cases. For example, comments
around the else keyword and long, unbreakable, single-line initializer
expressions, and long patterns.
This helps to prevent max width errors.
This allows users to configure the maximum length of a single line
`let-else` statements. `let-else` statements that otherwise meet the
requirements to be formatted on a single line will have their divergent
`else` block formatted over multiple lines if they exceed this length.

**Note**: `single_line_let_else_max_widt` will be introduced as a stable
configuration option.
By reversing the logic I felt that the code became a clearer. Also,
added a comment to make it clear that we need to take the trailing
semicolon for the `let-else` statement into account.
This rule wasn't explicity stated in the style guide so it was missed,
but luckily we caught it during testing.
@rustbot
Copy link
Collaborator

rustbot commented Jul 1, 2023

r? @Mark-Simulacrum

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 1, 2023
@rustbot
Copy link
Collaborator

rustbot commented Jul 1, 2023

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

@calebcartwright
Copy link
Member Author

r? @ghost

@rustbot
Copy link
Collaborator

rustbot commented Jul 1, 2023

Failed to set assignee to ghost: invalid assignee

Note: Only org members, users with write permissions, or people who have commented on the PR may be assigned.

@calebcartwright
Copy link
Member Author

r? calebcartwright

@rust-lang rust-lang deleted a comment from rustbot Jul 1, 2023
@calebcartwright
Copy link
Member Author

calebcartwright commented Jul 1, 2023

@bors r+ p=2 rollup=never

We just about always do a p 1-5 for subtree syncs, and going higher in this case to increase the chances of getting this out with the next nightly

@bors
Copy link
Contributor

bors commented Jul 1, 2023

📌 Commit 75a6675 has been approved by calebcartwright

It is now in the queue for this repository.

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 1, 2023
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jul 1, 2023
@bors
Copy link
Contributor

bors commented Jul 1, 2023

⌛ Testing commit 75a6675 with merge 6162f6f...

@bors
Copy link
Contributor

bors commented Jul 1, 2023

☀️ Test successful - checks-actions
Approved by: calebcartwright
Pushing 6162f6f to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 1, 2023
@bors bors merged commit 6162f6f into rust-lang:master Jul 1, 2023
@rustbot rustbot added this to the 1.72.0 milestone Jul 1, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (6162f6f): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.4% [-0.4%, -0.4%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -0.4% [-0.4%, -0.4%] 1

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.6% [0.4%, 1.4%] 7
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-3.1% [-3.2%, -3.0%] 2
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -0.2% [-3.2%, 1.4%] 9

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
2.2% [2.2%, 2.2%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 2.2% [2.2%, 2.2%] 1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 662.874s -> 661.654s (-0.18%)

@calebcartwright calebcartwright deleted the sync-rustfmt branch July 1, 2023 21:03
charliermarsh pushed a commit to astral-sh/ruff that referenced this pull request Jul 3, 2023
Support for `let…else` formatting was just merged to nightly
(rust-lang/rust#113225). Rerun `cargo fmt` with Rust nightly 2023-07-02
to pick this up. Followup to #939.

Signed-off-by: Anders Kaseorg <andersk@mit.edu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

let-else formatting tracking issue
7 participants