Keep resolving generics and templates until they're all resolved (#7957)

`FinishAst` resolves C++ template instantiation. Template instantiation
can complete Carbon types, generate thunks, and call generic functions.
These operations can add new entries to `definitions_required_by_use()`.
Similarly, resolving any of those can generate more C++ templates that
need instantiating. This change moves `FinishAst` ahead of
`CheckRequiredDefinitions` and loops until no new Carbon definitions
have been added. This prevents later passes from crashing when
evaluating specifics on unresolved generics or uninstantiated templates.

`FinishAst` also disconnects the AST. Subsequent definitions and `impl`s
may depend on C++ interop, so the AST must remain connected until all
the Carbon end-of-file checks complete. We achieve this by decoupling
AST disconnection from finalisation.

Assisted-by: Antigravity with Gemini
Assisted-by: Antigravity with Claude Opus 5.5 Max

toolchain/check/check_unit.cpp

@@ -20,6 +20,7 @@
 #include "toolchain/base/kind_switch.h"
 #include "toolchain/check/cpp/generate_ast.h"
 #include "toolchain/check/cpp/import.h"
+#include "toolchain/check/cpp/location.h"
 #include "toolchain/check/diagnostic_helpers.h"
 #include "toolchain/check/generic.h"
 #include "toolchain/check/handle.h"
@@ -96,7 +97,8 @@ auto CheckUnit::Run() -> void {
     context_.sem_ir().set_has_errors(true);
 
     // TODO: Are there more steps from `FinishRun` we should be doing here?
-    FinishAst(context_);
+    FinalizeCppAstFragment(context_);
+    DisconnectCppAst(context_);
     return;
   }
 
@@ -460,7 +462,7 @@ auto CheckUnit::CheckRequiredDeclarations() -> void {
   }
 }
 
-auto CheckUnit::CheckRequiredDefinitions() -> void {
+auto CheckUnit::CheckRequiredDefinitions() -> bool {
   CARBON_DIAGNOSTIC(MissingDefinitionInImpl, Error,
                     "no definition found for declaration in impl file");
   for (SemIR::InstId decl_inst_id : context_.definitions_required_by_decl()) {
@@ -503,25 +505,38 @@ auto CheckUnit::CheckRequiredDefinitions() -> void {
       }
     }
   }
+  context_.definitions_required_by_decl().clear();
 
+  auto new_resolution_occurred = false;
   for (auto [loc, specific_id] :
        GrowingRange(context_.definitions_required_by_use())) {
     // This is using the location for the use. We could track the
     // list of enclosing locations if this was used from a generic.
-    if (!ResolveSpecificDefinition(context_, loc, specific_id)) {
-      CARBON_DIAGNOSTIC(MissingGenericFunctionDefinition, Error,
-                        "use of undefined generic function");
-      CARBON_DIAGNOSTIC_LABEL(MissingGenericFunctionDefinitionHere, Info,
-                              "generic function declared here");
-      auto generic_decl_id =
-          context_.generics()
-              .Get(context_.specifics().Get(specific_id).generic_id)
-              .decl_id;
-      emitter_.Build(loc, MissingGenericFunctionDefinition)
-          .Attach(generic_decl_id, MissingGenericFunctionDefinitionHere)
-          .Emit();
+    switch (ResolveSpecificDefinition(context_, loc, specific_id)) {
+      case DefinitionResolution::NewlyResolved:
+        new_resolution_occurred = true;
+        break;
+      case DefinitionResolution::PreviouslyResolved:
+        break;
+      case DefinitionResolution::NotDefinedYet: {
+        CARBON_DIAGNOSTIC(MissingGenericFunctionDefinition, Error,
+                          "use of undefined generic function");
+        CARBON_DIAGNOSTIC_LABEL(MissingGenericFunctionDefinitionHere, Info,
+                                "generic function declared here");
+        auto generic_decl_id =
+            context_.generics()
+                .Get(context_.specifics().Get(specific_id).generic_id)
+                .decl_id;
+        emitter_.Build(loc, MissingGenericFunctionDefinition)
+            .Attach(generic_decl_id, MissingGenericFunctionDefinitionHere)
+            .Emit();
+        break;
+      }
     }
   }
+
+  context_.definitions_required_by_use().clear();
+  return new_resolution_occurred;
 }
 
 auto CheckUnit::CheckPoisonedConcreteImplLookupQueries() -> void {
@@ -613,13 +628,56 @@ auto CheckUnit::CheckPoisonedConcreteImplLookupQueries() -> void {
 auto CheckUnit::CheckImpls() -> void { ValidateImplsInFile(context_); }
 
 auto CheckUnit::FinishRun() -> void {
+  // TODO: Find a more optimal way to evaluate mutually-dependent interop
+  // between Carbon and C++.
+  //
+  // Experimental evidence indicates that each additional loop doubles the
+  // run-time of `fail_todo_bidirectional_intorp_too_deep.carbon`. That unit
+  // test isn't terribly complex, so we should probably solve this fairly soon.
+  //
+  // We chose 10 loops to make progress without significantly slowing down debug
+  // unit tests.
+  constexpr auto max_resolution_check_cycles = 10;
+  auto num_resolution_check_cycles = 0;
+
+  while (num_resolution_check_cycles++ < max_resolution_check_cycles) {
+    // Finalizes the C++ portion of the compilation, allowing any pending C++
+    // template instantiations to complete Carbon types and generate required
+    // thunks or function calls.
+    FinalizeCppAstFragment(context_);
+
+    if (!CheckRequiredDefinitions()) {
+      break;
+    }
+  }
+
+  if (num_resolution_check_cycles > max_resolution_check_cycles) {
+    // Point at the code that queued the next C++ instantiation, because that
+    // is what needed another pass. If nothing is queued, point at the top of
+    // the file.
+    auto loc_id = SemIR::LocId::None;
+    auto* cpp_context = context_.cpp_context();
+    if (cpp_context && !cpp_context->sema().PendingInstantiations.empty()) {
+      loc_id = AddImportIRInst(
+          context_.sem_ir(),
+          cpp_context->sema().PendingInstantiations.front().second);
+    }
+    context_.TODO(
+        loc_id, llvm::formatv(
+                    "maximum depth for bidirectional Carbon/C++ interop is {}",
+                    max_resolution_check_cycles));
+    if (cpp_context) {
+      cpp_context->sema().PendingInstantiations.clear();
+    }
+  }
+
   CheckRequiredDeclarations();
-  CheckRequiredDefinitions();
   CheckPoisonedConcreteImplLookupQueries();
   CheckImpls();
 
-  // Finalizes the C++ portion of the compilation.
-  FinishAst(context_);
+  // Disconnects AST generation from the context before popping the file-level
+  // scope and destroying the context.
+  DisconnectCppAst(context_);
 
   // Pop information for the file-level scope.
   context_.sem_ir().set_top_inst_block_id(context_.inst_block_stack().Pop());

toolchain/check/check_unit.h

@@ -175,9 +175,15 @@ class CheckUnit {
 
   // Checks that each required definition is available. If the definition can be
   // generated by resolving a specific, does so, otherwise emits a diagnostic
-  // for each declaration in context.definitions_required_by_decl() and
-  // context.definitions_required_by_use that doesn't have a definition.
-  auto CheckRequiredDefinitions() -> void;
+  // for each declaration in `context.definitions_required_by_decl()` and
+  // `context.definitions_required_by_use()` that doesn't have a definition.
+  // Each call clears both vectors.
+  //
+  // The C++ AST fragment may not be finalized if an element in
+  // `context.definitions_required_by_use()` is resolved by calling this
+  // function. This function returns true if an element was resolved, and false
+  // otherwise.
+  [[nodiscard]] auto CheckRequiredDefinitions() -> bool;
 
   // Re-run every impl lookup with a concrete result and make sure they find the
   // same witnesses.

toolchain/check/cpp/generate_ast.cpp

@@ -1097,13 +1097,42 @@ auto InjectAstFromInlineCode(Context& context, SemIR::LocId loc_id,
   ParseInjectedTokens(*cpp_context);
 }
 
-auto FinishAst(Context& context) -> void {
+auto FinalizeCppAstFragment(Context& context) -> void {
+  // `FinalizeCppAst` operates very close to where AST fragment is
+  // nullified. Builds may call this function with a null AST fragment,
+  // so we exit early in the case where it's null.
+  //
+  // TODO: investigate if we can avoid AST fragment resolution when the
+  // fragment is null.
+  if (!context.cpp_context()) {
+    return;
+  }
+
+  // Finalize the per-Context AST fragment. The final ActOnEndOfTranslationUnit
+  // call for the CppDomain is performed in FinalizeCppDomain once all files
+  // sharing the domain have been checked.
+  context.cpp_context()->sema().ActOnEndOfTranslationUnitFragment(
+      clang::TUFragmentKind::Normal);
+  FlushDiagnosticConsumer(
+      *context.cpp_context()->sema().getDiagnostics().getClient());
+  context.emitter().Flush();
+}
+
+auto DisconnectCppAst(Context& context) -> void {
+  // `DisconnectCppAst` operates very close to where AST fragment is
+  // nullified. Builds may call this function with a null AST fragment,
+  // so we exit early in the case where it's null.
+  //
+  // TODO: investigate if we can avoid AST fragment disconnection when
+  // the fragment is null.
   if (!context.cpp_context()) {
     return;
   }
 
   // Leave the module we entered to encapsulate the contents of this Carbon
-  // file.
+  // file. This happens here rather than in `FinalizeCppAstFragment`, because
+  // that can run more than once for a file, but the module is only entered
+  // once.
   auto end_loc_id =
       SemIR::LocId(*(context.sem_ir().parse_tree().postorder().end() - 1));
   // Shuffle the end of file location back by one character to work around a
@@ -1115,15 +1144,6 @@ auto FinishAst(Context& context) -> void {
   LeaveModule(*context.cpp_context(),
               GetCppLocation(context, end_loc_id).getLocWithOffset(-1));
 
-  // Finalize the per-Context AST fragment. The final ActOnEndOfTranslationUnit
-  // call for the CppDomain is performed in FinalizeCppDomain once all files
-  // sharing the domain have been checked.
-  context.cpp_context()->sema().ActOnEndOfTranslationUnitFragment(
-      clang::TUFragmentKind::Normal);
-  FlushDiagnosticConsumer(
-      *context.cpp_context()->sema().getDiagnostics().getClient());
-  context.emitter().Flush();
-
   // Hide the per-file lookup-only `::Carbon` namespace from the further lookup
   // results and remove it from the translation unit's name lookup table if it's
   // already there, so that subsequent files in a shared C++ AST domain do not

toolchain/check/cpp/generate_ast.h

@@ -52,9 +52,15 @@ auto GenerateAst(Context& context,
 auto InjectAstFromInlineCode(Context& context, SemIR::LocId loc_id,
                              llvm::StringRef source_code) -> void;
 
-// Finishes AST generation for the given checking context. Performs end of file
-// steps such as template instantiation and warning on unused declarations.
-auto FinishAst(Context& context) -> void;
+// Finalizes the C++ AST fragment for the given checking context. Performs end
+// of file steps such as template instantiation and warning on unused
+// declarations. This may be called more than once for the same context, to
+// perform template instantiations that later Carbon checking requires.
+auto FinalizeCppAstFragment(Context& context) -> void;
+
+// Disconnects C++ AST generation from the given checking context at the end of
+// checking.
+auto DisconnectCppAst(Context& context) -> void;
 
 // Finalizes a C++ domain at the end of checking all files.
 auto FinalizeCppDomain(SemIR::CppDomain& domain) -> void;

toolchain/check/function.cpp

@@ -591,7 +591,8 @@ auto ResolveReturnTypeInSpecific(Context& context, SemIR::LocId loc_id,
 
   // Otherwise, the return type and the return pattern were created
   // within the definition of the generic, and are computed by its eval block.
-  if (!ResolveSpecificDefinition(context, loc_id, specific_id)) {
+  if (ResolveSpecificDefinition(context, loc_id, specific_id) ==
+      DefinitionResolution::NotDefinedYet) {
     // We're within the definition of the function, after its return type was
     // deduced. In the self specific, the return type is just the deduced type,
     // but we can't compute it for any other specific until the definition is

toolchain/check/generic.cpp

@@ -764,24 +764,27 @@ auto MakeSelfSpecific(Context& context, SemIR::LocId loc_id,
 }
 
 auto ResolveSpecificDefinition(Context& context, SemIR::LocId loc_id,
-                               SemIR::SpecificId specific_id) -> bool {
+                               SemIR::SpecificId specific_id)
+    -> DefinitionResolution {
   // TODO: Handle recursive resolution of the same generic definition.
   auto& specific = context.specifics().Get(specific_id);
   auto generic_id = specific.generic_id;
   CARBON_CHECK(generic_id.has_value(), "Specific with no generic ID");
 
-  if (!specific.definition_block_id.has_value()) {
-    // Evaluate the eval block for the definition of the generic.
-    auto& generic = context.generics().Get(generic_id);
-    CARBON_CHECK(generic.decl_block_id.has_value(), "missing declaration");
-    if (!generic.definition_block_id.has_value()) {
-      // The generic is not defined yet.
-      return false;
-    }
-    TryEvalBlockForSpecific(context, loc_id, specific_id,
-                            SemIR::GenericInstIndex::Definition);
+  if (specific.definition_block_id.has_value()) {
+    return DefinitionResolution::PreviouslyResolved;
   }
-  return true;
+
+  // Evaluate the eval block for the definition of the generic.
+  auto& generic = context.generics().Get(generic_id);
+  CARBON_CHECK(generic.decl_block_id.has_value(), "missing declaration");
+  if (!generic.definition_block_id.has_value()) {
+    return DefinitionResolution::NotDefinedYet;
+  }
+
+  TryEvalBlockForSpecific(context, loc_id, specific_id,
+                          SemIR::GenericInstIndex::Definition);
+  return DefinitionResolution::NewlyResolved;
 }
 
 auto DiagnoseIfGenericMissingExplicitParameters(

toolchain/check/generic.h

@@ -123,11 +123,23 @@ auto MakeSelfSpecific(Context& context, SemIR::LocId loc_id,
 auto ResolveSpecificDecl(Context& context, SemIR::LocId loc_id,
                          SemIR::SpecificId specific_id) -> void;
 
+enum class DefinitionResolution : std::uint8_t {
+  // The definition is not available.
+  NotDefinedYet,
+
+  // The definition was resolved by this operation.
+  NewlyResolved,
+
+  // The definition was resolved by a previous operation.
+  PreviouslyResolved,
+};
+
 // Attempts to resolve the definition of the given specific, by evaluating the
 // eval block of the corresponding generic and storing a corresponding value
-// block in the specific. Returns false if a definition is not available.
+// block in the specific.
 auto ResolveSpecificDefinition(Context& context, SemIR::LocId loc_id,
-                               SemIR::SpecificId specific_id) -> bool;
+                               SemIR::SpecificId specific_id)
+    -> DefinitionResolution;
 
 // Diagnoses if an entity has implicit parameters, indicating it's generic, but
 // is missing explicit parameters.

toolchain/check/testdata/interop/cpp/template/bidirectional_generic_call.carbon

@@ -0,0 +1,94 @@
+// 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/int.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/template/bidirectional_generic_call.carbon
+// TIP: To dump output, run:
+// TIP:   bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/check/testdata/interop/cpp/template/bidirectional_generic_call.carbon
+
+// --- member_function_calls_carbon.carbon
+
+library "[[@TEST_NAME]]";
+
+// Resolving `Make({} as C)` at the end of the file uses the constructor of
+// `Wrapper(C)`, whose definition is only instantiated after that. The
+// constructor calls back into Carbon, so `C` must still be usable from C++.
+import Cpp inline '''
+template<typename T>
+struct Wrapper {
+  Wrapper() { T::Callback(); }
+};
+''';
+
+class C {
+  fn Callback() {}
+}
+
+fn Make[T: type](unused x: T) {
+  var unused w: Cpp.Wrapper(T);
+}
+
+fn Run() {
+  Make({} as C);
+}
+
+// --- alternating_passes.carbon
+
+library "[[@TEST_NAME]]";
+
+// Each step needs the other language to instantiate or resolve something new:
+//
+// 1. `Run` needs the C++ function template `CallCarbonGeneric<Box>`.
+// 2. `CallCarbonGeneric<Box>` needs the Carbon generic
+//    `UseCppClassTemplate(Box)`.
+// 3. `UseCppClassTemplate(Box)` needs the C++ class template
+//    `UsesCarbonInterface<Box>`.
+// 4. `UsesCarbonInterface<Box>::Get` needs `Box` to implement the Carbon
+//    interface `HasValue`, by calling `GetValue(Box)`. This is the base case.
+import Cpp inline '''
+template<typename T>
+struct UsesCarbonInterface {
+  static auto Get(const T& t) -> int;
+};
+''';
+
+interface HasValue {
+  fn Value(self) -> i32;
+}
+
+class Box {
+  var n: i32;
+
+  impl as HasValue {
+    fn Value(self) -> i32 { return self.n; }
+  }
+}
+
+fn GetValue[T: HasValue](t: T) -> i32 {
+  return t.Value();
+}
+
+fn UseCppClassTemplate[T: type](t: T) -> i32 {
+  return Cpp.UsesCarbonInterface(T).Get(t);
+}
+
+inline Cpp '''
+template<typename T>
+auto UsesCarbonInterface<T>::Get(const T& t) -> int {
+  return Carbon::GetValue(t);
+}
+
+template<typename T>
+auto CallCarbonGeneric(const T& t) -> int {
+  return Carbon::UseCppClassTemplate(t);
+}
+''';
+
+fn Run() -> i32 {
+  let b: Box = {.n = 42};
+  return Cpp.CallCarbonGeneric(b);
+}

toolchain/check/testdata/interop/cpp/template/fail_todo_bidirectional_intorp_too_deep.carbon

@@ -0,0 +1,37 @@
+// 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/convert.carbon
+// INCLUDE-FILE: toolchain/testing/testdata/min_prelude/parts/int.carbon
+// INCLUDE-FILE: toolchain/testing/testdata/min_prelude/parts/optional.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/template/fail_todo_bidirectional_intorp_too_deep.carbon
+// TIP: To dump output, run:
+// TIP:   bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/check/testdata/interop/cpp/template/fail_todo_bidirectional_intorp_too_deep.carbon
+
+import Core library "prelude/parts/optional";
+import Core library "prelude/parts/int";
+import Cpp;
+
+fn ManualMultiply[template T: type](p: T*) -> T*;
+
+inline Cpp '''c++
+template<typename T>
+auto ManualMultiply(T x) -> T {
+    return *Carbon::ManualMultiply(&x);
+  }
+''';
+
+fn ManualMultiply[template T: type](p: T*) -> T* {
+    // CHECK:STDERR: fail_todo_bidirectional_intorp_too_deep.carbon:[[@LINE+2]]:32: error: semantics TODO: `maximum depth for bidirectional Carbon/C++ interop is 10` [SemanticsTodo]
+    // CHECK:STDERR:
+    return Cpp.ManualMultiply(p);
+}
+
+fn Test() {
+  var v: i32 = 5;
+  ManualMultiply(&v);
+}