compiletest: warn if we see a // @<directive-name>

[jieyouxu]

Realized in #125808 (comment).

A test writer might accidentally write

// @ignore-cross-compile

which is parsed as a normal comment not a compiletest directive because compiletest directives require that the line starts with //@.

This typo is hard to spot: both by PR authors and the reviewers. It might not fail in PR CI -- it might not even fail in full build CI, but then explode when the conditions are just right.

We should detect these and issue at least warnings from compiletest to help test writers realize the problem.

[GuillaumeGomez]

Gonna take a look at it.

[fmease]

This should obviously skip tests/rustdoc/ and tests/rustdoc-json/ as // @ (\s@ to be precise) mark the start of {Html,Json}DocCk directives.

[jieyouxu]

Gonna take a look at it.

It might be worth waiting on some directive handling reworks (not concrete design and plan just yet unfortunately) I've been wanting to do

[jieyouxu]

This should obviously skip tests/rustdoc/ and tests/rustdoc-json/ as // @ (\s@ to be precise) mark the start of {Html,Json}DocCk directives.

Huh, many thanks for the heads up, I was not aware that we had these kind of directives lol

[ChrisDenton]

That seems confusing? Could that be changed?

[GuillaumeGomez]

yes, just need someone to do it (I can if you want?).

[fmease]

That seems confusing? Could that be changed?

Yes, it can. Coincidentally, I've started rewriting htmldocck.py from Python into Rust (for increased maintainability, extensibility and in order to finally use the latest version of XPath). See #125780.

The easiest solution would be to just register HtmlDocCk & JsonDockCk directives in compiletest and change the syntax accepted by src/tools/{html,json}docck/.

[jieyouxu]

That seems confusing? Could that be changed?

I think that is a symptom of a more general problem: historically compiletest directives used //, so test-mode/test-suite-specific directives had more syntax built upon just regular //. This is also the case for codegen tests: compiletest directives use //@ but filecheck directives (or annotations) use // again because the directives are implemented separately...
I think mir-opt tests(?) or another test suite has test suite specific directives with // yet again...

[fmease]

yes, just need someone to do it (I can if you want?).

I will definitely assign myself to the HtmlDocCk side of things given I've been working on it off and on for 3 weeks lol

[jieyouxu]

An alternative solution is for test-suite-specific directives to also use the new compiletest directive start //@: so tests/rustdoc annotations might be //@ @has which is now unambiguous.

I think this is probably more preferrable, because e.g. it allows filecheck directives to become //@ CHECK-LABEL instead of plain // CHECK-LABEL.

[fmease]

The easiest solution would be to just register HtmlDocCk & JsonDockCk directives in compiletest and change the syntax accepted by src/tools/{html,json}docck/.

The cleanest solution (long-term) would be to allow subtools to "register hooks"

[fmease]

An alternative solution is for test-suite-specific directives to also use the new compiletest directive start //@: so tests/rustdoc annotations might be //@ @has which is now unambiguous.

that would also be an option, I don't know if I like it visually lol

[jieyouxu]

Yeah, I'm not saying it's the most visually appealing thing ever :3 Having test suites being able to register suite-specific directives via hooks would be very cool, maybe ui_test supports it already? I haven't checked.

[GuillaumeGomez]

yes, just need someone to do it (I can if you want?).

I will definitely assign myself to the HtmlDocCk side of things given I've been working on it off and on for 3 weeks lol

I completely forgot you were working on it even though we talked about it recently. ><

Then I'll let you handle the html test side.

[fmease]

Btw, I don't think we should block "warn if we see a // @<directive-name>" on the hypothetical refactoring of the way we deal with (compiletest-foreign) directives from child test runners.

I'm fine with blocking this enhancement on "some directive handling reworks […] [jieyouxu has] been wanting to do" (#125813 (comment)). Namely #123765 I presume.

[jieyouxu]

Btw, I don't think we should block "warn if we see a // @<directive-name>" on the hypothetical refactoring of the way we deal with (compiletest-foreign) directives from child test runners.

I'm fine with blocking this enhancement on "some directive handling reworks […] [jieyouxu has] been wanting to do" (#125813 (comment)). Namely #123765 I presume.

To be fair, I don't want to block this on that either -- it's okay if someone wanted to improve this, the refactoring can account for this warning too.

[jieyouxu]

The progress on that is I'm trying to have a couple reading passes over compiletest to better understand what we currently do and then try to come up with how we can handle directives in a more robust way.

[workingjubilee]

An alternative solution is for test-suite-specific directives to also use the new compiletest directive start //@: so tests/rustdoc annotations might be //@ @has which is now unambiguous.

I think this is probably more preferrable, because e.g. it allows filecheck directives to become //@ CHECK-LABEL instead of plain // CHECK-LABEL.

the filecheck directives are processed by the filecheck tool itself, for the majority of cases compiletest gives no special instruction here.

[jieyouxu]

An alternative solution is for test-suite-specific directives to also use the new compiletest directive start //@: so tests/rustdoc annotations might be //@ @has which is now unambiguous.

I think this is probably more preferrable, because e.g. it allows filecheck directives to become //@ CHECK-LABEL instead of plain // CHECK-LABEL.

the filecheck directives are processed by the filecheck tool itself, for the majority of cases compiletest gives no special instruction here.

Ah good point, probably better to leave as-is then

[workingjubilee]

as stated in #125825:

fwiw despite noting the issue, I think the prefix doesn't matter too much: I reviewed FileCheck's code (as much as I understood it, anyways) and it seems FileCheck is very simple. I don't think it is aware of any comment styles itself. I think it literally just scans for the prefix on a line and assumes you won't introduce code that resembles a FileCheck directive.