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

Format all the let-chains in compiler crates #116688

Merged
merged 2 commits into from
Oct 15, 2023

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Oct 13, 2023

Since rust-lang/rustfmt#5910 has landed, soon we will have support for formatting let-chains (as soon as rustfmt syncs and beta gets bumped).

This PR applies the changes from master rustfmt to rust-lang/rust eagerly, so that the next beta bump does not have to deal with a 200+ file diff and can remain concerned with other things like cfg(bootstrap) -- #113637 was a pain to land, for example, because of let-else.

I will also add this commit to the ignore list after it has landed.

The commands that were run -- I'm not great at bash-foo, but this applies rustfmt to every compiler crate, and then reverts the two crates that should probably be formatted out-of-tree.

~/rustfmt $ ls -1d ~/rust/compiler/* | xargs -I@ cargo run --bin rustfmt -- @/src/lib.rs --config-path ~/rust --edition=2021 # format all of the compiler crates
~/rust $ git checkout HEAD -- compiler/rustc_codegen_{gcc,cranelift} # revert changes to cg-gcc and cg-clif

cc @rust-lang/rustfmt
r? @WaffleLapkin or @Nilstrieb who said they may be able to review this purely mechanical PR :>

cc @Mark-Simulacrum and @petrochenkov, who had some thoughts on the order of operations with big formatting changes in #95262 (comment). I think the situation has changed since then, given that let-chains support exists on master rustfmt now, and I'm fairly confident that this formatting PR should land even if bootstrap rustfmt doesn't yet format let-chains in order to lessen the burden of the next beta bump.

@rustbot
Copy link
Collaborator

rustbot commented Oct 13, 2023

Could not assign reviewer from: WaffleLapkin.
User(s) WaffleLapkin are either the PR author, already assigned, or on vacation, and there are no other candidates.
Use r? to specify someone else to assign.

@rustbot
Copy link
Collaborator

rustbot commented Oct 13, 2023

r? @b-naber

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

@compiler-errors
Copy link
Member Author

r? Nilstrieb

@rustbot rustbot assigned Noratrieb and unassigned b-naber Oct 13, 2023
@rust-log-analyzer

This comment has been minimized.

@rustbot rustbot added A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 13, 2023
@rustbot
Copy link
Collaborator

rustbot commented Oct 13, 2023

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

rustc_macros::diagnostics was changed

cc @davidtwco, @compiler-errors, @JohnTitor, @TaKO8Ki

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes might have occurred in exhaustiveness checking

cc @Nadrieril

Some changes occurred in need_type_info.rs

cc @lcnr

@rust-log-analyzer

This comment has been minimized.

Comment on lines -24 to +25
if let Some(name) = name && name == sym::main {
if let Some(name) = name
&& name == sym::main
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not this PR responsibility, but this should probably be just == Some(sym::main)...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or even if let Some(sym::main) = name :P since sym::main is a const.

@oli-obk
Copy link
Contributor

oli-obk commented Oct 13, 2023

@bors p=42 bitrotty

@Mark-Simulacrum
Copy link
Member

Note that we format with a (pinned) nightly rustfmt toolchain, so it may make sense to hold off until rustfmt support is in nightly and then do this via x.py fmt? That'll avoid any mistakes in how you ran the formatting and generally give a better contributor experience I think...

@WaffleLapkin
Copy link
Member

@Mark-Simulacrum I don't think it makes see to hold off this. Current pinned nightly rustfmt won't change all those cases (since it ignores let chains and stuff; more the ci passing). So I don't really see how holding this off will improve anyone's experience. Additionally rustfmt is not released often, so we might need to wait a relatively long time.

On the other hand we'll have to do those changes either way, so we can just as well do them now.

Copy link
Member

@WaffleLapkin WaffleLapkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=WaffleLapkin,Nilstrieb when Mark's concern is resolved ig

if let Some(def_id) = candidate.did
&& let Some(module) = self.r.get_module(def_id)
{
Some(def_id) != self.parent_scope.module.opt_def_id()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(not this PR) this should be arg swapped

.unwrap_or_else(|| tcx.def_span(body_owner_def_id));
diag.span_note(
sp,
"\
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(not this PR problem) this line is not good

@@ -319,7 +319,8 @@ impl<'body, 'tcx> VnState<'body, 'tcx> {
}

if let Some(local) = self.try_as_local(value, location)
&& local != place.local // in case we had no projection to begin with.
&& local != place.local
// in case we had no projection to begin with.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be moved above the ==

@WaffleLapkin
Copy link
Member

Unrelated, but why does this not have any S-* label? D:

@Noratrieb Noratrieb added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 13, 2023
@Mark-Simulacrum
Copy link
Member

@bors r=WaffleLapkin,Nilstrieb

I continue to expect that waiting until this is in nightly rustfmt (all that is needed for x.py to do this) makes sense and is the easier path for future improvements than manually hacking it together. However, after speaking with @WaffleLapkin we agreed that doesn't really need to block this existing PR.

@bors
Copy link
Contributor

bors commented Oct 15, 2023

📌 Commit e805151 has been approved by WaffleLapkin,Nilstrieb

It is now in the queue for this repository.

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

bors commented Oct 15, 2023

⌛ Testing commit e805151 with merge a483969...

@klensy
Copy link
Contributor

klensy commented Oct 15, 2023

Need to update .git-blame-ignore-revs? (i'm blind, ignore this.)

@compiler-errors
Copy link
Member Author

@klensy: I can do once I know a rev will actually merge :)

@bors
Copy link
Contributor

bors commented Oct 15, 2023

☀️ Test successful - checks-actions
Approved by: WaffleLapkin,Nilstrieb
Pushing a483969 to master...

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (a483969): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

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)
1.2% [0.4%, 1.8%] 8
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 1.2% [0.4%, 1.8%] 8

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)
- - 0
Regressions ❌
(secondary)
2.2% [2.2%, 2.2%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Binary size

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

Bootstrap: 623.941s -> 625.155s (0.19%)
Artifact size: 305.50 MiB -> 305.52 MiB (0.01%)

@compiler-errors compiler-errors deleted the rustfmt-up branch October 15, 2023 22:37
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Oct 15, 2023
Rollup merge of rust-lang#116771 - compiler-errors:blame-ignore-let-chains, r=Mark-Simulacrum

Ignore let-chains formatting

Follow-up to rust-lang#116688
bors-ferrocene bot added a commit to ferrocene/ferrocene that referenced this pull request Oct 17, 2023
54: Pull upstream master 2023 10 17 r=pietroalbini a=Veykril

* rust-lang/rust#116196
* rust-lang/rust#116824
* rust-lang/rust#116822
* rust-lang/rust#116477
* rust-lang/rust#116826
* rust-lang/rust#116820
  * rust-lang/rust#116811
  * rust-lang/rust#116808
  * rust-lang/rust#116805
  * rust-lang/rust#116800
  * rust-lang/rust#116798
  * rust-lang/rust#116754
* rust-lang/rust#114370
* rust-lang/rust#116804
  * rust-lang/rust#116802
  * rust-lang/rust#116790
  * rust-lang/rust#116786
  * rust-lang/rust#116709
  * rust-lang/rust#116430
  * rust-lang/rust#116257
  * rust-lang/rust#114157
* rust-lang/rust#116731
* rust-lang/rust#116550
* rust-lang/rust#114330
* rust-lang/rust#116724
* rust-lang/rust#116782
  * rust-lang/rust#116776
  * rust-lang/rust#115955
  * rust-lang/rust#115196
* rust-lang/rust#116775
* rust-lang/rust#114589
* rust-lang/rust#113747
* rust-lang/rust#116772
  * rust-lang/rust#116771
  * rust-lang/rust#116760
  * rust-lang/rust#116755
  * rust-lang/rust#116732
  * rust-lang/rust#116522
  * rust-lang/rust#116341
  * rust-lang/rust#116172
* rust-lang/rust#110604
* rust-lang/rust#110729
* rust-lang/rust#116527
* rust-lang/rust#116688
* rust-lang/rust#116757
  * rust-lang/rust#116753
  * rust-lang/rust#116748
  * rust-lang/rust#116741
  * rust-lang/rust#116594
* rust-lang/rust#116691
* rust-lang/rust#116643
* rust-lang/rust#116683
* rust-lang/rust#116635
* rust-lang/rust#115515
* rust-lang/rust#116742
  * rust-lang/rust#116661
  * rust-lang/rust#116576
  * rust-lang/rust#116540
* rust-lang/rust#116352
* rust-lang/rust#116737
  * rust-lang/rust#116730
  * rust-lang/rust#116723
  * rust-lang/rust#116715
  * rust-lang/rust#116603
  * rust-lang/rust#116591
  * rust-lang/rust#115439
* rust-lang/rust#116264
* rust-lang/rust#116727
  * rust-lang/rust#116704
  * rust-lang/rust#116696
  * rust-lang/rust#116695
  * rust-lang/rust#116644
  * rust-lang/rust#116630
* rust-lang/rust#116728
  * rust-lang/rust#116689
  * rust-lang/rust#116679
  * rust-lang/rust#116618
  * rust-lang/rust#116577
  * rust-lang/rust#115653
* rust-lang/rust#116702
* rust-lang/rust#116015
* rust-lang/rust#115822
* rust-lang/rust#116407
* rust-lang/rust#115719
* rust-lang/rust#115524
* rust-lang/rust#116705
* rust-lang/rust#116645
* rust-lang/rust#116233
* rust-lang/rust#115108
* rust-lang/rust#116670
* rust-lang/rust#116676
* rust-lang/rust#116666



Co-authored-by: Benoît du Garreau <[email protected]>
Co-authored-by: Colin Finck <[email protected]>
Co-authored-by: Ian Jackson <[email protected]>
Co-authored-by: Joshua Liebow-Feeser <[email protected]>
Co-authored-by: León Orell Valerian Liehr <[email protected]>
Co-authored-by: Trevor Gross <[email protected]>
Co-authored-by: Evan Merlock <[email protected]>
Co-authored-by: joboet <[email protected]>
Co-authored-by: Ralf Jung <[email protected]>
Co-authored-by: DaniPopes <[email protected]>
Co-authored-by: Mark Rousskov <[email protected]>
Co-authored-by: onur-ozkan <[email protected]>
Co-authored-by: Nicholas Nethercote <[email protected]>
Co-authored-by: The 8472 <[email protected]>
Co-authored-by: Samuel Thibault <[email protected]>
Co-authored-by: reez12g <[email protected]>
Co-authored-by: Jakub Beránek <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic 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. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.