Skip to content

Commit 5cf198d

Browse files
committed
Remove path choice from x fmt and add --all option.
By default, `x fmt` formats/checks modified files. But it also lets you choose one or more paths instead. This adds significant complexity to `x fmt`. Explicit paths are specified via `WalkBuilder::add` rather than `OverrideBuilder::add`. The `ignore` library is not simple, and predicting the interactions between the two mechanisms is difficult. Here's a particularly interesting case. - You can request a path P that is excluded by the `ignore` list in the `rustfmt.toml`. E.g. `x fmt tests/ui/` or `x fmt tests/ui/bitwise.rs`. - `x fmt` will add P to the walker (via `WalkBuilder::add`), traverse it (paying no attention to the `ignore` list from the `rustfmt.toml` file, due to the different mechanism), and call `rustfmt` on every `.rs` file within it. - `rustfmt` will do nothing to those `.rs` files, because it *also* reads `rustfmt.toml` and sees that they match the `ignore` list! It took me *ages* to debug and understand this behaviour. Not good! `x fmt` even lets you name a path below the current directory. This was intended to let you do things like `x fmt std` that mirror things like `x test std`. This works by looking for `std` and finding `library/std`, and then formatting that. Unfortuantely, this motivating case now gives an error. When support was added in #107944, `library/std` was the only directory named `std`. Since then, `tests/ui/std` was added, and so `x fmt std` now gives an error. In general, explicit paths don't seem particularly useful. The only two cases `x fmt` really needs are: - format/check the files I have modified (99% of uses) - format/check all files (While respecting the `ignore` list in `rustfmt.toml`, of course.) So this commit moves to that model. `x fmt` will now give an error if given an explicit path. `x fmt` now also supports a `--all` option. (And running with `GITHUB_ACTIONS=true` also causes everything to be formatted/checked, as before.) Much simpler!
1 parent 274499d commit 5cf198d

File tree

8 files changed

+34
-60
lines changed

8 files changed

+34
-60
lines changed

src/bootstrap/src/core/build_steps/format.rs

Lines changed: 15 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -97,10 +97,21 @@ struct RustfmtConfig {
9797
ignore: Vec<String>,
9898
}
9999

100-
pub fn format(build: &Builder<'_>, check: bool, paths: &[PathBuf]) {
100+
pub fn format(build: &Builder<'_>, check: bool, all: bool, paths: &[PathBuf]) {
101+
if !paths.is_empty() {
102+
eprintln!("path arguments are not accepted");
103+
crate::exit!(1);
104+
};
101105
if build.config.dry_run() {
102106
return;
103107
}
108+
109+
// By default, we only check modified files locally to speed up runtime. Exceptions are if
110+
// `--all` is specified or we are in CI. We check all files in CI to avoid bugs in
111+
// `get_modified_rs_files` letting regressions slip through; we also care about CI time less
112+
// since this is still very fast compared to building the compiler.
113+
let all = all || CiEnv::is_ci();
114+
104115
let mut builder = ignore::types::TypesBuilder::new();
105116
builder.add_defaults();
106117
builder.select("rust");
@@ -175,11 +186,7 @@ pub fn format(build: &Builder<'_>, check: bool, paths: &[PathBuf]) {
175186
untracked_count += 1;
176187
fmt_override.add(&format!("!/{untracked_path}")).expect(untracked_path);
177188
}
178-
// Only check modified files locally to speed up runtime. We still check all files in
179-
// CI to avoid bugs in `get_modified_rs_files` letting regressions slip through; we
180-
// also care about CI time less since this is still very fast compared to building the
181-
// compiler.
182-
if !CiEnv::is_ci() && paths.is_empty() {
189+
if !all {
183190
match get_modified_rs_files(build) {
184191
Ok(Some(files)) => {
185192
if files.len() <= 10 {
@@ -233,55 +240,8 @@ pub fn format(build: &Builder<'_>, check: bool, paths: &[PathBuf]) {
233240
assert!(rustfmt_path.exists(), "{}", rustfmt_path.display());
234241
let src = build.src.clone();
235242
let (tx, rx): (SyncSender<PathBuf>, _) = std::sync::mpsc::sync_channel(128);
236-
let walker = match paths.first() {
237-
Some(first) => {
238-
let find_shortcut_candidates = |p: &PathBuf| {
239-
let mut candidates = Vec::new();
240-
for entry in
241-
WalkBuilder::new(src.clone()).max_depth(Some(3)).build().map_while(Result::ok)
242-
{
243-
if let Some(dir_name) = p.file_name() {
244-
if entry.path().is_dir() && entry.file_name() == dir_name {
245-
candidates.push(entry.into_path());
246-
}
247-
}
248-
}
249-
candidates
250-
};
251-
252-
// Only try to look for shortcut candidates for single component paths like
253-
// `std` and not for e.g. relative paths like `../library/std`.
254-
let should_look_for_shortcut_dir = |p: &PathBuf| p.components().count() == 1;
255-
256-
let mut walker = if should_look_for_shortcut_dir(first) {
257-
if let [single_candidate] = &find_shortcut_candidates(first)[..] {
258-
WalkBuilder::new(single_candidate)
259-
} else {
260-
WalkBuilder::new(first)
261-
}
262-
} else {
263-
WalkBuilder::new(src.join(first))
264-
};
265-
266-
for path in &paths[1..] {
267-
if should_look_for_shortcut_dir(path) {
268-
if let [single_candidate] = &find_shortcut_candidates(path)[..] {
269-
walker.add(single_candidate);
270-
} else {
271-
walker.add(path);
272-
}
273-
} else {
274-
walker.add(src.join(path));
275-
}
276-
}
277-
278-
walker
279-
}
280-
None => WalkBuilder::new(src.clone()),
281-
}
282-
.types(matcher)
283-
.overrides(fmt_override)
284-
.build_parallel();
243+
let walker =
244+
WalkBuilder::new(src.clone()).types(matcher).overrides(fmt_override).build_parallel();
285245

286246
// There is a lot of blocking involved in spawning a child process and reading files to format.
287247
// Spawn more processes than available concurrency to keep the CPU busy.

src/bootstrap/src/core/build_steps/test.rs

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1140,7 +1140,13 @@ HELP: to skip test's attempt to check tidiness, pass `--skip src/tools/tidy` to
11401140
);
11411141
crate::exit!(1);
11421142
}
1143-
crate::core::build_steps::format::format(builder, !builder.config.cmd.bless(), &[]);
1143+
let all = false;
1144+
crate::core::build_steps::format::format(
1145+
builder,
1146+
!builder.config.cmd.bless(),
1147+
all,
1148+
&[],
1149+
);
11441150
}
11451151

11461152
builder.info("tidy check");

src/bootstrap/src/core/config/flags.rs

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -284,8 +284,8 @@ pub enum Subcommand {
284284
name = "fmt",
285285
long_about = "\n
286286
Arguments:
287-
This subcommand optionally accepts a `--check` flag which succeeds if formatting is correct and
288-
fails if it is not. For example:
287+
This subcommand optionally accepts a `--check` flag which succeeds if
288+
formatting is correct and fails if it is not. For example:
289289
./x.py fmt
290290
./x.py fmt --check"
291291
)]
@@ -294,6 +294,10 @@ pub enum Subcommand {
294294
/// check formatting instead of applying
295295
#[arg(long)]
296296
check: bool,
297+
298+
/// apply to all appropriate files, not just those that have been modified
299+
#[arg(long)]
300+
all: bool,
297301
},
298302
#[command(aliases = ["d"], long_about = "\n
299303
Arguments:

src/bootstrap/src/lib.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -660,10 +660,11 @@ impl Build {
660660

661661
// hardcoded subcommands
662662
match &self.config.cmd {
663-
Subcommand::Format { check } => {
663+
Subcommand::Format { check, all } => {
664664
return core::build_steps::format::format(
665665
&builder::Builder::new(self),
666666
*check,
667+
*all,
667668
&self.config.paths,
668669
);
669670
}

src/etc/completions/x.py.fish

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -216,6 +216,7 @@ complete -c x.py -n "__fish_seen_subcommand_from fmt" -l llvm-profile-use -d 'us
216216
complete -c x.py -n "__fish_seen_subcommand_from fmt" -l reproducible-artifact -d 'Additional reproducible artifacts that should be added to the reproducible artifacts archive' -r
217217
complete -c x.py -n "__fish_seen_subcommand_from fmt" -l set -d 'override options in config.toml' -r -f
218218
complete -c x.py -n "__fish_seen_subcommand_from fmt" -l check -d 'check formatting instead of applying'
219+
complete -c x.py -n "__fish_seen_subcommand_from fmt" -l all -d 'apply to all appropriate files, not just those that have been modified'
219220
complete -c x.py -n "__fish_seen_subcommand_from fmt" -s v -l verbose -d 'use verbose output (-vv for very verbose)'
220221
complete -c x.py -n "__fish_seen_subcommand_from fmt" -s i -l incremental -d 'use incremental compilation'
221222
complete -c x.py -n "__fish_seen_subcommand_from fmt" -l include-default-paths -d 'include default paths in addition to the provided ones'

src/etc/completions/x.py.ps1

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -275,6 +275,7 @@ Register-ArgumentCompleter -Native -CommandName 'x.py' -ScriptBlock {
275275
[CompletionResult]::new('--reproducible-artifact', 'reproducible-artifact', [CompletionResultType]::ParameterName, 'Additional reproducible artifacts that should be added to the reproducible artifacts archive')
276276
[CompletionResult]::new('--set', 'set', [CompletionResultType]::ParameterName, 'override options in config.toml')
277277
[CompletionResult]::new('--check', 'check', [CompletionResultType]::ParameterName, 'check formatting instead of applying')
278+
[CompletionResult]::new('--all', 'all', [CompletionResultType]::ParameterName, 'apply to all appropriate files, not just those that have been modified')
278279
[CompletionResult]::new('-v', 'v', [CompletionResultType]::ParameterName, 'use verbose output (-vv for very verbose)')
279280
[CompletionResult]::new('--verbose', 'verbose', [CompletionResultType]::ParameterName, 'use verbose output (-vv for very verbose)')
280281
[CompletionResult]::new('-i', 'i', [CompletionResultType]::ParameterName, 'use incremental compilation')

src/etc/completions/x.py.sh

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1077,7 +1077,7 @@ _x.py() {
10771077
return 0
10781078
;;
10791079
x.py__fmt)
1080-
opts="-v -i -j -h --check --verbose --incremental --config --build-dir --build --host --target --exclude --skip --include-default-paths --rustc-error-format --on-fail --dry-run --dump-bootstrap-shims --stage --keep-stage --keep-stage-std --src --jobs --warnings --error-format --json-output --color --bypass-bootstrap-lock --llvm-skip-rebuild --rust-profile-generate --rust-profile-use --llvm-profile-use --llvm-profile-generate --enable-bolt-settings --skip-stage0-validation --reproducible-artifact --set --help [PATHS]... [ARGS]..."
1080+
opts="-v -i -j -h --check --all --verbose --incremental --config --build-dir --build --host --target --exclude --skip --include-default-paths --rustc-error-format --on-fail --dry-run --dump-bootstrap-shims --stage --keep-stage --keep-stage-std --src --jobs --warnings --error-format --json-output --color --bypass-bootstrap-lock --llvm-skip-rebuild --rust-profile-generate --rust-profile-use --llvm-profile-use --llvm-profile-generate --enable-bolt-settings --skip-stage0-validation --reproducible-artifact --set --help [PATHS]... [ARGS]..."
10811081
if [[ ${cur} == -* || ${COMP_CWORD} -eq 2 ]] ; then
10821082
COMPREPLY=( $(compgen -W "${opts}" -- "${cur}") )
10831083
return 0

src/etc/completions/x.py.zsh

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -271,6 +271,7 @@ _arguments "${_arguments_options[@]}" \
271271
'*--reproducible-artifact=[Additional reproducible artifacts that should be added to the reproducible artifacts archive]:REPRODUCIBLE_ARTIFACT: ' \
272272
'*--set=[override options in config.toml]:section.option=value:( )' \
273273
'--check[check formatting instead of applying]' \
274+
'--all[apply to all appropriate files, not just those that have been modified]' \
274275
'*-v[use verbose output (-vv for very verbose)]' \
275276
'*--verbose[use verbose output (-vv for very verbose)]' \
276277
'-i[use incremental compilation]' \

0 commit comments

Comments
 (0)