Skip to content

Commit

Permalink
Rollup merge of #125637 - nnethercote:rustfmt-fixes, r=GuillaumeGomez
Browse files Browse the repository at this point in the history
rustfmt fixes

The `rmake.rs` entries in `rustfmt.toml` are causing major problems for `x fmt`. This PR removes them and does some minor related cleanups.

r? ``@GuillaumeGomez``
  • Loading branch information
matthiaskrgr committed May 28, 2024
2 parents de2bf36 + f1b0ca0 commit faabc74
Show file tree
Hide file tree
Showing 5 changed files with 37 additions and 40 deletions.
6 changes: 2 additions & 4 deletions compiler/rustc_mir_transform/src/instsimplify.rs
Original file line number Diff line number Diff line change
Expand Up @@ -219,10 +219,8 @@ impl<'tcx> InstSimplifyContext<'tcx, '_> {
for (i, field) in variant.fields.iter_enumerated() {
let field_ty = field.ty(self.tcx, args);
if field_ty == *cast_ty {
let place = place.project_deeper(
&[ProjectionElem::Field(i, *cast_ty)],
self.tcx,
);
let place = place
.project_deeper(&[ProjectionElem::Field(i, *cast_ty)], self.tcx);
let operand = if operand.is_move() {
Operand::Move(place)
} else {
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_mir_transform/src/validate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -691,7 +691,7 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> {
location,
format!(
"You can't project to field {f:?} of `DynMetadata` because \
layout is weird and thinks it doesn't have fields."
layout is weird and thinks it doesn't have fields."
),
);
}
Expand Down
8 changes: 2 additions & 6 deletions library/core/src/ptr/metadata.rs
Original file line number Diff line number Diff line change
Expand Up @@ -209,18 +209,14 @@ impl<Dyn: ?Sized> DynMetadata<Dyn> {
// Consider a reference like `&(i32, dyn Send)`: the vtable will only store the size of the
// `Send` part!
// SAFETY: DynMetadata always contains a valid vtable pointer
return unsafe {
crate::intrinsics::vtable_size(self.vtable_ptr() as *const ())
};
return unsafe { crate::intrinsics::vtable_size(self.vtable_ptr() as *const ()) };
}

/// Returns the alignment of the type associated with this vtable.
#[inline]
pub fn align_of(self) -> usize {
// SAFETY: DynMetadata always contains a valid vtable pointer
return unsafe {
crate::intrinsics::vtable_align(self.vtable_ptr() as *const ())
};
return unsafe { crate::intrinsics::vtable_align(self.vtable_ptr() as *const ()) };
}

/// Returns the size and alignment together as a `Layout`
Expand Down
19 changes: 6 additions & 13 deletions rustfmt.toml
Original file line number Diff line number Diff line change
Expand Up @@ -3,25 +3,18 @@ version = "Two"
use_small_heuristics = "Max"
merge_derives = false

# by default we ignore everything in the repository
# tidy only checks files which are not ignored, each entry follows gitignore style
# Files to ignore. Each entry uses gitignore syntax, but `!` prefixes aren't allowed.
ignore = [
"/build/",
"/*-build/",
"/build-*/",
"/vendor/",

# tests for now are not formatted, as they are sometimes pretty-printing constrained
# (and generally rustfmt can move around comments in UI-testing incompatible ways)
"/tests/*",
# Tests for now are not formatted, as they are sometimes pretty-printing constrained
# (and generally rustfmt can move around comments in UI-testing incompatible ways).
"/tests/",

# but we still want to format rmake.rs files in tests/run-make/ so we need to do this
# dance to avoid the parent directory from being excluded
"!/tests/run-make/",
"/tests/run-make/*/*.rs",
"!/tests/run-make/*/rmake.rs",

# do not format submodules
# Do not format submodules.
# FIXME: sync submodule list with tidy/bootstrap/etc
# tidy/src/walk.rs:filter_dirs
"library/backtrace",
Expand All @@ -43,7 +36,7 @@ ignore = [
"src/tools/rustc-perf",
"src/tools/rustfmt",

# these are ignored by a standard cargo fmt run
# These are ignored by a standard cargo fmt run.
"compiler/rustc_codegen_cranelift/scripts",
"compiler/rustc_codegen_cranelift/example/gen_block_iterate.rs", # uses edition 2024
]
42 changes: 26 additions & 16 deletions src/bootstrap/src/core/build_steps/format.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@ use std::sync::mpsc::SyncSender;

fn rustfmt(src: &Path, rustfmt: &Path, paths: &[PathBuf], check: bool) -> impl FnMut(bool) -> bool {
let mut cmd = Command::new(rustfmt);
// avoid the submodule config paths from coming into play,
// we only allow a single global config for the workspace for now
// Avoid the submodule config paths from coming into play. We only allow a single global config
// for the workspace for now.
cmd.arg("--config-path").arg(&src.canonicalize().unwrap());
cmd.arg("--edition").arg("2021");
cmd.arg("--unstable-features");
Expand All @@ -24,7 +24,7 @@ fn rustfmt(src: &Path, rustfmt: &Path, paths: &[PathBuf], check: bool) -> impl F
cmd.args(paths);
let cmd_debug = format!("{cmd:?}");
let mut cmd = cmd.spawn().expect("running rustfmt");
// poor man's async: return a closure that'll wait for rustfmt's completion
// Poor man's async: return a closure that'll wait for rustfmt's completion.
move |block: bool| -> bool {
if !block {
match cmd.try_wait() {
Expand Down Expand Up @@ -72,7 +72,7 @@ fn verify_rustfmt_version(build: &Builder<'_>) -> bool {
!program_out_of_date(&stamp_file, &version)
}

/// Updates the last rustfmt version used
/// Updates the last rustfmt version used.
fn update_rustfmt_version(build: &Builder<'_>) {
let Some((version, stamp_file)) = get_rustfmt_version(build) else {
return;
Expand Down Expand Up @@ -115,8 +115,15 @@ pub fn format(build: &Builder<'_>, check: bool, paths: &[PathBuf]) {
let rustfmt_config: RustfmtConfig = t!(toml::from_str(&rustfmt_config));
let mut fmt_override = ignore::overrides::OverrideBuilder::new(&build.src);
for ignore in rustfmt_config.ignore {
if let Some(ignore) = ignore.strip_prefix('!') {
fmt_override.add(ignore).expect(ignore);
if ignore.starts_with('!') {
// A `!`-prefixed entry could be added as a whitelisted entry in `fmt_override`, i.e.
// strip the `!` prefix. But as soon as whitelisted entries are added, an
// `OverrideBuilder` will only traverse those whitelisted entries, and won't traverse
// any files that aren't explicitly mentioned. No bueno! Maybe there's a way to combine
// explicit whitelisted entries and traversal of unmentioned files, but for now just
// forbid such entries.
eprintln!("`!`-prefixed entries are not supported in rustfmt.toml, sorry");
crate::exit!(1);
} else {
fmt_override.add(&format!("!{ignore}")).expect(&ignore);
}
Expand Down Expand Up @@ -168,9 +175,10 @@ pub fn format(build: &Builder<'_>, check: bool, paths: &[PathBuf]) {
untracked_count += 1;
fmt_override.add(&format!("!/{untracked_path}")).expect(untracked_path);
}
// Only check modified files locally to speed up runtime.
// We still check all files in CI to avoid bugs in `get_modified_rs_files` letting regressions slip through;
// we also care about CI time less since this is still very fast compared to building the compiler.
// Only check modified files locally to speed up runtime. We still check all files in
// CI to avoid bugs in `get_modified_rs_files` letting regressions slip through; we
// also care about CI time less since this is still very fast compared to building the
// compiler.
if !CiEnv::is_ci() && paths.is_empty() {
match get_modified_rs_files(build) {
Ok(Some(files)) => {
Expand Down Expand Up @@ -275,21 +283,23 @@ pub fn format(build: &Builder<'_>, check: bool, paths: &[PathBuf]) {
.overrides(fmt_override)
.build_parallel();

// there is a lot of blocking involved in spawning a child process and reading files to format.
// spawn more processes than available concurrency to keep the CPU busy
// There is a lot of blocking involved in spawning a child process and reading files to format.
// Spawn more processes than available concurrency to keep the CPU busy.
let max_processes = build.jobs() as usize * 2;

// spawn child processes on a separate thread so we can batch entries we have received from ignore
// Spawn child processes on a separate thread so we can batch entries we have received from
// ignore.
let thread = std::thread::spawn(move || {
let mut children = VecDeque::new();
while let Ok(path) = rx.recv() {
// try getting more paths from the channel to amortize the overhead of spawning processes
// Try getting more paths from the channel to amortize the overhead of spawning
// processes.
let paths: Vec<_> = rx.try_iter().take(63).chain(std::iter::once(path)).collect();

let child = rustfmt(&src, &rustfmt_path, paths.as_slice(), check);
children.push_back(child);

// poll completion before waiting
// Poll completion before waiting.
for i in (0..children.len()).rev() {
if children[i](false) {
children.swap_remove_back(i);
Expand All @@ -298,12 +308,12 @@ pub fn format(build: &Builder<'_>, check: bool, paths: &[PathBuf]) {
}

if children.len() >= max_processes {
// await oldest child
// Await oldest child.
children.pop_front().unwrap()(true);
}
}

// await remaining children
// Await remaining children.
for mut child in children {
child(true);
}
Expand Down

0 comments on commit faabc74

Please sign in to comment.