[verify] Improve the error messages with multiple active prefixes (#126068)

Multiple improvements to make the messages more concrete, actionable and
less confusing when multiple prefixes are used in `-verify=`. The common
theme among these was that prior to the patch all error messages would
use the alphabetically first prefix, even if the error was associated
with a different one.

- Mention the actual expected but unseen directive: Prior to this change
when reporting expected but unseen directive, the alphabetically first
one would be used to report the error even if that's not the one present
in the source. Reword the diagnostic if multiple prefixes are active and
include the real spelling of the expected directive for each expected
but not seen line in the output.

- Reword the seen but not expected error message if multiple directives
are active to avoid having to pick an arbitrary (the first) prefix for
it.

- Include the full spelling of the directive when reporting a directive
following the no-diagnostics directive. For example "'foo-error'
directive cannot follow 'foo-no-diagnostics' directive"

- Use the first appearing `-no-diagnostics` directive, in the above
message instead of the first one alphabetically.

The new wording
> diagnostics with '(error|warning|remark|note)' severity seen but not
expected

instead of

> '<prefix>-(error|warning|remark|note)' diagnostics seen but not
expected

is only used when multiple prefixes are present, the error messages stay
the same with a single prefix only.

Author: Maetveis

clang/include/clang/Basic/DiagnosticFrontendKinds.td

@@ -175,11 +175,11 @@ def err_verify_invalid_content : Error<
 def err_verify_missing_regex : Error<
     "cannot find start of regex ('{{') in %0">;
 def err_verify_inconsistent_diags : Error<
-    "'%0' diagnostics %select{expected|seen}1 but not %select{seen|expected}1: "
-    "%2">;
+    "%select{|'%1-%2' }0diagnostics %select{with '%2' severity |}0"
+    "%select{expected|seen}3 but not %select{seen|expected}3: "
+    "%4">;
 def err_verify_invalid_no_diags : Error<
-    "%select{expected|'%0-no-diagnostics'}1 directive cannot follow "
-    "%select{'%0-no-diagnostics' directive|other expected directives}1">;
+    "'%0' directive cannot follow %select{'%2' directive|other expected directives}1">;
 def err_verify_no_directives : Error<
     "no expected directives found: consider use of '%0-no-diagnostics'">;
 def err_verify_nonconst_addrspace : Error<

clang/include/clang/Frontend/VerifyDiagnosticConsumer.h

@@ -44,15 +44,17 @@ class VerifyDiagnosticConsumer: public DiagnosticConsumer,
   public:
     static std::unique_ptr<Directive>
     create(bool RegexKind, SourceLocation DirectiveLoc,
-           SourceLocation DiagnosticLoc, bool MatchAnyFileAndLine,
-           bool MatchAnyLine, StringRef Text, unsigned Min, unsigned Max);
+           SourceLocation DiagnosticLoc, StringRef Spelling,
+           bool MatchAnyFileAndLine, bool MatchAnyLine, StringRef Text,
+           unsigned Min, unsigned Max);
 
   public:
     /// Constant representing n or more matches.
     static const unsigned MaxCount = std::numeric_limits<unsigned>::max();
 
     SourceLocation DirectiveLoc;
     SourceLocation DiagnosticLoc;
+    const std::string Spelling;
     const std::string Text;
     unsigned Min, Max;
     bool MatchAnyLine;
@@ -71,10 +73,11 @@ class VerifyDiagnosticConsumer: public DiagnosticConsumer,
 
   protected:
     Directive(SourceLocation DirectiveLoc, SourceLocation DiagnosticLoc,
-              bool MatchAnyFileAndLine, bool MatchAnyLine, StringRef Text,
-              unsigned Min, unsigned Max)
-        : DirectiveLoc(DirectiveLoc), DiagnosticLoc(DiagnosticLoc), Text(Text),
-          Min(Min), Max(Max), MatchAnyLine(MatchAnyLine || MatchAnyFileAndLine),
+              StringRef Spelling, bool MatchAnyFileAndLine, bool MatchAnyLine,
+              StringRef Text, unsigned Min, unsigned Max)
+        : DirectiveLoc(DirectiveLoc), DiagnosticLoc(DiagnosticLoc),
+          Spelling(Spelling), Text(Text), Min(Min), Max(Max),
+          MatchAnyLine(MatchAnyLine || MatchAnyFileAndLine),
           MatchAnyFileAndLine(MatchAnyFileAndLine) {
       assert(!DirectiveLoc.isInvalid() && "DirectiveLoc is invalid!");
       assert((!DiagnosticLoc.isInvalid() || MatchAnyLine) &&
@@ -106,6 +109,11 @@ class VerifyDiagnosticConsumer: public DiagnosticConsumer,
     HasOtherExpectedDirectives
   };
 
+  struct ParsingState {
+    DirectiveStatus Status;
+    std::string FirstNoDiagnosticsDirective;
+  };
+
   class MarkerTracker;
 
 private:
@@ -118,7 +126,7 @@ class VerifyDiagnosticConsumer: public DiagnosticConsumer,
   const LangOptions *LangOpts = nullptr;
   SourceManager *SrcManager = nullptr;
   unsigned ActiveSourceFiles = 0;
-  DirectiveStatus Status;
+  ParsingState State;
   ExpectedData ED;
 
   void CheckDiagnostics();

clang/lib/Frontend/VerifyDiagnosticConsumer.cpp

@@ -89,9 +89,10 @@ namespace {
 class StandardDirective : public Directive {
 public:
   StandardDirective(SourceLocation DirectiveLoc, SourceLocation DiagnosticLoc,
-                    bool MatchAnyFileAndLine, bool MatchAnyLine, StringRef Text,
-                    unsigned Min, unsigned Max)
-      : Directive(DirectiveLoc, DiagnosticLoc, MatchAnyFileAndLine,
+                    StringRef Spelling, bool MatchAnyFileAndLine,
+                    bool MatchAnyLine, StringRef Text, unsigned Min,
+                    unsigned Max)
+      : Directive(DirectiveLoc, DiagnosticLoc, Spelling, MatchAnyFileAndLine,
                   MatchAnyLine, Text, Min, Max) {}
 
   bool isValid(std::string &Error) override {
@@ -106,9 +107,10 @@ class StandardDirective : public Directive {
 class RegexDirective : public Directive {
 public:
   RegexDirective(SourceLocation DirectiveLoc, SourceLocation DiagnosticLoc,
-                 bool MatchAnyFileAndLine, bool MatchAnyLine, StringRef Text,
-                 unsigned Min, unsigned Max, StringRef RegexStr)
-      : Directive(DirectiveLoc, DiagnosticLoc, MatchAnyFileAndLine,
+                 StringRef Spelling, bool MatchAnyFileAndLine,
+                 bool MatchAnyLine, StringRef Text, unsigned Min, unsigned Max,
+                 StringRef RegexStr)
+      : Directive(DirectiveLoc, DiagnosticLoc, Spelling, MatchAnyFileAndLine,
                   MatchAnyLine, Text, Min, Max),
         Regex(RegexStr) {}
 
@@ -285,6 +287,7 @@ class ParseHelper
 // The information necessary to create a directive.
 struct UnattachedDirective {
   DirectiveList *DL = nullptr;
+  std::string Spelling;
   bool RegexKind = false;
   SourceLocation DirectivePos, ContentBegin;
   std::string Text;
@@ -299,8 +302,8 @@ void attachDirective(DiagnosticsEngine &Diags, const UnattachedDirective &UD,
                      bool MatchAnyLine = false) {
   // Construct new directive.
   std::unique_ptr<Directive> D = Directive::create(
-      UD.RegexKind, UD.DirectivePos, ExpectedLoc, MatchAnyFileAndLine,
-      MatchAnyLine, UD.Text, UD.Min, UD.Max);
+      UD.RegexKind, UD.DirectivePos, ExpectedLoc, UD.Spelling,
+      MatchAnyFileAndLine, MatchAnyLine, UD.Text, UD.Min, UD.Max);
 
   std::string Error;
   if (!D->isValid(Error)) {
@@ -408,7 +411,7 @@ static std::string DetailedErrorString(const DiagnosticsEngine &Diags) {
 /// Returns true if any valid directives were found.
 static bool ParseDirective(StringRef S, ExpectedData *ED, SourceManager &SM,
                            Preprocessor *PP, SourceLocation Pos,
-                           VerifyDiagnosticConsumer::DirectiveStatus &Status,
+                           VerifyDiagnosticConsumer::ParsingState &State,
                            VerifyDiagnosticConsumer::MarkerTracker &Markers) {
   DiagnosticsEngine &Diags = PP ? PP->getDiagnostics() : SM.getDiagnostics();
 
@@ -440,8 +443,9 @@ static bool ParseDirective(StringRef S, ExpectedData *ED, SourceManager &SM,
     StringRef DToken = PH.Match();
     PH.Advance();
 
-    // Default directive kind.
     UnattachedDirective D;
+    D.Spelling = DToken;
+    // Default directive kind.
     const char *KindStr = "string";
 
     // Parse the initial directive token in reverse so we can easily determine
@@ -482,19 +486,24 @@ static bool ParseDirective(StringRef S, ExpectedData *ED, SourceManager &SM,
       continue;
 
     if (NoDiag) {
-      if (Status == VerifyDiagnosticConsumer::HasOtherExpectedDirectives)
+      if (State.Status ==
+          VerifyDiagnosticConsumer::HasOtherExpectedDirectives) {
         Diags.Report(Pos, diag::err_verify_invalid_no_diags)
-            << DetailedErrorString(Diags) << /*IsExpectedNoDiagnostics=*/true;
-      else
-        Status = VerifyDiagnosticConsumer::HasExpectedNoDiagnostics;
+            << D.Spelling << /*IsExpectedNoDiagnostics=*/true;
+      } else if (State.Status !=
+                 VerifyDiagnosticConsumer::HasExpectedNoDiagnostics) {
+        State.Status = VerifyDiagnosticConsumer::HasExpectedNoDiagnostics;
+        State.FirstNoDiagnosticsDirective = D.Spelling;
+      }
       continue;
     }
-    if (Status == VerifyDiagnosticConsumer::HasExpectedNoDiagnostics) {
+    if (State.Status == VerifyDiagnosticConsumer::HasExpectedNoDiagnostics) {
       Diags.Report(Pos, diag::err_verify_invalid_no_diags)
-          << DetailedErrorString(Diags) << /*IsExpectedNoDiagnostics=*/false;
+          << D.Spelling << /*IsExpectedNoDiagnostics=*/false
+          << State.FirstNoDiagnosticsDirective;
       continue;
     }
-    Status = VerifyDiagnosticConsumer::HasOtherExpectedDirectives;
+    State.Status = VerifyDiagnosticConsumer::HasOtherExpectedDirectives;
 
     // If a directive has been found but we're not interested
     // in storing the directive information, return now.
@@ -670,7 +679,7 @@ VerifyDiagnosticConsumer::VerifyDiagnosticConsumer(DiagnosticsEngine &Diags_)
     : Diags(Diags_), PrimaryClient(Diags.getClient()),
       PrimaryClientOwner(Diags.takeClient()),
       Buffer(new TextDiagnosticBuffer()), Markers(new MarkerTracker(Diags)),
-      Status(HasNoDirectives) {
+      State{HasNoDirectives, {}} {
   if (Diags.hasSourceManager())
     setSourceManager(Diags.getSourceManager());
 }
@@ -788,7 +797,7 @@ bool VerifyDiagnosticConsumer::HandleComment(Preprocessor &PP,
   // Fold any "\<EOL>" sequences
   size_t loc = C.find('\\');
   if (loc == StringRef::npos) {
-    ParseDirective(C, &ED, SM, &PP, CommentBegin, Status, *Markers);
+    ParseDirective(C, &ED, SM, &PP, CommentBegin, State, *Markers);
     return false;
   }
 
@@ -818,7 +827,7 @@ bool VerifyDiagnosticConsumer::HandleComment(Preprocessor &PP,
   }
 
   if (!C2.empty())
-    ParseDirective(C2, &ED, SM, &PP, CommentBegin, Status, *Markers);
+    ParseDirective(C2, &ED, SM, &PP, CommentBegin, State, *Markers);
   return false;
 }
 
@@ -843,8 +852,8 @@ static bool findDirectives(SourceManager &SM, FileID FID,
 
   Token Tok;
   Tok.setKind(tok::comment);
-  VerifyDiagnosticConsumer::DirectiveStatus Status =
-    VerifyDiagnosticConsumer::HasNoDirectives;
+  VerifyDiagnosticConsumer::ParsingState State = {
+      VerifyDiagnosticConsumer::HasNoDirectives, {}};
   while (Tok.isNot(tok::eof)) {
     RawLex.LexFromRawLexer(Tok);
     if (!Tok.is(tok::comment)) continue;
@@ -856,8 +865,8 @@ static bool findDirectives(SourceManager &SM, FileID FID,
     VerifyDiagnosticConsumer::MarkerTracker Markers(SM.getDiagnostics());
 
     // Find first directive.
-    if (ParseDirective(Comment, nullptr, SM, nullptr, Tok.getLocation(),
-                       Status, Markers))
+    if (ParseDirective(Comment, nullptr, SM, nullptr, Tok.getLocation(), State,
+                       Markers))
       return true;
   }
   return false;
@@ -887,10 +896,11 @@ static unsigned PrintUnexpected(DiagnosticsEngine &Diags, SourceManager *SourceM
     OS << ": " << I->second;
   }
 
+  const bool IsSinglePrefix =
+      Diags.getDiagnosticOptions().VerifyPrefixes.size() == 1;
   std::string Prefix = *Diags.getDiagnosticOptions().VerifyPrefixes.begin();
-  std::string KindStr = Prefix + "-" + Kind;
   Diags.Report(diag::err_verify_inconsistent_diags).setForceEmit()
-      << KindStr << /*Unexpected=*/true << OS.str();
+      << IsSinglePrefix << Prefix << Kind << /*Unexpected=*/true << OS.str();
   return std::distance(diag_begin, diag_end);
 }
 
@@ -902,6 +912,9 @@ static unsigned PrintExpected(DiagnosticsEngine &Diags,
   if (DL.empty())
     return 0;
 
+  const bool IsSinglePrefix =
+      Diags.getDiagnosticOptions().VerifyPrefixes.size() == 1;
+
   SmallString<256> Fmt;
   llvm::raw_svector_ostream OS(Fmt);
   for (const auto *D : DL) {
@@ -917,13 +930,14 @@ static unsigned PrintExpected(DiagnosticsEngine &Diags,
       OS << " (directive at "
          << SourceMgr.getFilename(D->DirectiveLoc) << ':'
          << SourceMgr.getPresumedLineNumber(D->DirectiveLoc) << ')';
+    if (!IsSinglePrefix)
+      OS << " \'" << D->Spelling << '\'';
     OS << ": " << D->Text;
   }
 
   std::string Prefix = *Diags.getDiagnosticOptions().VerifyPrefixes.begin();
-  std::string KindStr = Prefix + "-" + Kind;
   Diags.Report(diag::err_verify_inconsistent_diags).setForceEmit()
-      << KindStr << /*Unexpected=*/false << OS.str();
+      << IsSinglePrefix << Prefix << Kind << /*Unexpected=*/false << OS.str();
   return DL.size();
 }
 
@@ -1109,11 +1123,11 @@ void VerifyDiagnosticConsumer::CheckDiagnostics() {
   if (SrcManager) {
     // Produce an error if no expected-* directives could be found in the
     // source file(s) processed.
-    if (Status == HasNoDirectives) {
+    if (State.Status == HasNoDirectives) {
       Diags.Report(diag::err_verify_no_directives).setForceEmit()
           << DetailedErrorString(Diags);
       ++NumErrors;
-      Status = HasNoDirectivesReported;
+      State.Status = HasNoDirectivesReported;
     }
 
     // Check that the expected diagnostics occurred.
@@ -1142,15 +1156,14 @@ void VerifyDiagnosticConsumer::CheckDiagnostics() {
   ED.Reset();
 }
 
-std::unique_ptr<Directive> Directive::create(bool RegexKind,
-                                             SourceLocation DirectiveLoc,
-                                             SourceLocation DiagnosticLoc,
-                                             bool MatchAnyFileAndLine,
-                                             bool MatchAnyLine, StringRef Text,
-                                             unsigned Min, unsigned Max) {
+std::unique_ptr<Directive>
+Directive::create(bool RegexKind, SourceLocation DirectiveLoc,
+                  SourceLocation DiagnosticLoc, StringRef Spelling,
+                  bool MatchAnyFileAndLine, bool MatchAnyLine, StringRef Text,
+                  unsigned Min, unsigned Max) {
   if (!RegexKind)
     return std::make_unique<StandardDirective>(DirectiveLoc, DiagnosticLoc,
-                                               MatchAnyFileAndLine,
+                                               Spelling, MatchAnyFileAndLine,
                                                MatchAnyLine, Text, Min, Max);
 
   // Parse the directive into a regular expression.
@@ -1175,7 +1188,7 @@ std::unique_ptr<Directive> Directive::create(bool RegexKind,
     }
   }
 
-  return std::make_unique<RegexDirective>(DirectiveLoc, DiagnosticLoc,
+  return std::make_unique<RegexDirective>(DirectiveLoc, DiagnosticLoc, Spelling,
                                           MatchAnyFileAndLine, MatchAnyLine,
                                           Text, Min, Max, RegexStr);
 }

clang/test/Frontend/verify-mulptiple-prefixes.c

@@ -0,0 +1,48 @@
+// Test the diagnostic messages of -verify with multiple prefixes.
+// - Expected but not seen errors should contain the prefix of the directive
+// - Seen but not expected errors should not choose an arbitrary prefix
+// - "expected directive cannot follow '<prefix>-no-diagnostics'" should report an actual
+//    expected-no-diagnostics prefix present in the source.
+
+// RUN: not %clang_cc1 -verify=foo,bar %s 2>&1 | FileCheck %s --check-prefix=CHECK1
+// RUN: not %clang_cc1 -verify=bar,foo %s 2>&1 | FileCheck %s --check-prefix=CHECK1
+
+undefined_type x; // #1
+
+// foo-error{{there is no error here}}
+// bar-error{{error not seen}}
+// bar-note{{declared here}}
+// bar-error{{another error not seen}}
+// bar-error-re{{regex error{{}} not present}}
+
+// CHECK1: error: diagnostics with 'error' severity expected but not seen: 
+// CHECK1:   Line 12 'foo-error': there is no error here
+// CHECK1:   Line 13 'bar-error': error not seen
+// CHECK1:   Line 15 'bar-error': another error not seen
+// CHECK1:   Line 16 'bar-error-re': regex error{{{{[}][}]}} not present
+// CHECK1: error: diagnostics with 'error' severity seen but not expected: 
+// CHECK1:   Line 10: unknown type name 'undefined_type'
+// CHECK1: error: diagnostics with 'note' severity expected but not seen: 
+// CHECK1:   Line 14 'bar-note': declared here
+// CHECK1: 6 errors generated.
+
+// RUN: not %clang_cc1 -verify=baz,qux,quux %s 2>&1 | FileCheck %s --check-prefix=CHECK2
+
+// qux-no-diagnostics
+// baz-error@#1{{unknown type name 'undefined_type'}}
+// quux-no-diagnostics
+// qux-error-re@#1{{unknown type name 'undefined_type'}}
+
+// CHECK2: error: diagnostics with 'error' severity seen but not expected: 
+// CHECK2:   Line 10: unknown type name 'undefined_type'
+// CHECK2:   Line 32: 'baz-error' directive cannot follow 'qux-no-diagnostics' directive
+// CHECK2:   Line 34: 'qux-error-re' directive cannot follow 'qux-no-diagnostics' directive
+
+// RUN: not %clang_cc1 -verify=spam,eggs %s 2>&1 | FileCheck %s --check-prefix=CHECK3
+
+// eggs-error@#1{{unknown type name 'undefined_type'}}
+// spam-no-diagnostics
+
+// CHECK3: error: diagnostics with 'error' severity seen but not expected: 
+// CHECK3:   Line 44: 'spam-no-diagnostics' directive cannot follow other expected directives
+// CHECK3: 1 error generated.

clang/test/Frontend/verify.c

@@ -201,7 +201,7 @@ unexpected b; // expected-error@33 1-1 {{unknown type}}
 // foo-note {{}}
 
 //      CHECK11: error: 'foo-error' diagnostics seen but not expected:
-// CHECK11-NEXT:   Line 201: expected directive cannot follow 'foo-no-diagnostics' directive
+// CHECK11-NEXT:   Line 201: 'foo-note' directive cannot follow 'foo-no-diagnostics' directive
 // CHECK11-NEXT: 1 error generated.
 #endif
 

clang/test/Frontend/verify3.c

@@ -8,7 +8,7 @@
 // expected-note {{}}
 
 //      CHECK1: error: 'expected-error' diagnostics seen but not expected:
-// CHECK1-NEXT:   Line 8: expected directive cannot follow 'expected-no-diagnostics' directive
+// CHECK1-NEXT:   Line 8: 'expected-note' directive cannot follow 'expected-no-diagnostics' directive
 // CHECK1-NEXT: 1 error generated.
 #endif