Support import of typedefs. (#5787)

Unify code paths for importing classes by name and importing them
indirectly when their type is referenced. Switch to using the general
type import machinery to import all type declarations, which allows
typedef declarations naming importable types to be used too.

Fix up handling of error cases to consistently only produce an Error
InstId after actually producing an error message, so that we can produce
exactly one diagnostic in failure cases.

Remove TODO error for unions that previously was only produced when
importing them indirectly, not when importing them by name. Import of
unions is exactly as complete / incomplete as import of other class
types, so treating them differently doesn't seem necessary.

Author: zygoloid

toolchain/check/import_cpp.cpp

@@ -618,6 +618,12 @@ static auto ImportCXXRecordDecl(Context& context, SemIR::LocId loc_id,
     return SemIR::ErrorInst::InstId;
   }
 
+  if (clang_def->isUnion() && !clang_def->fields().empty()) {
+    context.TODO(loc_id, "Unsupported: Non-empty union");
+    MarkFailedDecl(context, clang_decl);
+    return SemIR::ErrorInst::InstId;
+  }
+
   auto [class_id, class_def_id] =
       BuildClassDefinition(context, parent_scope_id, name_id, clang_def);
 
@@ -662,61 +668,60 @@ static auto MapBuiltinType(Context& context, const clang::BuiltinType& type)
     default:
       break;
   }
-  return {.inst_id = SemIR::ErrorInst::TypeInstId,
-          .type_id = SemIR::ErrorInst::TypeId};
+  return {.inst_id = SemIR::TypeInstId::None, .type_id = SemIR::TypeId::None};
 }
 
 // Maps a C++ record type to a Carbon type.
 // TODO: Support more record types.
 static auto MapRecordType(Context& context, SemIR::LocId loc_id,
                           const clang::RecordType& type) -> TypeExpr {
   auto* record_decl = clang::dyn_cast<clang::CXXRecordDecl>(type.getDecl());
-  if (record_decl && !record_decl->isUnion()) {
-    auto& clang_decls = context.sem_ir().clang_decls();
-    SemIR::InstId record_inst_id = SemIR::InstId::None;
-    if (auto record_clang_decl_id = clang_decls.Lookup(record_decl);
-        record_clang_decl_id.has_value()) {
-      record_inst_id = clang_decls.Get(record_clang_decl_id).inst_id;
-    } else {
-      auto parent_inst_id =
-          AsCarbonNamespace(context, record_decl->getDeclContext());
-      auto parent_name_scope_id =
-          context.insts().GetAs<SemIR::Namespace>(parent_inst_id).name_scope_id;
-      SemIR::NameId record_name_id =
-          AddIdentifierName(context, record_decl->getName());
-      record_inst_id = ImportCXXRecordDecl(
-          context, loc_id, parent_name_scope_id, record_name_id, record_decl);
-      AddNameToScope(context, parent_name_scope_id, record_name_id,
-                     record_inst_id);
-    }
-    SemIR::TypeInstId record_type_inst_id =
-        context.types().GetAsTypeInstId(record_inst_id);
-    return {
-        .inst_id = record_type_inst_id,
-        .type_id = context.types().GetTypeIdForTypeInstId(record_type_inst_id)};
+  if (!record_decl) {
+    return {.inst_id = SemIR::TypeInstId::None, .type_id = SemIR::TypeId::None};
   }
 
-  return {.inst_id = SemIR::ErrorInst::TypeInstId,
-          .type_id = SemIR::ErrorInst::TypeId};
+  auto& clang_decls = context.sem_ir().clang_decls();
+  SemIR::InstId record_inst_id = SemIR::InstId::None;
+  if (auto record_clang_decl_id = clang_decls.Lookup(record_decl);
+      record_clang_decl_id.has_value()) {
+    record_inst_id = clang_decls.Get(record_clang_decl_id).inst_id;
+  } else {
+    auto parent_inst_id =
+        AsCarbonNamespace(context, record_decl->getDeclContext());
+    auto parent_name_scope_id =
+        context.insts().GetAs<SemIR::Namespace>(parent_inst_id).name_scope_id;
+    SemIR::NameId record_name_id =
+        AddIdentifierName(context, record_decl->getName());
+    record_inst_id = ImportCXXRecordDecl(context, loc_id, parent_name_scope_id,
+                                         record_name_id, record_decl);
+  }
+  SemIR::TypeInstId record_type_inst_id =
+      context.types().GetAsTypeInstId(record_inst_id);
+  return {
+      .inst_id = record_type_inst_id,
+      .type_id = context.types().GetTypeIdForTypeInstId(record_type_inst_id)};
 }
 
 // Maps a C++ non-pointer type to a Carbon type.
 // TODO: Support more types.
 static auto MapNonPointerType(Context& context, SemIR::LocId loc_id,
                               clang::QualType type) -> TypeExpr {
-  type = type.getCanonicalType();
+  if (type.hasQualifiers()) {
+    // TODO: Support type qualifiers.
+    return {.inst_id = SemIR::TypeInstId::None, .type_id = SemIR::TypeId::None};
+  }
+
   CARBON_CHECK(!type->isPointerType());
 
-  if (const auto* builtin_type = dyn_cast<clang::BuiltinType>(type)) {
+  if (const auto* builtin_type = type->getAs<clang::BuiltinType>()) {
     return MapBuiltinType(context, *builtin_type);
   }
 
-  if (const auto* record_type = clang::dyn_cast<clang::RecordType>(type)) {
+  if (const auto* record_type = type->getAs<clang::RecordType>()) {
     return MapRecordType(context, loc_id, *record_type);
   }
 
-  return {.inst_id = SemIR::ErrorInst::TypeInstId,
-          .type_id = SemIR::ErrorInst::TypeId};
+  return {.inst_id = SemIR::TypeInstId::None, .type_id = SemIR::TypeId::None};
 }
 
 // Maps a C++ pointer type to a Carbon pointer type.
@@ -744,10 +749,8 @@ static auto MapPointerType(Context& context, SemIR::LocId loc_id,
   }
 
   TypeExpr pointee_type_expr = MapNonPointerType(context, loc_id, pointee_type);
-  if (pointee_type_expr.inst_id == SemIR::ErrorInst::InstId ||
-      pointee_type_expr.type_id == SemIR::ErrorInst::TypeId) {
-    return {.inst_id = SemIR::ErrorInst::TypeInstId,
-            .type_id = SemIR::ErrorInst::TypeId};
+  if (!pointee_type_expr.inst_id.has_value()) {
+    return {.inst_id = SemIR::TypeInstId::None, .type_id = SemIR::TypeId::None};
   }
 
   SemIR::TypeId pointer_type_id =
@@ -770,7 +773,7 @@ static auto MapType(Context& context, SemIR::LocId loc_id, clang::QualType type)
 // Returns a block id for the explicit parameters of the given function
 // declaration. If the function declaration has no parameters, it returns
 // `SemIR::InstBlockId::Empty`. In the case of an unsupported parameter type, it
-// returns `SemIR::InstBlockId::None`.
+// produces an error and returns `SemIR::InstBlockId::None`.
 // TODO: Consider refactoring to extract and reuse more logic from
 // `HandleAnyBindingPattern()`.
 static auto MakeParamPatternsBlockId(Context& context, SemIR::LocId loc_id,
@@ -794,7 +797,7 @@ static auto MakeParamPatternsBlockId(Context& context, SemIR::LocId loc_id,
     SemIR::ExprRegionId type_expr_region_id =
         EndSubpatternAsExpr(context, type_inst_id);
 
-    if (type_id == SemIR::ErrorInst::TypeId) {
+    if (!type_id.has_value()) {
       context.TODO(loc_id, llvm::formatv("Unsupported: parameter type: {0}",
                                          param_type.getAsString()));
       return SemIR::InstBlockId::None;
@@ -830,7 +833,8 @@ static auto MakeParamPatternsBlockId(Context& context, SemIR::LocId loc_id,
 }
 
 // Returns the return type of the given function declaration. In case of an
-// unsupported return type, it returns `SemIR::ErrorInst::InstId`.
+// unsupported return type, it produces a diagnostic and returns
+// `SemIR::ErrorInst::InstId`.
 // TODO: Support more return types.
 static auto GetReturnType(Context& context, SemIR::LocId loc_id,
                           const clang::FunctionDecl* clang_decl)
@@ -841,7 +845,7 @@ static auto GetReturnType(Context& context, SemIR::LocId loc_id,
   }
 
   auto [type_inst_id, type_id] = MapType(context, loc_id, ret_type);
-  if (type_id == SemIR::ErrorInst::TypeId) {
+  if (!type_inst_id.has_value()) {
     context.TODO(loc_id, llvm::formatv("Unsupported: return type: {0}",
                                        ret_type.getAsString()));
     return SemIR::ErrorInst::InstId;
@@ -878,8 +882,8 @@ struct FunctionParamsInsts {
 // to create the Call parameters instructions block. Currently the implicit
 // parameter patterns are not taken into account. Returns the parameter patterns
 // block id, the return slot pattern id, and the call parameters block id.
-// Returns `std::nullopt` if the function declaration has an unsupported
-// parameter type.
+// Produces a diagnostic and returns `std::nullopt` if the function declaration
+// has an unsupported parameter type.
 static auto CreateFunctionParamsInsts(Context& context, SemIR::LocId loc_id,
                                       const clang::FunctionDecl* clang_decl)
     -> std::optional<FunctionParamsInsts> {
@@ -992,10 +996,15 @@ static auto ImportNameDecl(Context& context, SemIR::LocId loc_id,
     return ImportNamespaceDecl(context, scope_id, name_id,
                                clang_namespace_decl);
   }
-  if (auto* clang_record_decl =
-          clang::dyn_cast<clang::CXXRecordDecl>(clang_decl)) {
-    return ImportCXXRecordDecl(context, loc_id, scope_id, name_id,
-                               clang_record_decl);
+  if (auto* type_decl = clang::dyn_cast<clang::TypeDecl>(clang_decl)) {
+    auto type = type_decl->getASTContext().getTypeDeclType(type_decl);
+    auto type_inst_id = MapType(context, loc_id, type).inst_id;
+    if (!type_inst_id.has_value()) {
+      context.TODO(loc_id, llvm::formatv("Unsupported: Type declaration: {0}",
+                                         type.getAsString()));
+      return SemIR::ErrorInst::InstId;
+    }
+    return type_inst_id;
   }
 
   context.TODO(loc_id, llvm::formatv("Unsupported: Declaration type {0}",