(Repost, remembering to cc llvm-dev this time)
Thanks Jon!
+1
I’ve done some grepping for a few of these gotchas in llvm/test and clang/test. Of course this isn’t looking for the check prefixes that are actually used, but CHECK would be by far the most common, so I think we can take it as a non-definitive proxy for other cases.
Gotcha A (missing colon): This has a fair number of examples in the wild. Usually the colon is just missing, although I’ve seen a few examples that had a semicolon instead of colon, which is an easy typo to make. The real problem here is coming up with a heuristic that will distinguish “things that are likely a mistake” from “things that are likely just a comment” so that the diagnostic has a reasonably low false-positive rate.
For example, in the wild I’ve seen // TEMPORARY CHECK: X which FileCheck will treat as a real directive. On the other hand we don’t want it to look at // These CHECK lines are here on purpose and diagnose that as a missing colon.
I’ve been poking at this over the past day or so and I now think the most reasonable heuristic is: If the directive is preceded by an alphanumeric (+ whitespace) or an = or ‘,’ then it’s likely a comment or a RUN: line that we don’t want to diagnose. This means we would not diagnose a missing colon in // TEMPORARY CHECK; X but that sort of case is extremely rare. We would diagnose // (TEMPORARY) CHECK; X however.
The previous heuristic we discussed was that the diagnostic is reported only if, other than whitespace, the directive is at the beginning of a line or at the beginning of a comment (for some definition of comment). It appears that your new heuristic strictly expands the cases that would be diagnosed. Is that right? Is there a specific case you saw in the test suites that is covered by your new heuristic but not by the previous heuristic?
Your new heuristic does have additional false positives in the test suites, like the following:
// NOTE: CHECK lines have been autogenerated by gen_ast_dump_json_test.py
// FIXME: CHECK-NOT is broken somehow, it doesn’t work here. Check adjacency instead.
This makes me wonder if such heuristics are going to create a frustrating FileCheck user experience when trying to write comments.
Consider what happens if we implement your heuristic in a parameterized way using new environment variables with the following defaults:
FILECHECK_COMMENT_REGEX=‘([a-zA-Z0-9]\s*|[=,])$’
FILECHECK_COMMENT_APPLY=diagnostics
So far, this is exactly your proposed heuristic except:
-
It’s slightly more to implement: just a couple of environment variables to read. Not that hard.
-
We can easily tweak the regex per test suite as we discover how this heuristic fares with the rest of check-all and with the rest of the prefixes (beyond CHECK).
-
Specific test suites that want total freedom in writing comments and are ready to protect them with something like ‘COM:’ could specify something like:
FILECHECK_COMMENT_REGEX=‘\bRUN:|\bCOM:’
FILECHECK_COMMENT_APPLY=diagnostics,directives
At some point in the future, when all test suites are compatible with 3, it could become the default (and we might then drop FILECHECK_COMMENT_APPLY). At that point:
A. Diagnostics would no longer be limited by heuristics’ shaky assumptions about intentions within comments. They know precisely what are just comments.
B. Users would know precisely where they can and cannot write the names of directives (both with and without the colon).
C. Users would have a way to comment out directives while making their intention very obvious (no more mangling).
This seems like both a short-term and long-term win for very little extra upfront effort.
Another question was whether we should require FileCheck directives to be preceded by a punctuation mark of some kind. I think that ought to be its own separate discussion.
Well, if we eventually require directive-like text to be FileCheck-commented, as described above, this question goes away.
Gotcha B (space between the directive and the colon): Some tests have this bug, so it would be worth catching.
James Henderson observed that legalizing it could help prettify some cases where we’re matching whitespace or the entire line. I don’t think it’s that valuable personally.
If we implement a reasonable diagnostic heuristic for the missing-colon case (Gotcha A), then we’ll catch this mistake in the same net.
Agreed.
Gotchas C,D (missing hyphen): I found exactly one case in the wild. I’d say the value is debatable.
(It’s a CHECKNEXT in llvm/test/CodeGen/PowerPC/testCompareslleqsi.ll if someone wants to fix it.)
Gotcha E (underscore instead of hyphen): I found 40 examples across clang/test and llvm/test. I am certain I have caught a few cases in review and pretty sure I’ve had to fix some of these that I typo’d myself. I’d say this is worth doing.
No arguments here.
Multiple suffixes: I believe there are NO multiple-suffix combinations that FileCheck currently supports. The tool should detect any multiple suffix combinations and report them as errors. Currently it looks for a limited set (basically, -NOT in combination with almost anything else), but it’s easy for someone to infer that if FileCheck doesn’t complain, then it will Do The Right Thing™ with other combinations. We should not be that user-unfriendly; we should complain about all multiple-suffix combinations.
Agreed.
Thanks.
Joel