Fix determination of whether we're in a nested deferred definition scope. (#3844)

Instead of attempting to reconstruct this based on the properties of the
target scope of the declaration name, check whether the enclosing scope
of the inner deferred definition scope is the enclosing deferred
definition scope.

Also add verbose logging to the deferred definition handling code to
make it easier to see when we switch between handling the regular parse
nodes and the deferred function bodies.

toolchain/check/BUILD

@@ -124,6 +124,7 @@ cc_library(
         "//common:error",
         "//common:ostream",
         "//common:variant_helpers",
+        "//common:vlog",
         "//toolchain/base:pretty_stack_trace_function",
         "//toolchain/base:value_store",
         "//toolchain/diagnostics:diagnostic_emitter",

toolchain/check/check.cpp

@@ -8,6 +8,8 @@
 
 #include "common/check.h"
 #include "common/error.h"
+#include "common/variant_helpers.h"
+#include "common/vlog.h"
 #include "toolchain/base/pretty_stack_trace_function.h"
 #include "toolchain/check/context.h"
 #include "toolchain/check/diagnostic_helpers.h"
@@ -307,42 +309,6 @@ class NextDeferredDefinitionCache {
 };
 }  // namespace
 
-// Determines whether we are currently declaring a name in a scope in which
-// function definitions are deferred. When entering another deferred definition
-// scope, the inner scope's function definitions are checked at the end of the
-// outer scope, not the inner one.  For example:
-//
-// ```
-// class A {
-//   class B {
-//     fn F() -> A { return {}; }
-//   }
-// } // A.B.F is type-checked here, with A complete.
-//
-// fn F() {
-//   class C {
-//     fn G() {}
-//   } // C.G is type-checked here.
-// }
-// ```
-static auto IsInDeferredDefinitionScope(Context& context) -> bool {
-  auto inst_id = context.name_scopes().GetInstIdIfValid(
-      context.decl_name_stack().PeekTargetScope());
-  if (!inst_id.is_valid()) {
-    return false;
-  }
-  switch (context.insts().Get(inst_id).kind()) {
-    case SemIR::ClassDecl::Kind:
-    case SemIR::ImplDecl::Kind:
-    case SemIR::InterfaceDecl::Kind:
-      // TODO: Named constraints, mixins.
-      return true;
-
-    default:
-      return false;
-  }
-}
-
 // Determines whether this node kind is the start of a deferred definition
 // scope.
 static auto IsStartOfDeferredDefinitionScope(Parse::NodeKind kind) -> bool {
@@ -393,7 +359,21 @@ class DeferredDefinitionWorklist {
     std::optional<DeclNameStack::SuspendedName> suspended_name;
     // Whether this scope is itself within an outer deferred definition scope.
     // If so, we'll delay processing its contents until we reach the end of the
-    // enclosing scope.
+    // enclosing scope. For example:
+    //
+    // ```
+    // class A {
+    //   class B {
+    //     fn F() -> A { return {}; }
+    //   }
+    // } // A.B.F is type-checked here, with A complete.
+    //
+    // fn F() {
+    //   class C {
+    //     fn G() {}
+    //   } // C.G is type-checked here.
+    // }
+    // ```
     bool in_deferred_definition_scope;
   };
 
@@ -408,11 +388,15 @@ class DeferredDefinitionWorklist {
       std::variant<CheckSkippedDefinition, EnterDeferredDefinitionScope,
                    LeaveDeferredDefinitionScope>;
 
-  DeferredDefinitionWorklist() {
+  explicit DeferredDefinitionWorklist(llvm::raw_ostream* vlog_stream)
+      : vlog_stream_(vlog_stream) {
     // See declaration of `worklist_`.
     worklist_.reserve(64);
   }
 
+  static constexpr llvm::StringLiteral VlogPrefix =
+      "DeferredDefinitionWorklist ";
+
   // Suspend the current function definition and push a task onto the worklist
   // to finish it later.
   auto SuspendFunctionAndPush(Context& context,
@@ -421,14 +405,22 @@ class DeferredDefinitionWorklist {
       -> void {
     worklist_.push_back(CheckSkippedDefinition{
         index, HandleFunctionDefinitionSuspend(context, node_id)});
+    CARBON_VLOG() << VlogPrefix << "Push CheckSkippedDefinition " << index.index
+                  << "\n";
   }
 
   // Push a task to re-enter a function scope, so that functions defined within
   // it are type-checked in the right context.
   auto PushEnterDeferredDefinitionScope(Context& context) -> void {
-    enclosing_scopes_.push_back(worklist_.size());
-    worklist_.push_back(EnterDeferredDefinitionScope{
-        std::nullopt, IsInDeferredDefinitionScope(context)});
+    bool nested = !enclosing_scopes_.empty() &&
+                  enclosing_scopes_.back().scope_index ==
+                      context.decl_name_stack().PeekEnclosingScope();
+    enclosing_scopes_.push_back(
+        {.worklist_start_index = worklist_.size(),
+         .scope_index = context.scope_stack().PeekIndex()});
+    worklist_.push_back(EnterDeferredDefinitionScope{std::nullopt, nested});
+    CARBON_VLOG() << VlogPrefix << "Push EnterDeferredDefinitionScope "
+                  << (nested ? "(nested)" : "(non-nested)") << "\n";
   }
 
   // Suspend the current deferred definition scope, which is finished but still
@@ -438,7 +430,29 @@ class DeferredDefinitionWorklist {
   auto SuspendFinishedScopeAndPush(Context& context) -> bool;
 
   // Pop the next task off the worklist.
-  auto Pop() -> Task { return worklist_.pop_back_val(); }
+  auto Pop() -> Task {
+    if (vlog_stream_) {
+      VariantMatch(
+          worklist_.back(),
+          [&](CheckSkippedDefinition& definition) {
+            CARBON_VLOG() << VlogPrefix << "Handle CheckSkippedDefinition "
+                          << definition.definition_index.index << "\n";
+          },
+          [&](EnterDeferredDefinitionScope& enter) {
+            CARBON_CHECK(enter.in_deferred_definition_scope);
+            CARBON_VLOG() << VlogPrefix
+                          << "Handle EnterDeferredDefinitionScope (nested)\n";
+          },
+          [&](LeaveDeferredDefinitionScope& leave) {
+            bool nested = leave.in_deferred_definition_scope;
+            CARBON_VLOG() << VlogPrefix
+                          << "Handle LeaveDeferredDefinitionScope "
+                          << (nested ? "(nested)" : "(non-nested)") << "\n";
+          });
+    }
+
+    return worklist_.pop_back_val();
+  }
 
   // CHECK that the work list has no further work.
   auto VerifyEmpty() {
@@ -447,6 +461,8 @@ class DeferredDefinitionWorklist {
   }
 
  private:
+  llvm::raw_ostream* vlog_stream_;
+
   // A worklist of type-checking tasks we'll need to do later.
   //
   // Don't allocate any inline storage here. A Task is fairly large, so we never
@@ -454,27 +470,35 @@ class DeferredDefinitionWorklist {
   // constructor for a fairly large number of deferred definitions.
   llvm::SmallVector<Task, 0> worklist_;
 
-  // Indexes in `worklist` of deferred definition scopes that are currently
-  // still open.
-  llvm::SmallVector<size_t> enclosing_scopes_;
+  // A deferred definition scope that is currently still open.
+  struct EnclosingScope {
+    // The index in worklist_ of the EnterDeferredDefinitionScope task.
+    size_t worklist_start_index;
+    // The corresponding lexical scope index.
+    ScopeIndex scope_index;
+  };
+
+  // The deferred definition scopes enclosing the current checking actions.
+  llvm::SmallVector<EnclosingScope> enclosing_scopes_;
 };
 }  // namespace
 
 auto DeferredDefinitionWorklist::SuspendFinishedScopeAndPush(Context& context)
     -> bool {
-  auto scope_index = enclosing_scopes_.pop_back_val();
+  auto start_index = enclosing_scopes_.pop_back_val().worklist_start_index;
 
   // If we've not found any deferred definitions in this scope, clean up the
   // stack.
-  if (scope_index == worklist_.size() - 1) {
+  if (start_index == worklist_.size() - 1) {
     context.decl_name_stack().PopScope();
     worklist_.pop_back();
+    CARBON_VLOG() << VlogPrefix << "Pop EnterDeferredDefinitionScope (empty)\n";
     return false;
   }
 
   // If we're finishing a nested deferred definition scope, keep track of that
   // but don't type-check deferred definitions now.
-  auto& enter_scope = get<EnterDeferredDefinitionScope>(worklist_[scope_index]);
+  auto& enter_scope = get<EnterDeferredDefinitionScope>(worklist_[start_index]);
   if (enter_scope.in_deferred_definition_scope) {
     // This is a nested deferred definition scope. Suspend the inner scope so we
     // can restore it when we come to type-check the deferred definitions.
@@ -483,6 +507,8 @@ auto DeferredDefinitionWorklist::SuspendFinishedScopeAndPush(Context& context)
     // Enqueue a task to leave the nested scope.
     worklist_.push_back(
         LeaveDeferredDefinitionScope{.in_deferred_definition_scope = true});
+    CARBON_VLOG() << VlogPrefix
+                  << "Push LeaveDeferredDefinitionScope (nested)\n";
     return false;
   }
 
@@ -491,11 +517,13 @@ auto DeferredDefinitionWorklist::SuspendFinishedScopeAndPush(Context& context)
   // scope and end checking deferred definitions.
   worklist_.push_back(
       LeaveDeferredDefinitionScope{.in_deferred_definition_scope = false});
+  CARBON_VLOG() << VlogPrefix
+                << "Push LeaveDeferredDefinitionScope (non-nested)\n";
 
   // We'll process the worklist in reverse index order, so reverse the part of
   // it we're about to execute so we run our tasks in the order in which they
   // were pushed.
-  std::reverse(worklist_.begin() + scope_index, worklist_.end());
+  std::reverse(worklist_.begin() + start_index, worklist_.end());
 
   // Pop the `EnterDeferredDefinitionScope` that's now on the end of the
   // worklist. We stay in that scope rather than suspending then immediately
@@ -504,6 +532,8 @@ auto DeferredDefinitionWorklist::SuspendFinishedScopeAndPush(Context& context)
       holds_alternative<EnterDeferredDefinitionScope>(worklist_.back()))
       << "Unexpected task in worklist.";
   worklist_.pop_back();
+  CARBON_VLOG() << VlogPrefix
+                << "Handle EnterDeferredDefinitionScope (non-nested)\n";
   return true;
 }
 
@@ -512,8 +542,10 @@ namespace {
 // to check them.
 class NodeIdTraversal {
  public:
-  explicit NodeIdTraversal(Context& context)
-      : context_(context), next_deferred_definition_(&context.parse_tree()) {
+  explicit NodeIdTraversal(Context& context, llvm::raw_ostream* vlog_stream)
+      : context_(context),
+        next_deferred_definition_(&context.parse_tree()),
+        worklist_(vlog_stream) {
     chunks_.push_back(
         {.it = context.parse_tree().postorder().begin(),
          .end = context.parse_tree().postorder().end(),
@@ -656,10 +688,10 @@ auto NodeIdTraversal::Next() -> std::optional<Parse::NodeId> {
 // Loops over all nodes in the tree. On some errors, this may return early,
 // for example if an unrecoverable state is encountered.
 // NOLINTNEXTLINE(readability-function-size)
-static auto ProcessNodeIds(Context& context,
+static auto ProcessNodeIds(Context& context, llvm::raw_ostream* vlog_stream,
                            ErrorTrackingDiagnosticConsumer& err_tracker)
     -> bool {
-  NodeIdTraversal traversal(context);
+  NodeIdTraversal traversal(context, vlog_stream);
 
   while (auto maybe_node_id = traversal.Next()) {
     auto node_id = *maybe_node_id;
@@ -709,7 +741,7 @@ static auto CheckParseTree(
 
   InitPackageScopeAndImports(context, unit_info);
 
-  if (!ProcessNodeIds(context, unit_info.err_tracker)) {
+  if (!ProcessNodeIds(context, vlog_stream, unit_info.err_tracker)) {
     context.sem_ir().set_has_errors(true);
     return;
   }

toolchain/check/decl_name_stack.h

@@ -161,11 +161,18 @@ class DeclNameStack {
 
   // Peeks the current target scope of the name on top of the stack. Note that
   // if we're still processing the name qualifiers, this can change before the
-  // name is completed.
-  auto PeekTargetScope() -> SemIR::NameScopeId {
+  // name is completed. Also, if the name up to this point was already declared
+  // and is a scope, this will be that scope, rather than the scope enclosing
+  // it.
+  auto PeekTargetScope() const -> SemIR::NameScopeId {
     return decl_name_stack_.back().target_scope_id;
   }
 
+  // Peeks the enclosing scope index of the name on top of the stack.
+  auto PeekEnclosingScope() const -> ScopeIndex {
+    return decl_name_stack_.back().enclosing_scope;
+  }
+
   // Finishes the current declaration name processing, returning the final
   // context for adding the name to lookup.
   //

toolchain/check/testdata/class/fail_method_redefinition.carbon

@@ -0,0 +1,48 @@
+// 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
+//
+// AUTOUPDATE
+
+class Class {
+  fn F() {}
+  // CHECK:STDERR: fail_method_redefinition.carbon:[[@LINE+6]]:3: ERROR: Redefinition of function F.
+  // CHECK:STDERR:   fn F() {}
+  // CHECK:STDERR:   ^~~~~~~~
+  // CHECK:STDERR: fail_method_redefinition.carbon:[[@LINE-4]]:3: Previously defined here.
+  // CHECK:STDERR:   fn F() {}
+  // CHECK:STDERR:   ^~~~~~~~
+  fn F() {}
+}
+
+// CHECK:STDOUT: --- fail_method_redefinition.carbon
+// CHECK:STDOUT:
+// CHECK:STDOUT: constants {
+// CHECK:STDOUT:   %Class: type = class_type @Class [template]
+// CHECK:STDOUT:   %.1: type = struct_type {} [template]
+// CHECK:STDOUT: }
+// CHECK:STDOUT:
+// CHECK:STDOUT: file {
+// CHECK:STDOUT:   package: <namespace> = namespace [template] {
+// CHECK:STDOUT:     .Class = %Class.decl
+// CHECK:STDOUT:   }
+// CHECK:STDOUT:   %Class.decl: type = class_decl @Class [template = constants.%Class] {}
+// CHECK:STDOUT: }
+// CHECK:STDOUT:
+// CHECK:STDOUT: class @Class {
+// CHECK:STDOUT:   %F.loc8: <function> = fn_decl @F [template] {}
+// CHECK:STDOUT:   %F.loc15: <function> = fn_decl @F [template] {}
+// CHECK:STDOUT:
+// CHECK:STDOUT: !members:
+// CHECK:STDOUT:   .Self = constants.%Class
+// CHECK:STDOUT:   .F = %F.loc8
+// CHECK:STDOUT: }
+// CHECK:STDOUT:
+// CHECK:STDOUT: fn @F() {
+// CHECK:STDOUT: !entry:
+// CHECK:STDOUT:   return
+// CHECK:STDOUT:
+// CHECK:STDOUT: !.loc15:
+// CHECK:STDOUT:   return
+// CHECK:STDOUT: }
+// CHECK:STDOUT:

toolchain/check/testdata/class/fail_redefinition.carbon

@@ -7,29 +7,46 @@
 class Class {
   fn F();
   fn H();
+  fn I() {}
 }
 
 // CHECK:STDERR: fail_redefinition.carbon:[[@LINE+7]]:1: ERROR: Redefinition of class Class.
 // CHECK:STDERR: class Class {
 // CHECK:STDERR: ^~~~~~~~~~~~~
-// CHECK:STDERR: fail_redefinition.carbon:[[@LINE-8]]:1: Previous definition was here.
+// CHECK:STDERR: fail_redefinition.carbon:[[@LINE-9]]:1: Previous definition was here.
 // CHECK:STDERR: class Class {
 // CHECK:STDERR: ^~~~~~~~~~~~~
 // CHECK:STDERR:
 class Class {
   fn G();
-  // CHECK:STDERR: fail_redefinition.carbon:[[@LINE+6]]:3: ERROR: Redundant redeclaration of function H.
+  // CHECK:STDERR: fail_redefinition.carbon:[[@LINE+7]]:3: ERROR: Redundant redeclaration of function H.
   // CHECK:STDERR:   fn H();
   // CHECK:STDERR:   ^~~~~~~
-  // CHECK:STDERR: fail_redefinition.carbon:[[@LINE-15]]:3: Previously declared here.
+  // CHECK:STDERR: fail_redefinition.carbon:[[@LINE-16]]:3: Previously declared here.
   // CHECK:STDERR:   fn H();
   // CHECK:STDERR:   ^~~~~~~
+  // CHECK:STDERR:
   fn H();
+  // CHECK:STDERR: fail_redefinition.carbon:[[@LINE+7]]:3: ERROR: Redefinition of function I.
+  // CHECK:STDERR:   fn I() {}
+  // CHECK:STDERR:   ^~~~~~~~
+  // CHECK:STDERR: fail_redefinition.carbon:[[@LINE-23]]:3: Previously defined here.
+  // CHECK:STDERR:   fn I() {}
+  // CHECK:STDERR:   ^~~~~~~~
+  // CHECK:STDERR:
+  fn I() {}
 }
 
 fn Class.F() {}
 fn Class.G() {}
 fn Class.H() {}
+// CHECK:STDERR: fail_redefinition.carbon:[[@LINE+6]]:1: ERROR: Redefinition of function I.
+// CHECK:STDERR: fn Class.I() {}
+// CHECK:STDERR: ^~~~~~~~~~~~~~
+// CHECK:STDERR: fail_redefinition.carbon:[[@LINE-9]]:3: Previously defined here.
+// CHECK:STDERR:   fn I() {}
+// CHECK:STDERR:   ^~~~~~~~
+fn Class.I() {}
 
 // CHECK:STDOUT: --- fail_redefinition.carbon
 // CHECK:STDOUT:
@@ -43,20 +60,23 @@ fn Class.H() {}
 // CHECK:STDOUT:     .Class = %Class.decl.loc7
 // CHECK:STDOUT:   }
 // CHECK:STDOUT:   %Class.decl.loc7: type = class_decl @Class [template = constants.%Class] {}
-// CHECK:STDOUT:   %Class.decl.loc19: type = class_decl @Class [template = constants.%Class] {}
+// CHECK:STDOUT:   %Class.decl.loc20: type = class_decl @Class [template = constants.%Class] {}
 // CHECK:STDOUT:   %F: <function> = fn_decl @F [template] {}
 // CHECK:STDOUT:   %G: <function> = fn_decl @G [template] {}
 // CHECK:STDOUT:   %H: <function> = fn_decl @H [template] {}
+// CHECK:STDOUT:   %I: <function> = fn_decl @I [template] {}
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
 // CHECK:STDOUT: class @Class {
 // CHECK:STDOUT:   %G: <function> = fn_decl @G [template] {}
 // CHECK:STDOUT:   %H: <function> = fn_decl @H [template] {}
+// CHECK:STDOUT:   %I: <function> = fn_decl @I [template] {}
 // CHECK:STDOUT:
 // CHECK:STDOUT: !members:
 // CHECK:STDOUT:   .Self = constants.%Class
 // CHECK:STDOUT:   .F = <unexpected instref inst+3>
 // CHECK:STDOUT:   .H = <unexpected instref inst+4>
+// CHECK:STDOUT:   .I = <unexpected instref inst+5>
 // CHECK:STDOUT:   .G = %G
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
@@ -70,6 +90,17 @@ fn Class.H() {}
 // CHECK:STDOUT:   return
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
+// CHECK:STDOUT: fn @I() {
+// CHECK:STDOUT: !entry:
+// CHECK:STDOUT:   return
+// CHECK:STDOUT:
+// CHECK:STDOUT: !.loc37:
+// CHECK:STDOUT:   return
+// CHECK:STDOUT:
+// CHECK:STDOUT: !.loc49:
+// CHECK:STDOUT:   return
+// CHECK:STDOUT: }
+// CHECK:STDOUT:
 // CHECK:STDOUT: fn @G() {
 // CHECK:STDOUT: !entry:
 // CHECK:STDOUT:   return