Error when passing StringRef to CARBON_DIAGNOSTIC. (#3431)

This gets to a lifetime subtlety, particularly with things like the
sorting diagnostic consumer that delay output. In order to reduce the
chance of accidental references, disallow StringRef in the diagnostics.

For example:

```
./toolchain/diagnostics/diagnostic_emitter.h:162:5: error: static_assert failed due to requirement '!std::is_same_v<llvm::StringRef, llvm::StringRef>' "Use std::string or llvm::StringLiteral for diagnostic lifetimes."
    static_assert(
    ^
toolchain/check/convert.cpp:477:11: note: in instantiation of member function 'Carbon::Internal::DiagnosticBase<std::string, std::string, llvm::StringRef>::DiagnosticBase' requested here
          CARBON_DIAGNOSTIC(StructInitMissingFieldInConversion, Error,
          ^
./toolchain/diagnostics/diagnostic_emitter.h:47:7: note: expanded from macro 'CARBON_DIAGNOSTIC'
      ::Carbon::Internal::DiagnosticBase<__VA_ARGS__>(        \
      ^
```

---------

Co-authored-by: Chandler Carruth <chandlerc@gmail.com>

toolchain/check/check.cpp

@@ -339,16 +339,16 @@ static auto BuildApiMapAndDiagnosePackaging(
         if (packaging) {
           CARBON_DIAGNOSTIC(DuplicateLibraryApi, Error,
                             "Library's API previously provided by `{0}`.",
-                            llvm::StringRef);
+                            std::string);
           unit_info.emitter.Emit(packaging->names.node, DuplicateLibraryApi,
-                                 prev_filename);
+                                 prev_filename.str());
         } else {
           CARBON_DIAGNOSTIC(DuplicateMainApi, Error,
                             "Main//default previously provided by `{0}`.",
-                            llvm::StringRef);
+                            std::string);
           // Use the invalid node because there's no node to associate with.
           unit_info.emitter.Emit(Parse::Node::Invalid, DuplicateMainApi,
-                                 prev_filename);
+                                 prev_filename.str());
         }
       }
     }

toolchain/check/context.cpp

@@ -92,9 +92,8 @@ auto Context::DiagnoseDuplicateName(Parse::Node parse_node,
 
 auto Context::DiagnoseNameNotFound(Parse::Node parse_node,
                                    SemIR::NameId name_id) -> void {
-  CARBON_DIAGNOSTIC(NameNotFound, Error, "Name `{0}` not found.",
-                    llvm::StringRef);
-  emitter_->Emit(parse_node, NameNotFound, names().GetFormatted(name_id));
+  CARBON_DIAGNOSTIC(NameNotFound, Error, "Name `{0}` not found.", std::string);
+  emitter_->Emit(parse_node, NameNotFound, names().GetFormatted(name_id).str());
 }
 
 auto Context::NoteIncompleteClass(SemIR::ClassId class_id,

toolchain/check/convert.cpp

@@ -469,20 +469,20 @@ static auto ConvertStructToStructOrClass(Context& context,
           CARBON_DIAGNOSTIC(
               StructInitMissingFieldInLiteral, Error,
               "Missing value for field `{0}` in struct initialization.",
-              llvm::StringRef);
+              std::string);
           context.emitter().Emit(
               value.parse_node(), StructInitMissingFieldInLiteral,
-              sem_ir.names().GetFormatted(dest_field.name_id));
+              sem_ir.names().GetFormatted(dest_field.name_id).str());
         } else {
           CARBON_DIAGNOSTIC(StructInitMissingFieldInConversion, Error,
                             "Cannot convert from struct type `{0}` to `{1}`: "
                             "missing field `{2}` in source type.",
-                            std::string, std::string, llvm::StringRef);
+                            std::string, std::string, std::string);
           context.emitter().Emit(
               value.parse_node(), StructInitMissingFieldInConversion,
               sem_ir.StringifyType(value.type_id()),
               sem_ir.StringifyType(target.type_id),
-              sem_ir.names().GetFormatted(dest_field.name_id));
+              sem_ir.names().GetFormatted(dest_field.name_id).str());
         }
         return SemIR::InstId::BuiltinError;
       }

toolchain/check/handle_class.cpp

@@ -127,12 +127,12 @@ auto HandleClassDefinitionStart(Context& context, Parse::Node parse_node)
   // Track that this declaration is the definition.
   if (class_info.definition_id.is_valid()) {
     CARBON_DIAGNOSTIC(ClassRedefinition, Error, "Redefinition of class {0}.",
-                      llvm::StringRef);
+                      std::string);
     CARBON_DIAGNOSTIC(ClassPreviousDefinition, Note,
                       "Previous definition was here.");
     context.emitter()
         .Build(parse_node, ClassRedefinition,
-               context.names().GetFormatted(class_info.name_id))
+               context.names().GetFormatted(class_info.name_id).str())
         .Note(context.insts().Get(class_info.definition_id).parse_node(),
               ClassPreviousDefinition)
         .Emit();

toolchain/check/handle_function.cpp

@@ -168,12 +168,12 @@ auto HandleFunctionDefinitionStart(Context& context, Parse::Node parse_node)
   // Track that this declaration is the definition.
   if (function.definition_id.is_valid()) {
     CARBON_DIAGNOSTIC(FunctionRedefinition, Error,
-                      "Redefinition of function {0}.", llvm::StringRef);
+                      "Redefinition of function {0}.", std::string);
     CARBON_DIAGNOSTIC(FunctionPreviousDefinition, Note,
                       "Previous definition was here.");
     context.emitter()
         .Build(parse_node, FunctionRedefinition,
-               context.names().GetFormatted(function.name_id))
+               context.names().GetFormatted(function.name_id).str())
         .Note(context.insts().Get(function.definition_id).parse_node(),
               FunctionPreviousDefinition)
         .Emit();

toolchain/check/handle_name.cpp

@@ -190,10 +190,10 @@ auto HandleMemberAccessExpr(Context& context, Parse::Node parse_node) -> bool {
       }
       CARBON_DIAGNOSTIC(QualifiedExprNameNotFound, Error,
                         "Type `{0}` does not have a member `{1}`.", std::string,
-                        llvm::StringRef);
+                        std::string);
       context.emitter().Emit(parse_node, QualifiedExprNameNotFound,
                              context.sem_ir().StringifyType(base_type_id),
-                             context.names().GetFormatted(name_id));
+                             context.names().GetFormatted(name_id).str());
       break;
     }
     // TODO: `ConstType` should support member access just like the

toolchain/check/handle_pattern_binding.cpp

@@ -68,11 +68,12 @@ auto HandlePatternBinding(Context& context, Parse::Node parse_node) -> bool {
       auto enclosing_class_decl = context.GetCurrentScopeAs<SemIR::ClassDecl>();
       if (!context.TryToCompleteType(cast_type_id, [&] {
             CARBON_DIAGNOSTIC(IncompleteTypeInVarDecl, Error,
-                              "{0} has incomplete type `{1}`.", llvm::StringRef,
-                              std::string);
+                              "{0} has incomplete type `{1}`.",
+                              llvm::StringLiteral, std::string);
             return context.emitter().Build(
                 type_node_copy, IncompleteTypeInVarDecl,
-                enclosing_class_decl ? "Field" : "Variable",
+                enclosing_class_decl ? llvm::StringLiteral("Field")
+                                     : llvm::StringLiteral("Variable"),
                 context.sem_ir().StringifyType(cast_type_id, true));
           })) {
         cast_type_id = SemIR::TypeId::Error;

toolchain/check/handle_struct.cpp

@@ -65,13 +65,13 @@ static auto DiagnoseDuplicateNames(Context& context,
         names.insert({field_inst.name_id, field_inst.parse_node});
     if (!added) {
       CARBON_DIAGNOSTIC(StructNameDuplicate, Error,
-                        "Duplicated field name `{1}` in {0}.", llvm::StringRef,
-                        llvm::StringRef);
+                        "Duplicated field name `{1}` in {0}.", std::string,
+                        std::string);
       CARBON_DIAGNOSTIC(StructNamePrevious, Note,
                         "Field with the same name here.");
       context.emitter()
-          .Build(field_inst.parse_node, StructNameDuplicate, construct,
-                 sem_ir.names().GetFormatted(field_inst.name_id))
+          .Build(field_inst.parse_node, StructNameDuplicate, construct.str(),
+                 sem_ir.names().GetFormatted(field_inst.name_id).str())
           .Note(it->second, StructNamePrevious)
           .Emit();
       return true;

toolchain/diagnostics/diagnostic_emitter.h

@@ -158,7 +158,11 @@ template <typename... Args>
 struct DiagnosticBase {
   explicit constexpr DiagnosticBase(DiagnosticKind kind, DiagnosticLevel level,
                                     llvm::StringLiteral format)
-      : Kind(kind), Level(level), Format(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.");
+  }
 
   // Calls formatv with the diagnostic's arguments.
   auto FormatFn(const DiagnosticMessage& message) const -> std::string {

toolchain/diagnostics/diagnostic_emitter_test.cpp

@@ -52,7 +52,7 @@ TEST_F(DiagnosticEmitterTest, EmitSimpleWarning) {
 }
 
 TEST_F(DiagnosticEmitterTest, EmitOneArgDiagnostic) {
-  CARBON_DIAGNOSTIC(TestDiagnostic, Error, "arg: `{0}`", llvm::StringRef);
+  CARBON_DIAGNOSTIC(TestDiagnostic, Error, "arg: `{0}`", llvm::StringLiteral);
   EXPECT_CALL(consumer_, HandleDiagnostic(IsDiagnostic(
                              DiagnosticKind::TestDiagnostic,
                              DiagnosticLevel::Error, 1, 1, "arg: `str`")));

toolchain/diagnostics/sorting_diagnostic_consumer_test.cpp

@@ -17,7 +17,7 @@ namespace {
 using ::Carbon::Testing::IsDiagnostic;
 using ::testing::InSequence;
 
-CARBON_DIAGNOSTIC(TestDiagnostic, Error, "{0}", llvm::StringRef);
+CARBON_DIAGNOSTIC(TestDiagnostic, Error, "{0}", llvm::StringLiteral);
 
 struct FakeDiagnosticLocationTranslator
     : DiagnosticLocationTranslator<DiagnosticLocation> {

toolchain/lex/token_kind.h

@@ -82,7 +82,7 @@ class TokenKind : public CARBON_ENUM_BASE(TokenKind) {
 
   // If this token kind has a fixed spelling when in source code, returns it.
   // Otherwise returns an empty string.
-  auto fixed_spelling() const -> llvm::StringRef {
+  auto fixed_spelling() const -> llvm::StringLiteral {
     return FixedSpelling[AsInt()];
   };
 

toolchain/parse/context.cpp

@@ -106,7 +106,7 @@ auto Context::ConsumeAndAddCloseSymbol(Lex::Token expected_open,
     // TODO: Include the location of the matching opening delimiter in the
     // diagnostic.
     CARBON_DIAGNOSTIC(ExpectedCloseSymbol, Error,
-                      "Unexpected tokens before `{0}`.", llvm::StringRef);
+                      "Unexpected tokens before `{0}`.", llvm::StringLiteral);
     emitter_->Emit(*position_, ExpectedCloseSymbol,
                    open_token_kind.closing_symbol().fixed_spelling());
 

toolchain/parse/handle_brace_expr.cpp

@@ -29,11 +29,15 @@ static auto HandleBraceExprParamError(Context& context,
   bool is_unknown = param_finish_state == State::BraceExprParamFinishAsUnknown;
   CARBON_CHECK(is_type || is_value || is_unknown);
   CARBON_DIAGNOSTIC(ExpectedStructLiteralField, Error, "Expected {0}{1}{2}.",
-                    llvm::StringRef, llvm::StringRef, llvm::StringRef);
-  context.emitter().Emit(*context.position(), ExpectedStructLiteralField,
-                         (is_type || is_unknown) ? "`.field: field_type`" : "",
-                         is_unknown ? " or " : "",
-                         (is_value || is_unknown) ? "`.field = value`" : "");
+                    llvm::StringLiteral, llvm::StringLiteral,
+                    llvm::StringLiteral);
+  context.emitter().Emit(
+      *context.position(), ExpectedStructLiteralField,
+      (is_type || is_unknown) ? llvm::StringLiteral("`.field: field_type`")
+                              : llvm::StringLiteral(""),
+      is_unknown ? llvm::StringLiteral(" or ") : llvm::StringLiteral(""),
+      (is_value || is_unknown) ? llvm::StringLiteral("`.field = value`")
+                               : llvm::StringLiteral(""));
 
   state.state = param_finish_state;
   state.has_error = true;

toolchain/parse/handle_period.cpp

@@ -20,10 +20,10 @@ static auto HandlePeriodOrArrow(Context& context, NodeKind node_kind,
   if (!context.ConsumeAndAddLeafNodeIf(Lex::TokenKind::Identifier,
                                        NodeKind::Name)) {
     CARBON_DIAGNOSTIC(ExpectedIdentifierAfterDotOrArrow, Error,
-                      "Expected identifier after `{0}`.", llvm::StringRef);
-    context.emitter().Emit(*context.position(),
-                           ExpectedIdentifierAfterDotOrArrow,
-                           is_arrow ? "->" : ".");
+                      "Expected identifier after `{0}`.", llvm::StringLiteral);
+    context.emitter().Emit(
+        *context.position(), ExpectedIdentifierAfterDotOrArrow,
+        is_arrow ? llvm::StringLiteral("->") : llvm::StringLiteral("."));
     // If we see a keyword, assume it was intended to be a name.
     // TODO: Should keywords be valid here?
     if (context.PositionKind().is_keyword()) {