compiletest: FileCheck-based tests conflate FileCheck prefixes with compiletest revisions

[jieyouxu]

Auto-registering revision names as check prefix is a bit sketchy, and that having to pass --allow-unused-prefix is an unfortunate side-effect of not knowing whether the test author actually wanted revision-specific check prefixes or not.

-- #134463 (comment)

With the current scheme where compiletest //@ revisions imply FileCheck prefixes of the same names, we can't distinguish between whether the author wanted to:

1. Use compiletest revisions but ONLY for compiletest directive purposes.
2. Use compiletest revisions but ONLY for filecheck prefix purposes.
3. Use compiletest revisions for BOTH purposes.

We might want to investigate splitting these orthogonal concepts and have something like //@ filecheck-prefixes: XXX register FileCheck prefixes separately.

Marked as E-hard as this will take quite a bit of investigation/design, and E-tedious as this probably involves updating a whole bunch of tests, and I don't have the bandwidth to mentor.

[Zalathar]

One of the really scary things about trying to make this change is that it can be very difficult to detect cases where the test author intended for a FileCheck directive to be checked, but it actually silently does nothing.

Imagine that after requiring opt-in for revision-based prefixes, someone used to the old behaviour writes a new test that looks like this:

//@ revisions: FOO BAR

// CHECK: (this is checked in both revisions)
// FOO: (only checked in FOO revision)
// BAR: (only checked in BAR revision)

Because the test author forgot to opt in, those FOO and BAR lines are never checked. But because of the CHECK line that is checked in all revisions by default, FileCheck doesn't complain about “no checks found” either. So the test is just silently buggy.

(I don't think this danger is a dealbreaker, but is a reason to be very careful when trying to make a change like this.)

[Zalathar]

On the flip side, one of the big advantages of not registering revision-based prefixes by default is that we would be able to stop passing the --allow-unused-prefixes flag. That would help to detect some cases of FileCheck directives not being checked due to typos or other mistakes.

[jieyouxu]

Good points. Somewhat related is: #130981 and rust-lang/compiler-team#789, that we might be able to make FileCheck issues easier to detect. One possible idea is to require FileCheck annotations NOT use plain //, a bit like the original motivation for compiletest directives no longer using // but instead use //@, to make it possible to lint on malformed/unknown compiletest directives and not produce a whole bunch of annoying false-positives in regular comments. Maybe something like

//# CHECK: xxx

since FileCheck AFAIK does not care about the comment-style/prefix.

[Zalathar]

Actually, I'll phrase my previous comment even more strongly:

To me, the main downside of the current situation is that we have to pass --allow-unused-prefixes, so that tests that don't use the auto-registered revision prefixes still pass. That takes away one of the few tools we have for detecting misuse of FileCheck.