Refactor name addition, particularly for associated entities. (#3924)

This is so that the constant associated with a function is used after
the function declaration is complete, related to changing how function
constants work.

toolchain/check/decl_name_stack.cpp

@@ -10,6 +10,29 @@
 
 namespace Carbon::Check {
 
+auto DeclNameStack::NameContext::prev_inst_id() -> SemIR::InstId {
+  switch (state) {
+    case NameContext::State::Error:
+      // The name is invalid and a diagnostic has already been emitted.
+      return SemIR::InstId::Invalid;
+
+    case NameContext::State::Empty:
+      CARBON_FATAL()
+          << "Name is missing, not expected to call existing_inst_id (but "
+             "that may change based on error handling).";
+
+    case NameContext::State::Resolved:
+    case NameContext::State::ResolvedNonScope:
+      return resolved_inst_id;
+
+    case NameContext::State::Unresolved:
+      return SemIR::InstId::Invalid;
+
+    case NameContext::State::Finished:
+      CARBON_FATAL() << "Finished state should only be used internally";
+  }
+}
+
 auto DeclNameStack::MakeEmptyNameContext() -> NameContext {
   return NameContext{
       .enclosing_scope = context_->scope_stack().PeekIndex(),
@@ -94,20 +117,11 @@ auto DeclNameStack::Restore(SuspendedName sus) -> void {
   }
 }
 
-auto DeclNameStack::LookupOrAddName(NameContext name_context,
-                                    SemIR::InstId target_id) -> SemIR::InstId {
+auto DeclNameStack::AddName(NameContext name_context, SemIR::InstId target_id)
+    -> void {
   switch (name_context.state) {
     case NameContext::State::Error:
-      // The name is invalid and a diagnostic has already been emitted.
-      return SemIR::InstId::Invalid;
-
-    case NameContext::State::Empty:
-      CARBON_FATAL() << "Name is missing, not expected to call AddNameToLookup "
-                        "(but that may change based on error handling).";
-
-    case NameContext::State::Resolved:
-    case NameContext::State::ResolvedNonScope:
-      return name_context.resolved_inst_id;
+      return;
 
     case NameContext::State::Unresolved:
       if (!name_context.target_scope_id.is_valid()) {
@@ -141,21 +155,33 @@ auto DeclNameStack::LookupOrAddName(NameContext name_context,
             << name_context.unresolved_name_id << " in "
             << name_context.target_scope_id;
       }
-      return SemIR::InstId::Invalid;
+      break;
 
-    case NameContext::State::Finished:
-      CARBON_FATAL() << "Finished state should only be used internally";
+    default:
+      CARBON_FATAL() << "Should not be calling AddName";
+      break;
   }
 }
 
-auto DeclNameStack::AddNameToLookup(NameContext name_context,
-                                    SemIR::InstId target_id) -> void {
-  auto existing_inst_id = LookupOrAddName(name_context, target_id);
-  if (existing_inst_id.is_valid()) {
-    context_->DiagnoseDuplicateName(target_id, existing_inst_id);
+auto DeclNameStack::AddNameOrDiagnoseDuplicate(NameContext name_context,
+                                               SemIR::InstId target_id)
+    -> void {
+  if (auto id = name_context.prev_inst_id(); id.is_valid()) {
+    context_->DiagnoseDuplicateName(target_id, id);
+  } else {
+    AddName(name_context, target_id);
   }
 }
 
+auto DeclNameStack::LookupOrAddName(NameContext name_context,
+                                    SemIR::InstId target_id) -> SemIR::InstId {
+  if (auto id = name_context.prev_inst_id(); id.is_valid()) {
+    return id;
+  }
+  AddName(name_context, target_id);
+  return SemIR::InstId::Invalid;
+}
+
 auto DeclNameStack::ApplyNameQualifier(SemIR::LocId loc_id,
                                        SemIR::NameId name_id) -> void {
   ApplyNameQualifierTo(decl_name_stack_.back(), loc_id, name_id,

toolchain/check/decl_name_stack.h

@@ -91,15 +91,14 @@ class DeclNameStack {
       Error,
     };
 
-    // Returns whether the name resolved to an existing entity.
-    auto is_resolved() -> bool {
-      return state != State::Unresolved && state != State::Empty;
-    }
+    // Returns any name collision found, or Invalid.
+    auto prev_inst_id() -> SemIR::InstId;
 
     // Returns the name_id for a new instruction. This is invalid when the name
     // resolved.
     auto name_id_for_new_inst() -> SemIR::NameId {
-      return !is_resolved() ? unresolved_name_id : SemIR::NameId::Invalid;
+      return state == State::Unresolved ? unresolved_name_id
+                                        : SemIR::NameId::Invalid;
     }
 
     // Returns the enclosing_scope_id for a new instruction. This is invalid
@@ -107,7 +106,8 @@ class DeclNameStack {
     // the NameContext, which refers to the scope of the introducer rather than
     // the scope of the name.
     auto enclosing_scope_id_for_new_inst() -> SemIR::NameScopeId {
-      return !is_resolved() ? target_scope_id : SemIR::NameScopeId::Invalid;
+      return state == State::Unresolved ? target_scope_id
+                                        : SemIR::NameScopeId::Invalid;
     }
 
     // The current scope when this name began. This is the scope that we will
@@ -130,10 +130,10 @@ class DeclNameStack {
     union {
       // The ID of a resolved qualifier, including both identifiers and
       // expressions. Invalid indicates resolution failed.
-      SemIR::InstId resolved_inst_id = SemIR::InstId::Invalid;
+      SemIR::InstId resolved_inst_id;
 
       // The ID of an unresolved identifier.
-      SemIR::NameId unresolved_name_id;
+      SemIR::NameId unresolved_name_id = SemIR::NameId::Invalid;
     };
   };
 
@@ -214,9 +214,12 @@ class DeclNameStack {
   // describes an existing scope, such as a namespace or a defined class.
   auto ApplyNameQualifier(SemIR::LocId loc_id, SemIR::NameId name_id) -> void;
 
+  // Adds a name to name lookup. Assumes duplicates are already handled.
+  auto AddName(NameContext name_context, SemIR::InstId target_id) -> void;
+
   // Adds a name to name lookup. Prints a diagnostic for name conflicts.
-  auto AddNameToLookup(NameContext name_context, SemIR::InstId target_id)
-      -> void;
+  auto AddNameOrDiagnoseDuplicate(NameContext name_context,
+                                  SemIR::InstId target_id) -> void;
 
   // Adds a name to name lookup, or returns the existing instruction if this
   // name has already been declared in this scope.

toolchain/check/handle_alias.cpp

@@ -69,7 +69,7 @@ auto HandleAlias(Context& context, Parse::AliasId /*node_id*/) -> bool {
 
   // Add the name of the binding to the current scope.
   context.decl_name_stack().PopScope();
-  context.decl_name_stack().AddNameToLookup(name_context, alias_id);
+  context.decl_name_stack().AddNameOrDiagnoseDuplicate(name_context, alias_id);
   return true;
 }
 

toolchain/check/handle_class.cpp

@@ -435,7 +435,7 @@ auto HandleBaseDecl(Context& context, Parse::BaseDeclId node_id) -> bool {
        SemIR::StructTypeField{SemIR::NameId::Base, base_info.type_id}}));
 
   // Bind the name `base` in the class to the base field.
-  context.decl_name_stack().AddNameToLookup(
+  context.decl_name_stack().AddNameOrDiagnoseDuplicate(
       context.decl_name_stack().MakeUnqualifiedName(node_id,
                                                     SemIR::NameId::Base),
       class_info.base_id);

toolchain/check/handle_function.cpp

@@ -132,24 +132,8 @@ static auto BuildFunctionDecl(Context& context,
     function_info.definition_id = function_info.decl_id;
   }
 
-  // At interface scope, a function declaration introduces an associated
-  // function.
-  auto lookup_result_id = function_info.decl_id;
-  if (name_context.enclosing_scope_id_for_new_inst().is_valid() &&
-      !name_context.has_qualifiers) {
-    auto scope_inst_id = context.name_scopes().GetInstIdIfValid(
-        name_context.enclosing_scope_id_for_new_inst());
-    if (auto interface_scope =
-            context.insts().TryGetAsIfValid<SemIR::InterfaceDecl>(
-                scope_inst_id)) {
-      lookup_result_id = BuildAssociatedEntity(
-          context, interface_scope->interface_id, function_info.decl_id);
-    }
-  }
-
   // Check whether this is a redeclaration.
-  auto prev_id =
-      context.decl_name_stack().LookupOrAddName(name_context, lookup_result_id);
+  auto prev_id = name_context.prev_inst_id();
   if (prev_id.is_valid()) {
     auto prev_inst_for_merge =
         ResolvePrevInstForMerge(context, node_id, prev_id);
@@ -179,6 +163,27 @@ static auto BuildFunctionDecl(Context& context,
   // Write the function ID into the FunctionDecl.
   context.ReplaceInstBeforeConstantUse(function_info.decl_id, function_decl);
 
+  // Check if we need to add this to name lookup, now that the function decl is
+  // done.
+  if (!prev_id.is_valid()) {
+    // At interface scope, a function declaration introduces an associated
+    // function.
+    auto lookup_result_id = function_info.decl_id;
+    if (name_context.enclosing_scope_id_for_new_inst().is_valid() &&
+        !name_context.has_qualifiers) {
+      auto scope_inst_id = context.name_scopes().GetInstIdIfValid(
+          name_context.enclosing_scope_id_for_new_inst());
+      if (auto interface_scope =
+              context.insts().TryGetAsIfValid<SemIR::InterfaceDecl>(
+                  scope_inst_id)) {
+        lookup_result_id = BuildAssociatedEntity(
+            context, interface_scope->interface_id, function_info.decl_id);
+      }
+    }
+
+    context.decl_name_stack().AddName(name_context, lookup_result_id);
+  }
+
   if (SemIR::IsEntryPoint(context.sem_ir(), function_decl.function_id)) {
     // TODO: Update this once valid signatures for the entry point are decided.
     if (!context.inst_blocks().Get(implicit_param_refs_id).empty() ||

toolchain/check/handle_let.cpp

@@ -56,7 +56,7 @@ static auto BuildAssociatedConstantDecl(
   auto assoc_id = BuildAssociatedEntity(context, interface_id, decl_id);
   auto name_context =
       context.decl_name_stack().MakeUnqualifiedName(pattern.loc_id, name_id);
-  context.decl_name_stack().AddNameToLookup(name_context, assoc_id);
+  context.decl_name_stack().AddNameOrDiagnoseDuplicate(name_context, assoc_id);
 }
 
 auto HandleLetDecl(Context& context, Parse::LetDeclId node_id) -> bool {

toolchain/check/handle_variable.cpp

@@ -57,14 +57,16 @@ auto HandleVariableDecl(Context& context, Parse::VariableDeclId node_id)
     auto name_context = context.decl_name_stack().MakeUnqualifiedName(
         context.insts().GetLocId(value_id),
         context.bind_names().Get(bind_name->bind_name_id).name_id);
-    context.decl_name_stack().AddNameToLookup(name_context, value_id);
+    context.decl_name_stack().AddNameOrDiagnoseDuplicate(name_context,
+                                                         value_id);
     value_id = bind_name->value_id;
   } else if (auto field_decl =
                  context.insts().TryGetAs<SemIR::FieldDecl>(value_id)) {
     // Introduce the field name into the class.
     auto name_context = context.decl_name_stack().MakeUnqualifiedName(
         context.insts().GetLocId(value_id), field_decl->name_id);
-    context.decl_name_stack().AddNameToLookup(name_context, value_id);
+    context.decl_name_stack().AddNameOrDiagnoseDuplicate(name_context,
+                                                         value_id);
   }
   // TODO: Handle other kinds of pattern.