From 84a8c458f968a483b167a04bbc1d3dee482a5307 Mon Sep 17 00:00:00 2001 From: Jon Ross-Perkins Date: Wed, 19 Mar 2025 16:33:52 -0700 Subject: [PATCH] Error when using emitter.Build().Emit() (#5152) I've found myself cleaning up other cases of this, so moving to disallow it. ``` In file included from toolchain/check/merge.cpp:5: In file included from ./toolchain/check/merge.h:8: In file included from ./toolchain/check/context.h:13: In file included from ./toolchain/check/decl_introducer_state.h:8: In file included from ./toolchain/check/keyword_modifier_set.h:11: In file included from ./toolchain/sem_ir/name_scope.h:10: In file included from ./toolchain/sem_ir/ids.h:12: ./toolchain/diagnostics/diagnostic_emitter.h:95:11: error: static assertion failed: Use `emitter.Emit(...)` or `emitter.Build(...).Note(...).Emit(...)` instead of `emitter.Build(...).Emit(...)` 95 | false, | ^~~~~ toolchain/check/merge.cpp:92:61: note: in instantiation of function template specialization 'Carbon::DiagnosticEmitter::DiagnosticBuilder::Emit<>' requested here 92 | context.emitter().Build(loc, ExternRequiresDeclInApiFile).Emit(); | ^ 1 error generated. ``` --- toolchain/check/merge.cpp | 2 +- toolchain/diagnostics/diagnostic_emitter.h | 29 +++++++++++++++++++--- 2 files changed, 26 insertions(+), 5 deletions(-) diff --git a/toolchain/check/merge.cpp b/toolchain/check/merge.cpp index 4e2734fbb26c..86e5037ad234 100644 --- a/toolchain/check/merge.cpp +++ b/toolchain/check/merge.cpp @@ -89,7 +89,7 @@ auto DiagnoseExternRequiresDeclInApiFile(Context& context, SemIRLoc loc) CARBON_DIAGNOSTIC( ExternRequiresDeclInApiFile, Error, "`extern` entities must have a declaration in the API file"); - context.emitter().Build(loc, ExternRequiresDeclInApiFile).Emit(); + context.emitter().Emit(loc, ExternRequiresDeclInApiFile); } auto DiagnoseIfInvalidRedecl(Context& context, Lex::TokenKind decl_kind, diff --git a/toolchain/diagnostics/diagnostic_emitter.h b/toolchain/diagnostics/diagnostic_emitter.h index 995590379372..3119636045cb 100644 --- a/toolchain/diagnostics/diagnostic_emitter.h +++ b/toolchain/diagnostics/diagnostic_emitter.h @@ -86,7 +86,11 @@ class DiagnosticEmitter { // Emits the built diagnostic and its attached notes. // For the expected usage see the builder API: `DiagnosticEmitter::Build`. template - auto Emit() -> void; + auto Emit() & -> void; + + // Prevent trivial uses of the builder; always `static_assert`s. + template + auto Emit() && -> void; // Returns true if this DiagnosticBuilder may emit a diagnostic. Can be used // to avoid excess work computing notes, etc, if no diagnostic is going to @@ -282,7 +286,7 @@ auto DiagnosticEmitter::DiagnosticBuilder::Note( template template -auto DiagnosticEmitter::DiagnosticBuilder::Emit() -> void { +auto DiagnosticEmitter::DiagnosticBuilder::Emit() & -> void { if (!emitter_) { return; } @@ -292,6 +296,22 @@ auto DiagnosticEmitter::DiagnosticBuilder::Emit() -> void { emitter_->consumer_->HandleDiagnostic(std::move(diagnostic_)); } +namespace Internal { +template +concept AlwaysFalse = false; +} // namespace Internal + +template +template +auto DiagnosticEmitter::DiagnosticBuilder::Emit() && -> void { + // TODO: This is required by clang-16, but `false` may work in newer clang + // versions. Replace when possible. + static_assert(Internal::AlwaysFalse, + "Use `emitter.Emit(...)` or " + "`emitter.Build(...).Note(...).Emit(...)` " + "instead of `emitter.Build(...).Emit(...)`"); +} + template template DiagnosticEmitter::DiagnosticBuilder::DiagnosticBuilder( @@ -364,8 +384,9 @@ template auto DiagnosticEmitter::Emit( LocT loc, const DiagnosticBase& diagnostic_base, Internal::NoTypeDeduction... args) -> void { - DiagnosticBuilder(this, loc, diagnostic_base, {MakeAny(args)...}) - .Emit(); + DiagnosticBuilder builder(this, loc, diagnostic_base, + {MakeAny(args)...}); + builder.Emit(); } template