Collapse diagnostic errors and notes into a single vector. (#3805)

On #3792 it was requested to put context-related diagnostic messages
prior to the diagnostic error. In order to support that, remove the
distinction that an error needs to come first on DiagnosticMessage.

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<llvm::Any> format_args,
-      std::function<std::string(const DiagnosticMessage&)> 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<DiagnosticMessage> notes;
+  // Messages related to the diagnostic. Only one should be a warning or error;
+  // other messages provide context.
+  llvm::SmallVector<DiagnosticMessage> messages;
 };
 
 namespace Internal {

toolchain/diagnostics/diagnostic_consumer.cpp

@@ -10,42 +10,36 @@
 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;
-    if (message.location.column_number > 0) {
-      *stream_ << ":" << message.location.column_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_ << ": " << 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<int32_t>(message.location.line.size()) -
-                              message.location.column_number);
-    for (int i = 0; i < underline_length; ++i) {
-      *stream_ << "~";
+    *stream_ << ": ";
+    if (message.level == DiagnosticLevel::Error) {
+      *stream_ << "ERROR: ";
+    }
+    *stream_ << 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<int32_t>(message.location.line.size()) -
+                                message.location.column_number);
+      for (int i = 0; i < underline_length; ++i) {
+        *stream_ << "~";
+      }
+      *stream_ << "\n";
     }
-    *stream_ << "\n";
   }
 }
 

toolchain/diagnostics/diagnostic_emitter.h

@@ -65,9 +65,8 @@ class DiagnosticEmitter {
               Internal::NoTypeDeduction<Args>... args) -> DiagnosticBuilder& {
       CARBON_CHECK(diagnostic_base.Level == DiagnosticLevel::Note)
           << static_cast<int>(diagnostic_base.Level);
-      diagnostic_.notes.push_back(
-          MakeMessage(emitter_, location, diagnostic_base,
-                      {emitter_->MakeAny<Args>(args)...}));
+      AddMessage(emitter_, location, diagnostic_base,
+                 {emitter_->MakeAny<Args>(args)...});
       return *this;
     }
 
@@ -89,26 +88,25 @@ class DiagnosticEmitter {
         DiagnosticEmitter<LocationT>* emitter, LocationT location,
         const Internal::DiagnosticBase<Args...>& diagnostic_base,
         llvm::SmallVector<llvm::Any> 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 <typename... Args>
-    static auto MakeMessage(
-        DiagnosticEmitter<LocationT>* emitter, LocationT location,
-        const Internal::DiagnosticBase<Args...>& diagnostic_base,
-        llvm::SmallVector<llvm::Any> 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<LocationT>* emitter, LocationT location,
+                    const Internal::DiagnosticBase<Args...>& diagnostic_base,
+                    llvm::SmallVector<llvm::Any> 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<Args...>(
                 message, std::make_index_sequence<sizeof...(Args)>());
-          });
+          }});
     }
 
     // Handles the cast of llvm::Any to Args types for formatv.

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<int> {
   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

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) {

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<DiagnosticKind> kind,
-                         testing::Matcher<DiagnosticLevel> level,
-                         testing::Matcher<int> line_number,
-                         testing::Matcher<int> column_number,
-                         testing::Matcher<std::string> message) {
+inline auto IsDiagnosticMessage(testing::Matcher<DiagnosticKind> kind,
+                                testing::Matcher<DiagnosticLevel> level,
+                                testing::Matcher<int> line_number,
+                                testing::Matcher<int> column_number,
+                                testing::Matcher<std::string> message)
+    -> testing::Matcher<DiagnosticMessage> {
+  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<DiagnosticLevel> level,
+    testing::Matcher<llvm::SmallVector<DiagnosticMessage>> elements)
+    -> testing::Matcher<Diagnostic> {
   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<DiagnosticKind> kind,
+                               testing::Matcher<DiagnosticLevel> level,
+                               testing::Matcher<int> line_number,
+                               testing::Matcher<int> column_number,
+                               testing::Matcher<std::string> message)
+    -> testing::Matcher<Diagnostic> {
+  return IsDiagnostic(
+      level, testing::ElementsAre(IsDiagnosticMessage(kind, level, line_number,
+                                                      column_number, message)));
 }
 
 }  // namespace Carbon::Testing

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));

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();
 }
 

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);
 }