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 <gromer@google.com>

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<int>(entity_kind_for_diagnostic), params.size())
         .Note(callee_info.callee_loc, InCallToEntity,
-              entity_kind_for_diagnostic)
+              static_cast<int>(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<SemIR::InstId> 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;

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();
       }

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 = "<error>";
+  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:
       // <signed min> % -1 overflows because <signed min> / -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});
   }
 

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`

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<Args, llvm::StringRef>),
-                  "Use std::string or llvm::StringLiteral for diagnostics to "
-                  "avoid lifetime issues.");
+    static_assert((... && !(std::is_same_v<Args, llvm::StringRef> ||
+                            std::is_same_v<Args, llvm::StringLiteral>)),
+                  "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.

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`")));

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<DiagnosticLoc> {
   auto ConvertLoc(DiagnosticLoc loc, ContextFnT /*context_fn*/) const
@@ -32,12 +32,12 @@ TEST(SortedDiagnosticEmitterTest, SortErrors) {
   SortingDiagnosticConsumer sorting_consumer(consumer);
   DiagnosticEmitter<DiagnosticLoc> 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(

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

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")

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);

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);