Refactor Impl construction to clarify and reduce extraneous work

toolchain/check/BUILD

@@ -56,6 +56,7 @@ cc_library(
         "name_component.cpp",
         "name_lookup.cpp",
         "name_ref.cpp",
+        "name_scope.cpp",
         "operator.cpp",
         "pattern.cpp",
         "pattern_match.cpp",
@@ -111,6 +112,7 @@ cc_library(
         "name_component.h",
         "name_lookup.h",
         "name_ref.h",
+        "name_scope.h",
         "operator.h",
         "param_and_arg_refs_stack.h",
         "pattern.h",

toolchain/check/eval.cpp

@@ -867,6 +867,14 @@ static auto ResolveSpecificDeclForInst(EvalContext& eval_context,
         for (const auto& interface : info.self_impls_constraints) {
           ResolveSpecificDeclForSpecificId(eval_context, interface.specific_id);
         }
+        for (const auto& constraint : info.extend_named_constraints) {
+          ResolveSpecificDeclForSpecificId(eval_context,
+                                           constraint.specific_id);
+        }
+        for (const auto& constraint : info.self_impls_named_constraints) {
+          ResolveSpecificDeclForSpecificId(eval_context,
+                                           constraint.specific_id);
+        }
         break;
       }
       case CARBON_KIND(SemIR::SpecificId specific_id): {

toolchain/check/facet_type.cpp

@@ -53,22 +53,13 @@ static auto WitnessQueryMatchesInterface(
 }
 
 static auto IncompleteFacetTypeDiagnosticBuilder(
-    Context& context, SemIR::LocId loc_id, SemIR::TypeInstId facet_type_inst_id,
-    bool is_definition) -> DiagnosticBuilder {
-  if (is_definition) {
-    CARBON_DIAGNOSTIC(ImplAsIncompleteFacetTypeDefinition, Error,
-                      "definition of impl as incomplete facet type {0}",
-                      InstIdAsType);
-    return context.emitter().Build(loc_id, ImplAsIncompleteFacetTypeDefinition,
-                                   facet_type_inst_id);
-  } else {
-    CARBON_DIAGNOSTIC(
-        ImplAsIncompleteFacetTypeRewrites, Error,
-        "declaration of impl as incomplete facet type {0} with rewrites",
-        InstIdAsType);
-    return context.emitter().Build(loc_id, ImplAsIncompleteFacetTypeRewrites,
-                                   facet_type_inst_id);
-  }
+    Context& context, SemIR::LocId loc_id, SemIR::TypeInstId facet_type_inst_id)
+    -> DiagnosticBuilder {
+  CARBON_DIAGNOSTIC(ImplAsIncompleteFacetTypeDefinition, Error,
+                    "definition of impl as incomplete facet type {0}",
+                    InstIdAsType);
+  return context.emitter().Build(loc_id, ImplAsIncompleteFacetTypeDefinition,
+                                 facet_type_inst_id);
 }
 
 auto GetImplWitnessAccessWithoutSubstitution(Context& context,
@@ -93,7 +84,18 @@ auto InitialFacetTypeImplWitness(
   const auto& facet_type_info =
       context.facet_types().Get(facet_type.facet_type_id);
 
-  if (!is_definition && facet_type_info.rewrite_constraints.empty()) {
+  auto rewrites_into_interface_to_witness = llvm::make_filter_range(
+      facet_type_info.rewrite_constraints,
+      [&](const SemIR::FacetTypeInfo::RewriteConstraint& rewrite) {
+        auto access = context.insts().GetAs<SemIR::ImplWitnessAccess>(
+            GetImplWitnessAccessWithoutSubstitution(context, rewrite.lhs_id));
+        return WitnessQueryMatchesInterface(context, access.witness_id,
+                                            interface_to_witness);
+      });
+
+  bool need_witness_table =
+      is_definition || !rewrites_into_interface_to_witness.empty();
+  if (!need_witness_table) {
     auto witness_table_inst_id = AddInst<SemIR::ImplWitnessTable>(
         context, witness_loc_id,
         {.elements_id = context.inst_blocks().AddPlaceholder(),
@@ -105,16 +107,15 @@ auto InitialFacetTypeImplWitness(
          .specific_id = self_specific_id});
   }
 
-  if (!RequireCompleteType(
-          context, facet_type_id, SemIR::LocId(facet_type_inst_id), [&] {
-            return IncompleteFacetTypeDiagnosticBuilder(
-                context, witness_loc_id, facet_type_inst_id, is_definition);
-          })) {
+  const auto& interface =
+      context.interfaces().Get(interface_to_witness.interface_id);
+  if (!interface.is_complete()) {
+    // This is a declaration with rewrite constraints into `.Self`, but the
+    // interface is not complete. Those rewrites would have been diagnosed as an
+    // error in their member access.
     return SemIR::ErrorInst::InstId;
   }
 
-  const auto& interface =
-      context.interfaces().Get(interface_to_witness.interface_id);
   auto assoc_entities =
       context.inst_blocks().Get(interface.associated_entities_id);
   // TODO: When this function is used for things other than just impls, may want
@@ -145,13 +146,9 @@ auto InitialFacetTypeImplWitness(
          .specific_id = self_specific_id});
   }
 
-  for (auto rewrite : facet_type_info.rewrite_constraints) {
+  for (auto rewrite : rewrites_into_interface_to_witness) {
     auto access = context.insts().GetAs<SemIR::ImplWitnessAccess>(
         GetImplWitnessAccessWithoutSubstitution(context, rewrite.lhs_id));
-    if (!WitnessQueryMatchesInterface(context, access.witness_id,
-                                      interface_to_witness)) {
-      continue;
-    }
     auto& table_entry = table[access.index.index];
     if (table_entry == SemIR::ErrorInst::InstId) {
       // Don't overwrite an error value. This prioritizes not generating
@@ -250,12 +247,11 @@ auto RequireCompleteFacetTypeForImplDefinition(
     -> bool {
   auto facet_type_id =
       context.types().GetTypeIdForTypeInstId(facet_type_inst_id);
-  return RequireCompleteType(
-      context, facet_type_id, SemIR::LocId(facet_type_inst_id), [&] {
-        return IncompleteFacetTypeDiagnosticBuilder(context, loc_id,
-                                                    facet_type_inst_id,
-                                                    /*is_definition=*/true);
-      });
+  return RequireCompleteType(context, facet_type_id,
+                             SemIR::LocId(facet_type_inst_id), [&] {
+                               return IncompleteFacetTypeDiagnosticBuilder(
+                                   context, loc_id, facet_type_inst_id);
+                             });
 }
 
 auto AllocateFacetTypeImplWitness(Context& context,

toolchain/check/handle_impl.cpp

@@ -14,16 +14,28 @@
 #include "toolchain/check/inst.h"
 #include "toolchain/check/modifiers.h"
 #include "toolchain/check/name_lookup.h"
+#include "toolchain/check/name_scope.h"
 #include "toolchain/check/pattern_match.h"
 #include "toolchain/check/type.h"
 #include "toolchain/check/type_completion.h"
+#include "toolchain/parse/node_ids.h"
 #include "toolchain/parse/typed_nodes.h"
 #include "toolchain/sem_ir/generic.h"
 #include "toolchain/sem_ir/ids.h"
+#include "toolchain/sem_ir/specific_interface.h"
 #include "toolchain/sem_ir/typed_insts.h"
 
 namespace Carbon::Check {
 
+// Returns the implicit `Self` type for an `impl` when it's in a `class`
+// declaration.
+static auto GetImplDefaultSelfType(Context& context,
+                                   const ClassScope& class_scope)
+    -> SemIR::TypeId {
+  // TODO: This is also valid in a mixin.
+  return context.classes().Get(class_scope.class_decl.class_id).self_type_id;
+}
+
 auto HandleParseNode(Context& context, Parse::ImplIntroducerId node_id)
     -> bool {
   // This might be a generic impl.
@@ -56,23 +68,57 @@ auto HandleParseNode(Context& context, Parse::ForallId /*node_id*/) -> bool {
 
 auto HandleParseNode(Context& context, Parse::ImplTypeAsId node_id) -> bool {
   auto [self_node, self_id] = context.node_stack().PopExprWithNodeId();
-  auto self_type_inst_id = ExprAsType(context, self_node, self_id).inst_id;
-  context.node_stack().Push(node_id, self_type_inst_id);
+  auto self_type = ExprAsType(context, self_node, self_id);
+
+  const auto& introducer = context.decl_introducer_state_stack().innermost();
+  if (introducer.modifier_set.HasAnyOf(KeywordModifierSet::Extend)) {
+    auto class_scope =
+        TryAsClassScope(context, context.decl_name_stack().PeekParentScopeId());
+    if (class_scope) {
+      // If we're not inside a class at all, that will be diagnosed against the
+      // `extend` elsewhere.
+      auto extend_node = introducer.modifier_node_id(ModifierOrder::Extend);
+      CARBON_DIAGNOSTIC(ExtendImplSelfAs, Error,
+                        "cannot `extend` an `impl` with an explicit self type");
+      auto diag = context.emitter().Build(extend_node, ExtendImplSelfAs);
+
+      if (self_type.type_id != GetImplDefaultSelfType(context, *class_scope)) {
+        // If the explicit self type is not the default, the self-type is an
+        // error.
+        if (self_type.type_id != SemIR::ErrorInst::TypeId) {
+          diag.Emit();
+        }
+        self_type.inst_id = SemIR::ErrorInst::TypeInstId;
+      } else {
+        // Otherwise, suggest removing the self-type, and continue as if there
+        // was not error.
+        CARBON_DIAGNOSTIC(ExtendImplSelfAsDefault, Note,
+                          "remove the explicit `Self` type here");
+        diag.Note(self_node, ExtendImplSelfAsDefault);
+        if (self_type.type_id != SemIR::ErrorInst::TypeId) {
+          diag.Emit();
+        }
+      }
+    }
+  }
 
   // Introduce `Self`. Note that we add this name lexically rather than adding
   // to the `NameScopeId` of the `impl`, because this happens before we enter
   // the `impl` scope or even identify which `impl` we're declaring.
   // TODO: Revisit this once #3714 is resolved.
-  AddNameToLookup(context, SemIR::NameId::SelfType, self_type_inst_id);
+  AddNameToLookup(context, SemIR::NameId::SelfType, self_type.inst_id);
+  context.node_stack().Push(node_id, self_type.inst_id);
   return true;
 }
 
 auto HandleParseNode(Context& context, Parse::ImplDefaultSelfAsId node_id)
     -> bool {
   auto self_inst_id = SemIR::TypeInstId::None;
 
-  if (auto self_type_id = GetImplDefaultSelfType(context);
-      self_type_id.has_value()) {
+  auto class_scope =
+      TryAsClassScope(context, context.decl_name_stack().PeekParentScopeId());
+  if (class_scope) {
+    auto self_type_id = GetImplDefaultSelfType(context, *class_scope);
     // Build the implicit access to the enclosing `Self`.
     // TODO: Consider calling `HandleNameAsExpr` to build this implicit `Self`
     // expression. We've already done the work to check that the enclosing
@@ -151,7 +197,7 @@ static auto PopImplIntroducerAndParamsAsNameComponent(
 // common logic shared by impl forward declarations and impl definitions.
 static auto BuildImplDecl(Context& context, Parse::AnyImplDeclId node_id,
                           bool is_definition)
-    -> std::pair<SemIR::ImplId, SemIR::InstId> {
+    -> std::tuple<SemIR::ImplId, SemIR::InstId> {
   auto [constraint_node, constraint_id] =
       context.node_stack().PopExprWithNodeId();
   auto [self_type_node, self_type_inst_id] =
@@ -185,54 +231,71 @@ static auto BuildImplDecl(Context& context, Parse::AnyImplDeclId node_id,
                          SemIR::ImplDecl{.impl_id = SemIR::ImplId::None,
                                          .decl_block_id = decl_block_id});
 
-  SemIR::Impl impl_info = {name_context.MakeEntityWithParamsBase(
-                               name, impl_decl_id,
-                               /*is_extern=*/false, SemIR::LibraryNameId::None),
-                           {.self_id = self_type_inst_id,
-                            .constraint_id = constraint_type_inst_id,
-                            // This requires that the facet type is identified.
-                            .interface = CheckConstraintIsInterface(
-                                context, impl_decl_id, constraint_type_inst_id),
-                            .is_final = is_final}};
-
-  std::optional<ExtendImplDecl> extend_impl;
-  if (introducer.modifier_set.HasAnyOf(KeywordModifierSet::Extend)) {
-    extend_impl = ExtendImplDecl{
-        .self_type_node_id = self_type_node,
-        .constraint_type_id = constraint_type_id,
-        .extend_node_id = introducer.modifier_node_id(ModifierOrder::Extend),
-    };
+  // This requires that the facet type is identified. It returns None if an
+  // error was diagnosed.
+  auto specific_interface = CheckConstraintIsInterface(context, impl_decl_id,
+                                                       constraint_type_inst_id);
+
+  auto impl_id = SemIR::ImplId::None;
+  {
+    SemIR::Impl impl_info = {
+        name_context.MakeEntityWithParamsBase(name, impl_decl_id,
+                                              /*is_extern=*/false,
+                                              SemIR::LibraryNameId::None),
+        {.self_id = self_type_inst_id,
+         .constraint_id = constraint_type_inst_id,
+         .interface = specific_interface,
+         .is_final = is_final}};
+    auto extend_node = introducer.modifier_node_id(ModifierOrder::Extend);
+    impl_id = GetOrAddImpl(context, node_id, name.implicit_params_loc_id,
+                           impl_info, is_definition, extend_node);
   }
 
-  return StartImplDecl(context, SemIR::LocId(node_id),
-                       name.implicit_params_loc_id, impl_info, is_definition,
-                       extend_impl);
+  // StartImplDecl either filled in the `impl_info` and returned a fresh ImplId,
+  // or if we're redeclaring a previous impl, returned an existing ImplId. Write
+  // that ImplId into the ImplDecl instruction and finish it.
+  auto impl_decl = context.insts().GetAs<SemIR::ImplDecl>(impl_decl_id);
+  impl_decl.impl_id = impl_id;
+  ReplaceInstBeforeConstantUse(context, impl_decl_id, impl_decl);
+
+  return {impl_id, impl_decl_id};
 }
 
 auto HandleParseNode(Context& context, Parse::ImplDeclId node_id) -> bool {
-  BuildImplDecl(context, node_id, /*is_definition=*/false);
+  auto [impl_id, impl_decl_id] =
+      BuildImplDecl(context, node_id, /*is_definition=*/false);
+  auto& impl = context.impls().Get(impl_id);
+
   context.decl_name_stack().PopScope();
+
+  // Impl definitions are required in the same file as the declaration. We skip
+  // this requirement if we've already issued an invalid redeclaration error, or
+  // there is an error that would prevent the impl from being legal to define.
+  if (impl.witness_id != SemIR::ErrorInst::InstId) {
+    context.definitions_required_by_decl().push_back(impl_decl_id);
+  }
+
   return true;
 }
 
 auto HandleParseNode(Context& context, Parse::ImplDefinitionStartId node_id)
     -> bool {
   auto [impl_id, impl_decl_id] =
       BuildImplDecl(context, node_id, /*is_definition=*/true);
-  auto& impl_info = context.impls().Get(impl_id);
+  auto& impl = context.impls().Get(impl_id);
 
-  CARBON_CHECK(!impl_info.has_definition_started());
-  impl_info.definition_id = impl_decl_id;
-  impl_info.scope_id =
+  CARBON_CHECK(!impl.has_definition_started());
+  impl.definition_id = impl_decl_id;
+  impl.scope_id =
       context.name_scopes().Add(impl_decl_id, SemIR::NameId::None,
                                 context.decl_name_stack().PeekParentScopeId());
 
   context.scope_stack().PushForEntity(
-      impl_decl_id, impl_info.scope_id,
-      context.generics().GetSelfSpecific(impl_info.generic_id));
-  StartGenericDefinition(context, impl_info.generic_id);
+      impl_decl_id, impl.scope_id,
+      context.generics().GetSelfSpecific(impl.generic_id));
+  StartGenericDefinition(context, impl.generic_id);
   // This requires that the facet type is complete.
-  ImplWitnessStartDefinition(context, impl_info);
+  ImplWitnessStartDefinition(context, impl);
   context.inst_block_stack().Push();
   context.node_stack().Push(node_id, impl_id);
 
@@ -245,7 +308,7 @@ auto HandleParseNode(Context& context, Parse::ImplDefinitionStartId node_id)
   //
   // We may need to track a list of instruction blocks here, as we do for a
   // function.
-  impl_info.body_block_id = context.inst_block_stack().PeekOrAdd();
+  impl.body_block_id = context.inst_block_stack().PeekOrAdd();
   return true;
 }