[PATCH] D53710: [FileCheck] Warn if a prefix is only used in LABEL checks

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added a comment.

Hi Paul and Joel, thanks for your help here, appreciate it! I thought this was
a nice little addition, low-hanging fruit, but it looks like it turns into
proper project! :-) Also from our discussions, it looks like we have not just
one but a few problems. If I try to summarise them:

1. FileCheck: it is lacking diagnostics, not revealing all sorts of problems in tests. This is a very serious issue, because with this patch it took no time to find a long list of test issue, severe and minor, see next point.
2. Test issues: perhaps we can classify tests in this way, we have:
  1. Broken tests: they don't do or test anything because the non-LABEL prefixes used in tests are "undefined": they do not occur on the FileCheck and --check-prefix command line. This can easily happen by making a typo, or refactoring of the tests, etc.
  2. Dodgy tests: looks like we have "compile-only tests". They do check the given --check-prefix, but only the CHECK-LABEL variant and not the non-LABEL prefixes.
  3. Reasonable tests: multiple RUN lines, each with multiple --check-prefixes: one prefix is used just for LABELs ("shared label prefix"), and other prefixes to test variations within each test functions.

I think the objective is to produce a diagnostic for the broken and dodgy
tests, but (obviously) don't produce false positives for the reasonable tests.

>> In https://reviews.llvm.org/D53710#1290508, @probinson wrote:
>>  This seems like a reasonable use-case to me.
> 
> Agreed, but this test case shouldn't produce the warning because there's always an active prefix that has a non-LABEL directive. Or did I miss something?

I think/hope we were talking about the same things. Paul's previous example is a
reasonable use-case, and we shouldn't produce a warning. A more minimal example, a
test that should be added to this patch, is this one:

  // RUN: FileCheck -vv -check-prefix=SHARED -check-prefix=FOO -input-file %s %s 2>&1 | FileCheck -check-prefix=DONTWARN %s
  // RUN: FileCheck -vv -check-prefix=SHARED -check-prefix=BAR -input-file %s %s 2>&1 | FileCheck -check-prefix=DONTWARN %s
  ; This is a very normal test, so we shouldn't complain about it:
  ;
  ; DONTWARN-NOT: warning: {{.*}} only occurs in a LABEL check, this is probably not what you want.
  foo
  bar
  ; SHARED-LABEL: foo
  ; FOO:          bar

But this test will fail. This is (another) example of a false positive, and that's
what we were talking about I think.

About how to continue:

I think the issues are too serious to ignore, so we should progress this somehow.
A RFC and a proposal sounds like the right approach indeed. I don't mind starting
with the RFC for example. But I will be doing this and the follow up work on the side
(I do need to progress other things as well), so I think I can definitely use some
help. Based on this wall of text (this comment), but of course properly introduced
and with some examples, I can have a RFC ready mid/end of next week. But if you
want to do this, if you e.g. have more bandwidth, can or would like to progress this 
faster, etc., please go ahead! Just let me know...


https://reviews.llvm.org/D53710