Improve infrastructure for formatting types in diagnostics. (#4374)

Instead of stringifying types in the caller in some cases, add new types
to represent:

- `InstIdAsType`: an `InstId` diagnostic argument that represents a type
expression that should be included in the diagnostic
- `InstIdAsTypeOfExpr`: an `InstId` diagnostic argument that represents
an expression whose type should be included in the diagnostic

For these cases, we can produce more user-friendly descriptions of a
type than we can with a canonicalized `TypeId`. Add comments to
discourage using `TypeId` diagnostic arguments when one of the above can
be used, and move over existing uses where it's straightforward to do
so.

Move type stringification code to its own files and out of `SemIR::File`
to make `File` smaller and to further discourage the direct use of the
stringification logic.

Also update type printing to include the `` ` `` delimiters surrounding
the type. The intent is that we will eventually want to include other
information when formatting a type, like Clang does when printing a
typedef (`'string' (aka 'std::basic_string<char>')`), and such
formatting requires that the diagnostic machinery produces the `` ` ``s
itself.

There are a couple of cases where we really want to format valid Carbon
type syntax directly into a diagnostic, rather than an `aka` or similar,
because the diagnostic text includes part of the type itself, for
example: ``"consider using `partial {0}`"``. For such cases, a `Raw`
form of the diagnostic argument types is added: `TypeIdAsRawType` and
`InstIdAsRawType`. In principle we could instead use ``"consider using
`partial {0:raw}`"``, but our diagnostic machinery isn't set up for
that.

---------

Co-authored-by: josh11b <[email protected]>

toolchain/check/BUILD

@@ -249,6 +249,7 @@ cc_library(
         "//toolchain/diagnostics:diagnostic_emitter",
         "//toolchain/parse:tree_node_diagnostic_converter",
         "//toolchain/sem_ir:file",
+        "//toolchain/sem_ir:stringify_type",
         "@llvm-project//llvm:Support",
     ],
 )

toolchain/check/call.cpp

@@ -128,10 +128,8 @@ auto PerformCall(Context& context, SemIR::LocId loc_id, SemIR::InstId callee_id,
       default: {
         if (!callee_function.is_error) {
           CARBON_DIAGNOSTIC(CallToNonCallable, Error,
-                            "value of type `{0}` is not callable",
-                            SemIR::TypeId);
-          context.emitter().Emit(loc_id, CallToNonCallable,
-                                 context.insts().Get(callee_id).type_id());
+                            "value of type {0} is not callable", TypeOfInstId);
+          context.emitter().Emit(loc_id, CallToNonCallable, callee_id);
         }
         return SemIR::InstId::BuiltinError;
       }

toolchain/check/context.cpp

@@ -352,7 +352,7 @@ static auto DiagnoseInvalidQualifiedNameAccess(Context& context, SemIRLoc loc,
   auto class_info = context.classes().Get(class_type->class_id);
 
   CARBON_DIAGNOSTIC(ClassInvalidMemberAccess, Error,
-                    "cannot access {0} member `{1}` of type `{2}`",
+                    "cannot access {0} member `{1}` of type {2}",
                     SemIR::AccessKind, SemIR::NameId, SemIR::TypeId);
   CARBON_DIAGNOSTIC(ClassMemberDefinition, Note,
                     "the {0} member `{1}` is defined here", SemIR::AccessKind,

toolchain/check/convert.cpp

@@ -466,12 +466,12 @@ static auto ConvertStructToStructOrClass(Context& context,
                                  dest_field.name_id);
         } else {
           CARBON_DIAGNOSTIC(StructInitMissingFieldInConversion, Error,
-                            "cannot convert from struct type `{0}` to `{1}`: "
+                            "cannot convert from struct type {0} to {1}: "
                             "missing field `{2}` in source type",
-                            SemIR::TypeId, SemIR::TypeId, SemIR::NameId);
-          context.emitter().Emit(
-              value_loc_id, StructInitMissingFieldInConversion, value.type_id(),
-              target.type_id, dest_field.name_id);
+                            TypeOfInstId, SemIR::TypeId, SemIR::NameId);
+          context.emitter().Emit(value_loc_id,
+                                 StructInitMissingFieldInConversion, value_id,
+                                 target.type_id, dest_field.name_id);
         }
         return SemIR::InstId::BuiltinError;
       }
@@ -537,7 +537,7 @@ static auto ConvertStructToClass(Context& context, SemIR::StructType src_type,
     CARBON_DIAGNOSTIC(ConstructionOfAbstractClass, Error,
                       "cannot construct instance of abstract class; "
                       "consider using `partial {0}` instead",
-                      SemIR::TypeId);
+                      TypeIdAsRawType);
     context.emitter().Emit(value_id, ConstructionOfAbstractClass,
                            target.type_id);
     return SemIR::InstId::BuiltinError;
@@ -909,8 +909,8 @@ static auto PerformCopy(Context& context, SemIR::InstId expr_id)
   // TODO: We don't yet have rules for whether and when a class type is
   // copyable, or how to perform the copy.
   CARBON_DIAGNOSTIC(CopyOfUncopyableType, Error,
-                    "cannot copy value of type `{0}`", SemIR::TypeId);
-  context.emitter().Emit(expr_id, CopyOfUncopyableType, type_id);
+                    "cannot copy value of type {0}", TypeOfInstId);
+  context.emitter().Emit(expr_id, CopyOfUncopyableType, expr_id);
   return SemIR::InstId::BuiltinError;
 }
 
@@ -939,14 +939,13 @@ auto Convert(Context& context, SemIR::LocId loc_id, SemIR::InstId expr_id,
   // We can only perform initialization for complete types.
   if (!context.TryToCompleteType(target.type_id, [&] {
         CARBON_DIAGNOSTIC(IncompleteTypeInInit, Error,
-                          "initialization of incomplete type `{0}`",
+                          "initialization of incomplete type {0}",
                           SemIR::TypeId);
         CARBON_DIAGNOSTIC(IncompleteTypeInValueConversion, Error,
-                          "forming value of incomplete type `{0}`",
+                          "forming value of incomplete type {0}",
                           SemIR::TypeId);
         CARBON_DIAGNOSTIC(IncompleteTypeInConversion, Error,
-                          "invalid use of incomplete type `{0}`",
-                          SemIR::TypeId);
+                          "invalid use of incomplete type {0}", SemIR::TypeId);
         return context.emitter().Build(loc_id,
                                        target.is_initializer()
                                            ? IncompleteTypeInInit
@@ -978,16 +977,16 @@ auto Convert(Context& context, SemIR::LocId loc_id, SemIR::InstId expr_id,
     };
     expr_id = BuildUnaryOperator(context, loc_id, op, expr_id, [&] {
       CARBON_DIAGNOSTIC(ImplicitAsConversionFailure, Error,
-                        "cannot implicitly convert from `{0}` to `{1}`",
-                        SemIR::TypeId, SemIR::TypeId);
+                        "cannot implicitly convert from {0} to {1}",
+                        TypeOfInstId, SemIR::TypeId);
       CARBON_DIAGNOSTIC(ExplicitAsConversionFailure, Error,
-                        "cannot convert from `{0}` to `{1}` with `as`",
-                        SemIR::TypeId, SemIR::TypeId);
+                        "cannot convert from {0} to {1} with `as`",
+                        TypeOfInstId, SemIR::TypeId);
       return context.emitter().Build(loc_id,
                                      target.kind == ConversionTarget::ExplicitAs
                                          ? ExplicitAsConversionFailure
                                          : ImplicitAsConversionFailure,
-                                     expr.type_id(), target.type_id);
+                                     expr_id, target.type_id);
     });
   }
 

toolchain/check/decl_name_stack.cpp

@@ -312,7 +312,7 @@ static auto DiagnoseQualifiedDeclInIncompleteClassScope(Context& context,
                                                         SemIR::ClassId class_id)
     -> void {
   CARBON_DIAGNOSTIC(QualifiedDeclInIncompleteClassScope, Error,
-                    "cannot declare a member of incomplete class `{0}`",
+                    "cannot declare a member of incomplete class {0}",
                     SemIR::TypeId);
   auto builder =
       context.emitter().Build(loc, QualifiedDeclInIncompleteClassScope,
@@ -327,13 +327,10 @@ static auto DiagnoseQualifiedDeclInUndefinedInterfaceScope(
     Context& context, SemIRLoc loc, SemIR::InterfaceId interface_id,
     SemIR::InstId interface_inst_id) -> void {
   CARBON_DIAGNOSTIC(QualifiedDeclInUndefinedInterfaceScope, Error,
-                    "cannot declare a member of undefined interface `{0}`",
-                    std::string);
+                    "cannot declare a member of undefined interface {0}",
+                    InstIdAsType);
   auto builder = context.emitter().Build(
-      loc, QualifiedDeclInUndefinedInterfaceScope,
-      context.sem_ir().StringifyTypeExpr(
-          context.sem_ir().constant_values().GetConstantInstId(
-              interface_inst_id)));
+      loc, QualifiedDeclInUndefinedInterfaceScope, interface_inst_id);
   context.NoteUndefinedInterface(interface_id, builder);
   builder.Emit();
 }

toolchain/check/diagnostic_helpers.h

@@ -41,6 +41,75 @@ inline auto TokenOnly(SemIR::LocId loc_id) -> SemIRLoc {
   return SemIRLoc(loc_id, true);
 }
 
+// An expression whose type should be rendered in a diagnostic. The diagnostic
+// rendering will include enclosing "`"s, and may also include extra information
+// about the type if it might otherwise be ambiguous or context-dependent, such
+// as the targets of aliases used in the type.
+//
+// TODO: Include such additional information where relevant. For example:
+// "`StdString` (aka `Cpp.std.basic_string(Char)`)".
+//
+// This should be used instead of `TypeId` as a diagnostic argument wherever
+// possible, because we should eventually be able to produce a sugared type name
+// in this case, whereas a `TypeId` will render as a canonical type.
+struct TypeOfInstId {
+  using DiagnosticType = DiagnosticTypeInfo<std::string>;
+
+  // NOLINTNEXTLINE(google-explicit-constructor)
+  TypeOfInstId(SemIR::InstId inst_id) : inst_id(inst_id) {}
+
+  SemIR::InstId inst_id;
+};
+
+// A type expression, for rendering in a diagnostic. The diagnostic rendering
+// will include enclosing "`"s, and may also include extra information about the
+// type if it would otherwise be ambiguous.
+//
+// TODO: Include such additional information where relevant.
+//
+// This should be used when the source expression used to construct a type is
+// available.
+struct InstIdAsType {
+  using DiagnosticType = DiagnosticTypeInfo<std::string>;
+
+  // NOLINTNEXTLINE(google-explicit-constructor)
+  InstIdAsType(SemIR::InstId inst_id) : inst_id(inst_id) {}
+
+  SemIR::InstId inst_id;
+};
+
+// A type expression, for rendering in a diagnostic as a raw type. When
+// formatting as a raw type in a diagnostic, the type will be formatted as a
+// simple Carbon expression, without enclosing "`"s. Once we start including
+// extra information about types, such annotations will also not be included for
+// raw types.
+//
+// This is intended for cases where the type is part of a larger syntactic
+// construct in a diagnostic, such as "redefinition of `impl {0} as {1}`".
+struct InstIdAsRawType {
+  using DiagnosticType = DiagnosticTypeInfo<std::string>;
+
+  // NOLINTNEXTLINE(google-explicit-constructor)
+  InstIdAsRawType(SemIR::InstId inst_id) : inst_id(inst_id) {}
+
+  SemIR::InstId inst_id;
+};
+
+// A type value for rendering in a diagnostic without enclosing "`"s. See
+// `InstIdAsRawType` for details on raw type formatting.
+//
+// As with `TypeId`, this should be avoided as a diagnostic argument where
+// possible, because it can't be formatted with syntactic sugar such as aliases
+// that describe how the type was written.
+struct TypeIdAsRawType {
+  using DiagnosticType = DiagnosticTypeInfo<std::string>;
+
+  // NOLINTNEXTLINE(google-explicit-constructor)
+  TypeIdAsRawType(SemIR::TypeId type_id) : type_id(type_id) {}
+
+  SemIR::TypeId type_id;
+};
+
 // An integer value together with its type. The type is used to determine how to
 // format the value in diagnostics.
 struct TypedInt {

toolchain/check/eval.cpp

@@ -512,7 +512,7 @@ static auto PerformArrayIndex(EvalContext& eval_context, SemIR::Inst inst)
               .Get(bound->int_id)
               .ule(index_val.getZExtValue())) {
         CARBON_DIAGNOSTIC(ArrayIndexOutOfBounds, Error,
-                          "array index `{0}` is past the end of type `{1}`",
+                          "array index `{0}` is past the end of type {1}",
                           TypedInt, SemIR::TypeId);
         eval_context.emitter().Emit(
             index_inst.index_id, ArrayIndexOutOfBounds,

toolchain/check/function.cpp

@@ -34,7 +34,7 @@ auto CheckFunctionTypeMatches(Context& context,
                                                   prev_return_type_id)) {
     CARBON_DIAGNOSTIC(
         FunctionRedeclReturnTypeDiffers, Error,
-        "function redeclaration differs because return type is `{0}`",
+        "function redeclaration differs because return type is {0}",
         SemIR::TypeId);
     CARBON_DIAGNOSTIC(
         FunctionRedeclReturnTypeDiffersNoReturn, Error,
@@ -48,7 +48,7 @@ auto CheckFunctionTypeMatches(Context& context,
                                       FunctionRedeclReturnTypeDiffersNoReturn);
     if (prev_return_type_id.is_valid()) {
       CARBON_DIAGNOSTIC(FunctionRedeclReturnTypePrevious, Note,
-                        "previously declared with return type `{0}`",
+                        "previously declared with return type {0}",
                         SemIR::TypeId);
       diag.Note(prev_function.latest_decl_id(),
                 FunctionRedeclReturnTypePrevious, prev_return_type_id);
@@ -77,8 +77,7 @@ auto CheckFunctionReturnType(Context& context, SemIRLoc loc,
   if (return_info.init_repr.kind == SemIR::InitRepr::Incomplete) {
     auto diagnose_incomplete_return_type = [&] {
       CARBON_DIAGNOSTIC(IncompleteTypeInFunctionReturnType, Error,
-                        "function returns incomplete type `{0}`",
-                        SemIR::TypeId);
+                        "function returns incomplete type {0}", SemIR::TypeId);
       return context.emitter().Build(loc, IncompleteTypeInFunctionReturnType,
                                      return_info.type_id);
     };

toolchain/check/handle_binding_pattern.cpp

@@ -14,7 +14,8 @@ namespace Carbon::Check {
 static auto HandleAnyBindingPattern(Context& context, Parse::NodeId node_id,
                                     bool is_generic) -> bool {
   auto [type_node, parsed_type_id] = context.node_stack().PopExprWithNodeId();
-  auto cast_type_id = ExprAsType(context, type_node, parsed_type_id).type_id;
+  auto [cast_type_inst_id, cast_type_id] =
+      ExprAsType(context, type_node, parsed_type_id);
 
   // TODO: Handle `_` bindings.
 
@@ -101,13 +102,13 @@ static auto HandleAnyBindingPattern(Context& context, Parse::NodeId node_id,
       auto parent_class_decl = context.GetCurrentScopeAs<SemIR::ClassDecl>();
       cast_type_id = context.AsCompleteType(cast_type_id, [&] {
         CARBON_DIAGNOSTIC(IncompleteTypeInVarDecl, Error,
-                          "{0} has incomplete type `{1}`", llvm::StringLiteral,
-                          SemIR::TypeId);
+                          "{0} has incomplete type {1}", llvm::StringLiteral,
+                          InstIdAsType);
         return context.emitter().Build(type_node, IncompleteTypeInVarDecl,
                                        parent_class_decl
                                            ? llvm::StringLiteral("Field")
                                            : llvm::StringLiteral("Variable"),
-                                       cast_type_id);
+                                       cast_type_inst_id);
       });
       if (parent_class_decl) {
         CARBON_CHECK(context_node_kind == Parse::NodeKind::VariableIntroducer,
@@ -187,10 +188,10 @@ static auto HandleAnyBindingPattern(Context& context, Parse::NodeId node_id,
     case Parse::NodeKind::LetIntroducer: {
       cast_type_id = context.AsCompleteType(cast_type_id, [&] {
         CARBON_DIAGNOSTIC(IncompleteTypeInLetDecl, Error,
-                          "`let` binding has incomplete type `{0}`",
-                          SemIR::TypeId);
+                          "`let` binding has incomplete type {0}",
+                          InstIdAsType);
         return context.emitter().Build(type_node, IncompleteTypeInLetDecl,
-                                       cast_type_id);
+                                       cast_type_inst_id);
       });
       // Create the instruction, but don't add it to a block until after we've
       // formed its initializer.

toolchain/check/handle_class.cpp

@@ -391,14 +391,13 @@ auto HandleParseNode(Context& context, Parse::AdaptDeclId node_id) -> bool {
     return true;
   }
 
-  auto adapted_type_id =
-      ExprAsType(context, node_id, adapted_type_expr_id).type_id;
+  auto [adapted_type_inst_id, adapted_type_id] =
+      ExprAsType(context, node_id, adapted_type_expr_id);
   adapted_type_id = context.AsCompleteType(adapted_type_id, [&] {
     CARBON_DIAGNOSTIC(IncompleteTypeInAdaptDecl, Error,
-                      "adapted type `{0}` is an incomplete type",
-                      SemIR::TypeId);
+                      "adapted type {0} is an incomplete type", InstIdAsType);
     return context.emitter().Build(node_id, IncompleteTypeInAdaptDecl,
-                                   adapted_type_id);
+                                   adapted_type_inst_id);
   });
 
   // Build a SemIR representation for the declaration.
@@ -457,23 +456,24 @@ constexpr BaseInfo BaseInfo::Error = {.type_id = SemIR::TypeId::Error,
 
 // Diagnoses an attempt to derive from a final type.
 static auto DiagnoseBaseIsFinal(Context& context, Parse::NodeId node_id,
-                                SemIR::TypeId base_type_id) -> void {
+                                SemIR::InstId base_type_inst_id) -> void {
   CARBON_DIAGNOSTIC(BaseIsFinal, Error,
-                    "deriving from final type `{0}`; base type must be an "
+                    "deriving from final type {0}; base type must be an "
                     "`abstract` or `base` class",
-                    SemIR::TypeId);
-  context.emitter().Emit(node_id, BaseIsFinal, base_type_id);
+                    InstIdAsType);
+  context.emitter().Emit(node_id, BaseIsFinal, base_type_inst_id);
 }
 
 // Checks that the specified base type is valid.
 static auto CheckBaseType(Context& context, Parse::NodeId node_id,
                           SemIR::InstId base_expr_id) -> BaseInfo {
-  auto base_type_id = ExprAsType(context, node_id, base_expr_id).type_id;
+  auto [base_type_inst_id, base_type_id] =
+      ExprAsType(context, node_id, base_expr_id);
   base_type_id = context.AsCompleteType(base_type_id, [&] {
     CARBON_DIAGNOSTIC(IncompleteTypeInBaseDecl, Error,
-                      "base `{0}` is an incomplete type", SemIR::TypeId);
+                      "base {0} is an incomplete type", InstIdAsType);
     return context.emitter().Build(node_id, IncompleteTypeInBaseDecl,
-                                   base_type_id);
+                                   base_type_inst_id);
   });
 
   if (base_type_id == SemIR::TypeId::Error) {
@@ -488,11 +488,11 @@ static auto CheckBaseType(Context& context, Parse::NodeId node_id,
     // declaration as being final classes.
     // TODO: Once we have a better idea of which types are considered to be
     // classes, produce a better diagnostic for deriving from a non-class type.
-    DiagnoseBaseIsFinal(context, node_id, base_type_id);
+    DiagnoseBaseIsFinal(context, node_id, base_type_inst_id);
     return BaseInfo::Error;
   }
   if (base_class_info->inheritance_kind == SemIR::Class::Final) {
-    DiagnoseBaseIsFinal(context, node_id, base_type_id);
+    DiagnoseBaseIsFinal(context, node_id, base_type_inst_id);
   }
 
   CARBON_CHECK(base_class_info->scope_id.is_valid(),

toolchain/check/handle_function.cpp

@@ -367,11 +367,11 @@ static auto HandleFunctionDefinitionAfterSignature(
     context.TryToCompleteType(param_info.inst.type_id, [&] {
       CARBON_DIAGNOSTIC(
           IncompleteTypeInFunctionParam, Error,
-          "parameter has incomplete type `{0}` in function definition",
-          SemIR::TypeId);
+          "parameter has incomplete type {0} in function definition",
+          TypeOfInstId);
       return context.emitter().Build(param_info.inst_id,
                                      IncompleteTypeInFunctionParam,
-                                     param_info.inst.type_id);
+                                     param_info.inst_id);
     });
   }