Stop UI tests if an unknown revision name is specified

[tgross35]

There currently isn't anything to let you know that an invalid revision is specified in an error directive like //[foo,bar,unexpected]~. Add an error if this happens.

There might be a better place to do this, is there anywhere we validate the test file before running?

[rustbot]

r? @jieyouxu

rustbot has assigned @jieyouxu.
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

[rustbot]

Some changes occurred in src/tools/compiletest

cc @jieyouxu

[tgross35]

Hm, looks like we use this behavior to disable tests. Is there a better way to mark that specific revisions should be skipped?

A similar check should probably be added to //@ attributes

[rust-log-analyzer]

The job x86_64-gnu-llvm-17 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

#16 exporting to docker image format
#16 sending tarball 29.3s done
#16 DONE 44.9s
##[endgroup]
Setting extra environment values for docker:  --env ENABLE_GCC_CODEGEN=1 --env GCC_EXEC_PREFIX=/usr/lib/gcc/
[CI_JOB_NAME=x86_64-gnu-llvm-17]
---
sccache: Starting the server...
##[group]Configure the build
configure: processing command line
configure: 
configure: build.configure-args := ['--build=x86_64-unknown-linux-gnu', '--llvm-root=/usr/lib/llvm-17', '--enable-llvm-link-shared', '--set', 'rust.thin-lto-import-instr-limit=10', '--set', 'change-id=99999999', '--enable-verbose-configure', '--enable-sccache', '--disable-manage-submodules', '--enable-locked-deps', '--enable-cargo-native-static', '--set', 'rust.codegen-units-std=1', '--set', 'dist.compression-profile=balanced', '--dist-compression-formats=xz', '--disable-dist-src', '--release-channel=nightly', '--enable-debug-assertions', '--enable-overflow-checks', '--enable-llvm-assertions', '--set', 'rust.verify-llvm-ir', '--set', 'rust.codegen-backends=llvm,cranelift,gcc', '--set', 'llvm.static-libstdcpp', '--enable-new-symbol-mangling']
configure: target.x86_64-unknown-linux-gnu.llvm-config := /usr/lib/llvm-17/bin/llvm-config
configure: llvm.link-shared     := True
configure: rust.thin-lto-import-instr-limit := 10
configure: change-id            := 99999999
---

failures:

---- [ui] tests/ui/associated-types/bound-lifetime-constrained.rs#clause stdout ----
thread '[ui] tests/ui/associated-types/bound-lifetime-constrained.rs#clause' panicked at src/tools/compiletest/src/errors.rs:141:21:
found unexpected revision 'ok' at line 48. Expected one of [Some("func"), Some("object"), Some("clause")]

---- [ui] tests/ui/associated-types/bound-lifetime-constrained.rs#func stdout ----
---- [ui] tests/ui/associated-types/bound-lifetime-constrained.rs#func stdout ----
thread '[ui] tests/ui/associated-types/bound-lifetime-constrained.rs#func' panicked at src/tools/compiletest/src/errors.rs:141:21:
found unexpected revision 'ok' at line 48. Expected one of [Some("func"), Some("object"), Some("clause")]
---- [ui] tests/ui/associated-types/bound-lifetime-constrained.rs#object stdout ----
thread '[ui] tests/ui/associated-types/bound-lifetime-constrained.rs#object' panicked at src/tools/compiletest/src/errors.rs:141:21:
thread '[ui] tests/ui/associated-types/bound-lifetime-constrained.rs#object' panicked at src/tools/compiletest/src/errors.rs:141:21:
found unexpected revision 'ok' at line 48. Expected one of [Some("func"), Some("object"), Some("clause")]
---- [ui] tests/ui/borrowck/two-phase-activation-sharing-interference.rs#nll_target stdout ----
thread '[ui] tests/ui/borrowck/two-phase-activation-sharing-interference.rs#nll_target' panicked at src/tools/compiletest/src/errors.rs:141:21:
thread '[ui] tests/ui/borrowck/two-phase-activation-sharing-interference.rs#nll_target' panicked at src/tools/compiletest/src/errors.rs:141:21:
found unexpected revision 'lxl_beyond' at line 39. Expected one of [Some("nll_target")]
---- [ui] tests/ui/borrowck/two-phase-allow-access-during-reservation.rs#nll_target stdout ----
thread '[ui] tests/ui/borrowck/two-phase-allow-access-during-reservation.rs#nll_target' panicked at src/tools/compiletest/src/errors.rs:141:21:
thread '[ui] tests/ui/borrowck/two-phase-allow-access-during-reservation.rs#nll_target' panicked at src/tools/compiletest/src/errors.rs:141:21:
found unexpected revision 'nll_beyond' at line 31. Expected one of [Some("nll_target")]
---- [ui] tests/ui/borrowck/two-phase-reservation-sharing-interference.rs#nll_target stdout ----
thread '[ui] tests/ui/borrowck/two-phase-reservation-sharing-interference.rs#nll_target' panicked at src/tools/compiletest/src/errors.rs:141:21:
thread '[ui] tests/ui/borrowck/two-phase-reservation-sharing-interference.rs#nll_target' panicked at src/tools/compiletest/src/errors.rs:141:21:
found unexpected revision 'nll_beyond' at line 33. Expected one of [Some("nll_target")]
---- [ui] tests/ui/check-cfg/exhaustive-names-values.rs#empty_cfg stdout ----
---- [ui] tests/ui/check-cfg/exhaustive-names-values.rs#empty_cfg stdout ----
thread '[ui] tests/ui/check-cfg/exhaustive-names-values.rs#empty_cfg' panicked at src/tools/compiletest/src/errors.rs:141:21:
found unexpected revision 'empty_names_values' at line 19. Expected one of [Some("empty_cfg"), Some("feature"), Some("full")]
---- [ui] tests/ui/check-cfg/exhaustive-names-values.rs#full stdout ----
---- [ui] tests/ui/check-cfg/exhaustive-names-values.rs#full stdout ----
thread '[ui] tests/ui/check-cfg/exhaustive-names-values.rs#full' panicked at src/tools/compiletest/src/errors.rs:141:21:
found unexpected revision 'empty_names_values' at line 19. Expected one of [Some("empty_cfg"), Some("feature"), Some("full")]
---- [ui] tests/ui/check-cfg/exhaustive-names-values.rs#feature stdout ----
thread '[ui] tests/ui/check-cfg/exhaustive-names-values.rs#feature' panicked at src/tools/compiletest/src/errors.rs:141:21:
thread '[ui] tests/ui/check-cfg/exhaustive-names-values.rs#feature' panicked at src/tools/compiletest/src/errors.rs:141:21:
found unexpected revision 'empty_names_values' at line 19. Expected one of [Some("empty_cfg"), Some("feature"), Some("full")]
---- [ui] tests/ui/dyn-star/param-env-region-infer.rs#current stdout ----
---- [ui] tests/ui/dyn-star/param-env-region-infer.rs#current stdout ----
thread '[ui] tests/ui/dyn-star/param-env-region-infer.rs#current' panicked at src/tools/compiletest/src/errors.rs:141:21:
found unexpected revision 'next' at line 18. Expected one of [Some("current")]

failures:
    [ui] tests/ui/associated-types/bound-lifetime-constrained.rs#clause
    [ui] tests/ui/associated-types/bound-lifetime-constrained.rs#func

[jieyouxu]

There are mostly two places that deal with directive parsing:

1. EarlyProps directive parsing:

   rust/src/tools/compiletest/src/lib.rs

   Line 762 in 7106800

   let early_props = EarlyProps::from_file(&config, &test_path);

2. TestProps directive parsing:

   rust/src/tools/compiletest/src/runtest.rs

   Line 127 in 7106800

   let mut props = TestProps::from_file(&testpaths.file, revision, &config);

   (note that immediately after generic test prop handling there's also revision-specific test prop handling).

This is currently a mess -- both in terms of parsing / validationg / updating test props and in terms of error handling. I've been meaning to rework this a little but didn't get to it yet.

Hm, looks like we use this behavior to disable tests. Is there a better way to mark that specific revisions should be skipped?

The proper fix I would think is to either introduce a new directive, something like skip-revisions: a b c, or to change the grammar of revisions to accept something like a, !b where b is skipped.

A similar check should probably be added to //@ attributes

Yes, right now compiletest directive parsing is super lenient, but that is not user friendly -- as a test writer, I want to have helpful errors pointing out my tests have unknown revisions etc. The current way directive parsing is setup is split into (I think) three phases: early test prop parsing (for gathering aux builds and revisions), late test prop parsing (for generic non-revisioned directives) and late per-revision test prop parsing (for revisioned directives). I want to say that we really should be unifying the directive parsing, validation and config update to not have these three separate phases because it can let some malformed/unknown revision directives silently slip through.

[jieyouxu]

But validations like the one proposed on this PR ought to not be so painful and tricky to add. I feel like we should collect all the directives and error annotations in one go, then perform parsing and validations immediately after.

[jieyouxu]

I think the long term solution (before ui_test migration is viable), is to rework how directives and error annotations are parsed in compiletest to use something like https://github.com/oli-obk/ui_test/blob/c8e97af5262c7039955d6508fcf1054793f52277/src/parser.rs (but will probably need various different designs to accomondate the needs of compiletest).

[jieyouxu]

Update: we have a tidy check for unknown revisions (implemented by #124706) with an //@ unused-revision-names escape hatch, which seems like a good interim solution before compiletest's directive handling (especially revisions) are reworked. Should this PR be closed?

In any case, thank you very much for the PR, we really do have some whacky footguns in our test suites / test runner that is definitely worth fixing.

[jieyouxu]

Closing this PR since it has been addressed by a tidy check, thank you for the PR!

[tgross35]

Ah I totally forgot about this one, thank you for the update!