compiletest: Make `DirectiveLine` responsible for name/value splitting#147288

Zalathar · 2025-10-03T03:55:41Z

Follow-up to compiletest: Pass around DirectiveLine instead of bare strings #147170.

Now that all of the directive-parsing functions have access to a DirectiveLine, we can move all of the ad-hoc name/value splitting code into DirectiveLine itself, making directive parsing simpler and more consistent.

The first commit is just moving code into a submodule, so the actual changes can be seen in the subsequent commits.

r? jieyouxu

rustbot · 2025-10-03T03:55:44Z

Some changes occurred in src/tools/compiletest

cc @jieyouxu

rustbot · 2025-10-03T03:55:48Z

jieyouxu is currently at their maximum review capacity.
They may take a while to respond.

Zalathar · 2025-10-03T04:03:48Z

Fixed soft conflict with #146166.

jieyouxu

Thanks

View changes since this review

jieyouxu · 2025-10-04T06:41:31Z

src/tools/compiletest/src/directives.rs

+        // FIXME(Zalathar): Ideally, this should raise an error if a name-only
+        // directive is followed by a colon, since that's the wrong syntax.
+        // But we would need to fix tests that rely on the current behaviour.
+        line.name == directive


jieyouxu · 2025-10-04T06:42:37Z

src/tools/compiletest/src/directives.rs

+        // FIXME(Zalathar): This silently discards directives with a matching
+        // name but no colon. Unfortunately, some directives (e.g. "pp-exact")
+        // currently rely on _not_ panicking here.
+        let value = line.value_after_colon()?;
+        debug!("{}: {}", directive, value);
+        let value = expand_variables(value.to_owned(), self);
+
+        if value.is_empty() {
+            error!("{testfile}:{line_number}: empty value for directive `{directive}`");
+            help!("expected syntax is: `{directive}: value`");
+            panic!("empty directive value detected");
        }
+
+        Some(value)


Remark: yeah, I would honestly vote for hard reject eventually.

jieyouxu · 2025-10-04T06:43:26Z

src/tools/compiletest/src/directives/cfg.rs

+    // FIXME(Zalathar): This currently allows either a space or a colon, and
+    // treats any "value" after a colon as though it were a remark.
+    // We should instead forbid the colon syntax for these directives.
+    let comment = line.remark_after_space().or_else(|| line.value_after_colon());


Remark: agreed, we should reject the colon case eventually

jieyouxu · 2025-10-04T06:44:39Z

src/tools/compiletest/src/directives/needs.rs

-    // delineate value.
    if name == "needs-target-has-atomic" {
-        let Some(rest) = rest else {
+        let Some(rest) = ln.value_after_colon() else {


Remark: hm, I suppose we don't have any tests relying on the old behavior? In any case, this is stricter so.

jieyouxu · 2025-10-04T06:45:22Z

@bors r+ rollup

jieyouxu · 2025-10-04T06:46:32Z

@bors ping

jieyouxu · 2025-10-04T06:53:08Z

@bors r+ rollup

bors · 2025-10-04T06:53:11Z

📌 Commit e73c221 has been approved by jieyouxu

It is now in the queue for this repository.

Zalathar · 2025-10-04T06:53:49Z

Yeah, I ended up doing a mixture of “same behaviour” and ”stricter behaviour” changes, depending on what was easier in context and what I could get away with.

jieyouxu · 2025-10-04T06:55:59Z

👍 If we can get away with stricter, fantastic.

compiletest: Make `DirectiveLine` responsible for name/value splitting - Follow-up to rust-lang#147170. --- Now that all of the directive-parsing functions have access to a `DirectiveLine`, we can move all of the ad-hoc name/value splitting code into `DirectiveLine` itself, making directive parsing simpler and more consistent. The first commit is just moving code into a submodule, so the actual changes can be seen in the subsequent commits. r? jieyouxu

Rollup of 14 pull requests Successful merges: - #142670 (Document fully-qualified syntax in `as`' keyword doc) - #145685 (add CloneFromCell and Cell::get_cloned) - #146330 (Bump unicode_data and printables to version 17.0.0) - #146451 (Fix atan2 inaccuracy in documentation) - #146479 (add mem::conjure_zst) - #146874 (compiler: Hint at multiple crate versions if trait impl is for wrong ADT ) - #147117 (interpret `#[used]` as `#[used(compiler)]` on illumos) - #147190 (std: `sys::net` cleanups) - #147251 (Do not assert that a change in global cache only happens when concurrent) - #147280 (Return to needs-llvm-components being info-only) - #147288 (compiletest: Make `DirectiveLine` responsible for name/value splitting) - #147309 (Add documentation about unwinding to wasm targets) - #147315 (bless autodiff batching test) - #147323 (Fix top level ui tests check in tidy) r? `@ghost` `@rustbot` modify labels: rollup

compiletest: Make `DirectiveLine` responsible for name/value splitting - Follow-up to rust-lang#147170. --- Now that all of the directive-parsing functions have access to a `DirectiveLine`, we can move all of the ad-hoc name/value splitting code into `DirectiveLine` itself, making directive parsing simpler and more consistent. The first commit is just moving code into a submodule, so the actual changes can be seen in the subsequent commits. r? jieyouxu

Rollup of 11 pull requests Successful merges: - #142670 (Document fully-qualified syntax in `as`' keyword doc) - #145685 (add CloneFromCell and Cell::get_cloned) - #146330 (Bump unicode_data and printables to version 17.0.0) - #146451 (Fix atan2 inaccuracy in documentation) - #146479 (add mem::conjure_zst) - #147117 (interpret `#[used]` as `#[used(compiler)]` on illumos) - #147190 (std: `sys::net` cleanups) - #147251 (Do not assert that a change in global cache only happens when concurrent) - #147280 (Return to needs-llvm-components being info-only) - #147288 (compiletest: Make `DirectiveLine` responsible for name/value splitting) - #147315 (bless autodiff batching test) r? `@ghost` `@rustbot` modify labels: rollup

matthiaskrgr · 2025-10-04T15:11:54Z

@bors r-
#147330 (comment)

rustbot · 2025-10-04T23:46:15Z

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

Zalathar · 2025-10-04T23:51:41Z

Linking to items in docs is always a mistake.

Anyway, let's run a few try jobs to make sure that this doesn't accidentally break anything in exotic places.

@bors try jobs=x86_64-msvc-1,i686-msvc-1,x86_64-mingw-1,test-various,armhf-gnu,aarch64-apple

compiletest: Make `DirectiveLine` responsible for name/value splitting try-job: x86_64-msvc-1 try-job: i686-msvc-1 try-job: x86_64-mingw-1 try-job: test-various try-job: armhf-gnu try-job: aarch64-apple

rust-bors · 2025-10-05T02:58:45Z

☀️ Try build successful (CI)
Build commit: a984016 (a984016f410340123a5e61af016d0f18cd5c5a3b, parent: 2cb4e7dce84fdebc0279159f1082f92b99299d87)

Zalathar · 2025-10-05T03:04:22Z

@bors r=jieyouxu

bors · 2025-10-05T03:04:25Z

📌 Commit 33c99a0 has been approved by jieyouxu

It is now in the queue for this repository.

compiletest: Make `DirectiveLine` responsible for name/value splitting - Follow-up to rust-lang#147170. --- Now that all of the directive-parsing functions have access to a `DirectiveLine`, we can move all of the ad-hoc name/value splitting code into `DirectiveLine` itself, making directive parsing simpler and more consistent. The first commit is just moving code into a submodule, so the actual changes can be seen in the subsequent commits. r? jieyouxu

Rollup of 8 pull requests Successful merges: - #143900 ([rustdoc] Correctly handle `should_panic` doctest attribute and fix `--no-run` test flag on the 2024 edition) - #147288 (compiletest: Make `DirectiveLine` responsible for name/value splitting) - #147309 (Add documentation about unwinding to wasm targets) - #147310 (Mark `PatternTypo` suggestion as maybe incorrect) - #147320 (Avoid to suggest pattern match on the similarly named in fn signature) - #147328 (Implement non-poisoning `Mutex::with_mut`, `RwLock::with` and `RwLock::with_mut`) - #147337 (Make `fmt::Write` a diagnostic item) - #147349 (Improve the advice given by panic_immediate_abort) r? `@ghost` `@rustbot` modify labels: rollup

Rollup of 7 pull requests Successful merges: - #147288 (compiletest: Make `DirectiveLine` responsible for name/value splitting) - #147309 (Add documentation about unwinding to wasm targets) - #147310 (Mark `PatternTypo` suggestion as maybe incorrect) - #147320 (Avoid to suggest pattern match on the similarly named in fn signature) - #147328 (Implement non-poisoning `Mutex::with_mut`, `RwLock::with` and `RwLock::with_mut`) - #147337 (Make `fmt::Write` a diagnostic item) - #147349 (Improve the advice given by panic_immediate_abort) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of #147288 - Zalathar:directive, r=jieyouxu compiletest: Make `DirectiveLine` responsible for name/value splitting - Follow-up to #147170. --- Now that all of the directive-parsing functions have access to a `DirectiveLine`, we can move all of the ad-hoc name/value splitting code into `DirectiveLine` itself, making directive parsing simpler and more consistent. The first commit is just moving code into a submodule, so the actual changes can be seen in the subsequent commits. r? jieyouxu

rustbot assigned jieyouxu Oct 3, 2025

Zalathar force-pushed the directive branch from 046b205 to e73c221 Compare October 3, 2025 04:03

This comment has been minimized.

Sign in to view

jieyouxu approved these changes Oct 4, 2025

View reviewed changes

jieyouxu closed this Oct 4, 2025

rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 4, 2025

jieyouxu reopened this Oct 4, 2025

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 4, 2025

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 4, 2025

Zalathar mentioned this pull request Oct 4, 2025

Rollup of 13 pull requests #147324

Closed

Zalathar mentioned this pull request Oct 4, 2025

Rollup of 14 pull requests #147326

Closed

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 4, 2025

Zalathar force-pushed the directive branch from e73c221 to 96a5544 Compare October 4, 2025 23:46

Zalathar added 3 commits October 5, 2025 10:47

Move DirectiveLine into its own submodule

757d98c

Allow easy extraction of name/value from a DirectiveLine

6783e94

Use new DirectiveLine features in directive parsing

33c99a0

Zalathar force-pushed the directive branch from 96a5544 to 33c99a0 Compare October 4, 2025 23:47

This comment has been minimized.

Sign in to view

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 5, 2025

Zalathar mentioned this pull request Oct 5, 2025

Rollup of 8 pull requests #147359

Closed

Zalathar mentioned this pull request Oct 5, 2025

Rollup of 7 pull requests #147363

Merged

bors merged commit 59982f9 into rust-lang:master Oct 5, 2025
11 checks passed

rustbot added this to the 1.92.0 milestone Oct 5, 2025

Zalathar deleted the directive branch October 5, 2025 07:51

Uh oh!

Conversation

Zalathar commented Oct 3, 2025

Uh oh!

rustbot commented Oct 3, 2025

Uh oh!

rustbot commented Oct 3, 2025

Uh oh!

This comment has been minimized.

Zalathar commented Oct 3, 2025

Uh oh!

jieyouxu 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!

jieyouxu Oct 4, 2025

Choose a reason for hiding this comment

Uh oh!

jieyouxu Oct 4, 2025

Choose a reason for hiding this comment

Uh oh!

jieyouxu Oct 4, 2025

Choose a reason for hiding this comment

Uh oh!

jieyouxu Oct 4, 2025

Choose a reason for hiding this comment

Uh oh!

jieyouxu commented Oct 4, 2025

Uh oh!

jieyouxu commented Oct 4, 2025

Uh oh!

jieyouxu commented Oct 4, 2025

Uh oh!

bors commented Oct 4, 2025

Uh oh!

Zalathar commented Oct 4, 2025

Uh oh!

jieyouxu commented Oct 4, 2025

Uh oh!

matthiaskrgr commented Oct 4, 2025

Uh oh!

rustbot commented Oct 4, 2025

Uh oh!

Zalathar commented Oct 4, 2025

Uh oh!

This comment has been minimized.

rust-bors bot commented Oct 5, 2025

Uh oh!

Zalathar commented Oct 5, 2025

Uh oh!

bors commented Oct 5, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jieyouxu left a comment •

edited by rustbot

Loading