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.

[Zalathar]

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

IIRC FileCheck doesn't even know what a comment is; it's basically just doing a pure substring match (with a little bit of extra logic to prevent different prefixes from overlapping with each other). The comment is just there to make sure that the compiler doesn't see the FileCheck directives.

[tgross35]

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.

I wonder if we could request (or somebody just author) a change to LLVM filecheck so you can pass --allow-unused-prefixes=REVISION1,REVISION2 to allow specific prefixes, rather than just allowing everything.

[RalfJung]

Indeed this would be great to have; we already have tests in the repo that uses non-existent prefixes and hence accidentally don't test anything:

rust/tests/codegen/float/f16.rs

Line 147 in 57d5f86

// other-LABEL: half @f16_as_self(

Use compiletest revisions but ONLY for filecheck prefix purposes.

What are "filecheck prefixes", other than the ones used for revisions?

I wonder if we could request (or somebody just author) a change to LLVM filecheck so you can pass --allow-unused-prefixes=REVISION1,REVISION2 to allow specific prefixes, rather than just allowing everything.

That sounds like a great idea.

[jieyouxu]

What are "filecheck prefixes", other than the ones used for revisions?

TL;DR:

• LLVM FileCheck (the tool) has its own default FileCheck prefixes. Namely, COM: or RUN: for no-op comments, // CHECK: ..., // CHECK-NEXT: ... etc. (// doesn't matter).
• compiletest has an orthogonal concept of test revisions (e.g. //@ revisions: foo bar).
• In the current setup, compiletest revisions are forwarded to FileCheck as custom FileCheck prefixes (--check-prefix).
  • Example: given compiletest //@ revisions: foo bar, then this is forwarded to FileCheck as custom prefixes foo and bar, so FileCheck will recognize e.g. // foo-SAME: or // foo-NEXT: instead of the default prefix CHECK.

[RalfJung]

Clearly we want to allow specific FileCheck annotations for specific revisions. So I don't think I understand the proposed alternative where compiletest revisions are not used for FileCheck prefix alternatives. What are the "orthogonal concepts" you are talking about in the issue description? foo-SAME and foo-NEXT etc make perfect sense, I can't see any mixing of orthogonal concerns here. (The syntax is a bit too implicit for my taste, but that's a FileCheck thing and nothing we can change, I assume.)

[jieyouxu]

What are the "orthogonal concepts" you are talking about in the issue description? foo-SAME and foo-NEXT etc make perfect sense, I can't see any mixing of orthogonal concerns here.

By that I meant that you may want compiletest revisions, but don't want to forward them as custom FileCheck prefixes.

The current situation is that we have to pass --allow-unused-prefixes for tests that don't use the auto-registered custom FileCheck prefixes to pass. See the comment above: #134510 (comment).

[RalfJung]

By that I meant that you may want compiletest revisions, but don't want to forward them as custom FileCheck prefixes.

Do you have an example of where that would be useful? So far, whenever I used revisions in a codegen test, it was precisely to have slightly different FileCheck annotations. Otherwise, all we have is different compile flags or so?

compiletest revisions for ui tests also allow different ERROR annotations in the same file, specific to each revision. It would be quite strange if the same did not work in codegen tests. I don't think this is confusing at all, nor is it mixing up orthogonal concerns. The only problem here is that we can accidentally use entirely non-existent revisions without getting any error.

[jieyouxu]

Do you have an example of where that would be useful? So far, whenever I used revisions in a codegen test, it was precisely to have slightly different FileCheck annotations. Otherwise, all we have is different compile flags or so?

My memory is a bit hazy on this, I'll double check what happens if I try to remove the --allow-unused-prefix passed to FileCheck.

[RalfJung]

I assume it complains about the prefixes that are for other revisions. That makes perfect sense from a FileCheck perspective; FileCheck does not understand the "revisions" annotation after all.

But we absolutely need the ability to tie FileCheck annotations to a particular revision (to e.g. select things via only/ignore or correlate with particular compiler flags). So I don't see what decoupling compiletest revisions from FileCheck prefixes is supposed to achieve.

[jieyouxu]

Sure. This was mostly an experimental issue FWIW. I think the proper approach is instead probably to

[...] request (or somebody just author) a change to LLVM filecheck so you can pass --allow-unused-prefixes=REVISION1,REVISION2 to allow specific prefixes, rather than just allowing everything.

That way, we'd be able to keep the current compiletest revision => FileCheck custom prefix, yet be able to not have to pass unconditional --allow-unused-prefixes that allows all unused prefixes.

EDIT: e.g. example from #137142 (comment)

---- [mir-opt] tests/mir-opt/optimize_none.rs#NO-OPT stdout ----

error in revision `NO-OPT`: verification with 'FileCheck' failed
[...]
error: no check strings found with prefix 'NO-OPT:'