Refactor Impl construction to clarify and reduce extraneous work

toolchain/check/BUILD

@@ -57,6 +57,7 @@ cc_library(
         "name_component.cpp",
         "name_lookup.cpp",
         "name_ref.cpp",
+        "name_scope.cpp",
         "operator.cpp",
         "pattern.cpp",
         "pattern_match.cpp",
@@ -113,6 +114,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/facet_type.cpp

@@ -53,12 +53,8 @@ static auto WitnessQueryMatchesInterface(
 }
 
 static auto IncompleteFacetTypeDiagnosticBuilder(
-    Context& context, SemIR::LocId loc_id, SemIR::TypeInstId facet_type_inst_id,
-    bool is_definition) -> DiagnosticBuilder {
-  // TODO: Remove this parameter. Facet types don't need to be complete for impl
-  // declarations, unless there's a rewrite into `.Self`. But that completeness
-  // is checked/required by the member access of the rewrite.
-  CARBON_CHECK(is_definition);
+    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);
@@ -80,15 +76,29 @@ auto InitialFacetTypeImplWitness(
     Context& context, SemIR::LocId witness_loc_id,
     SemIR::TypeInstId facet_type_inst_id, SemIR::TypeInstId self_type_inst_id,
     const SemIR::SpecificInterface& interface_to_witness,
-    SemIR::SpecificId self_specific_id, bool is_definition) -> SemIR::InstId {
+    SemIR::SpecificId self_specific_id) -> SemIR::InstId {
   auto facet_type_id =
       context.types().GetTypeIdForTypeInstId(facet_type_inst_id);
   CARBON_CHECK(facet_type_id != SemIR::ErrorInst::TypeId);
   auto facet_type = context.types().GetAs<SemIR::FacetType>(facet_type_id);
   const auto& facet_type_info =
       context.facet_types().Get(facet_type.facet_type_id);
 
-  if (!is_definition && facet_type_info.rewrite_constraints.empty()) {
+  // An iterator over the rewrite_constraints where the LHS of the rewrite names
+  // a member of the `interface_to_witness`. This filters out rewrites of names
+  // from other interfaces, as they do not set values in the witness table.
+  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);
+      });
+
+  if (rewrites_into_interface_to_witness.empty()) {
+    // The witness table is not needed until the definition. Make a placeholder
+    // for the declaration.
     auto witness_table_inst_id = AddInst<SemIR::ImplWitnessTable>(
         context, witness_loc_id,
         {.elements_id = context.inst_blocks().AddPlaceholder(),
@@ -100,19 +110,15 @@ auto InitialFacetTypeImplWitness(
          .specific_id = self_specific_id});
   }
 
-  // The presence of any rewrite constraints requires that we know how many
-  // entries to allocate in the witness table, which requires the entire facet
-  // type to be complete, even if this was a declaration.
-  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 have already 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
@@ -143,13 +149,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
@@ -248,12 +250,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/facet_type.h

@@ -39,10 +39,10 @@ auto GetImplWitnessAccessWithoutSubstitution(Context& context,
                                              SemIR::InstId inst_id)
     -> SemIR::InstId;
 
-// Creates a impl witness instruction for a facet type. The facet type is
-// required to be complete if `is_definition` is true or the facet type has
-// rewrites. Otherwise a placeholder witness is created, and
-// `AllocateFacetTypeImplWitness` can be used at the `impl` definition.
+// Creates a impl witness instruction for a facet type. A placeholder witness
+// table is created if there are no rewrites in the declaration, and
+// `AllocateFacetTypeImplWitness` will construct the table of the appropriate
+// size at the `impl` definition.
 //
 // Adds and returns an `ImplWitness` instruction (created with location set to
 // `witness_loc_id`) that shows "`Self` type" of type "facet type" (the value of
@@ -59,14 +59,13 @@ auto GetImplWitnessAccessWithoutSubstitution(Context& context,
 // bind the inner `.Self` to the outer `.Self`.
 //
 // If the facet type contains a rewrite, we may have deferred converting the
-// rewritten value to the type of the associated constant. That conversion
-// will also be performed as part of resolution, and may depend on the
-// `Self` type.
+// rewritten value to the type of the associated constant. That conversion will
+// also be performed as part of resolution, and may depend on the `Self` type.
 auto InitialFacetTypeImplWitness(
     Context& context, SemIR::LocId witness_loc_id,
     SemIR::TypeInstId facet_type_inst_id, SemIR::TypeInstId self_type_inst_id,
     const SemIR::SpecificInterface& interface_to_witness,
-    SemIR::SpecificId self_specific_id, bool is_definition) -> SemIR::InstId;
+    SemIR::SpecificId self_specific_id) -> SemIR::InstId;
 
 // Returns `true` if the facet type is complete. Otherwise issues a diagnostic
 // and returns `false`.

toolchain/check/handle_impl.cpp

@@ -14,16 +14,29 @@
 #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.
+//
+// TODO: Mixin scopes also have a default `Self` type.
+static auto GetImplDefaultSelfType(Context& context,
+                                   const ClassScope& class_scope)
+    -> SemIR::TypeId {
+  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 +69,58 @@ 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)) {
+    // TODO: Also handle the parent scope being a mixin.
+    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 the default, suggest removing it with a
+        // diagnostic, but continue as if no error occurred since the self-type
+        // is semantically valid.
+        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();
+        }
+      } else {
+        // Otherwise, the self-type is an error.
+        if (self_type.type_id != SemIR::ErrorInst::TypeId) {
+          diag.Emit();
+          self_type.inst_id = SemIR::ErrorInst::TypeInstId;
+        }
+      }
+    }
+  }
 
   // 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
@@ -149,8 +197,7 @@ static auto PopImplIntroducerAndParamsAsNameComponent(
 
 // Build an ImplDecl describing the signature of an impl. This handles the
 // common logic shared by impl forward declarations and impl definitions.
-static auto BuildImplDecl(Context& context, Parse::AnyImplDeclId node_id,
-                          bool is_definition)
+static auto BuildImplDecl(Context& context, Parse::AnyImplDeclId node_id)
     -> std::pair<SemIR::ImplId, SemIR::InstId> {
   auto [constraint_node, constraint_id] =
       context.node_stack().PopExprWithNodeId();
@@ -185,54 +232,69 @@ 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, extend_node);
   }
 
-  return StartImplDecl(context, SemIR::LocId(node_id),
-                       name.implicit_params_loc_id, impl_info, is_definition,
-                       extend_impl);
+  // `GetOrAddImpl` 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);
+  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_id, impl_decl_id] = BuildImplDecl(context, node_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);
-  // This requires that the facet type is complete.
-  ImplWitnessStartDefinition(context, impl_info);
+      impl_decl_id, impl.scope_id,
+      context.generics().GetSelfSpecific(impl.generic_id));
+
+  StartGenericDefinition(context, impl.generic_id);
+  ImplWitnessStartDefinition(context, impl);
   context.inst_block_stack().Push();
   context.node_stack().Push(node_id, impl_id);
 
@@ -245,7 +307,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;
 }