Add a way for diagnostics to sort on more than last_byte_offset. (#6687)

The intent is that `last_byte_offset` is still the main sorting key.
Diagnostics issued normally (e.g. in an expression) will keep sorting
the same, and come before the new diagnostic sort. Diagnostics issued at
the end of a scope (e.g. `unused`) can request sorting by their start
location, and would become interleaved through that.

Choosing "on scope" because I think that's the main way we'll use this
functionality (on scope changes); can always rename later if usage
expands.

Assisted-by: Google Antigravity with Gemini 3 Flash

toolchain/diagnostics/coverage_test.cpp

@@ -22,6 +22,7 @@ constexpr Kind Kinds[] = {
 constexpr Kind UntestedKinds[] = {
     // These exist only for unit tests.
     Kind::TestDiagnostic,
+    Kind::TestDiagnosticOnScope,
     Kind::TestDiagnosticNote,
 
     // Diagnosing erroneous install conditions, but test environments are

toolchain/diagnostics/diagnostic.h

@@ -40,11 +40,21 @@ enum class Level : int8_t {
 // https://llvm.org/doxygen/FormatVariadic_8h_source.html
 //
 // See `Diagnostics::Emitter::Emit` for comments about argument lifetimes.
-#define CARBON_DIAGNOSTIC(DiagnosticName, LevelValue, Format, ...) \
-  static constexpr auto DiagnosticName =                           \
-      ::Carbon::Diagnostics::DiagnosticBase<__VA_ARGS__>(          \
-          ::Carbon::Diagnostics::Kind::DiagnosticName,             \
-          ::Carbon::Diagnostics::Level::LevelValue, Format)
+#define CARBON_DIAGNOSTIC(DiagnosticName, LevelValue, Format, ...)         \
+  static constexpr auto DiagnosticName =                                   \
+      ::Carbon::Diagnostics::DiagnosticBase<__VA_ARGS__>(                  \
+          ::Carbon::Diagnostics::Kind::DiagnosticName,                     \
+          ::Carbon::Diagnostics::Level::LevelValue, /*is_on_scope=*/false, \
+          Format)
+
+// Similar to `CARBON_DIAGNOSTIC`, but for diagnostics that are generated on a
+// scope; see `Diagnostic::is_on_scope` for details.
+#define CARBON_DIAGNOSTIC_ON_SCOPE(DiagnosticName, LevelValue, Format, ...) \
+  static constexpr auto DiagnosticName =                                    \
+      ::Carbon::Diagnostics::DiagnosticBase<__VA_ARGS__>(                   \
+          ::Carbon::Diagnostics::Kind::DiagnosticName,                      \
+          ::Carbon::Diagnostics::Level::LevelValue, /*is_on_scope=*/true,   \
+          Format)
 
 // A location for a diagnostic in a file. The lifetime of a Loc
 // is required to be less than SourceBuffer that it refers to due to the
@@ -117,6 +127,12 @@ struct Diagnostic {
   // The diagnostic's level.
   Level level;
 
+  // Whether a diagnostic should only sort by `last_byte_offset` (which is
+  // normal), or if it's generated on a scope and should be sorted based on the
+  // first message's line and column when the `last_byte_offset` is equal.
+  // This is used by `SortingConsumer`.
+  bool is_on_scope;
+
   // The byte offset of the final token which is associated with the diagnostic.
   // This is used by `SortingConsumer`. This is separate from the
   // `Loc` because it must refer to a position in the primary file
@@ -138,9 +154,9 @@ struct Diagnostic {
 // This stores static information about a diagnostic category.
 template <typename... Args>
 struct DiagnosticBase {
-  explicit constexpr DiagnosticBase(Kind kind, Level level,
+  explicit constexpr DiagnosticBase(Kind kind, Level level, bool is_on_scope,
                                     llvm::StringLiteral format)
-      : Kind(kind), Level(level), Format(format) {
+      : Kind(kind), Level(level), IsOnScope(is_on_scope), Format(format) {
     static_assert((... && !(std::is_same_v<Args, llvm::StringRef> ||
                             std::is_same_v<Args, llvm::StringLiteral>)),
                   "String type disallowed in diagnostics. See "
@@ -152,6 +168,8 @@ struct DiagnosticBase {
   Kind Kind;
   // The diagnostic's level.
   Level Level;
+  // See `Diagnostic::is_on_scope`.
+  bool IsOnScope;
   // The diagnostic's format for llvm::formatv.
   llvm::StringLiteral Format;
 };

toolchain/diagnostics/emitter.h

@@ -366,7 +366,9 @@ template <typename... Args>
 Emitter<LocT>::Builder::Builder(Emitter<LocT>* emitter, LocT loc,
                                 const DiagnosticBase<Args...>& diagnostic_base,
                                 llvm::SmallVector<llvm::Any> args)
-    : emitter_(emitter), diagnostic_({.level = diagnostic_base.Level}) {
+    : emitter_(emitter),
+      diagnostic_({.level = diagnostic_base.Level,
+                   .is_on_scope = diagnostic_base.IsOnScope}) {
   AddMessage(LocT(loc), diagnostic_base, std::move(args));
   CARBON_CHECK(diagnostic_base.Level != Level::Note);
 }

toolchain/diagnostics/kind.def

@@ -580,6 +580,7 @@ CARBON_DIAGNOSTIC_KIND(LanguageServerDiagnosticInWrongFile)
 
 // TestDiagnostic is only for unit tests.
 CARBON_DIAGNOSTIC_KIND(TestDiagnostic)
+CARBON_DIAGNOSTIC_KIND(TestDiagnosticOnScope)
 CARBON_DIAGNOSTIC_KIND(TestDiagnosticNote)
 
 #undef CARBON_DIAGNOSTIC_KIND

toolchain/diagnostics/sorting_consumer.h

@@ -40,10 +40,23 @@ class SortingConsumer : public Consumer {
 
   // Sorts and flushes buffered diagnostics.
   auto Flush() -> void override {
-    llvm::stable_sort(diagnostics_,
-                      [](const Diagnostic& lhs, const Diagnostic& rhs) {
-                        return lhs.last_byte_offset < rhs.last_byte_offset;
-                      });
+    llvm::stable_sort(
+        diagnostics_, [](const Diagnostic& lhs, const Diagnostic& rhs) {
+          if (lhs.last_byte_offset != rhs.last_byte_offset) {
+            return lhs.last_byte_offset < rhs.last_byte_offset;
+          }
+
+          if (lhs.is_on_scope && rhs.is_on_scope) {
+            // When both are on-scope, we need to compare the locations.
+            const auto& lhs_loc = lhs.messages[0].loc;
+            const auto& rhs_loc = rhs.messages[0].loc;
+            return std::tie(lhs_loc.line_number, lhs_loc.column_number) <
+                   std::tie(rhs_loc.line_number, rhs_loc.column_number);
+          } else {
+            // Order non-on-scope before on-scope diagnostics.
+            return !lhs.is_on_scope && rhs.is_on_scope;
+          }
+        });
     for (auto& diag : diagnostics_) {
       next_consumer_->HandleDiagnostic(std::move(diag));
     }

toolchain/diagnostics/sorting_consumer_test.cpp

@@ -16,47 +16,127 @@ namespace {
 
 using ::Carbon::Testing::IsSingleDiagnostic;
 using ::testing::_;
-using ::testing::InSequence;
+using ::testing::ElementsAreArray;
 
-CARBON_DIAGNOSTIC(TestDiagnostic, Error, "M{0}", int);
+CARBON_DIAGNOSTIC(TestDiagnostic, Error, "Diag{0}", int);
+CARBON_DIAGNOSTIC_ON_SCOPE(TestDiagnosticOnScope, Error, "DiagOnScope{0}", int);
 
-class FakeEmitter : public Emitter<int32_t> {
+// Sorting-related locations, consumed by `FakeEmitter::ConvertLoc`.
+struct TestLoc {
+  int32_t last_byte_offset;
+  int32_t line_number;
+  int32_t column_number;
+};
+
+// Emits a diagnostic with the requested location.
+class FakeEmitter : public Emitter<TestLoc> {
  public:
   using Emitter::Emitter;
 
  protected:
-  auto ConvertLoc(int32_t last_byte_offset, ContextFnT /*context_fn*/) const
+  auto ConvertLoc(TestLoc test_loc, ContextFnT /*context_fn*/) const
       -> ConvertedLoc override {
-    return {.loc = {}, .last_byte_offset = last_byte_offset};
+    return {.loc = {.line_number = test_loc.line_number,
+                    .column_number = test_loc.column_number},
+            .last_byte_offset = test_loc.last_byte_offset};
   }
 };
 
+// Stores diagnostics in a vector for `ElementsArray` matching.
+class VectorConsumer : public Consumer {
+ public:
+  auto HandleDiagnostic(Diagnostic diagnostic) -> void override {
+    diagnostics_.push_back(std::move(diagnostic));
+  }
+
+  auto Flush() -> void override {}
+
+  auto diagnostics() const -> llvm::ArrayRef<Diagnostic> {
+    return diagnostics_;
+  }
+
+ private:
+  llvm::SmallVector<Diagnostic> diagnostics_;
+};
+
 TEST(SortedEmitterTest, SortErrors) {
-  Testing::MockDiagnosticConsumer consumer;
+  VectorConsumer consumer;
   SortingConsumer sorting_consumer(consumer);
   FakeEmitter emitter(&sorting_consumer);
 
-  emitter.Emit(1, TestDiagnostic, 1);
-  emitter.Emit(-1, TestDiagnostic, 2);
-  emitter.Emit(0, TestDiagnostic, 3);
-  emitter.Emit(4, TestDiagnostic, 4);
-  emitter.Emit(3, TestDiagnostic, 5);
-  emitter.Emit(3, TestDiagnostic, 6);
+  emitter.Emit({1, 10, 1}, TestDiagnostic, 1);
+  emitter.Emit({-1, 11, 1}, TestDiagnostic, 2);
+  emitter.Emit({0, 12, 1}, TestDiagnostic, 3);
+  emitter.Emit({4, 13, 1}, TestDiagnostic, 4);
+  emitter.Emit({3, 14, 1}, TestDiagnostic, 5);
+  emitter.Emit({3, 15, 1}, TestDiagnostic, 6);
 
-  InSequence s;
-  EXPECT_CALL(consumer, HandleDiagnostic(IsSingleDiagnostic(
-                            Kind::TestDiagnostic, Level::Error, _, _, "M2")));
-  EXPECT_CALL(consumer, HandleDiagnostic(IsSingleDiagnostic(
-                            Kind::TestDiagnostic, Level::Error, _, _, "M3")));
-  EXPECT_CALL(consumer, HandleDiagnostic(IsSingleDiagnostic(
-                            Kind::TestDiagnostic, Level::Error, _, _, "M1")));
-  EXPECT_CALL(consumer, HandleDiagnostic(IsSingleDiagnostic(
-                            Kind::TestDiagnostic, Level::Error, _, _, "M5")));
-  EXPECT_CALL(consumer, HandleDiagnostic(IsSingleDiagnostic(
-                            Kind::TestDiagnostic, Level::Error, _, _, "M6")));
-  EXPECT_CALL(consumer, HandleDiagnostic(IsSingleDiagnostic(
-                            Kind::TestDiagnostic, Level::Error, _, _, "M4")));
+  EXPECT_TRUE(consumer.diagnostics().empty());
   sorting_consumer.Flush();
+  EXPECT_THAT(
+      consumer.diagnostics(),
+      ElementsAreArray({
+          IsSingleDiagnostic(Kind::TestDiagnostic, Level::Error, _, _, "Diag2"),
+          IsSingleDiagnostic(Kind::TestDiagnostic, Level::Error, _, _, "Diag3"),
+          IsSingleDiagnostic(Kind::TestDiagnostic, Level::Error, _, _, "Diag1"),
+          IsSingleDiagnostic(Kind::TestDiagnostic, Level::Error, _, _, "Diag5"),
+          IsSingleDiagnostic(Kind::TestDiagnostic, Level::Error, _, _, "Diag6"),
+          IsSingleDiagnostic(Kind::TestDiagnostic, Level::Error, _, _, "Diag4"),
+      }));
+}
+
+TEST(SortedEmitterTest, SortOnScope) {
+  VectorConsumer consumer;
+  SortingConsumer sorting_consumer(consumer);
+  FakeEmitter emitter(&sorting_consumer);
+
+  emitter.Emit({2, 15, 1}, TestDiagnosticOnScope, 1);
+  emitter.Emit({4, 1, 1}, TestDiagnosticOnScope, 2);
+  // Same `last_byte_offset`, should sort by line/column.
+  emitter.Emit({3, 10, 5}, TestDiagnosticOnScope, 3);
+  emitter.Emit({3, 10, 1}, TestDiagnosticOnScope, 4);
+  emitter.Emit({3, 5, 20}, TestDiagnosticOnScope, 5);
+  emitter.Emit({3, 10, 3}, TestDiagnosticOnScope, 6);
+
+  EXPECT_TRUE(consumer.diagnostics().empty());
+  sorting_consumer.Flush();
+
+  EXPECT_THAT(consumer.diagnostics(),
+              ElementsAreArray({
+                  IsSingleDiagnostic(Kind::TestDiagnosticOnScope, Level::Error,
+                                     _, _, "DiagOnScope1"),
+                  IsSingleDiagnostic(Kind::TestDiagnosticOnScope, Level::Error,
+                                     _, _, "DiagOnScope5"),
+                  IsSingleDiagnostic(Kind::TestDiagnosticOnScope, Level::Error,
+                                     _, _, "DiagOnScope4"),
+                  IsSingleDiagnostic(Kind::TestDiagnosticOnScope, Level::Error,
+                                     _, _, "DiagOnScope6"),
+                  IsSingleDiagnostic(Kind::TestDiagnosticOnScope, Level::Error,
+                                     _, _, "DiagOnScope3"),
+                  IsSingleDiagnostic(Kind::TestDiagnosticOnScope, Level::Error,
+                                     _, _, "DiagOnScope2"),
+              }));
+}
+
+TEST(SortedEmitterTest, MixedScope) {
+  VectorConsumer consumer;
+  SortingConsumer sorting_consumer(consumer);
+  FakeEmitter emitter(&sorting_consumer);
+
+  // Difference in on-scope means only `last_byte_offset` is used.
+  emitter.Emit({3, 3, 1}, TestDiagnosticOnScope, 1);
+  emitter.Emit({3, 10, 1}, TestDiagnostic, 2);
+
+  EXPECT_TRUE(consumer.diagnostics().empty());
+  sorting_consumer.Flush();
+
+  EXPECT_THAT(
+      consumer.diagnostics(),
+      ElementsAreArray({
+          IsSingleDiagnostic(Kind::TestDiagnostic, Level::Error, _, _, "Diag2"),
+          IsSingleDiagnostic(Kind::TestDiagnosticOnScope, Level::Error, _, _,
+                             "DiagOnScope1"),
+      }));
 }
 
 }  // namespace