From 302aa1bb30b0c9940a3974db9f2b9131c7106713 Mon Sep 17 00:00:00 2001 From: Jon Ross-Perkins Date: Mon, 21 Oct 2024 12:39:53 -0700 Subject: [PATCH] Remove uses of StringLiteral in format strings. (#4416) Building on #4411, avoid using StringLiteral in format strings. This includes a diagnostic check to prevent regressions (which is also how I gathered issues). Note, I haven't looked at `std::string` uses yet, but we might need things like that to be able to pass strings in code back to the user. StringLiteral though means that it's literally written down in the toolchain, at which point it should probably be written in the format string instead of separately. --------- Co-authored-by: Geoff Romer --- toolchain/check/call.cpp | 41 +++++++++++++------ toolchain/check/check.cpp | 20 ++++----- toolchain/check/eval.cpp | 32 +++++++-------- .../testdata/packages/fail_extension.carbon | 12 +++--- toolchain/diagnostics/diagnostic.h | 8 ++-- .../diagnostics/diagnostic_emitter_test.cpp | 2 +- .../sorting_diagnostic_consumer_test.cpp | 14 +++---- toolchain/docs/diagnostics.md | 7 +++- toolchain/lex/token_kind.def | 1 - toolchain/parse/context.cpp | 4 +- toolchain/parse/handle_binding_pattern.cpp | 12 +++--- 11 files changed, 87 insertions(+), 66 deletions(-) diff --git a/toolchain/check/call.cpp b/toolchain/check/call.cpp index 8052ef726d3e..07075683fb93 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 fef83b9bf0cc..c0bc8554fdf0 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 e0fb16592050..7225e1618c93 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 37e7b851b1fe..8ada13ec247b 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 dbbc51cceaea..975476432420 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 50ad5a0536ee..d6e704b77be4 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 d97f535b063f..46e53deb61f9 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 603b553614e6..f0715feefbf3 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 78c26a75cba7..6177fbeb6605 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 ca5cfe21ce85..9509dcb6cff0 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 d77063f3bb0b..79556418e548 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);