Support importing nested types from C++. (#5955)

Fix the algorithm for importing declarations in dependency order to
properly walk the dependency graph. Add the parent declaration of a
declaration to the dependency set so that we have a parent declaration
context to import a declaration into.

Fixes a crash when attempting to import a class whose parent is not
imported.

Author: zygoloid

toolchain/check/import_cpp.cpp

@@ -680,8 +680,8 @@ static auto ImportNamespaceDecl(Context& context,
   return result.inst_id;
 }
 
-static auto MapType(Context& context, SemIR::LocId loc_id, clang::QualType type)
-    -> TypeExpr;
+static auto ImportTypeAndDependencies(Context& context, SemIR::LocId loc_id,
+                                      clang::QualType type) -> TypeExpr;
 
 // Creates a class declaration for the given class name in the given scope.
 // Returns the `InstId` for the declaration.
@@ -828,7 +828,7 @@ static auto ImportClassObjectRepr(Context& context, SemIR::ClassId class_id,
                  "Should not import definition for class with a virtual base");
 
     auto [base_type_inst_id, base_type_id] =
-        MapType(context, import_ir_inst_id, base.getType());
+        ImportTypeAndDependencies(context, import_ir_inst_id, base.getType());
     if (!base_type_id.has_value()) {
       // TODO: If the base class's type can't be mapped, skip it.
       continue;
@@ -898,7 +898,7 @@ static auto ImportClassObjectRepr(Context& context, SemIR::ClassId class_id,
 
     auto field_name_id = AddIdentifierName(context, field->getName());
     auto [field_type_inst_id, field_type_id] =
-        MapType(context, import_ir_inst_id, field->getType());
+        ImportTypeAndDependencies(context, import_ir_inst_id, field->getType());
     if (!field_type_inst_id.has_value()) {
       // TODO: For now, just skip over fields whose types we can't map.
       continue;
@@ -1577,22 +1577,33 @@ static auto ImportFunctionDecl(Context& context, SemIR::LocId loc_id,
   return function_info.first_owning_decl_id;
 }
 
-using DeclSet = llvm::SetVector<clang::Decl*>;
+namespace {
+// An item to be imported in an import worklist.
+// TODO: If worklists ever become particularly large, consider changing this
+// to use a `PointerIntPair`.
+struct ImportItem {
+  // A declaration that we want to import.
+  clang::Decl* decl;
+  // Whether we have added `decl`'s dependencies to the worklist.
+  bool added_dependencies;
+};
+// A worklist of declarations to import.
+using ImportWorklist = llvm::SmallVector<ImportItem>;
+}  // namespace
 
 // Adds the given declaration to our list of declarations to import.
 static auto AddDependentDecl(const Context& context, clang::Decl* decl,
-                             DeclSet& decls) -> void {
-  // TODO: Do we need to also add the parent of the declaration, recursively?
+                             ImportWorklist& worklist) -> void {
   if (!IsClangDeclImported(context, decl)) {
-    decls.insert(decl);
+    worklist.push_back({.decl = decl, .added_dependencies = false});
   }
 }
 
 // Finds all decls that need to be imported before importing the given type and
 // adds them to the given set.
 static auto AddDependentUnimportedTypeDecls(const Context& context,
                                             clang::QualType type,
-                                            DeclSet& decls) -> void {
+                                            ImportWorklist& worklist) -> void {
   while (true) {
     if (type->isPointerType() || type->isReferenceType()) {
       type = type->getPointeeType();
@@ -1605,41 +1616,45 @@ static auto AddDependentUnimportedTypeDecls(const Context& context,
   }
 
   if (const auto* record_type = type->getAs<clang::RecordType>()) {
-    AddDependentDecl(context, record_type->getDecl(), decls);
+    AddDependentDecl(context, record_type->getDecl(), worklist);
   }
 }
 
 // Finds all decls that need to be imported before importing the given function
 // and adds them to the given set.
 static auto AddDependentUnimportedFunctionDecls(
     const Context& context, const clang::FunctionDecl& clang_decl,
-    DeclSet& decls) -> void {
+    ImportWorklist& worklist) -> void {
   for (const auto* param : clang_decl.parameters()) {
-    AddDependentUnimportedTypeDecls(context, param->getType(), decls);
+    AddDependentUnimportedTypeDecls(context, param->getType(), worklist);
   }
-  AddDependentUnimportedTypeDecls(context, clang_decl.getReturnType(), decls);
+  AddDependentUnimportedTypeDecls(context, clang_decl.getReturnType(),
+                                  worklist);
 }
 
 // Finds all decls that need to be imported before importing the given
 // declaration and adds them to the given set.
 static auto AddDependentUnimportedDecls(const Context& context,
-                                        clang::Decl* clang_decl, DeclSet& decls)
-    -> void {
-  if (auto* parent_decl = GetParentDecl(clang_decl)) {
-    AddDependentDecl(context, parent_decl, decls);
-  }
-
+                                        clang::Decl* clang_decl,
+                                        ImportWorklist& worklist) -> void {
   if (auto* clang_function_decl = clang_decl->getAsFunction()) {
-    AddDependentUnimportedFunctionDecls(context, *clang_function_decl, decls);
+    AddDependentUnimportedFunctionDecls(context, *clang_function_decl,
+                                        worklist);
   } else if (auto* type_decl = dyn_cast<clang::TypeDecl>(clang_decl)) {
-    AddDependentUnimportedTypeDecls(
-        context, type_decl->getASTContext().getTypeDeclType(type_decl), decls);
+    if (!isa<clang::RecordDecl>(clang_decl)) {
+      AddDependentUnimportedTypeDecls(
+          context, type_decl->getASTContext().getTypeDeclType(type_decl),
+          worklist);
+    }
+  }
+  if (!isa<clang::TranslationUnitDecl>(clang_decl)) {
+    AddDependentDecl(context, GetParentDecl(clang_decl), worklist);
   }
 }
 
-// Imports a declaration from Clang to Carbon. If successful, returns the
-// instruction for the new Carbon declaration. Assumes all dependencies have
-// already been imported.
+// Imports a declaration from Clang to Carbon. Returns the instruction for the
+// new Carbon declaration, which will be an ErrorInst on failure. Assumes all
+// dependencies have already been imported.
 static auto ImportDeclAfterDependencies(Context& context, SemIR::LocId loc_id,
                                         clang::Decl* clang_decl)
     -> SemIR::InstId {
@@ -1658,6 +1673,8 @@ static auto ImportDeclAfterDependencies(Context& context, SemIR::LocId loc_id,
                                  type.getAsString()));
       return SemIR::ErrorInst::InstId;
     }
+    context.sem_ir().clang_decls().Add(
+        {.decl = clang_decl, .inst_id = type_inst_id});
     return type_inst_id;
   }
   if (isa<clang::FieldDecl, clang::IndirectFieldDecl>(clang_decl)) {
@@ -1678,30 +1695,71 @@ static auto ImportDeclAfterDependencies(Context& context, SemIR::LocId loc_id,
   return SemIR::ErrorInst::InstId;
 }
 
+// Attempts to import a set of declarations. Returns `false` if an error was
+// produced, `true` otherwise.
+static auto ImportDeclSet(Context& context, SemIR::LocId loc_id,
+                          ImportWorklist& worklist) -> bool {
+  // Walk the dependency graph in depth-first order, and import declarations
+  // once we've imported all of their dependencies.
+  while (!worklist.empty()) {
+    auto& item = worklist.back();
+    if (!item.added_dependencies) {
+      // Skip items we've already imported. We checked this when initially
+      // adding the item to the worklist, but it might have been added to the
+      // worklist twice before the first time we visited it. For example, this
+      // happens for `fn F(a: Cpp.T, b: Cpp.T)`.
+      if (IsClangDeclImported(context, item.decl)) {
+        worklist.pop_back();
+        continue;
+      }
+
+      // First time visiting this declaration (preorder): add its dependencies
+      // to the work list.
+      item.added_dependencies = true;
+      AddDependentUnimportedDecls(context, item.decl, worklist);
+    } else {
+      // Second time visiting this declaration (postorder): its dependencies are
+      // already imported, so we can import it now.
+      auto* decl = worklist.pop_back_val().decl;
+      auto inst_id = ImportDeclAfterDependencies(context, loc_id, decl);
+      CARBON_CHECK(inst_id.has_value());
+      if (inst_id == SemIR::ErrorInst::InstId) {
+        return false;
+      }
+      CARBON_CHECK(IsClangDeclImported(context, decl));
+    }
+  }
+
+  return true;
+}
+
 // Imports a declaration from Clang to Carbon. If successful, returns the
-// instruction for the new Carbon declaration. All unimported dependencies would
-// be imported first.
+// instruction for the new Carbon declaration. All unimported dependencies are
+// imported first.
 static auto ImportDeclAndDependencies(Context& context, SemIR::LocId loc_id,
                                       clang::Decl* clang_decl)
     -> SemIR::InstId {
-  // Collect dependencies.
-  llvm::SetVector<clang::Decl*> clang_decls;
-  clang_decls.insert(clang_decl);
-  for (size_t i = 0; i < clang_decls.size(); ++i) {
-    AddDependentUnimportedDecls(context, clang_decls[i], clang_decls);
-  }
-
-  // Import dependencies in reverse order.
-  auto inst_id = SemIR::InstId::None;
-  for (clang::Decl* clang_decl_to_import : llvm::reverse(clang_decls)) {
-    inst_id =
-        ImportDeclAfterDependencies(context, loc_id, clang_decl_to_import);
-    if (!inst_id.has_value()) {
-      break;
-    }
+  // Collect dependencies by walking the dependency graph in depth-first order.
+  ImportWorklist worklist;
+  AddDependentDecl(context, clang_decl, worklist);
+  if (!ImportDeclSet(context, loc_id, worklist)) {
+    return SemIR::ErrorInst::InstId;
   }
+  return LookupClangDeclInstId(context, clang_decl);
+}
 
-  return inst_id;
+// Imports a type from Clang to Carbon. If successful, returns the imported
+// TypeId. All unimported dependencies are imported first.
+static auto ImportTypeAndDependencies(Context& context, SemIR::LocId loc_id,
+                                      clang::QualType type) -> TypeExpr {
+  // Collect dependencies by walking the dependency graph in depth-first order.
+  ImportWorklist worklist;
+  AddDependentUnimportedTypeDecls(context, type, worklist);
+  if (!ImportDeclSet(context, loc_id, worklist)) {
+    return {.inst_id = SemIR::ErrorInst::TypeInstId,
+            .type_id = SemIR::ErrorInst::TypeId};
+  }
+  return MapType(context, loc_id, type);
 }
 
 // Maps `clang::AccessSpecifier` to `SemIR::AccessKind`.

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

@@ -0,0 +1,30 @@
+// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
+// Exceptions. See /LICENSE for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+// INCLUDE-FILE: toolchain/testing/testdata/min_prelude/primitives.carbon
+//
+// AUTOUPDATE
+// TIP: To test this file alone, run:
+// TIP:   bazel test //toolchain/testing:file_test --test_arg=--file_tests=toolchain/check/testdata/interop/cpp/class/nested.carbon
+// TIP: To dump output, run:
+// TIP:   bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/check/testdata/interop/cpp/class/nested.carbon
+
+// --- import_nested_class.carbon
+
+library "[[@TEST_NAME]]";
+
+import Cpp inline '''
+struct A {
+  struct B {
+    struct C {
+      int abc;
+    };
+  };
+  B::C c;
+};
+''';
+
+fn F(p: Cpp.A*) -> i32 {
+  return p->c.abc;
+}

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

@@ -2,7 +2,7 @@
 // Exceptions. See /LICENSE for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
-// INCLUDE-FILE: toolchain/testing/testdata/min_prelude/destroy.carbon
+// INCLUDE-FILE: toolchain/testing/testdata/min_prelude/primitives.carbon
 //
 // AUTOUPDATE
 // TIP: To test this file alone, run:
@@ -207,6 +207,32 @@ fn MyF() {
   //@dump-sem-ir-end
 }
 
+// ============================================================================
+// Nested namespaces.
+// ============================================================================
+
+// --- nested.carbon
+
+library "[[@TEST_NAME]]";
+
+import Cpp inline '''
+namespace A {
+namespace B {
+namespace C {
+struct X { int n; };
+}
+}
+}
+
+struct Y {
+  A::B::C::X x;
+};
+''';
+
+fn Use(y: Cpp.Y) -> i32 {
+  return y.x.n;
+}
+
 // CHECK:STDOUT: --- import_single.carbon
 // CHECK:STDOUT:
 // CHECK:STDOUT: constants {