compiletest: Further clean up directive parsing by fmease · Pull Request #165651 · rust-lang/rust

fmease · 2026-11-08T16:48:32Z

See individual commits for details. Follow-up to #165576 and #165483.

r? jieyouxu

_{(No LLM was or will be used by me during the entire creation process of this PR)}

…olon}`

rustbot · 2026-11-08T16:48:36Z

compiletest directives have been modified. Please add or update docs for the
new or modified directive in src/doc/rustc-dev-guide/.

Some changes occurred in src/tools/compiletest

cc @jieyouxu

fmease · 2026-11-08T16:51:04Z

I'm sure about all changes except for the last commit ("auto-trim directive payload") because I don't know if we intentionally don't trim the payload of {cdb,lldb,gdb}-{command,check,repr}.

Please let me know if trimming there would be incorrect. I don't have access to a Windows machine to test CDB; I saw that some cdb-check directives had "intentionally indented" payloads...

I've g2g that's why I can't spend time anymore researching the exact format of debugger commands. Might do so some other time.

View changes since the review

Huh, interesting. I will double-check.

Okay I checked again, this does not matter for any of the debuggers.

The cdb ones specically have indent whitespace because that's literally what cdb outputs. So the expected output is actually copied verbatim. This is convenient to help you diff if a newer cdb changed output since you don't have to account for the whitespaces 😓

fmease · 2026-11-09T19:59:25Z

These refactorings aren't the most exciting, I wanted to do more but ran out of time last Sunday as I had to dip.

One "large" thing I want to do eventually is to avoid reparsing directives. AFAICT for the UI test suite, we always parse directives twice, first when "constructing" the test (make_test) to find & extract revisions, only-*, needs-* & ignore-*. Then we drop the DirectiveLines (instead of, e.g., storing them in CollectedTestDesc) and reparse the directives when executing the test / constructing the TestProps to handle all remaining directives.

For that DirectiveLine would need to store Strings instead of &str since the content of the file gets dropped at the end of make_test IIRC. I won't attempt this in this PR.

I could be mistaken but for the rustdoc-html test suite it seemed like we parse directives 4 times. Needs investigation.

Once we only parse directives once, it should be easier to make errors in line_directive etc. non-fatal (without reporting duplicate errors).

jieyouxu

Largely looks good, ~~I still need to check the cdb case~~.

EDIT: tested locally, this still passes cdb for me except for #165815 on 10.0.28000.2957.

@rustbot author

View changes since this review

jieyouxu · 2026-11-10T01:35:54Z

I think now

aux-build, aux-bin

parse_aux_crate, parse_proc_macros

error-pattern / regex-error-pattern (in runtest not here)

also no longer need the trims.

jieyouxu · 2026-11-10T01:36:15Z

-    /// Invariant: `self.raw_directive.starts_with(self.name)`
    pub(crate) name: &'a str,
+
+    /// The (whitespace-trimmed) playload of the directive.


Nit: payload

jieyouxu · 2026-11-10T01:36:35Z

-
-    let extra_directives = match config.mode {
+    // Inject certain directives depending on the test mode, so test writers don't need to specify
+    // then manually in every test file.


Nit: I think you meant "them" here

jieyouxu · 2026-11-10T01:37:10Z

 impl<'ln> DirectiveLine<'ln> {
-    pub(crate) fn applies_to_test_revision(&self, test_revision: Option<&str>) -> bool {
-        self.revision.is_none() || self.revision == test_revision
+    pub(crate) fn synthetic(name: &'static str, file_path: &'ln Utf8Path) -> Self {


NB: I need to think about this later, it feels quite strange using synthetic directives 🤔

Ideally we wouldn't have them. My change is just a stop-gap and removes only a single layer of the onion by no longer passing them to line_directive. This change is part of my aforementioned goal to call line_directive only once per test file -- replacing line_directive with synthetic in iter_directive is one tiny step towards it.

The real fix would be to look at what the respective directive handlers are doing and to call the handler directly in the relevant places (or rather not the "handler" itself but its body / the underlying logic, extracted into separate function that doesn't need to know anything about directives).

jieyouxu · 2026-11-10T01:38:21Z

+    let mut kind: fn(_) -> _ = DirectivePayload::Value;
+    let (name, payload) = directive
+        .split_once(|char| {
+            match char {
+                ' ' => kind = DirectivePayload::Remark,
+                ':' => {}
+                _ => return false,
+            };
+            true
+        })
+        .map(|(name, payload)| (name, Some(kind(payload.trim()))))
+        .unwrap_or((directive, None));


Nit (non-blocking): this reads a little bit clever, maybe .find([' ', ':'])? But also doesn't really matter.

No, you're right, it's a bit obfuscated.

jieyouxu · 2026-11-10T01:40:22Z

+    /// The playload of the directive.
+    ///
+    /// If the payload was separated using a colon (`:`) it will get represented as a
+    /// [`DirectivePayload::Value`]. If the payload was separated using a space, it will be
+    /// represented as a [`DirectivePayload::Remark`]. If the directive doesn't contain either colon
+    /// or space, the payload will be `None`.
+    pub(crate) payload: Option<DirectivePayload<'a>>,


I was looking at this and it occurred to me that:

//@ foo => Remark("")

//@ foo: => Value("")

I wonder if we should change //@ foo to be no payload and reject //@ foo: with no non-whitespace value 🤔 But also doesn't matter and doesn't need to be this PR.

No, that's a great observation and suggestion!

jieyouxu · 2026-11-10T01:42:23Z

 }

 #[test]
 fn test_unknown_directive_check() {


Could you also add a test case too for the whitespace trimming around value? I thought we had a test for it but I'm not finding it. For sth like

//@ directive-name: val

=> Value("val").

fmease added 7 commits November 8, 2026 17:48

compiletest: Inline fn check_directive and type CheckDirectiveResult

e47df24

compiletest: Slightly improve error handling of validate_name

9957581

compiletest: Detect trailing key-value directives

85258b0

compiletest: Don't pass synthetic directives through line_directive

eed25c5

compiletest: Separate the payload from the name in DirectiveLine

710ed9c

compiletest: Inline `DirectiveLine::{remark_after_space,value_after_c…

9917b49

…olon}`

compiletest: Auto-trim the payload of directives

5f8f169

rustbot added A-compiletest Area: The compiletest test runner A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 8, 2026

rustbot assigned jieyouxu Nov 8, 2026

rustbot added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Nov 8, 2026

fmease commented Nov 8, 2026

View reviewed changes

jieyouxu approved these changes Nov 10, 2026

View reviewed changes

rustbot added S-waiting-on-author Status: This is 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 Nov 11, 2026

Conversation

fmease commented Nov 8, 2026

Uh oh!

rustbot commented Nov 8, 2026

Uh oh!

fmease Nov 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fmease commented Nov 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fmease Nov 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jieyouxu Nov 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jieyouxu Nov 10, 2026 • edited 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

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fmease Nov 8, 2026 •

edited

Loading

fmease commented Nov 9, 2026 •

edited

Loading

jieyouxu left a comment •

edited

Loading

fmease Nov 11, 2026 •

edited

Loading

jieyouxu Nov 10, 2026 •

edited

Loading

jieyouxu Nov 10, 2026 •

edited

Loading