Don't import a C++ class definition until the class is required to be complete. (#5865)

When importing a class definition, ask Clang to complete it first. This
causes class template specializations to get instantiated as needed when
the type-checking of Carbon requires a C++ class to be complete. It also
allows Clang to implement things like modules-aware definition
visibility checking.

Don't reject importing a class with a virtual base if it's never
required to be complete. Instead, defer diagnosing until the definition
is required.

This also removes the recursion from `MapType`, as mapping a class type
no longer maps its definition.

In order to get diagnostics from instantiation failures, fix a bug that
caused any Clang diagnostics produced after the initial building of the
`ASTUnit` to get discarded. This exposed some duplicate diagnostic
issues in `ImportNameFromCpp` which are fixed here too.

Author: zygoloid

toolchain/check/import_cpp.cpp

@@ -525,6 +525,32 @@ static auto BuildClassDecl(Context& context,
   return {class_decl.class_id, context.types().GetAsTypeInstId(class_decl_id)};
 }
 
+// Imports a record declaration from Clang to Carbon. If successful, returns
+// the new Carbon class declaration `InstId`.
+static auto ImportCXXRecordDecl(Context& context,
+                                clang::CXXRecordDecl* clang_decl)
+    -> SemIR::InstId {
+  auto import_ir_inst_id = AddImportIRInst(context, clang_decl->getLocation());
+
+  auto [class_id, class_inst_id] = BuildClassDecl(
+      context, import_ir_inst_id, GetParentNameScopeId(context, clang_decl),
+      AddIdentifierName(context, clang_decl->getName()));
+
+  // TODO: The caller does the same lookup. Avoid doing it twice.
+  auto clang_decl_id = context.sem_ir().clang_decls().Add(
+      {.decl = clang_decl->getCanonicalDecl(), .inst_id = class_inst_id});
+
+  // Name lookup into the Carbon class looks in the C++ class definition.
+  auto& class_info = context.classes().Get(class_id);
+  class_info.scope_id = context.name_scopes().Add(
+      class_inst_id, SemIR::NameId::None, class_info.parent_scope_id);
+  context.name_scopes()
+      .Get(class_info.scope_id)
+      .set_clang_decl_context_id(clang_decl_id);
+
+  return class_inst_id;
+}
+
 // Determines the Carbon inheritance kind to use for a C++ class definition.
 static auto GetInheritanceKind(clang::CXXRecordDecl* class_def)
     -> SemIR::Class::InheritanceKind {
@@ -558,8 +584,6 @@ static auto GetInheritanceKind(clang::CXXRecordDecl* class_def)
 
 // Checks that the specified finished class definition is valid and builds and
 // returns a corresponding complete type witness instruction.
-// TODO: Remove recursion into mapping field types.
-// NOLINTNEXTLINE(misc-no-recursion)
 static auto ImportClassObjectRepr(Context& context, SemIR::ClassId class_id,
                                   SemIR::ImportIRInstId import_ir_inst_id,
                                   SemIR::TypeInstId class_type_inst_id,
@@ -726,21 +750,14 @@ static auto ImportClassObjectRepr(Context& context, SemIR::ClassId class_id,
 
 // Creates a class definition based on the information in the given Clang
 // declaration, which is assumed to be for a class definition.
-// TODO: Remove recursion into mapping field types.
-// NOLINTNEXTLINE(misc-no-recursion)
 static auto BuildClassDefinition(Context& context,
                                  SemIR::ImportIRInstId import_ir_inst_id,
                                  SemIR::ClassId class_id,
                                  SemIR::TypeInstId class_inst_id,
-                                 SemIR::ClangDeclId clang_decl_id,
                                  clang::CXXRecordDecl* clang_def) -> void {
   auto& class_info = context.classes().Get(class_id);
-  StartClassDefinition(context, class_info, class_inst_id);
-
-  // Name lookup into the Carbon class looks in the C++ class definition.
-  context.name_scopes()
-      .Get(class_info.scope_id)
-      .set_clang_decl_context_id(clang_decl_id);
+  CARBON_CHECK(!class_info.has_definition_started());
+  class_info.definition_id = class_inst_id;
 
   context.inst_block_stack().Push();
 
@@ -757,41 +774,53 @@ static auto BuildClassDefinition(Context& context,
   class_info.body_block_id = context.inst_block_stack().Pop();
 }
 
-// Mark the given `Decl` as failed in `clang_decls`.
-static auto MarkFailedDecl(Context& context, clang::Decl* clang_decl) {
-  context.sem_ir().clang_decls().Add({.decl = clang_decl->getCanonicalDecl(),
-                                      .inst_id = SemIR::ErrorInst::InstId});
-}
-
-// Imports a record declaration from Clang to Carbon. If successful, returns
-// the new Carbon class declaration `InstId`.
-// TODO: Change `clang_decl` to `const &` when lookup is using `clang::DeclID`
-// and we don't need to store the decl for lookup context.
-// TODO: Remove recursion into mapping field types.
-// NOLINTNEXTLINE(misc-no-recursion)
-static auto ImportCXXRecordDecl(Context& context,
-                                clang::CXXRecordDecl* clang_decl)
-    -> SemIR::InstId {
-  clang::CXXRecordDecl* clang_def = clang_decl->getDefinition();
-  if (clang_def) {
-    clang_decl = clang_def;
-  }
-  auto import_ir_inst_id = AddImportIRInst(context, clang_decl->getLocation());
+auto ImportCppClassDefinition(Context& context, SemIR::LocId loc_id,
+                              SemIR::ClassId class_id,
+                              SemIR::ClangDeclId clang_decl_id) -> bool {
+  clang::ASTUnit* ast = context.sem_ir().cpp_ast();
+  CARBON_CHECK(ast);
 
-  auto [class_id, class_inst_id] = BuildClassDecl(
-      context, import_ir_inst_id, GetParentNameScopeId(context, clang_decl),
-      AddIdentifierName(context, clang_decl->getName()));
+  auto* clang_decl = cast<clang::CXXRecordDecl>(
+      context.sem_ir().clang_decls().Get(clang_decl_id).decl);
+  auto class_inst_id = context.types().GetAsTypeInstId(
+      context.classes().Get(class_id).first_owning_decl_id);
 
-  // TODO: The caller does the same lookup. Avoid doing it twice.
-  auto clang_decl_id = context.sem_ir().clang_decls().Add(
-      {.decl = clang_decl->getCanonicalDecl(), .inst_id = class_inst_id});
+  // TODO: Map loc_id into a clang location and use it for diagnostics if
+  // instantiation fails, instead of annotating the diagnostic with another
+  // location.
+  clang::SourceLocation loc = clang_decl->getLocation();
+  Diagnostics::AnnotationScope annotate_diagnostics(
+      &context.emitter(), [&](auto& builder) {
+        CARBON_DIAGNOSTIC(InCppTypeCompletion, Note,
+                          "while completing C++ class type {0}", SemIR::TypeId);
+        builder.Note(loc_id, InCppTypeCompletion,
+                     context.classes().Get(class_id).self_type_id);
+      });
 
-  if (clang_def) {
-    BuildClassDefinition(context, import_ir_inst_id, class_id, class_inst_id,
-                         clang_decl_id, clang_def);
+  // Ask Clang whether the type is complete. This triggers template
+  // instantiation if necessary.
+  clang::DiagnosticErrorTrap trap(ast->getDiagnostics());
+  if (!ast->getSema().isCompleteType(
+          loc, context.ast_context().getRecordType(clang_decl))) {
+    // Type is incomplete. Nothing more to do, but tell the caller if we
+    // produced an error.
+    return !trap.hasErrorOccurred();
   }
 
-  return class_inst_id;
+  clang::CXXRecordDecl* clang_def = clang_decl->getDefinition();
+  CARBON_CHECK(clang_def, "Complete type has no definition");
+
+  auto import_ir_inst_id =
+      context.insts().GetCanonicalLocId(class_inst_id).import_ir_inst_id();
+  BuildClassDefinition(context, import_ir_inst_id, class_id, class_inst_id,
+                       clang_def);
+  return true;
+}
+
+// Mark the given `Decl` as failed in `clang_decls`.
+static auto MarkFailedDecl(Context& context, clang::Decl* clang_decl) {
+  context.sem_ir().clang_decls().Add({.decl = clang_decl->getCanonicalDecl(),
+                                      .inst_id = SemIR::ErrorInst::InstId});
 }
 
 // Creates an integer type of the given size.
@@ -837,9 +866,6 @@ static auto MapBuiltinType(Context& context, clang::QualType qual_type,
 }
 
 // Maps a C++ record type to a Carbon type.
-// TODO: Support more record types.
-// TODO: Remove recursion mapping fields of class types.
-// NOLINTNEXTLINE(misc-no-recursion)
 static auto MapRecordType(Context& context, const clang::RecordType& type)
     -> TypeExpr {
   auto* record_decl = clang::dyn_cast<clang::CXXRecordDecl>(type.getDecl());
@@ -862,8 +888,6 @@ static auto MapRecordType(Context& context, const clang::RecordType& type)
 // Maps a C++ type that is not a wrapper type such as a pointer to a Carbon
 // type.
 // TODO: Support more types.
-// TODO: Remove recursion mapping fields of class types.
-// NOLINTNEXTLINE(misc-no-recursion)
 static auto MapNonWrapperType(Context& context, clang::QualType type)
     -> TypeExpr {
   if (const auto* builtin_type = type->getAs<clang::BuiltinType>()) {
@@ -928,8 +952,6 @@ static auto MapPointerType(Context& context, SemIR::LocId loc_id,
 // Maps a C++ type to a Carbon type. `type` should not be canonicalized because
 // we check for pointer nullability and nullability will be lost by
 // canonicalization.
-// TODO: Remove recursion mapping fields of class types.
-// NOLINTNEXTLINE(misc-no-recursion)
 static auto MapType(Context& context, SemIR::LocId loc_id, clang::QualType type)
     -> TypeExpr {
   // Unwrap any type modifiers and wrappers.
@@ -1318,7 +1340,6 @@ static auto AddDependentUnimportedTypeDecls(const Context& context,
 
   if (const auto* record_type = type->getAs<clang::RecordType>()) {
     AddDependentDecl(context, record_type->getDecl(), decls);
-    // TODO: Also import bases and fields if the class is defined.
   }
 }
 

toolchain/check/import_cpp.h

@@ -30,6 +30,13 @@ auto ImportNameFromCpp(Context& context, SemIR::LocId loc_id,
                        SemIR::NameScopeId scope_id, SemIR::NameId name_id)
     -> SemIR::ScopeLookupResult;
 
+// Given a class declaration that was imported from C++, attempt to import a
+// corresponding class definition. Returns true if nothing went wrong (whether
+// or not a definition could be imported), false if a diagnostic was produced.
+auto ImportCppClassDefinition(Context& context, SemIR::LocId loc_id,
+                              SemIR::ClassId class_id,
+                              SemIR::ClangDeclId clang_decl_id) -> bool;
+
 }  // namespace Carbon::Check
 
 #endif  // CARBON_TOOLCHAIN_CHECK_IMPORT_CPP_H_

toolchain/check/testdata/interop/cpp/class/base.carbon

@@ -158,6 +158,19 @@ fn ConvertB(p: Cpp.C*) -> Cpp.B* {
 struct A { int a; };
 struct B : virtual A {};
 
+void UseB(B * _Nonnull p);
+
+// --- use_virtual_inheritance_incomplete.carbon
+
+library "[[@TEST_NAME]]";
+
+import Cpp library "virtual_inheritance.h";
+
+fn Convert(p: Cpp.B*) {
+  // OK, doesn't require `Cpp.B` to be a complete type.
+  Cpp.UseB(p);
+}
+
 // --- fail_todo_use_virtual_inheritance.carbon
 
 library "[[@TEST_NAME]]";
@@ -166,11 +179,11 @@ library "[[@TEST_NAME]]";
 // CHECK:STDERR: ./virtual_inheritance.h:3: error: semantics TODO: `class with virtual bases` [SemanticsTodo]
 import Cpp library "virtual_inheritance.h";
 
-// CHECK:STDERR: fail_todo_use_virtual_inheritance.carbon:[[@LINE+4]]:15: note: in `Cpp` name lookup for `B` [InCppNameLookup]
-// CHECK:STDERR: fn Convert(p: Cpp.B*) -> Cpp.A* {
-// CHECK:STDERR:               ^~~~~
-// CHECK:STDERR:
 fn Convert(p: Cpp.B*) -> Cpp.A* {
+  // CHECK:STDERR: fail_todo_use_virtual_inheritance.carbon:[[@LINE+11]]:3: note: while completing C++ class type `Cpp.B` [InCppTypeCompletion]
+  // CHECK:STDERR:   return p;
+  // CHECK:STDERR:   ^~~~~~~~~
+  // CHECK:STDERR:
   // CHECK:STDERR: fail_todo_use_virtual_inheritance.carbon:[[@LINE+7]]:3: error: cannot implicitly convert expression of type `Cpp.B*` to `Cpp.A*` [ConversionFailure]
   // CHECK:STDERR:   return p;
   // CHECK:STDERR:   ^~~~~~~~~
@@ -221,9 +234,9 @@ class V {
 // CHECK:STDOUT:
 // CHECK:STDOUT: constants {
 // CHECK:STDOUT:   %Derived: type = class_type @Derived [concrete]
-// CHECK:STDOUT:   %Base: type = class_type @Base [concrete]
 // CHECK:STDOUT:   %ptr.ddb: type = ptr_type %Derived [concrete]
 // CHECK:STDOUT:   %pattern_type.5c8: type = pattern_type %ptr.ddb [concrete]
+// CHECK:STDOUT:   %Base: type = class_type @Base [concrete]
 // CHECK:STDOUT:   %ptr.fb2: type = ptr_type %Base [concrete]
 // CHECK:STDOUT:   %pattern_type.72a: type = pattern_type %ptr.fb2 [concrete]
 // CHECK:STDOUT:   %ConvertPtr.type: type = fn_type @ConvertPtr [concrete]
@@ -345,9 +358,9 @@ class V {
 // CHECK:STDOUT:
 // CHECK:STDOUT: constants {
 // CHECK:STDOUT:   %Derived: type = class_type @Derived [concrete]
-// CHECK:STDOUT:   %Base: type = class_type @Base [concrete]
 // CHECK:STDOUT:   %int_32: Core.IntLiteral = int_value 32 [concrete]
 // CHECK:STDOUT:   %i32: type = class_type @Int, @Int(%int_32) [concrete]
+// CHECK:STDOUT:   %Base: type = class_type @Base [concrete]
 // CHECK:STDOUT:   %Base.elem: type = unbound_element_type %Base, %i32 [concrete]
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
@@ -389,8 +402,8 @@ class V {
 // CHECK:STDOUT:
 // CHECK:STDOUT: constants {
 // CHECK:STDOUT:   %Derived: type = class_type @Derived [concrete]
-// CHECK:STDOUT:   %Base: type = class_type @Base [concrete]
 // CHECK:STDOUT:   %empty_tuple.type: type = tuple_type () [concrete]
+// CHECK:STDOUT:   %Base: type = class_type @Base [concrete]
 // CHECK:STDOUT:   %Base.f.type: type = fn_type @Base.f [concrete]
 // CHECK:STDOUT:   %Base.f: %Base.f.type = struct_value () [concrete]
 // CHECK:STDOUT:   %Base.g.type: type = fn_type @Base.g [concrete]

toolchain/check/testdata/interop/cpp/class/class.carbon

@@ -439,10 +439,10 @@ fn MyF(bar: Cpp.Bar*);
 // CHECK:STDOUT: constants {
 // CHECK:STDOUT:   %Bar: type = class_type @Bar [concrete]
 // CHECK:STDOUT:   %ptr.f68: type = ptr_type %Bar [concrete]
-// CHECK:STDOUT:   %Bar.elem: type = unbound_element_type %Bar, %ptr.f68 [concrete]
 // CHECK:STDOUT:   %pattern_type: type = pattern_type %ptr.f68 [concrete]
 // CHECK:STDOUT:   %MyF.type: type = fn_type @MyF [concrete]
 // CHECK:STDOUT:   %MyF: %MyF.type = struct_value () [concrete]
+// CHECK:STDOUT:   %Bar.elem: type = unbound_element_type %Bar, %ptr.f68 [concrete]
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
 // CHECK:STDOUT: imports {

toolchain/check/testdata/interop/cpp/class/field.carbon

@@ -165,10 +165,10 @@ fn G(s: Cpp.Union) -> i32 {
 // CHECK:STDOUT:   %Struct: type = class_type @Struct [concrete]
 // CHECK:STDOUT:   %int_32: Core.IntLiteral = int_value 32 [concrete]
 // CHECK:STDOUT:   %i32: type = class_type @Int, @Int(%int_32) [concrete]
+// CHECK:STDOUT:   %tuple.type.189: type = tuple_type (%i32, %i32, %i32) [concrete]
 // CHECK:STDOUT:   %Struct.elem.86b: type = unbound_element_type %Struct, %i32 [concrete]
 // CHECK:STDOUT:   %ptr.235: type = ptr_type %i32 [concrete]
 // CHECK:STDOUT:   %Struct.elem.765: type = unbound_element_type %Struct, %ptr.235 [concrete]
-// CHECK:STDOUT:   %tuple.type.189: type = tuple_type (%i32, %i32, %i32) [concrete]
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
 // CHECK:STDOUT: imports {

toolchain/check/testdata/interop/cpp/class/struct.carbon

@@ -451,10 +451,10 @@ fn MyF(bar: Cpp.Bar*);
 // CHECK:STDOUT: constants {
 // CHECK:STDOUT:   %Bar: type = class_type @Bar [concrete]
 // CHECK:STDOUT:   %ptr.f68: type = ptr_type %Bar [concrete]
-// CHECK:STDOUT:   %Bar.elem: type = unbound_element_type %Bar, %ptr.f68 [concrete]
 // CHECK:STDOUT:   %pattern_type: type = pattern_type %ptr.f68 [concrete]
 // CHECK:STDOUT:   %MyF.type: type = fn_type @MyF [concrete]
 // CHECK:STDOUT:   %MyF: %MyF.type = struct_value () [concrete]
+// CHECK:STDOUT:   %Bar.elem: type = unbound_element_type %Bar, %ptr.f68 [concrete]
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
 // CHECK:STDOUT: imports {