Stricter test directives by ytmimi · Pull Request #7165 · rust-lang/rustfmt

ytmimi · 2026-10-07T04:26:21Z

I did not use an LLM to create a change in this PR.
I used an LLM to create a change in this PR, and I have explained below how it was used.

Discussed in Zulip #t-rustfmt > Stricter test directives?.

I updated the entire test suite to use //@ rustfmt-* directives when setting configuration values for tests. There's also some validation to make sure that the directives aren't malformed.

jieyouxu · 2026-10-07T09:58:21Z

Nice! I'll review this gradually over this week

matthewhughes934

A quick first look over the changes.

I was wondering: should we add any handling for any in-flight changes/open PRs that will be using the old format? Currently any old-style // rustfmt-* line will just be silently ignored (but hopefully at least result in a test failure) which I think could cause confusion

View changes since this review

matthewhughes934 · 2026-10-07T17:11:01Z

    let reader = BufReader::new(file);
-    let pattern = r"^\s*//\s*rustfmt-([^:]+):\s*(\S+)";
+    // Matches `//@ rustfmt-{name}: {value}`
+    let pattern = r"^\s*//@\s+rustfmt-(?P<name>[A-Za-z_]+)\s?:\s*(?P<value>\S+)$";


Is there anything us from being stricter here, there are currently no comments in files that need the extra optional spaces

Suggested change

let pattern = r"^\s*//@\s+rustfmt-(?P<name>[A-Za-z_]+)\s?:\s*(?P<value>\S+)$";

let pattern = r"^//@\s+rustfmt-(?P<name>[A-Za-z_]+):\s*(?P<value>\S+)$";

verified via

# no comments use a space between line start and `//` git grep --perl-regexp '^\s+//@\s+rustfmt-(?P<name>[A-Za-z_]+)\s?:\s*(?P<value>\S+)$' -- tests/ # ^ changed '*' to '+' # similarly, no comment uses a space between 'rustfmt-<name>' and then ':' git grep --perl-regexp '^\s*//@\s+rustfmt-(?P<name>[A-Za-z_]+)\s+:\s*(?P<value>\S+)$' -- tests/ # ^ changed '?' to '+'

We could probably remove the second \s?, but I see some value in allowing arbitrary whitespace before the directive. Maybe we want to set the test directive in a nested comment instead of at the top of the file.

We could probably remove the second \s?, but I see some value in allowing arbitrary whitespace before the directive. Maybe we want to set the test directive in a nested comment instead of at the top of the file.

My preference is to limit things to what we know/expect we will need, then change it in the future if needed. But I'm not opposed to leaving it as-is.

matthewhughes934 · 2026-10-07T17:29:55Z

+
            config.override_value(key, val);
+
+            if !config.was_option_set(key) {


what motivated adding this check/how can we end up in this situation?

Just an extra check to make sure that values are actually getting set as we'd expect. I don't see any harm in keeping this check, but let me know if you think otherwise.

Replace `// rustfmt-*` test config comments with `//@ rustfmt-*`.

…}: {value}` Panics if a test directive is malformed.

test authors will now get feedback when test directive: 1. are malformed 2. use invalid config option names 3. use invalid config values 4. somehow don't set configs

Make using `// rustfmt-{name}: {value}` a hard error. Also informs users that they should be using `//@ rustfmt-{name}: {value}` instead.

ytmimi · 2026-10-07T19:29:14Z

I was wondering: should we add any handling for any in-flight changes/open PRs that will be using the old format? Currently any old-style // rustfmt-* line will just be silently ignored (but hopefully at least result in a test failure) which I think could cause confusion

@matthewhughes934 Thank you for the suggestion. I added a new check that will lead to a hard error if you're using the old directive syntax.

matthewhughes934 · 2026-10-08T19:12:43Z


    for file in &files {
-        let mut config = read_config(file);
+        let mut config = read_config(file).expect("config is valid");


I think this causes confusing error messages, e.g. using //@ rustfmt-foo: true, the error printed is:

config is valid: "tests/source/doc.rs set unrecognized config option \"foo\""

Reading that: is the error telling me the config is valid?. The docs recommend using should, and I think that would work:

Suggested change

let mut config = read_config(file).expect("config is valid");

let mut config = read_config(file).expect("config should be valid");

View changes since the review

Thoughts on updating all the expect calls to say something like "invalid config" or "expected valid config"?

Thoughts on updating all the expect calls to say something like "invalid config" or "expected valid config"?

👍 sounds good to me

ytmimi · 2026-10-09T15:24:59Z

Had to change this test because it used //@

View changes since the review

ytmimi · 2026-10-09T15:27:00Z

Same as the source file. Had to change this test because it used //@

View changes since the review

ytmimi · 2026-10-09T15:32:21Z

Need to double check, but I changed this from just Bar -> struct Bar because I was seeing parse errors. I think my editor changed the line ending and that might have accidentally changed the test. Will double check this one. Same goes for the tests/source/preserves_carriage_return_for_windows.rs test and their target files.

View changes since the review

rustbot added the S-waiting-on-review Status: awaiting review from the assignee but also interested parties. label Oct 7, 2026

ytmimi assigned jieyouxu Oct 7, 2026

jieyouxu added the A-test-harness Area: rustfmt test harness label Oct 7, 2026

ytmimi changed the title ~~Stricter test directive~~ Stricter test directives Oct 7, 2026

matthewhughes934 reviewed Oct 7, 2026

View reviewed changes

ytmimi added 5 commits October 7, 2026 15:23

test: stricter test directives

2d467c7

Replace `// rustfmt-*` test config comments with `//@ rustfmt-*`.

test: read_significant_comments regex now matches `//@rustfmt-{option…

4549cdc

…}: {value}` Panics if a test directive is malformed.

test: Add more validation for //@ rustfmt- test directives

8be56b1

test authors will now get feedback when test directive: 1. are malformed 2. use invalid config option names 3. use invalid config values 4. somehow don't set configs

test: fix test cases after adding test directive validation

55f071f

test: don't silently let old test directive syntax slip through

64dca2f

Make using `// rustfmt-{name}: {value}` a hard error. Also informs users that they should be using `//@ rustfmt-{name}: {value}` instead.

ytmimi force-pushed the stricter_test_directive branch from 8417490 to 64dca2f Compare October 7, 2026 19:28

matthewhughes934 reviewed Oct 8, 2026

View reviewed changes

ytmimi commented Oct 9, 2026

View reviewed changes

	let pattern = r"^\s//@\s+rustfmt-(?P<name>[A-Za-z_]+)\s?:\s(?P<value>\S+)$";
	let pattern = r"^//@\s+rustfmt-(?P<name>[A-Za-z_]+):\s*(?P<value>\S+)$";


		config.override_value(key, val);

		if !config.was_option_set(key) {

	let mut config = read_config(file).expect("config is valid");
	let mut config = read_config(file).expect("config should be valid");

Conversation

ytmimi commented Oct 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jieyouxu commented Oct 7, 2026

Uh oh!

matthewhughes934 left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthewhughes934 Oct 7, 2026

Choose a reason for hiding this comment

Uh oh!

ytmimi Oct 7, 2026

Choose a reason for hiding this comment

Uh oh!

matthewhughes934 Oct 8, 2026

Choose a reason for hiding this comment

Uh oh!

matthewhughes934 Oct 7, 2026

Choose a reason for hiding this comment

Uh oh!

ytmimi Oct 7, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ytmimi commented Oct 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matthewhughes934 Oct 8, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ytmimi Oct 8, 2026

Choose a reason for hiding this comment

Uh oh!

matthewhughes934 Oct 9, 2026

Choose a reason for hiding this comment

Uh oh!

ytmimi Oct 9, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ytmimi Oct 9, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ytmimi Oct 9, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ytmimi commented Oct 7, 2026 •

edited

Loading

matthewhughes934 left a comment •

edited by rustbot

Loading

ytmimi commented Oct 7, 2026 •

edited

Loading

matthewhughes934 Oct 8, 2026 •

edited by rustbot

Loading

ytmimi Oct 9, 2026 •

edited by rustbot

Loading

ytmimi Oct 9, 2026 •

edited by rustbot

Loading

ytmimi Oct 9, 2026 •

edited by rustbot

Loading