compiletest silently fails on a name-value directive with a known name but missing colon but does not report an error

[jieyouxu]

compiletest expects that a name-value directive, like //@ revisions: foo, to have the colon :

https://github.com/rust-lang/rust/blob/b14d8b2ef20c64c1002e2c6c724025c3d0846b91/src/tools/compiletest/src/header.rs#L1137-L1146

If the name-value directive contains a known directive name like revisions but does not have the colon (i.e. //@ revisions foo), then:

• compiletest known directive check accepts the known directive name revisions
• parse_name_value_directive expects revisions: but only got revisions, so parsing fails
• no other compiletest directive parsing rules accept revisions directive name
• no errors raised yet there is no effect (no revisions in this example).

compiletest should not silently fail here because it's very surprising and a pain to debug unless you know exactly what's wrong.

[jieyouxu]

I think an actual ui test has this problem?

https://github.com/rust-lang/rust/blob/b14d8b2ef20c64c1002e2c6c724025c3d0846b91/tests/ui/precondition-checks/cfg-ub-checks-default.rs#L2

[jieyouxu]

Wait if I change it to //@ revisions: YES NO which actually enables the compile-flags directives, this test still passes. What is this test actually checking then? Maybe it just doesn't actually need the compile-flags?

[fmease]

Wait if I change it to //@ revisions: YES NO which actually enables the compile-flags directives, this test still passes. What is this test actually checking then? Maybe it just doesn't actually need the compile-flags?

Well, the test “needs” the compile-flags because it's checking that cfg ub_checks is equivalent to cfg debug_assertions for all values of debug_assertions, i.e., yes and no (under the assumption that -Zub-checks wasn't passed which holds). If you remove the flags the test covers fewer cases.

cc #123411 &

https://github.com/rust-lang/rust/blob/b3bd7058c139e71bae0862ef8f8ac936208873e9/compiler/rustc_session/src/options.rs#L1997

[jieyouxu]

Yeah, I talked to Saethlin, makes sense. It's just compiletest silently failing here is awful 😆

[jieyouxu]

I briefly looked at this, the easy fix is trivial, but the error handling is entirely not maintainable and involves propagating a poisoned: &mut bool status bool through many callsites. The easy fix is like

I think the directive parsing is long past due for a rework. The error handling needs to not rely on passing a &mut bool around everywhere.