[FileCheck] Make invalid prefix diagnostics more precise

[jdenny]

Summary

This will prove especially helpful after D79276, which introduces
comment prefixes. Specifically, identifying whether there's a
uniqueness violation will be helpful as prefixes will be required to
be unique across both check prefixes and comment prefixes.

Also, remove a related comment about cl::list that no longer seems
relevant now that FileCheck is also a library.

[jhenderson]

llvm/lib/Support/FileCheck.cpp

1884

ArrayRef

1887

How about WithColor::error() instead?

1905

This is only used in one place. Why not just explicitly provide it there? Alternatively, should this be defined in the top-level bit of FileCheck, so that client libraries can have different default check prefixes?

[jhenderson]

llvm/lib/Support/FileCheck.cpp

1884

Also, could you name these variables more distinctly so that it's obvious which is which by name.

[jdenny]

Applied most reviewer suggestions.

[jdenny]

llvm/lib/Support/FileCheck.cpp

1887

Good idea. But should that be a separate patch that changes all FileCheck diagnostics at once?

1905

This is only used in one place. Why not just explicitly provide it there?

I changed it. It was that way because I extracted it from D79276, which still uses it in two places.

Alternatively, should this be defined in the top-level bit of FileCheck, so that client libraries can have different default check prefixes?

Perhaps so. But I feel like that's an orthogonal issue and belongs in a separate patch. What do you think?

[thopre]

llvm/lib/Support/FileCheck.cpp

1891–1893

Could you make ValidateCheckPrefix throw a DiagnosticError and print the error here?

1905

I don't think we should allow customized default directive until the need arises.

[jdenny]

llvm/lib/Support/FileCheck.cpp

1891–1893

That seems ok, but what's the motivation? Is it a preferred style, or is there a technical advantage?

1905

I agree. Let's wait until there's a specific need that cannot be handled by -check-prefix[es] or FileCheckRequest::CheckPrefixes.

[thopre]

llvm/lib/Support/FileCheck.cpp

1891–1893

Future proofing: the function could start getting used elsewhere so having the error message there makes sense. Feel free to disagree.

[jdenny]

llvm/lib/Support/FileCheck.cpp

1891–1893

So the goal is to put the diagnostic that describes the constraint in the function that checks the constraint. Makes sense to me.

Do you think it's worthwhile for ValidateCheckPrefix to be a separate function? It's so small, I'm thinking it does more to obfuscate the logic in ValidatePrefixes than to improve its readability.

[jhenderson]

llvm/lib/Support/FileCheck.cpp

1884

ArrayRef is like StringRef, immutable and basically a pointer by default, so you don't need the const & bit when using it.

1887

Yes, possibly. As most of these are new checks, that's why I brought it up, but at least one of them is moving, so that's fine.

1891–1893

Passing an Error up the stack rather than relying on booleans seems like a good idea to me too (see my lightning talk from a couple of years ago which touched on this: https://www.youtube.com/watch?v=YSEY4pg1YB0).

I think it makes sense to fold in the function, but have no strong preference, if there's a good reason not to.

1905

Okay, seems fair.

[jdenny]

Addressed remaining reviewer comments.

llvm/lib/Support/FileCheck.cpp

1887

OK, for now I'll leave that for a separate patch.

1891–1893

Passing an Error up the stack rather than relying on booleans seems like a good idea to me too (see my lightning talk from a couple of years ago which touched on this: https://www.youtube.com/watch?v=YSEY4pg1YB0).

Nice talk. Make sense. You're talking about libraries there. That means the bool return type for FileCheck::ValidateCheckPrefixes would need to change in addition to the local functions we were discussing. FileCheck::readCheckFile is another that should change. Especially because those changes would be library interface changes, I think they should go in a separate patch.

I think it makes sense to fold in the function, but have no strong preference, if there's a good reason not to.

I've folded in ValidateCheckPrefix. I think the result is easier to read, but I'm happy to try something else if people disagree.

[jhenderson]

LGTM.

llvm/lib/Support/FileCheck.cpp

1891–1893

Yeah, I am talking about libraries, although some principles (don't use bools for success/fail when error messages are available) might be more widely applicable. Anyway, not a change for this patch as you say.

[jdenny]

Thanks for the review!

llvm/lib/Support/FileCheck.cpp

1891–1893

some principles (don't use bools for success/fail when error messages are available) might be more widely applicable.

Agreed.

[thopre]

LGTM too

llvm/lib/Support/FileCheck.cpp

 }
 
 // A check prefix must contain only alphanumeric, hyphens and underscores.
 static bool ValidateCheckPrefix(StringRef CheckPrefix) {
   static const Regex Validator("^[a-zA-Z0-9_-]*$");
   return Validator.match(CheckPrefix);
 }
 
-bool FileCheck::ValidateCheckPrefixes() {
-  StringSet<> PrefixSet;
-
-  for (StringRef Prefix : Req.CheckPrefixes) {
-    // Reject empty prefixes.
-    if (Prefix.empty())
+static bool ValidatePrefixes(StringSet<> &UniquePrefixes,
+                             const ArrayRef<StringRef> &SuppliedPrefixes) {
+  for (StringRef Prefix : SuppliedPrefixes) {
+    if (Prefix.empty()) {
+      errs() << "error: supplied check prefix must not be the empty string\n";
       return false;
-
-    if (!PrefixSet.insert(Prefix).second)
+    }
+    if (!ValidateCheckPrefix(Prefix)) {
+      errs() << "error: supplied check prefix must start with a letter and "
+             << "contain only alphanumeric characters, hyphens, and "
+             << "underscores: '" << Prefix << "'\n";
       return false;
-
-    if (!ValidateCheckPrefix(Prefix))
+    }
+    if (!UniquePrefixes.insert(Prefix).second) {
+      errs() << "error: supplied check prefix must be unique among check "
+             << "prefixes: '" << Prefix << "'\n";
       return false;
     }
+  }
+  return true;
+}
 
+bool FileCheck::ValidateCheckPrefixes() {
+  StringSet<> UniquePrefixes;
+  if (!ValidatePrefixes(UniquePrefixes, Req.CheckPrefixes))
+    return false;
   return true;
 }
 
 Regex FileCheck::buildCheckPrefixRegex() {
-  // I don't think there's a way to specify an initial value for cl::list,
-  // so if nothing was specified, add the default
   if (Req.CheckPrefixes.empty()) {
     Req.CheckPrefixes.push_back("CHECK");
     Req.IsDefaultCheckPrefix = true;
   }
 
   // We already validated the contents of CheckPrefixes so just concatenate
   // them as alternatives.
   SmallString<32> PrefixRegexStr;

llvm/test/FileCheck/validate-check-prefix.txt

 // RUN: %ProtectFileCheckOutput not FileCheck -check-prefix=A! -input-file %s %s 2>&1 | FileCheck -check-prefix=BAD_PREFIX %s
 // RUN: FileCheck -check-prefix=A1a-B_c -input-file %s %s
-// RUN: %ProtectFileCheckOutput not FileCheck -check-prefix=REPEAT -check-prefix=REPEAT -input-file %s %s 2>&1 | FileCheck -check-prefix=BAD_PREFIX %s
+// RUN: %ProtectFileCheckOutput not FileCheck -check-prefix=REPEAT -check-prefix=REPEAT -input-file %s %s 2>&1 | FileCheck -check-prefix=DUPLICATE_PREFIX %s
 // RUN: %ProtectFileCheckOutput not FileCheck -check-prefix=VALID -check-prefix=A! -input-file %s %s 2>&1 | FileCheck -check-prefix=BAD_PREFIX %s
-// RUN: %ProtectFileCheckOutput not FileCheck -check-prefix= -input-file %s %s 2>&1 | FileCheck -check-prefix=BAD_PREFIX %s
+// RUN: %ProtectFileCheckOutput not FileCheck -check-prefix= -input-file %s %s 2>&1 | FileCheck -check-prefix=EMPTY_PREFIX %s
 foobar
 ; A1a-B_c: foobar
 
-; BAD_PREFIX: Supplied check-prefix is invalid! Prefixes must be
-  unique and start with a letter and contain only alphanumeric characters, hyphens and underscores
+; BAD_PREFIX: supplied check prefix must start with a letter and contain only alphanumeric characters, hyphens, and underscores: 'A!'
+
+; DUPLICATE_PREFIX: error: supplied check prefix must be unique among check prefixes: 'REPEAT'
+
+; EMPTY_PREFIX: error: supplied check prefix must not be the empty string

llvm/utils/FileCheck/FileCheck.cpp

int main(int argc, char **argv) {
   Req.NoCanonicalizeWhiteSpace = NoCanonicalizeWhiteSpace;
   Req.MatchFullLines = MatchFullLines;
   Req.IgnoreCase = IgnoreCase;
 
   if (VerboseVerbose)
     Req.Verbose = true;
 
   FileCheck FC(Req);
-  if (!FC.ValidateCheckPrefixes()) {
-    errs() << "Supplied check-prefix is invalid! Prefixes must be unique and "
-              "start with a letter and contain only alphanumeric characters, "
-              "hyphens and underscores\n";
+  if (!FC.ValidateCheckPrefixes())
     return 2;
-  }
 
   Regex PrefixRE = FC.buildCheckPrefixRegex();
   std::string REError;
   if (!PrefixRE.isValid(REError)) {
     errs() << "Unable to combine check-prefix strings into a prefix regular "
               "expression! This is likely a bug in FileCheck's verification of "
               "the check-prefix strings. Regular expression parsing failed "
               "with the following error: "