[clang] d91e5c3 - [verify] Improve the error messages with multiple active prefixes (#126068)
via cfe-commits
cfe-commits at lists.llvm.org
Thu Feb 27 08:58:38 PST 2025
Author: Mészáros Gergely
Date: 2025-02-27T17:58:34+01:00
New Revision: d91e5c301353b012b338aa9920a941d8b5fc28a4
URL: https://github.com/llvm/llvm-project/commit/d91e5c301353b012b338aa9920a941d8b5fc28a4
DIFF: https://github.com/llvm/llvm-project/commit/d91e5c301353b012b338aa9920a941d8b5fc28a4.diff
LOG: [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.
Added:
clang/test/Frontend/verify-mulptiple-prefixes.c
Modified:
clang/include/clang/Basic/DiagnosticFrontendKinds.td
clang/include/clang/Frontend/VerifyDiagnosticConsumer.h
clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
clang/test/Frontend/verify.c
clang/test/Frontend/verify3.c
Removed:
################################################################################
diff --git a/clang/include/clang/Basic/DiagnosticFrontendKinds.td b/clang/include/clang/Basic/DiagnosticFrontendKinds.td
index f3593f5313340..5a05f6c4e3e30 100644
--- a/clang/include/clang/Basic/DiagnosticFrontendKinds.td
+++ b/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<
diff --git a/clang/include/clang/Frontend/VerifyDiagnosticConsumer.h b/clang/include/clang/Frontend/VerifyDiagnosticConsumer.h
index ddfae2666c4c3..b4c613712ed9b 100644
--- a/clang/include/clang/Frontend/VerifyDiagnosticConsumer.h
+++ b/clang/include/clang/Frontend/VerifyDiagnosticConsumer.h
@@ -44,8 +44,9 @@ 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.
@@ -53,6 +54,7 @@ class VerifyDiagnosticConsumer: public DiagnosticConsumer,
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();
diff --git a/clang/lib/Frontend/VerifyDiagnosticConsumer.cpp b/clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
index 48330e9361718..6de19d689988e 100644
--- a/clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
+++ b/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);
}
diff --git a/clang/test/Frontend/verify-mulptiple-prefixes.c b/clang/test/Frontend/verify-mulptiple-prefixes.c
new file mode 100644
index 0000000000000..a42de9cedc525
--- /dev/null
+++ b/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.
diff --git a/clang/test/Frontend/verify.c b/clang/test/Frontend/verify.c
index afd1c7d6907e2..4a0a6b6a23d9c 100644
--- a/clang/test/Frontend/verify.c
+++ b/clang/test/Frontend/verify.c
@@ -201,7 +201,7 @@ unexpected b; // expected-error at 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
diff --git a/clang/test/Frontend/verify3.c b/clang/test/Frontend/verify3.c
index e414c7370fad7..6d3ab79ae7c91 100644
--- a/clang/test/Frontend/verify3.c
+++ b/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
More information about the cfe-commits
mailing list