[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

1878

ArrayRef

1881

How about WithColor::error() instead?

1900

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

1878

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

1881

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

1900

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

1885–1887

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

1900

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

[jdenny]

llvm/lib/Support/FileCheck.cpp

1885–1887

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

1900

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

1885–1887

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

1885–1887

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

1878

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

1881

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.

1885–1887

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.

1900

Okay, seems fair.

[jdenny]

Addressed remaining reviewer comments.

llvm/lib/Support/FileCheck.cpp

1881

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

1885–1887

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

1885–1887

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

1885–1887

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

if (std::next(PatItr) == PatEnd ||
       // Don't waste time checking for (impossible) overlaps before that.
       MatchRanges.clear();
     }
   }
 
   return StartPos;
 }
 
-// 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,
+                             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)
+    }
+    static const Regex Validator("^[a-zA-Z0-9_-]*$");
+    if (!Validator.match(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: "