Skip to content

Keep --check non-mutating when emit_mode is set via --config - #7002

Merged
ytmimi merged 2 commits into
rust-lang:mainfrom
DoTuanAnh2k1:check-non-mutating-with-config-emit
Aug 1, 2026
Merged

Keep --check non-mutating when emit_mode is set via --config#7002
ytmimi merged 2 commits into
rust-lang:mainfrom
DoTuanAnh2k1:check-non-mutating-with-config-emit

Conversation

@DoTuanAnh2k1

@DoTuanAnh2k1 DoTuanAnh2k1 commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

--check set the emit mode to Diff in apply_to_config before the inline --config overrides were applied, so rustfmt --check --config emit_mode=files <file> clobbered that with emit_mode=files and formatted the file in place — the same combination that --emit files --check already rejects.

This moves the --check → Diff enforcement to after the inline --config overrides so it can't be clobbered, keeping --check non-mutating regardless of the supplied config. Added a regression test.

Before / after on fn main(){println!("hello");}:

  • before: --check --config emit_mode=files exits 0 and rewrites the file in place
  • after: it prints the diff, exits 1, and leaves the file unchanged (plain --emit files still writes, as expected)

Closes #6999

Disclosure: prepared with AI assistance.

@rustbot rustbot added the S-waiting-on-review Status: awaiting review from the assignee but also interested parties. label Aug 1, 2026
ytmimi
ytmimi requested changes Aug 1, 2026
View reviewed changes

@ytmimi ytmimi left a comment •
edited by rustbot
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is not the direction I'd like to go with. Maybe it wasn't clear from my earlier comment (#6999 (comment)), but I'd like rustfmt to treat this as a hard error. When passing --check from the CLI user's already can't set --emit so I'd also like to prevent them from setting --config=emit_mode={any_emit_mode}

View changes since this review

@rustbot rustbot added S-waiting-on-author Status: awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: awaiting review from the assignee but also interested parties. labels Aug 1, 2026
@rustbot

rustbot commented Aug 1, 2026

Copy link
Copy Markdown
Collaborator

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@DoTuanAnh2k1
DoTuanAnh2k1 force-pushed the check-non-mutating-with-config-emit branch from 62135a9 to 4759ee3 Compare August 1, 2026 02:57
@DoTuanAnh2k1

DoTuanAnh2k1 commented Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the steer — updated to reject the combination instead of silently overriding. --check with --config emit_mode=<mode> now errors just like --emit ... --check does, and the regression test checks that.

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: awaiting some action (such as code changes or more information) from the author. labels Aug 1, 2026
ytmimi
ytmimi requested changes Aug 1, 2026
View reviewed changes

@ytmimi ytmimi left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I know you're using AI, and I know that AI loves to leave comments, but this one isn't necessary. Let's remove it.
View changes since this review

Comment thread src/bin/main.rs Outdated
Comment on lines +633 to +636
// `--check` runs in a non-mutating mode, so just as `--emit` is rejected
// above, reject an `emit_mode` supplied through `--config` (e.g.
// `--config=emit_mode=files`), which would otherwise let a check write
// the file in place.

@ytmimi ytmimi Aug 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think this comment is necessary. The code is self explanatory

@rustbot rustbot added S-waiting-on-author Status: awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: awaiting review from the assignee but also interested parties. labels Aug 1, 2026
@DoTuanAnh2k1
Reject --config=emit_mode=... when --check is set
8f2a9c3 
`rustfmt --check --config emit_mode=files <file>` bypassed the existing `--emit files` + `--check` incompatibility check and formatted the file in place. Reject an `emit_mode` supplied through `--config` in check mode too, so `--check` stays non-mutating.
@DoTuanAnh2k1
DoTuanAnh2k1 force-pushed the check-non-mutating-with-config-emit branch from 4759ee3 to 8f2a9c3 Compare August 1, 2026 03:22
@DoTuanAnh2k1

DoTuanAnh2k1 commented Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

Removed, thanks. @rustbot ready

@rustbot rustbot added S-waiting-on-review Status: awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: awaiting some action (such as code changes or more information) from the author. labels Aug 1, 2026
ytmimi
ytmimi requested changes Aug 1, 2026
View reviewed changes

@ytmimi ytmimi left a comment •
edited by rustbot
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for applying the previous feedback. Here's the follow up change request.

View changes since this review

Comment thread src/bin/main.rs
if let Some(ref emit_str) = matches.opt_str("emit") {
if options.check {
return Err(format_err!("Invalid to use `--emit` and `--check`"));
}

@ytmimi ytmimi Aug 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For completeness I think we should also disallow --emit and --config=emit_mode, since you could set contradictory values for each, which is the same issue we're trying to fix for --check. For example, something like --emit=stdout --config=emit_mode=File.

Comment thread src/bin/main.rs Outdated
Comment on lines +840 to +844
let opts = make_opts();
let matches = opts
.parse(["--check", "--config", "emit_mode=Files"])
.unwrap();
assert!(GetOptsOptions::from_matches(&matches).is_err());

@ytmimi ytmimi Aug 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For completeness let's check all emit_mod variants.

@rustbot rustbot added S-waiting-on-author Status: awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: awaiting review from the assignee but also interested parties. labels Aug 1, 2026
@DoTuanAnh2k1

DoTuanAnh2k1 commented Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

Done — the emit_mode config check now also rejects it alongside --emit (not just --check), and the test covers all emit modes for both. @rustbot ready

@rustbot rustbot added S-waiting-on-review Status: awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: awaiting some action (such as code changes or more information) from the author. labels Aug 1, 2026
ytmimi
ytmimi approved these changes Aug 1, 2026
View reviewed changes

@ytmimi ytmimi left a comment •
edited by rustbot
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for applying the feedback and adding the --emit logic alongside the original --check issue.

View changes since this review

@ytmimi
ytmimi added this pull request to the merge queue Aug 1, 2026
Merged via the queue into rust-lang:main with commit cdbf525 Aug 1, 2026
27 checks passed
@rustbot rustbot added release-notes Needs an associated changelog entry and removed S-waiting-on-review Status: awaiting review from the assignee but also interested parties. labels Aug 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ytmimi ytmimi ytmimi approved these changes

Assignees

No one assigned

Labels

release-notes Needs an associated changelog entry

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

--check can overwrite files when --config emit_mode=files is used

3 participants

@DoTuanAnh2k1 @rustbot @ytmimi