Remove a footgun-y feature / relic of the past from the compiletest DSL

[fmease]

The compiletest DSL still features a historical remnant from the time when its directives were merely prefixed with // instead of //@ when unknown directive names weren't rejected since they could just as well be part of prose:

As an "optimization", it stops looking for directives once it stumbles upon a line which starts with either fn or mod. This is super footgun-y as it obviously leads to any seeming compiletest directives below fn and mod items getting completely ignored.

See #136403 for a practical example. As well the assembly test updated in this PR.

Blocked on #136403. (merged)

[rustbot]

r? @oli-obk

rustbot has assigned @oli-obk.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[Noratrieb]

This doesn't seem to be an optimization with a warm page cache. Maybe with a cold one.

lol

(cc @jieyouxu)

[jieyouxu]

Yes this is known, I'll double-check why we didn't remove this a bit later today

[fmease]

double-check why we didn't remove this

Prolly cuz of the long time window in which we kept trying to detect directives in non-//@ comments in order to direct people to the new syntax. If we had removed it while that was still in place, compiletest would've likely started misinterpreting a lot of normal comments as directives where it wouldn't've done so before

[jieyouxu]

Prolly cuz of the long time window in which we kept trying to detect directives in non-//@ comments in order to direct people to the new syntax. If we had removed it while that was still in place, compiletest would've likely started misinterpreting a lot of normal comments as directives where it wouldn't've done so before

It's actually very simple (since the comment says // FIXME(jieyouxu)). The reason is that the PR which removed some // detection was authored by me, and then this person @jieyouxu forgot about the follow-up (which is actually what this PR does). Oops. It was left as a follow-up because at the time I didn't do any benchmarking.

#131392 (comment)

[jieyouxu]

r? jieyouxu

[jieyouxu]

I added a temp commit to fix the test, then benchmarked locally on x86_64-unknown-linux-gnu¹:

hyperfine --runs 5 --ignore-failure "./x test tests/ui --stage 1 --force-rerun" --show-output

Before removing this "optimization" (by reverting this PR):

Build completed successfully in 0:01:16
  Time (mean ± σ):     67.854 s ±  0.822 s    [User: 918.613 s, System: 309.796 s]
  Range (min … max):   67.287 s … 69.307 s    5 runs

After removing this "optimization":

Build completed successfully in 0:01:14
  Time (mean ± σ):     67.683 s ±  0.332 s    [User: 909.790 s, System: 305.177 s]
  Range (min … max):   67.196 s … 68.064 s    5 runs

Looks good to me as well. Thanks for removing this!

Footnotes

1. This benchmark is very very coarse, I was only looking for big regressions. This obviously doesn't test cold page cache, but I don't think that's super worth to optimize for, versus actual problems of ignoring //@ in tests. ↩

[fmease]

I had to update tests/assembly/small_data_threshold.rs which needlessly/confusingly used //@ for those LLVM CHECK directives. It's good that we're rejecting it now (until "compiletest"/… uses //@ for real for FileCheck, cc #125813). Waiting on CI.

[rustbot]

Some changes occurred in src/tools/compiletest

cc @jieyouxu

[jieyouxu]

I had to update tests/assembly/small_data_threshold.rs which needlessly/confusingly used //@ for those LLVM CHECK directives. It's good that we're rejecting it now (until "compiletest"/… uses //@ for real for FileCheck, cc #125813). Waiting on CI.

Fortunately (or unfortunately) FileCheck doesn't care about the comment prefix

[fmease]

@bors r=Noratrieb,jieyouxu rollup

[bors]

📌 Commit f88f0a8 has been approved by Noratrieb,jieyouxu

It is now in the queue for this repository.