diff --git a/toolchain/diagnostics/diagnostic_emitter.h b/toolchain/diagnostics/diagnostic_emitter.h index cfe9d0e1425c..94eb424c08bf 100644 --- a/toolchain/diagnostics/diagnostic_emitter.h +++ b/toolchain/diagnostics/diagnostic_emitter.h @@ -8,9 +8,9 @@ #include #include #include +#include #include "llvm/ADT/Any.h" -#include "llvm/ADT/STLExtras.h" #include "llvm/ADT/SmallVector.h" #include "llvm/ADT/StringRef.h" #include "llvm/Support/FormatVariadic.h" @@ -20,6 +20,9 @@ namespace Carbon { enum class DiagnosticLevel : int8_t { + // A note, not indicating an error on its own, but possibly providing context + // for an error. + Note, // A warning diagnostic, indicating a likely problem with the program. Warning, // An error diagnostic, indicating that the program is not valid. @@ -56,6 +59,26 @@ struct DiagnosticLocation { // An instance of a single error or warning. Information about the diagnostic // can be recorded into it for more complex consumers. struct Diagnostic { + Diagnostic(DiagnosticKind kind, DiagnosticLevel level, + DiagnosticLocation location, llvm::StringLiteral format, + llvm::SmallVector format_args, + std::function format_fn, + llvm::SmallVector> notes) + : kind(kind), + level(level), + location(std::move(location)), + format(format), + format_args(std::move(format_args)), + format_fn(std::move(format_fn)), + notes(std::move(notes)) {} + + Diagnostic(Diagnostic&& other) = default; + auto operator=(Diagnostic&& other) -> Diagnostic& = default; + + // The copy operations are implicitly deleted -- this is just being explicit. + Diagnostic(const Diagnostic&) = delete; + auto operator=(const Diagnostic&) -> Diagnostic& = delete; + // The diagnostic's kind. DiagnosticKind kind; @@ -79,6 +102,9 @@ struct Diagnostic { // Returns the formatted string. By default, this uses llvm::formatv. std::function format_fn; + + // Notes that add context or supplemental information to the diagnostic. + llvm::SmallVector> notes; }; // Receives diagnostics as they are emitted. @@ -87,7 +113,7 @@ class DiagnosticConsumer { virtual ~DiagnosticConsumer() = default; // Handle a diagnostic. - virtual auto HandleDiagnostic(const Diagnostic& diagnostic) -> void = 0; + virtual auto HandleDiagnostic(Diagnostic diagnostic) -> void = 0; // Flushes any buffered input. virtual auto Flush() -> void {} @@ -155,6 +181,62 @@ struct DiagnosticBase { template class DiagnosticEmitter { public: + // A builder-pattern type to provide a fluent interface for constructing + // a more complex diagnostic. See `DiagnosticEmitter::Build` for the + // expected usage. + class DiagnosticBuilder { + public: + // Adds a note diagnostic attached to the main diagnostic being built. + // The API mirrors the main emission API: `DiagnosticEmitter::Emit`. + // For the expected usage see the builder API: `DiagnosticEmitter::Build`. + template + auto Note(LocationT location, + const Internal::DiagnosticBase& diagnostic_base, + // Disable type deduction based on `args`; the type of + // `diagnostic_base` determines the diagnostic's parameter types. + typename std::common_type_t... args) -> DiagnosticBuilder& { + diagnostic_.notes.emplace_back(std::make_unique( + diagnostic_base.Kind, diagnostic_base.Level, + emitter_->translator_->GetLocation(location), diagnostic_base.Format, + llvm::SmallVector({std::move(args)...}), + [&diagnostic_base](const Diagnostic& diagnostic) -> std::string { + return diagnostic_base.FormatFn(diagnostic); + }, + llvm::SmallVector>())); + return *this; + } + + // Emits the built diagnostic and its attached notes. + // For the expected usage see the builder API: `DiagnosticEmitter::Build`. + template + auto Emit() -> void { + emitter_->consumer_->HandleDiagnostic(std::move(diagnostic_)); + } + + private: + friend class DiagnosticEmitter; + + template + explicit DiagnosticBuilder( + DiagnosticEmitter* emitter, LocationT location, + const Internal::DiagnosticBase& diagnostic_base, + // Disable type deduction based on `args`; the type of + // `diagnostic_base` determines the diagnostic's parameter types. + typename std::common_type_t... args) + : emitter_(emitter), + diagnostic_( + diagnostic_base.Kind, diagnostic_base.Level, + emitter_->translator_->GetLocation(location), + diagnostic_base.Format, {std::move(args)...}, + [&diagnostic_base](const Diagnostic& diagnostic) -> std::string { + return diagnostic_base.FormatFn(diagnostic); + }, + {}) {} + + DiagnosticEmitter* emitter_; + Diagnostic diagnostic_; + }; + // The `translator` and `consumer` are required to outlive the diagnostic // emitter. explicit DiagnosticEmitter( @@ -168,20 +250,29 @@ class DiagnosticEmitter { // When passing arguments, they may be buffered. As a consequence, lifetimes // may outlive the `Emit` call. template - void Emit(LocationT location, + auto Emit(LocationT location, const Internal::DiagnosticBase& diagnostic_base, // Disable type deduction based on `args`; the type of // `diagnostic_base` determines the diagnostic's parameter types. - typename std::common_type_t... args) { - consumer_->HandleDiagnostic({ - .kind = diagnostic_base.Kind, - .level = diagnostic_base.Level, - .location = translator_->GetLocation(location), - .format = diagnostic_base.Format, - .format_args = {std::move(args)...}, - .format_fn = [&diagnostic_base](const Diagnostic& diagnostic) - -> std::string { return diagnostic_base.FormatFn(diagnostic); }, - }); + typename std::common_type_t... args) -> void { + DiagnosticBuilder(this, location, diagnostic_base, std::move(args)...) + .Emit(); + } + + // A fluent interface for building a diagnostic and attaching notes for added + // context or information. For example: + // + // emitter_.Build(location1, MyDiagnostic) + // .Note(location2, MyDiagnosticNote) + // .Emit(); + template + auto Build(LocationT location, + const Internal::DiagnosticBase& diagnostic_base, + // Disable type deduction based on `args`; the type of + // `diagnostic_base` determines the diagnostic's parameter types. + typename std::common_type_t... args) -> DiagnosticBuilder { + return DiagnosticBuilder(this, location, diagnostic_base, + std::move(args)...); } private: @@ -190,8 +281,14 @@ class DiagnosticEmitter { }; inline auto ConsoleDiagnosticConsumer() -> DiagnosticConsumer& { - struct Consumer : DiagnosticConsumer { - auto HandleDiagnostic(const Diagnostic& diagnostic) -> void override { + class Consumer : public DiagnosticConsumer { + auto HandleDiagnostic(Diagnostic diagnostic) -> void override { + Print(diagnostic); + for (auto& note : diagnostic.notes) { + Print(*note); + } + } + auto Print(const Diagnostic& diagnostic) -> void { llvm::errs() << diagnostic.location.file_name << ":" << diagnostic.location.line_number << ":" << diagnostic.location.column_number << ": " @@ -209,9 +306,9 @@ class ErrorTrackingDiagnosticConsumer : public DiagnosticConsumer { explicit ErrorTrackingDiagnosticConsumer(DiagnosticConsumer& next_consumer) : next_consumer_(&next_consumer) {} - auto HandleDiagnostic(const Diagnostic& diagnostic) -> void override { + auto HandleDiagnostic(Diagnostic diagnostic) -> void override { seen_error_ |= diagnostic.level == DiagnosticLevel::Error; - next_consumer_->HandleDiagnostic(diagnostic); + next_consumer_->HandleDiagnostic(std::move(diagnostic)); } // Reset whether we've seen an error. diff --git a/toolchain/diagnostics/diagnostic_emitter_test.cpp b/toolchain/diagnostics/diagnostic_emitter_test.cpp index c4d2fae4c0b8..cdc0bc45a372 100644 --- a/toolchain/diagnostics/diagnostic_emitter_test.cpp +++ b/toolchain/diagnostics/diagnostic_emitter_test.cpp @@ -58,5 +58,14 @@ TEST_F(DiagnosticEmitterTest, EmitOneArgDiagnostic) { emitter_.Emit(1, TestDiagnostic, "str"); } +TEST_F(DiagnosticEmitterTest, EmitNote) { + CARBON_DIAGNOSTIC(TestDiagnostic, Warning, "simple warning"); + EXPECT_CALL(consumer_, + HandleDiagnostic(IsDiagnostic(DiagnosticKind::TestDiagnostic, + DiagnosticLevel::Warning, 1, 1, + "simple warning"))); + emitter_.Build(1, TestDiagnostic).Note(2, TestDiagnostic).Emit(); +} + } // namespace } // namespace Carbon::Testing diff --git a/toolchain/diagnostics/mocks.h b/toolchain/diagnostics/mocks.h index b51f864cca74..55c75e53db72 100644 --- a/toolchain/diagnostics/mocks.h +++ b/toolchain/diagnostics/mocks.h @@ -13,8 +13,7 @@ namespace Carbon::Testing { class MockDiagnosticConsumer : public DiagnosticConsumer { public: - MOCK_METHOD(void, HandleDiagnostic, (const Diagnostic& diagnostic), - (override)); + MOCK_METHOD(void, HandleDiagnostic, (Diagnostic diagnostic), (override)); }; MATCHER_P(IsDiagnosticMessage, matcher, "") { diff --git a/toolchain/diagnostics/null_diagnostics.h b/toolchain/diagnostics/null_diagnostics.h index 91bfd785c30b..2d3bfe438293 100644 --- a/toolchain/diagnostics/null_diagnostics.h +++ b/toolchain/diagnostics/null_diagnostics.h @@ -23,7 +23,7 @@ inline auto NullDiagnosticLocationTranslator() inline auto NullDiagnosticConsumer() -> DiagnosticConsumer& { struct Consumer : DiagnosticConsumer { - auto HandleDiagnostic(const Diagnostic& d) -> void override {} + auto HandleDiagnostic(Diagnostic d) -> void override {} }; static auto* consumer = new Consumer; return *consumer; diff --git a/toolchain/diagnostics/sorting_diagnostic_consumer.h b/toolchain/diagnostics/sorting_diagnostic_consumer.h index 42f0944c02a1..d22003569745 100644 --- a/toolchain/diagnostics/sorting_diagnostic_consumer.h +++ b/toolchain/diagnostics/sorting_diagnostic_consumer.h @@ -20,8 +20,8 @@ class SortingDiagnosticConsumer : public DiagnosticConsumer { ~SortingDiagnosticConsumer() override { Flush(); } // Buffers the diagnostic. - auto HandleDiagnostic(const Diagnostic& diagnostic) -> void override { - diagnostics_.push_back(diagnostic); + auto HandleDiagnostic(Diagnostic diagnostic) -> void override { + diagnostics_.push_back(std::move(diagnostic)); } // Sorts and flushes buffered diagnostics. @@ -31,8 +31,8 @@ class SortingDiagnosticConsumer : public DiagnosticConsumer { return std::tie(lhs.location.line_number, lhs.location.column_number) < std::tie(rhs.location.line_number, rhs.location.column_number); }); - for (const auto& diagnostic : diagnostics_) { - next_consumer_->HandleDiagnostic(diagnostic); + for (auto& diag : diagnostics_) { + next_consumer_->HandleDiagnostic(std::move(diag)); } diagnostics_.clear(); } diff --git a/toolchain/semantics/semantics_parse_tree_handler.cpp b/toolchain/semantics/semantics_parse_tree_handler.cpp index 9d06caaa913a..f321b5260a7c 100644 --- a/toolchain/semantics/semantics_parse_tree_handler.cpp +++ b/toolchain/semantics/semantics_parse_tree_handler.cpp @@ -108,15 +108,13 @@ auto SemanticsParseTreeHandler::BindName(ParseTree::Node name_node, } else { CARBON_DIAGNOSTIC(NameRedefined, Error, "Redefining {0} in the same scope.", llvm::StringRef); - emitter_->Emit(name_node, NameRedefined, name_str); - - // TODO: This should be a note and sorted with the above diagnostic. - // But that depends on more diagnostic support we currently don't have. + CARBON_DIAGNOSTIC(PreviousDefinition, Note, "Previous definition is here."); auto prev_def_id = name_lookup_[name_id].back(); auto prev_def = semantics_->GetNode(prev_def_id); - CARBON_DIAGNOSTIC(PreviousDefinition, Error, - "Previous definition is here."); - emitter_->Emit(prev_def.parse_node(), PreviousDefinition); + + emitter_->Build(name_node, NameRedefined, name_str) + .Note(prev_def.parse_node(), PreviousDefinition) + .Emit(); } } diff --git a/toolchain/semantics/testdata/var/fail_duplicate_decl.carbon b/toolchain/semantics/testdata/var/fail_duplicate_decl.carbon index dcf6dd9e5594..b784f4f82d72 100644 --- a/toolchain/semantics/testdata/var/fail_duplicate_decl.carbon +++ b/toolchain/semantics/testdata/var/fail_duplicate_decl.carbon @@ -50,8 +50,8 @@ fn Main() { - // CHECK:STDERR: {{.*}}/toolchain/semantics/testdata/var/fail_duplicate_decl.carbon:[[@LINE+1]]:7: Previous definition is here. var x: i32 = 0; - // CHECK:STDERR: {{.*}}/toolchain/semantics/testdata/var/fail_duplicate_decl.carbon:[[@LINE+1]]:7: Redefining x in the same scope. + // CHECK:STDERR: {{.*}}/toolchain/semantics/testdata/var/fail_duplicate_decl.carbon:[[@LINE+2]]:7: Redefining x in the same scope. + // CHECK:STDERR: {{.*}}/toolchain/semantics/testdata/var/fail_duplicate_decl.carbon:[[@LINE-2]]:7: Previous definition is here. var x: i32 = 0; }