diff --git a/toolchain/diagnostics/coverage_test.cpp b/toolchain/diagnostics/coverage_test.cpp index f3e1641e9f27..d16e5dbc4bbd 100644 --- a/toolchain/diagnostics/coverage_test.cpp +++ b/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 diff --git a/toolchain/diagnostics/diagnostic.h b/toolchain/diagnostics/diagnostic.h index 171452662d01..b960f7c0c8fb 100644 --- a/toolchain/diagnostics/diagnostic.h +++ b/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 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 || std::is_same_v)), "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; }; diff --git a/toolchain/diagnostics/emitter.h b/toolchain/diagnostics/emitter.h index 832c48c644ec..5e8ced272962 100644 --- a/toolchain/diagnostics/emitter.h +++ b/toolchain/diagnostics/emitter.h @@ -366,7 +366,9 @@ template Emitter::Builder::Builder(Emitter* emitter, LocT loc, const DiagnosticBase& diagnostic_base, llvm::SmallVector 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); } diff --git a/toolchain/diagnostics/kind.def b/toolchain/diagnostics/kind.def index e0061fb258f1..67a2f20b4714 100644 --- a/toolchain/diagnostics/kind.def +++ b/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 diff --git a/toolchain/diagnostics/sorting_consumer.h b/toolchain/diagnostics/sorting_consumer.h index 4d08db3cfd8e..cd09bdfe30bb 100644 --- a/toolchain/diagnostics/sorting_consumer.h +++ b/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)); } diff --git a/toolchain/diagnostics/sorting_consumer_test.cpp b/toolchain/diagnostics/sorting_consumer_test.cpp index 7c89dfa732e4..b7c7581465d7 100644 --- a/toolchain/diagnostics/sorting_consumer_test.cpp +++ b/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 { +// 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 { 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 { + return diagnostics_; + } + + private: + llvm::SmallVector 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