diff --git a/toolchain/diagnostics/diagnostic.h b/toolchain/diagnostics/diagnostic.h index a55872d79a25..dff96f8d51bb 100644 --- a/toolchain/diagnostics/diagnostic.h +++ b/toolchain/diagnostics/diagnostic.h @@ -61,19 +61,12 @@ struct DiagnosticLocation { // A message composing a diagnostic. This may be the main message, but can also // be notes providing more information. struct DiagnosticMessage { - explicit DiagnosticMessage( - DiagnosticKind kind, DiagnosticLocation location, - llvm::StringLiteral format, llvm::SmallVector format_args, - std::function format_fn) - : kind(kind), - location(location), - format(format), - format_args(std::move(format_args)), - format_fn(std::move(format_fn)) {} - // The diagnostic's kind. DiagnosticKind kind; + // The diagnostic's level. + DiagnosticLevel level; + // The calculated location of the diagnostic. DiagnosticLocation location; @@ -99,11 +92,9 @@ struct Diagnostic { // The diagnostic's level. DiagnosticLevel level; - // The main error or warning. - DiagnosticMessage message; - - // Notes that add context or supplemental information to the diagnostic. - llvm::SmallVector notes; + // Messages related to the diagnostic. Only one should be a warning or error; + // other messages provide context. + llvm::SmallVector messages; }; namespace Internal { diff --git a/toolchain/diagnostics/diagnostic_consumer.cpp b/toolchain/diagnostics/diagnostic_consumer.cpp index 745f0effba38..7296e9462081 100644 --- a/toolchain/diagnostics/diagnostic_consumer.cpp +++ b/toolchain/diagnostics/diagnostic_consumer.cpp @@ -10,43 +10,37 @@ namespace Carbon { auto StreamDiagnosticConsumer::HandleDiagnostic(Diagnostic diagnostic) -> void { - std::string prefix; - if (diagnostic.level == DiagnosticLevel::Error) { - prefix = "ERROR: "; - } - Print(diagnostic.message, prefix); - for (const auto& note : diagnostic.notes) { - Print(note, ""); - } -} - -auto StreamDiagnosticConsumer::Print(const DiagnosticMessage& message, - llvm::StringRef prefix) -> void { - *stream_ << message.location.filename; - if (message.location.line_number > 0) { - *stream_ << ":" << message.location.line_number; + for (const auto& message : diagnostic.messages) { + *stream_ << message.location.filename; + if (message.location.line_number > 0) { + *stream_ << ":" << message.location.line_number; + if (message.location.column_number > 0) { + *stream_ << ":" << message.location.column_number; + } + } + *stream_ << ": "; + if (message.level == DiagnosticLevel::Error) { + *stream_ << "ERROR: "; + } + *stream_ << message.format_fn(message) << "\n"; if (message.location.column_number > 0) { - *stream_ << ":" << message.location.column_number; + *stream_ << message.location.line << "\n"; + stream_->indent(message.location.column_number - 1); + *stream_ << "^"; + int underline_length = std::max(0, message.location.length - 1); + // We want to ensure that we don't underline past the end of the line in + // case of a multiline token. + // TODO: revisit this once we can reference multiple ranges on multiple + // lines in a single diagnostic message. + underline_length = std::min( + underline_length, static_cast(message.location.line.size()) - + message.location.column_number); + for (int i = 0; i < underline_length; ++i) { + *stream_ << "~"; + } + *stream_ << "\n"; } } - *stream_ << ": " << prefix << message.format_fn(message) << "\n"; - if (message.location.column_number > 0) { - *stream_ << message.location.line << "\n"; - stream_->indent(message.location.column_number - 1); - *stream_ << "^"; - int underline_length = std::max(0, message.location.length - 1); - // We want to ensure that we don't underline past the end of the line in - // case of a multiline token. - // TODO: revisit this once we can reference multiple ranges on multiple - // lines in a single diagnostic message. - underline_length = std::min( - underline_length, static_cast(message.location.line.size()) - - message.location.column_number); - for (int i = 0; i < underline_length; ++i) { - *stream_ << "~"; - } - *stream_ << "\n"; - } } auto ConsoleDiagnosticConsumer() -> DiagnosticConsumer& { diff --git a/toolchain/diagnostics/diagnostic_emitter.h b/toolchain/diagnostics/diagnostic_emitter.h index 04e01c0a4b60..22c6e347f24e 100644 --- a/toolchain/diagnostics/diagnostic_emitter.h +++ b/toolchain/diagnostics/diagnostic_emitter.h @@ -65,9 +65,8 @@ class DiagnosticEmitter { Internal::NoTypeDeduction... args) -> DiagnosticBuilder& { CARBON_CHECK(diagnostic_base.Level == DiagnosticLevel::Note) << static_cast(diagnostic_base.Level); - diagnostic_.notes.push_back( - MakeMessage(emitter_, location, diagnostic_base, - {emitter_->MakeAny(args)...})); + AddMessage(emitter_, location, diagnostic_base, + {emitter_->MakeAny(args)...}); return *this; } @@ -89,26 +88,25 @@ class DiagnosticEmitter { DiagnosticEmitter* emitter, LocationT location, const Internal::DiagnosticBase& diagnostic_base, llvm::SmallVector args) - : emitter_(emitter), - diagnostic_( - {.level = diagnostic_base.Level, - .message = MakeMessage(emitter, location, diagnostic_base, - std::move(args))}) { + : emitter_(emitter), diagnostic_({.level = diagnostic_base.Level}) { + AddMessage(emitter, location, diagnostic_base, std::move(args)); CARBON_CHECK(diagnostic_base.Level != DiagnosticLevel::Note); } template - static auto MakeMessage( - DiagnosticEmitter* emitter, LocationT location, - const Internal::DiagnosticBase& diagnostic_base, - llvm::SmallVector args) -> DiagnosticMessage { - return DiagnosticMessage( - diagnostic_base.Kind, emitter->converter_->ConvertLocation(location), - diagnostic_base.Format, std::move(args), - [](const DiagnosticMessage& message) -> std::string { + auto AddMessage(DiagnosticEmitter* emitter, LocationT location, + const Internal::DiagnosticBase& diagnostic_base, + llvm::SmallVector args) -> void { + diagnostic_.messages.emplace_back(DiagnosticMessage{ + .kind = diagnostic_base.Kind, + .level = diagnostic_base.Level, + .location = emitter->converter_->ConvertLocation(location), + .format = diagnostic_base.Format, + .format_args = std::move(args), + .format_fn = [](const DiagnosticMessage& message) -> std::string { return FormatFn( message, std::make_index_sequence()); - }); + }}); } // Handles the cast of llvm::Any to Args types for formatv. diff --git a/toolchain/diagnostics/diagnostic_emitter_test.cpp b/toolchain/diagnostics/diagnostic_emitter_test.cpp index 15c47d0d208a..c689c62e157e 100644 --- a/toolchain/diagnostics/diagnostic_emitter_test.cpp +++ b/toolchain/diagnostics/diagnostic_emitter_test.cpp @@ -10,10 +10,12 @@ #include "llvm/ADT/StringRef.h" #include "toolchain/diagnostics/mocks.h" -namespace Carbon { +namespace Carbon::Testing { namespace { using ::Carbon::Testing::IsDiagnostic; +using ::Carbon::Testing::IsSingleDiagnostic; +using testing::ElementsAre; struct FakeDiagnosticConverter : DiagnosticConverter { auto ConvertLocation(int n) const -> DiagnosticLocation override { @@ -32,10 +34,10 @@ class DiagnosticEmitterTest : public ::testing::Test { TEST_F(DiagnosticEmitterTest, EmitSimpleError) { CARBON_DIAGNOSTIC(TestDiagnostic, Error, "simple error"); - EXPECT_CALL(consumer_, HandleDiagnostic(IsDiagnostic( + EXPECT_CALL(consumer_, HandleDiagnostic(IsSingleDiagnostic( DiagnosticKind::TestDiagnostic, DiagnosticLevel::Error, 1, 1, "simple error"))); - EXPECT_CALL(consumer_, HandleDiagnostic(IsDiagnostic( + EXPECT_CALL(consumer_, HandleDiagnostic(IsSingleDiagnostic( DiagnosticKind::TestDiagnostic, DiagnosticLevel::Error, 1, 2, "simple error"))); emitter_.Emit(1, TestDiagnostic); @@ -45,15 +47,15 @@ TEST_F(DiagnosticEmitterTest, EmitSimpleError) { TEST_F(DiagnosticEmitterTest, EmitSimpleWarning) { CARBON_DIAGNOSTIC(TestDiagnostic, Warning, "simple warning"); EXPECT_CALL(consumer_, - HandleDiagnostic(IsDiagnostic(DiagnosticKind::TestDiagnostic, - DiagnosticLevel::Warning, 1, 1, - "simple warning"))); + HandleDiagnostic(IsSingleDiagnostic( + DiagnosticKind::TestDiagnostic, DiagnosticLevel::Warning, 1, + 1, "simple warning"))); emitter_.Emit(1, TestDiagnostic); } TEST_F(DiagnosticEmitterTest, EmitOneArgDiagnostic) { CARBON_DIAGNOSTIC(TestDiagnostic, Error, "arg: `{0}`", llvm::StringLiteral); - EXPECT_CALL(consumer_, HandleDiagnostic(IsDiagnostic( + EXPECT_CALL(consumer_, HandleDiagnostic(IsSingleDiagnostic( DiagnosticKind::TestDiagnostic, DiagnosticLevel::Error, 1, 1, "arg: `str`"))); emitter_.Emit(1, TestDiagnostic, "str"); @@ -62,12 +64,18 @@ TEST_F(DiagnosticEmitterTest, EmitOneArgDiagnostic) { TEST_F(DiagnosticEmitterTest, EmitNote) { CARBON_DIAGNOSTIC(TestDiagnostic, Warning, "simple warning"); CARBON_DIAGNOSTIC(TestDiagnosticNote, Note, "note"); - EXPECT_CALL(consumer_, - HandleDiagnostic(IsDiagnostic(DiagnosticKind::TestDiagnostic, - DiagnosticLevel::Warning, 1, 1, - "simple warning"))); + EXPECT_CALL( + consumer_, + HandleDiagnostic(IsDiagnostic( + DiagnosticLevel::Warning, + ElementsAre( + IsDiagnosticMessage(DiagnosticKind::TestDiagnostic, + DiagnosticLevel::Warning, 1, 1, + "simple warning"), + IsDiagnosticMessage(DiagnosticKind::TestDiagnosticNote, + DiagnosticLevel::Note, 1, 2, "note"))))); emitter_.Build(1, TestDiagnostic).Note(2, TestDiagnosticNote).Emit(); } } // namespace -} // namespace Carbon +} // namespace Carbon::Testing diff --git a/toolchain/diagnostics/mocks.cpp b/toolchain/diagnostics/mocks.cpp index 8483122b2913..8ca096c6c7fa 100644 --- a/toolchain/diagnostics/mocks.cpp +++ b/toolchain/diagnostics/mocks.cpp @@ -7,12 +7,14 @@ namespace Carbon { void PrintTo(const Diagnostic& diagnostic, std::ostream* os) { - *os << "Diagnostic{" << diagnostic.message.kind << ", "; + *os << "Diagnostic{"; PrintTo(diagnostic.level, os); - *os << ", " << diagnostic.message.location.filename << ":" - << diagnostic.message.location.line_number << ":" - << diagnostic.message.location.column_number << ", \"" - << diagnostic.message.format_fn(diagnostic.message) << "\"}"; + for (const auto& message : diagnostic.messages) { + *os << ", {" << message.location.filename << ":" + << message.location.line_number << ":" << message.location.column_number + << ", \"" << message.format_fn(message) << "}"; + } + *os << "\"}"; } void PrintTo(DiagnosticLevel level, std::ostream* os) { diff --git a/toolchain/diagnostics/mocks.h b/toolchain/diagnostics/mocks.h index 77afecd515f2..dbf77ebf17f6 100644 --- a/toolchain/diagnostics/mocks.h +++ b/toolchain/diagnostics/mocks.h @@ -17,33 +17,49 @@ class MockDiagnosticConsumer : public DiagnosticConsumer { }; // NOLINTNEXTLINE(modernize-use-trailing-return-type): From the macro. -MATCHER_P(IsDiagnosticMessage, matcher, "") { - const Diagnostic& diag = arg; - return testing::ExplainMatchResult( - matcher, diag.message.format_fn(diag.message), result_listener); +MATCHER_P(IsDiagnosticMessageString, matcher, "") { + const DiagnosticMessage& message = arg; + return testing::ExplainMatchResult(matcher, message.format_fn(message), + result_listener); } -inline auto IsDiagnostic(testing::Matcher kind, - testing::Matcher level, - testing::Matcher line_number, - testing::Matcher column_number, - testing::Matcher message) { +inline auto IsDiagnosticMessage(testing::Matcher kind, + testing::Matcher level, + testing::Matcher line_number, + testing::Matcher column_number, + testing::Matcher message) + -> testing::Matcher { + using testing::AllOf; + using testing::Field; + return AllOf( + Field("kind", &DiagnosticMessage::kind, kind), + Field("level", &DiagnosticMessage::level, level), + Field(&DiagnosticMessage::location, + AllOf(Field("line_number", &DiagnosticLocation::line_number, + line_number), + Field("column_number", &DiagnosticLocation::column_number, + column_number))), + IsDiagnosticMessageString(message)); +} + +inline auto IsDiagnostic( + testing::Matcher level, + testing::Matcher> elements) + -> testing::Matcher { return testing::AllOf( testing::Field("level", &Diagnostic::level, level), - testing::Field( - "message", &Diagnostic::message, - testing::AllOf( - testing::Field("kind", &DiagnosticMessage::kind, kind), - testing::Field( - &DiagnosticMessage::location, - testing::AllOf( - testing::Field("line_number", - &DiagnosticLocation::line_number, - line_number), - testing::Field("column_number", - &DiagnosticLocation::column_number, - column_number))))), - IsDiagnosticMessage(message)); + testing::Field("messages", &Diagnostic::messages, elements)); +} + +inline auto IsSingleDiagnostic(testing::Matcher kind, + testing::Matcher level, + testing::Matcher line_number, + testing::Matcher column_number, + testing::Matcher message) + -> testing::Matcher { + return IsDiagnostic( + level, testing::ElementsAre(IsDiagnosticMessage(kind, level, line_number, + column_number, message))); } } // namespace Carbon::Testing diff --git a/toolchain/diagnostics/sorting_diagnostic_consumer.h b/toolchain/diagnostics/sorting_diagnostic_consumer.h index e36ac6e6267e..79175ea936ec 100644 --- a/toolchain/diagnostics/sorting_diagnostic_consumer.h +++ b/toolchain/diagnostics/sorting_diagnostic_consumer.h @@ -35,10 +35,12 @@ class SortingDiagnosticConsumer : public DiagnosticConsumer { void Flush() override { llvm::stable_sort(diagnostics_, [](const Diagnostic& lhs, const Diagnostic& rhs) { - return std::tie(lhs.message.location.line_number, - lhs.message.location.column_number) < - std::tie(rhs.message.location.line_number, - rhs.message.location.column_number); + const auto& lhs_loc = lhs.messages[0].location; + const auto& rhs_loc = rhs.messages[0].location; + return std::tie(lhs_loc.filename, lhs_loc.line_number, + lhs_loc.column_number) < + std::tie(rhs_loc.filename, rhs_loc.line_number, + rhs_loc.column_number); }); for (auto& diag : diagnostics_) { next_consumer_->HandleDiagnostic(std::move(diag)); diff --git a/toolchain/diagnostics/sorting_diagnostic_consumer_test.cpp b/toolchain/diagnostics/sorting_diagnostic_consumer_test.cpp index 493241ec4458..ef237e3d4ba0 100644 --- a/toolchain/diagnostics/sorting_diagnostic_consumer_test.cpp +++ b/toolchain/diagnostics/sorting_diagnostic_consumer_test.cpp @@ -14,7 +14,7 @@ namespace Carbon { namespace { -using ::Carbon::Testing::IsDiagnostic; +using ::Carbon::Testing::IsSingleDiagnostic; using ::testing::InSequence; CARBON_DIAGNOSTIC(TestDiagnostic, Error, "{0}", llvm::StringLiteral); @@ -40,24 +40,24 @@ TEST(SortedDiagnosticEmitterTest, SortErrors) { emitter.Emit({"f", "line", 3, 2}, TestDiagnostic, "M6"); InSequence s; - EXPECT_CALL(consumer, HandleDiagnostic( - IsDiagnostic(DiagnosticKind::TestDiagnostic, - DiagnosticLevel::Error, 1, 1, "M2"))); - EXPECT_CALL(consumer, HandleDiagnostic( - IsDiagnostic(DiagnosticKind::TestDiagnostic, - DiagnosticLevel::Error, 1, 3, "M3"))); - EXPECT_CALL(consumer, HandleDiagnostic( - IsDiagnostic(DiagnosticKind::TestDiagnostic, - DiagnosticLevel::Error, 2, 1, "M1"))); - EXPECT_CALL(consumer, HandleDiagnostic( - IsDiagnostic(DiagnosticKind::TestDiagnostic, - DiagnosticLevel::Error, 3, 2, "M5"))); - EXPECT_CALL(consumer, HandleDiagnostic( - IsDiagnostic(DiagnosticKind::TestDiagnostic, - DiagnosticLevel::Error, 3, 2, "M6"))); - EXPECT_CALL(consumer, HandleDiagnostic( - IsDiagnostic(DiagnosticKind::TestDiagnostic, - DiagnosticLevel::Error, 3, 4, "M4"))); + EXPECT_CALL(consumer, HandleDiagnostic(IsSingleDiagnostic( + DiagnosticKind::TestDiagnostic, + DiagnosticLevel::Error, 1, 1, "M2"))); + EXPECT_CALL(consumer, HandleDiagnostic(IsSingleDiagnostic( + DiagnosticKind::TestDiagnostic, + DiagnosticLevel::Error, 1, 3, "M3"))); + EXPECT_CALL(consumer, HandleDiagnostic(IsSingleDiagnostic( + DiagnosticKind::TestDiagnostic, + DiagnosticLevel::Error, 2, 1, "M1"))); + EXPECT_CALL(consumer, HandleDiagnostic(IsSingleDiagnostic( + DiagnosticKind::TestDiagnostic, + DiagnosticLevel::Error, 3, 2, "M5"))); + EXPECT_CALL(consumer, HandleDiagnostic(IsSingleDiagnostic( + DiagnosticKind::TestDiagnostic, + DiagnosticLevel::Error, 3, 2, "M6"))); + EXPECT_CALL(consumer, HandleDiagnostic(IsSingleDiagnostic( + DiagnosticKind::TestDiagnostic, + DiagnosticLevel::Error, 3, 4, "M4"))); sorting_consumer.Flush(); } diff --git a/toolchain/lex/tokenized_buffer_test.cpp b/toolchain/lex/tokenized_buffer_test.cpp index 777574742143..b18d6b9653cd 100644 --- a/toolchain/lex/tokenized_buffer_test.cpp +++ b/toolchain/lex/tokenized_buffer_test.cpp @@ -23,7 +23,7 @@ namespace Carbon::Lex { namespace { using ::Carbon::Testing::ExpectedToken; -using ::Carbon::Testing::IsDiagnostic; +using ::Carbon::Testing::IsSingleDiagnostic; using ::Carbon::Testing::TestRawOstream; using ::testing::_; using ::testing::ElementsAre; @@ -966,7 +966,7 @@ TEST_F(LexerTest, TypeLiteralTooManyDigits) { Testing::MockDiagnosticConsumer consumer; EXPECT_CALL(consumer, - HandleDiagnostic(IsDiagnostic( + HandleDiagnostic(IsSingleDiagnostic( DiagnosticKind::TooManyDigits, DiagnosticLevel::Error, 1, 2, HasSubstr(llvm::formatv(" {0} ", Count))))); auto buffer = Lex(code, consumer); @@ -990,15 +990,15 @@ TEST_F(LexerTest, DiagnosticTrailingComment) { )"; Testing::MockDiagnosticConsumer consumer; - EXPECT_CALL(consumer, - HandleDiagnostic(IsDiagnostic(DiagnosticKind::TrailingComment, - DiagnosticLevel::Error, 3, 19, _))); + EXPECT_CALL(consumer, HandleDiagnostic(IsSingleDiagnostic( + DiagnosticKind::TrailingComment, + DiagnosticLevel::Error, 3, 19, _))); Lex(testcase, consumer); } TEST_F(LexerTest, DiagnosticWhitespace) { Testing::MockDiagnosticConsumer consumer; - EXPECT_CALL(consumer, HandleDiagnostic(IsDiagnostic( + EXPECT_CALL(consumer, HandleDiagnostic(IsSingleDiagnostic( DiagnosticKind::NoWhitespaceAfterCommentIntroducer, DiagnosticLevel::Error, 1, 3, _))); Lex("//no space after comment", consumer); @@ -1006,7 +1006,7 @@ TEST_F(LexerTest, DiagnosticWhitespace) { TEST_F(LexerTest, DiagnosticUnrecognizedEscape) { Testing::MockDiagnosticConsumer consumer; - EXPECT_CALL(consumer, HandleDiagnostic(IsDiagnostic( + EXPECT_CALL(consumer, HandleDiagnostic(IsSingleDiagnostic( DiagnosticKind::UnknownEscapeSequence, DiagnosticLevel::Error, 1, 8, HasSubstr("`b`")))); Lex(R"("hello\bworld")", consumer); @@ -1014,7 +1014,7 @@ TEST_F(LexerTest, DiagnosticUnrecognizedEscape) { TEST_F(LexerTest, DiagnosticBadHex) { Testing::MockDiagnosticConsumer consumer; - EXPECT_CALL(consumer, HandleDiagnostic(IsDiagnostic( + EXPECT_CALL(consumer, HandleDiagnostic(IsSingleDiagnostic( DiagnosticKind::HexadecimalEscapeMissingDigits, DiagnosticLevel::Error, 1, 9, _))); Lex(R"("hello\xabworld")", consumer); @@ -1022,7 +1022,7 @@ TEST_F(LexerTest, DiagnosticBadHex) { TEST_F(LexerTest, DiagnosticInvalidDigit) { Testing::MockDiagnosticConsumer consumer; - EXPECT_CALL(consumer, HandleDiagnostic(IsDiagnostic( + EXPECT_CALL(consumer, HandleDiagnostic(IsSingleDiagnostic( DiagnosticKind::InvalidDigit, DiagnosticLevel::Error, 1, 6, HasSubstr("'a'")))); Lex("0x123abc", consumer); @@ -1030,17 +1030,17 @@ TEST_F(LexerTest, DiagnosticInvalidDigit) { TEST_F(LexerTest, DiagnosticMissingTerminator) { Testing::MockDiagnosticConsumer consumer; - EXPECT_CALL(consumer, - HandleDiagnostic(IsDiagnostic(DiagnosticKind::UnterminatedString, - DiagnosticLevel::Error, 1, 1, _))); + EXPECT_CALL(consumer, HandleDiagnostic(IsSingleDiagnostic( + DiagnosticKind::UnterminatedString, + DiagnosticLevel::Error, 1, 1, _))); Lex(R"(#" ")", consumer); } TEST_F(LexerTest, DiagnosticUnrecognizedChar) { Testing::MockDiagnosticConsumer consumer; - EXPECT_CALL(consumer, HandleDiagnostic( - IsDiagnostic(DiagnosticKind::UnrecognizedCharacters, - DiagnosticLevel::Error, 1, 1, _))); + EXPECT_CALL(consumer, HandleDiagnostic(IsSingleDiagnostic( + DiagnosticKind::UnrecognizedCharacters, + DiagnosticLevel::Error, 1, 1, _))); Lex("\b", consumer); }