Clean up helpers for GetOrAddImpl (Refactor Impl construction 5/7 and 6/7) (#6469)

Make the `AssignImplIdInWitness` into a `static` helper function since
it's only used inside `GetOrAddImpl`. Restructure the diagnostic for
unused generic bindings to move more logic into the helper, and out of
`GetOrAddImpl` so that it has more clear steps.

This is part of #6420 which is being split up into a chain of smaller
PRs. It is based on #6468.

toolchain/check/impl.cpp

@@ -262,22 +262,6 @@ auto FillImplWitnessWithErrors(Context& context, SemIR::Impl& impl) -> void {
   impl.witness_id = SemIR::ErrorInst::InstId;
 }
 
-auto AssignImplIdInWitness(Context& context, SemIR::ImplId impl_id,
-                           SemIR::InstId witness_id) -> void {
-  if (witness_id == SemIR::ErrorInst::InstId) {
-    return;
-  }
-  auto witness = context.insts().GetAs<SemIR::ImplWitness>(witness_id);
-  auto witness_table =
-      context.insts().GetAs<SemIR::ImplWitnessTable>(witness.witness_table_id);
-  witness_table.impl_id = impl_id;
-  // Note: The `ImplWitnessTable` instruction is `Unique`, so while this marks
-  // the instruction as being a dependent instruction of a generic impl, it will
-  // not be substituted into the eval block.
-  ReplaceInstBeforeConstantUse(context, witness.witness_table_id,
-                               witness_table);
-}
-
 auto IsImplEffectivelyFinal(Context& context, const SemIR::Impl& impl) -> bool {
   return impl.is_final ||
          (context.constant_values().Get(impl.self_id).is_concrete() &&
@@ -379,6 +363,65 @@ static auto IsValidImplRedecl(Context& context, SemIR::Impl& new_impl,
   return true;
 }
 
+// Sets the `ImplId` in the `ImplWitnessTable`.
+static auto AssignImplIdInWitness(Context& context, SemIR::ImplId impl_id,
+                                  SemIR::InstId witness_id) -> void {
+  if (witness_id == SemIR::ErrorInst::InstId) {
+    return;
+  }
+  auto witness = context.insts().GetAs<SemIR::ImplWitness>(witness_id);
+  auto witness_table =
+      context.insts().GetAs<SemIR::ImplWitnessTable>(witness.witness_table_id);
+  witness_table.impl_id = impl_id;
+  // Note: The `ImplWitnessTable` instruction is `Unique`, so while this marks
+  // the instruction as being a dependent instruction of a generic impl, it will
+  // not be substituted into the eval block.
+  ReplaceInstBeforeConstantUse(context, witness.witness_table_id,
+                               witness_table);
+}
+
+// Looks for any unused generic bindings. If one is found, it is diagnosed and
+// false is returned.
+static auto VerifyAllGenericBindingsUsed(Context& context, SemIR::LocId loc_id,
+                                         SemIR::LocId implicit_params_loc_id,
+                                         SemIR::Impl& impl) -> bool {
+  if (impl.witness_id == SemIR::ErrorInst::InstId) {
+    return true;
+  }
+  if (!impl.generic_id.has_value()) {
+    return true;
+  }
+
+  if (impl.implicit_param_patterns_id.has_value()) {
+    for (auto inst_id :
+         context.inst_blocks().Get(impl.implicit_param_patterns_id)) {
+      if (inst_id == SemIR::ErrorInst::InstId) {
+        // An error was already diagnosed for a generic binding.
+        return true;
+      }
+    }
+  }
+
+  auto deduced_specific_id = DeduceImplArguments(
+      context, loc_id, impl, context.constant_values().Get(impl.self_id),
+      impl.interface.specific_id);
+  if (deduced_specific_id.has_value()) {
+    // Deduction succeeded, all bindings were used.
+    return true;
+  }
+
+  CARBON_DIAGNOSTIC(ImplUnusedBinding, Error,
+                    "`impl` with unused generic binding");
+  // TODO: This location may be incorrect, the binding may be inherited
+  // from an outer declaration. It would be nice to get the particular
+  // binding that was undeducible back from DeduceImplArguments here and
+  // use that.
+  auto diag_loc_id =
+      implicit_params_loc_id.has_value() ? implicit_params_loc_id : loc_id;
+  context.emitter().Emit(diag_loc_id, ImplUnusedBinding);
+  return false;
+}
+
 // Apply an `extend impl` declaration by extending the parent scope with the
 // `impl`. If there's an error it is diagnosed and false is returned.
 static auto ApplyExtendImplAs(Context& context, SemIR::LocId loc_id,
@@ -443,38 +486,6 @@ static auto ApplyExtendImplAs(Context& context, SemIR::LocId loc_id,
   return true;
 }
 
-// Diagnoses when an impl has an unused binding.
-static auto DiagnoseUnusedGenericBinding(Context& context, SemIR::LocId loc_id,
-                                         SemIR::LocId implicit_params_loc_id,
-                                         SemIR::ImplId impl_id) -> void {
-  auto& impl = context.impls().Get(impl_id);
-  if (!impl.generic_id.has_value() ||
-      impl.witness_id == SemIR::ErrorInst::InstId) {
-    return;
-  }
-
-  auto deduced_specific_id = DeduceImplArguments(
-      context, loc_id, impl, context.constant_values().Get(impl.self_id),
-      impl.interface.specific_id);
-  if (deduced_specific_id.has_value()) {
-    // Deduction succeeded, all bindings were used.
-    return;
-  }
-
-  CARBON_DIAGNOSTIC(ImplUnusedBinding, Error,
-                    "`impl` with unused generic binding");
-  // TODO: This location may be incorrect, the binding may be inherited
-  // from an outer declaration. It would be nice to get the particular
-  // binding that was undeducible back from DeduceImplArguments here and
-  // use that.
-  auto diag_loc_id =
-      implicit_params_loc_id.has_value() ? implicit_params_loc_id : loc_id;
-  context.emitter().Emit(diag_loc_id, ImplUnusedBinding);
-  // Don't try to match the impl at all, save us work and possible future
-  // diagnostics.
-  FillImplWitnessWithErrors(context, context.impls().Get(impl_id));
-}
-
 auto GetOrAddImpl(Context& context, SemIR::LocId loc_id,
                   SemIR::LocId implicit_params_loc_id, SemIR::Impl impl,
                   bool is_definition, Parse::NodeId extend_node)
@@ -535,37 +546,23 @@ auto GetOrAddImpl(Context& context, SemIR::LocId loc_id,
   // in order to make and see any changes to the `Impl`.
   impl_id = context.impls().Add(impl);
   lookup_bucket_ref.push_back(impl_id);
-
   AssignImplIdInWitness(context, impl_id, impl.witness_id);
 
-  auto& stored_impl_info = context.impls().Get(impl_id);
-
-  // Looking to see if there are any generic bindings on the `impl`
-  // declaration that are not deducible. If so, and the `impl` does not
-  // actually use all its generic bindings, and will never be matched. This
-  // should be diagnossed to the user.
-  bool has_error_in_implicit_pattern = false;
-  if (impl.implicit_param_patterns_id.has_value()) {
-    for (auto inst_id :
-         context.inst_blocks().Get(impl.implicit_param_patterns_id)) {
-      if (inst_id == SemIR::ErrorInst::InstId) {
-        has_error_in_implicit_pattern = true;
-        break;
-      }
-    }
-  }
+  auto& stored_impl = context.impls().Get(impl_id);
 
-  if (!has_error_in_implicit_pattern) {
-    DiagnoseUnusedGenericBinding(context, loc_id, implicit_params_loc_id,
-                                 impl_id);
+  // Look to see if there are any generic bindings on the `impl` declaration
+  // that are not deducible. If so, and the `impl` does not actually use all its
+  // generic bindings, and will never be matched. This should be diagnosed to
+  // the user.
+  if (!VerifyAllGenericBindingsUsed(context, loc_id, implicit_params_loc_id,
+                                    stored_impl)) {
+    FillImplWitnessWithErrors(context, stored_impl);
   }
 
-  // For an `extend impl` declaration, mark the impl as extending this `impl`.
   if (extend_node.has_value()) {
-    if (!ApplyExtendImplAs(context, loc_id, stored_impl_info, extend_node,
+    if (!ApplyExtendImplAs(context, loc_id, stored_impl, extend_node,
                            implicit_params_loc_id)) {
-      // Don't allow the invalid impl to be used.
-      FillImplWitnessWithErrors(context, stored_impl_info);
+      FillImplWitnessWithErrors(context, stored_impl);
     }
   }
 

toolchain/check/impl.h

@@ -27,10 +27,6 @@ auto FinishImplWitness(Context& context, SemIR::ImplId impl_id) -> void;
 // sets the witness id in the `Impl` to an error.
 auto FillImplWitnessWithErrors(Context& context, SemIR::Impl& impl) -> void;
 
-// Sets the `ImplId` in the `ImplWitnessTable`.
-auto AssignImplIdInWitness(Context& context, SemIR::ImplId impl_id,
-                           SemIR::InstId witness_id) -> void;
-
 // Returns whether the impl is either `final` explicitly, or implicitly due to
 // being concrete.
 auto IsImplEffectivelyFinal(Context& context, const SemIR::Impl& impl) -> bool;