diff --git a/toolchain/check/call.cpp b/toolchain/check/call.cpp index 8052ef726d3e8..07075683fb939 100644 --- a/toolchain/check/call.cpp +++ b/toolchain/check/call.cpp @@ -19,6 +19,15 @@ namespace Carbon::Check { +namespace { +// Entity kinds, for diagnostics. Converted to an int for a select. +enum class EntityKind : uint8_t { + Function = 0, + GenericClass = 1, + GenericInterface = 2, +}; +} // namespace + // Resolves the callee expression in a call to a specific callee, or diagnoses // if no specific callee can be identified. This verifies the arity of the // callee and determines any compile-time arguments, but doesn't check that the @@ -31,7 +40,7 @@ namespace Carbon::Check { // callee is not generic, or `nullopt` if an error has been diagnosed. static auto ResolveCalleeInCall(Context& context, SemIR::LocId loc_id, const SemIR::EntityWithParamsBase& entity, - llvm::StringLiteral entity_kind_for_diagnostic, + EntityKind entity_kind_for_diagnostic, SemIR::GenericId entity_generic_id, SemIR::SpecificId enclosing_specific_id, SemIR::InstId self_id, @@ -43,16 +52,20 @@ static auto ResolveCalleeInCall(Context& context, SemIR::LocId loc_id, auto params = context.inst_blocks().GetOrEmpty(callee_info.param_refs_id); if (arg_ids.size() != params.size()) { CARBON_DIAGNOSTIC(CallArgCountMismatch, Error, - "{0} argument{0:s} passed to {1} expecting " - "{2} argument{2:s}", - IntAsSelect, llvm::StringLiteral, IntAsSelect); - CARBON_DIAGNOSTIC(InCallToEntity, Note, "calling {0} declared here", - llvm::StringLiteral); + "{0} argument{0:s} passed to " + "{1:=0:function|=1:generic class|=2:generic interface}" + " expecting {2} argument{2:s}", + IntAsSelect, IntAsSelect, IntAsSelect); + CARBON_DIAGNOSTIC( + InCallToEntity, Note, + "calling {0:=0:function|=1:generic class|=2:generic interface}" + " declared here", + IntAsSelect); context.emitter() .Build(loc_id, CallArgCountMismatch, arg_ids.size(), - entity_kind_for_diagnostic, params.size()) + static_cast(entity_kind_for_diagnostic), params.size()) .Note(callee_info.callee_loc, InCallToEntity, - entity_kind_for_diagnostic) + static_cast(entity_kind_for_diagnostic)) .Emit(); return std::nullopt; } @@ -80,8 +93,9 @@ static auto PerformCallToGenericClass(Context& context, SemIR::LocId loc_id, -> SemIR::InstId { const auto& generic_class = context.classes().Get(class_id); auto callee_specific_id = ResolveCalleeInCall( - context, loc_id, generic_class, "generic class", generic_class.generic_id, - enclosing_specific_id, /*self_id=*/SemIR::InstId::Invalid, arg_ids); + context, loc_id, generic_class, EntityKind::GenericClass, + generic_class.generic_id, enclosing_specific_id, + /*self_id=*/SemIR::InstId::Invalid, arg_ids); if (!callee_specific_id) { return SemIR::InstId::BuiltinError; } @@ -99,8 +113,9 @@ static auto PerformCallToGenericInterface( llvm::ArrayRef arg_ids) -> SemIR::InstId { const auto& interface = context.interfaces().Get(interface_id); auto callee_specific_id = ResolveCalleeInCall( - context, loc_id, interface, "generic interface", interface.generic_id, - enclosing_specific_id, /*self_id=*/SemIR::InstId::Invalid, arg_ids); + context, loc_id, interface, EntityKind::GenericInterface, + interface.generic_id, enclosing_specific_id, + /*self_id=*/SemIR::InstId::Invalid, arg_ids); if (!callee_specific_id) { return SemIR::InstId::BuiltinError; } @@ -143,7 +158,7 @@ auto PerformCall(Context& context, SemIR::LocId loc_id, SemIR::InstId callee_id, // If the callee is a generic function, determine the generic argument values // for the call. auto callee_specific_id = ResolveCalleeInCall( - context, loc_id, callable, "function", callable.generic_id, + context, loc_id, callable, EntityKind::Function, callable.generic_id, callee_function.enclosing_specific_id, callee_function.self_id, arg_ids); if (!callee_specific_id) { return SemIR::InstId::BuiltinError; diff --git a/toolchain/check/check.cpp b/toolchain/check/check.cpp index fef83b9bf0cc7..c0bc8554fdf02 100644 --- a/toolchain/check/check.cpp +++ b/toolchain/check/check.cpp @@ -23,6 +23,7 @@ #include "toolchain/check/sem_ir_diagnostic_converter.h" #include "toolchain/diagnostics/diagnostic.h" #include "toolchain/diagnostics/diagnostic_emitter.h" +#include "toolchain/diagnostics/format_providers.h" #include "toolchain/lex/token_kind.h" #include "toolchain/parse/node_ids.h" #include "toolchain/parse/tree.h" @@ -1189,19 +1190,18 @@ static auto BuildApiMapAndDiagnosePackaging( bool is_api_with_impl_ext = !is_impl && filename.ends_with(ImplExt); auto want_ext = is_impl ? ImplExt : ApiExt; if (is_api_with_impl_ext || !filename.ends_with(want_ext)) { - CARBON_DIAGNOSTIC(IncorrectExtension, Error, - "file extension of `{0}` required for `{1}`", - llvm::StringLiteral, Lex::TokenKind); + CARBON_DIAGNOSTIC( + IncorrectExtension, Error, + "file extension of `{0:.impl|}.carbon` required for {0:`impl`|api}", + BoolAsSelect); auto diag = unit_info.emitter.Build( packaging ? packaging->names.node_id : Parse::NodeId::Invalid, - IncorrectExtension, want_ext, - is_impl ? Lex::TokenKind::Impl : Lex::TokenKind::Api); + IncorrectExtension, is_impl); if (is_api_with_impl_ext) { - CARBON_DIAGNOSTIC(IncorrectExtensionImplNote, Note, - "file extension of `{0}` only allowed for `{1}`", - llvm::StringLiteral, Lex::TokenKind); - diag.Note(Parse::NodeId::Invalid, IncorrectExtensionImplNote, ImplExt, - Lex::TokenKind::Impl); + CARBON_DIAGNOSTIC( + IncorrectExtensionImplNote, Note, + "file extension of `.impl.carbon` only allowed for `impl`"); + diag.Note(Parse::NodeId::Invalid, IncorrectExtensionImplNote); } diag.Emit(); } diff --git a/toolchain/check/eval.cpp b/toolchain/check/eval.cpp index e0fb165920501..7225e1618c935 100644 --- a/toolchain/check/eval.cpp +++ b/toolchain/check/eval.cpp @@ -682,65 +682,65 @@ static auto PerformBuiltinBinaryIntOp(Context& context, SemIRLoc loc, bool overflow = false; llvm::APInt result_val; - llvm::StringLiteral op_str = ""; + Lex::TokenKind op_token = Lex::TokenKind::Not; switch (builtin_kind) { // Arithmetic. case SemIR::BuiltinFunctionKind::IntSAdd: result_val = lhs_val.sadd_ov(rhs_val, overflow); - op_str = "+"; + op_token = Lex::TokenKind::Plus; break; case SemIR::BuiltinFunctionKind::IntSSub: result_val = lhs_val.ssub_ov(rhs_val, overflow); - op_str = "-"; + op_token = Lex::TokenKind::Minus; break; case SemIR::BuiltinFunctionKind::IntSMul: result_val = lhs_val.smul_ov(rhs_val, overflow); - op_str = "*"; + op_token = Lex::TokenKind::Star; break; case SemIR::BuiltinFunctionKind::IntSDiv: result_val = lhs_val.sdiv_ov(rhs_val, overflow); - op_str = "/"; + op_token = Lex::TokenKind::Slash; break; case SemIR::BuiltinFunctionKind::IntSMod: result_val = lhs_val.srem(rhs_val); // LLVM weirdly lacks `srem_ov`, so we work it out for ourselves: // % -1 overflows because / -1 overflows. overflow = lhs_val.isMinSignedValue() && rhs_val.isAllOnes(); - op_str = "%"; + op_token = Lex::TokenKind::Percent; break; case SemIR::BuiltinFunctionKind::IntUAdd: result_val = lhs_val + rhs_val; - op_str = "+"; + op_token = Lex::TokenKind::Plus; break; case SemIR::BuiltinFunctionKind::IntUSub: result_val = lhs_val - rhs_val; - op_str = "-"; + op_token = Lex::TokenKind::Minus; break; case SemIR::BuiltinFunctionKind::IntUMul: result_val = lhs_val * rhs_val; - op_str = "*"; + op_token = Lex::TokenKind::Star; break; case SemIR::BuiltinFunctionKind::IntUDiv: result_val = lhs_val.udiv(rhs_val); - op_str = "/"; + op_token = Lex::TokenKind::Slash; break; case SemIR::BuiltinFunctionKind::IntUMod: result_val = lhs_val.urem(rhs_val); - op_str = "%"; + op_token = Lex::TokenKind::Percent; break; // Bitwise. case SemIR::BuiltinFunctionKind::IntAnd: result_val = lhs_val & rhs_val; - op_str = "&"; + op_token = Lex::TokenKind::And; break; case SemIR::BuiltinFunctionKind::IntOr: result_val = lhs_val | rhs_val; - op_str = "|"; + op_token = Lex::TokenKind::Pipe; break; case SemIR::BuiltinFunctionKind::IntXor: result_val = lhs_val ^ rhs_val; - op_str = "^"; + op_token = Lex::TokenKind::Caret; break; // Bit shift. @@ -777,9 +777,9 @@ static auto PerformBuiltinBinaryIntOp(Context& context, SemIRLoc loc, if (overflow) { CARBON_DIAGNOSTIC(CompileTimeIntegerOverflow, Error, "integer overflow in calculation {0} {1} {2}", TypedInt, - llvm::StringLiteral, TypedInt); + Lex::TokenKind, TypedInt); context.emitter().Emit(loc, CompileTimeIntegerOverflow, - {.type = lhs.type_id, .value = lhs_val}, op_str, + {.type = lhs.type_id, .value = lhs_val}, op_token, {.type = rhs.type_id, .value = rhs_val}); } diff --git a/toolchain/check/testdata/packages/fail_extension.carbon b/toolchain/check/testdata/packages/fail_extension.carbon index 37e7b851b1fe9..8ada13ec247b6 100644 --- a/toolchain/check/testdata/packages/fail_extension.carbon +++ b/toolchain/check/testdata/packages/fail_extension.carbon @@ -7,11 +7,11 @@ // TIP: bazel test //toolchain/testing:file_test --test_arg=--file_tests=toolchain/check/testdata/packages/fail_extension.carbon // TIP: To dump output, run: // TIP: bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/check/testdata/packages/fail_extension.carbon -// CHECK:STDERR: fail_main.incorrect: error(IncorrectExtension): file extension of `.carbon` required for `api` +// CHECK:STDERR: fail_main.incorrect: error(IncorrectExtension): file extension of `.carbon` required for api // CHECK:STDERR: // CHECK:STDERR: fail_main_redundant_with_swapped_ext.impl.carbon: error(DuplicateMainApi): `Main//default` previously provided by `fail_main.incorrect` // CHECK:STDERR: -// CHECK:STDERR: fail_main_redundant_with_swapped_ext.impl.carbon: error(IncorrectExtension): file extension of `.carbon` required for `api` +// CHECK:STDERR: fail_main_redundant_with_swapped_ext.impl.carbon: error(IncorrectExtension): file extension of `.carbon` required for api // CHECK:STDERR: fail_main_redundant_with_swapped_ext.impl.carbon: note(IncorrectExtensionImplNote): file extension of `.impl.carbon` only allowed for `impl` // CHECK:STDERR: @@ -21,7 +21,7 @@ // --- fail_main_lib.incorrect -// CHECK:STDERR: fail_main_lib.incorrect:[[@LINE+4]]:1: error(IncorrectExtension): file extension of `.carbon` required for `api` +// CHECK:STDERR: fail_main_lib.incorrect:[[@LINE+4]]:1: error(IncorrectExtension): file extension of `.carbon` required for api // CHECK:STDERR: library "main_lib"; // CHECK:STDERR: ^~~~~~~ // CHECK:STDERR: @@ -37,7 +37,7 @@ impl library "[[@TEST_NAME]]"; // --- fail_package.incorrect -// CHECK:STDERR: fail_package.incorrect:[[@LINE+4]]:1: error(IncorrectExtension): file extension of `.carbon` required for `api` +// CHECK:STDERR: fail_package.incorrect:[[@LINE+4]]:1: error(IncorrectExtension): file extension of `.carbon` required for api // CHECK:STDERR: package Package; // CHECK:STDERR: ^~~~~~~ // CHECK:STDERR: @@ -53,7 +53,7 @@ impl package Package; // --- fail_package_lib.incorrect -// CHECK:STDERR: fail_package_lib.incorrect:[[@LINE+4]]:1: error(IncorrectExtension): file extension of `.carbon` required for `api` +// CHECK:STDERR: fail_package_lib.incorrect:[[@LINE+4]]:1: error(IncorrectExtension): file extension of `.carbon` required for api // CHECK:STDERR: package Package library "package_lib"; // CHECK:STDERR: ^~~~~~~ // CHECK:STDERR: @@ -69,7 +69,7 @@ impl package Package library "[[@TEST_NAME]]"; // --- fail_swapped_ext.impl.carbon -// CHECK:STDERR: fail_swapped_ext.impl.carbon:[[@LINE+5]]:1: error(IncorrectExtension): file extension of `.carbon` required for `api` +// CHECK:STDERR: fail_swapped_ext.impl.carbon:[[@LINE+5]]:1: error(IncorrectExtension): file extension of `.carbon` required for api // CHECK:STDERR: package SwappedExt; // CHECK:STDERR: ^~~~~~~ // CHECK:STDERR: fail_swapped_ext.impl.carbon: note(IncorrectExtensionImplNote): file extension of `.impl.carbon` only allowed for `impl` diff --git a/toolchain/diagnostics/diagnostic.h b/toolchain/diagnostics/diagnostic.h index dbbc51cceaeae..9754764324207 100644 --- a/toolchain/diagnostics/diagnostic.h +++ b/toolchain/diagnostics/diagnostic.h @@ -117,9 +117,11 @@ struct DiagnosticBase { explicit constexpr DiagnosticBase(DiagnosticKind kind, DiagnosticLevel level, llvm::StringLiteral format) : Kind(kind), Level(level), Format(format) { - static_assert((... && !std::is_same_v), - "Use std::string or llvm::StringLiteral for diagnostics to " - "avoid lifetime issues."); + static_assert((... && !(std::is_same_v || + std::is_same_v)), + "String type disallowed in diagnostics. See " + "https://github.com/carbon-language/carbon-lang/blob/trunk/" + "toolchain/docs/diagnostics.md#diagnostic-parameter-types"); } // The diagnostic's kind. diff --git a/toolchain/diagnostics/diagnostic_emitter_test.cpp b/toolchain/diagnostics/diagnostic_emitter_test.cpp index 50ad5a0536eea..d6e704b77be47 100644 --- a/toolchain/diagnostics/diagnostic_emitter_test.cpp +++ b/toolchain/diagnostics/diagnostic_emitter_test.cpp @@ -55,7 +55,7 @@ TEST_F(DiagnosticEmitterTest, EmitSimpleWarning) { } TEST_F(DiagnosticEmitterTest, EmitOneArgDiagnostic) { - CARBON_DIAGNOSTIC(TestDiagnostic, Error, "arg: `{0}`", llvm::StringLiteral); + CARBON_DIAGNOSTIC(TestDiagnostic, Error, "arg: `{0}`", std::string); EXPECT_CALL(consumer_, HandleDiagnostic(IsSingleDiagnostic( DiagnosticKind::TestDiagnostic, DiagnosticLevel::Error, 1, 1, "arg: `str`"))); diff --git a/toolchain/diagnostics/sorting_diagnostic_consumer_test.cpp b/toolchain/diagnostics/sorting_diagnostic_consumer_test.cpp index d97f535b063f3..46e53deb61f98 100644 --- a/toolchain/diagnostics/sorting_diagnostic_consumer_test.cpp +++ b/toolchain/diagnostics/sorting_diagnostic_consumer_test.cpp @@ -17,7 +17,7 @@ namespace { using ::Carbon::Testing::IsSingleDiagnostic; using ::testing::InSequence; -CARBON_DIAGNOSTIC(TestDiagnostic, Error, "{0}", llvm::StringLiteral); +CARBON_DIAGNOSTIC(TestDiagnostic, Error, "M{0}", int); struct FakeDiagnosticConverter : DiagnosticConverter { auto ConvertLoc(DiagnosticLoc loc, ContextFnT /*context_fn*/) const @@ -32,12 +32,12 @@ TEST(SortedDiagnosticEmitterTest, SortErrors) { SortingDiagnosticConsumer sorting_consumer(consumer); DiagnosticEmitter emitter(converter, sorting_consumer); - emitter.Emit({"f", "line", 2, 1}, TestDiagnostic, "M1"); - emitter.Emit({"f", "line", 1, 1}, TestDiagnostic, "M2"); - emitter.Emit({"f", "line", 1, 3}, TestDiagnostic, "M3"); - emitter.Emit({"f", "line", 3, 4}, TestDiagnostic, "M4"); - emitter.Emit({"f", "line", 3, 2}, TestDiagnostic, "M5"); - emitter.Emit({"f", "line", 3, 2}, TestDiagnostic, "M6"); + emitter.Emit({"f", "line", 2, 1}, TestDiagnostic, 1); + emitter.Emit({"f", "line", 1, 1}, TestDiagnostic, 2); + emitter.Emit({"f", "line", 1, 3}, TestDiagnostic, 3); + emitter.Emit({"f", "line", 3, 4}, TestDiagnostic, 4); + emitter.Emit({"f", "line", 3, 2}, TestDiagnostic, 5); + emitter.Emit({"f", "line", 3, 2}, TestDiagnostic, 6); InSequence s; EXPECT_CALL(consumer, HandleDiagnostic(IsSingleDiagnostic( diff --git a/toolchain/docs/diagnostics.md b/toolchain/docs/diagnostics.md index 603b553614e6e..f0715feefbf39 100644 --- a/toolchain/docs/diagnostics.md +++ b/toolchain/docs/diagnostics.md @@ -167,9 +167,12 @@ methods for formatting arguments: - This includes `char` and integer types (`int`, `int32_t`, and so on). - String types can be added as needed, but stringifying values using the methods noted below is preferred. - - Use `llvm::StringLiteral` where appropriate; use `std::string` when - allocations are required. + - Use `std::string` when allocations are required. - `llvm::StringRef` is disallowed due to lifetime issues. + - `llvm::StringLiteral` is disallowed because format providers such as + `BoolAsSelect` should work in cases where a `StringLiteral` could be + used, and because string literal parameters tend to make the + resulting diagnostics hard to translate. - `llvm::format_provider<...>` specializations. - `BoolAsSelect` and `IntAsSelect` from [format_providers.h](/toolchain/diagnostics/format_providers.h) are diff --git a/toolchain/lex/token_kind.def b/toolchain/lex/token_kind.def index 78c26a75cba77..6177fbeb66058 100644 --- a/toolchain/lex/token_kind.def +++ b/toolchain/lex/token_kind.def @@ -167,7 +167,6 @@ CARBON_KEYWORD_TOKEN(Abstract, "abstract") CARBON_KEYWORD_TOKEN(Addr, "addr") CARBON_TOKEN_WITH_VIRTUAL_NODE( CARBON_KEYWORD_TOKEN(And, "and")) -CARBON_KEYWORD_TOKEN(Api, "api") CARBON_KEYWORD_TOKEN(As, "as") CARBON_KEYWORD_TOKEN(Auto, "auto") CARBON_KEYWORD_TOKEN(Bool, "bool") diff --git a/toolchain/parse/context.cpp b/toolchain/parse/context.cpp index ca5cfe21ce85e..9509dcb6cff0f 100644 --- a/toolchain/parse/context.cpp +++ b/toolchain/parse/context.cpp @@ -77,9 +77,9 @@ auto Context::ConsumeAndAddCloseSymbol(Lex::TokenIndex expected_open, // TODO: Include the location of the matching opening delimiter in the // diagnostic. CARBON_DIAGNOSTIC(ExpectedCloseSymbol, Error, - "unexpected tokens before `{0}`", llvm::StringLiteral); + "unexpected tokens before `{0}`", Lex::TokenKind); emitter_->Emit(*position_, ExpectedCloseSymbol, - open_token_kind.closing_symbol().fixed_spelling()); + open_token_kind.closing_symbol()); SkipTo(tokens().GetMatchedClosingToken(expected_open)); AddNode(close_kind, Consume(), /*has_error=*/true); diff --git a/toolchain/parse/handle_binding_pattern.cpp b/toolchain/parse/handle_binding_pattern.cpp index d77063f3bb0b7..79556418e5482 100644 --- a/toolchain/parse/handle_binding_pattern.cpp +++ b/toolchain/parse/handle_binding_pattern.cpp @@ -2,6 +2,7 @@ // Exceptions. See /LICENSE for license information. // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +#include "toolchain/diagnostics/format_providers.h" #include "toolchain/parse/context.h" #include "toolchain/parse/handle.h" @@ -25,12 +26,13 @@ auto HandleBindingPattern(Context& context) -> void { } // Handle an invalid pattern introducer for parameters and variables. - auto on_error = [&](llvm::StringLiteral expected) { + auto on_error = [&](bool expected_name) { if (!state.has_error) { CARBON_DIAGNOSTIC(ExpectedBindingPattern, Error, - "expected {0} in binding pattern", llvm::StringLiteral); + "expected {0:name|`:` or `:!`} in binding pattern", + BoolAsSelect); context.emitter().Emit(*context.position(), ExpectedBindingPattern, - expected); + expected_name); state.has_error = true; } }; @@ -51,7 +53,7 @@ auto HandleBindingPattern(Context& context) -> void { // Add a placeholder for the name. context.AddLeafNode(NodeKind::IdentifierName, *context.position(), /*has_error=*/true); - on_error("name"); + on_error(/*expected_name=*/true); } if (auto kind = context.PositionKind(); @@ -64,7 +66,7 @@ auto HandleBindingPattern(Context& context) -> void { context.PushState(state); context.PushStateForExpr(PrecedenceGroup::ForType()); } else { - on_error("`:` or `:!`"); + on_error(/*expected_name=*/false); // Add a placeholder for the type. context.AddLeafNode(NodeKind::InvalidParse, *context.position(), /*has_error=*/true);