Stop allowing impl redeclarations to differ syntactically in where clause (#4850)

Based on [the lastest thinking on
#4672](#4672 (comment))
, require a full syntactic match for impl redeclaration, instead of
excluding the `where` restriction. This means no updates to the impl
witness on redeclaration, and no diagnostics that those updates are
consistent.

Not included in this PR, but will need to be done in the future:
* Support for assigning values to associated constants in the body of
the impl definition. This will require moving the checking that
non-function associated constants are set from the definition start to
definition end.
* Identify semantic redeclarations that are not syntactic matches to
give a failed redeclaration diagnostic. This should be done once we are
already identifying impl declarations with the same type structure in
order to require they be identified in an impl_priority/match_first
block.
* Merging of the functions in `check/impl.cpp` that are now always
called together.

Also add some test coverage of `where` parsing I developed in PR I've
now abandoned because of this new simplification of the impl
redeclaration semantics.

---------

Co-authored-by: Josh L <[email protected]>
Co-authored-by: Jon Ross-Perkins <[email protected]>

Author: josh11b

toolchain/check/handle_impl.cpp

@@ -214,40 +214,11 @@ static auto PopImplIntroducerAndParamsAsNameComponent(
 
   Parse::NodeId first_param_node_id =
       context.node_stack().PopForSoloNodeId<Parse::NodeKind::ImplIntroducer>();
-
   // Subtracting 1 since we don't want to include the final `{` or `;` of the
   // declaration when performing syntactic match.
-  auto end_node_kind = context.parse_tree().node_kind(end_of_decl_node_id);
-  CARBON_CHECK(end_node_kind == Parse::NodeKind::ImplDefinitionStart ||
-               end_node_kind == Parse::NodeKind::ImplDecl);
   Parse::Tree::PostorderIterator last_param_iter(end_of_decl_node_id);
   --last_param_iter;
 
-  // Following proposal #3763, exclude a final `where` clause, if present. See:
-  // https://github.com/carbon-language/carbon-lang/blob/trunk/proposals/p3763.md#redeclarations
-
-  // Caches the NodeKind for the current value of *last_param_iter so
-  if (context.parse_tree().node_kind(*last_param_iter) ==
-      Parse::NodeKind::WhereExpr) {
-    int where_operands_to_skip = 1;
-    --last_param_iter;
-    CARBON_CHECK(Parse::Tree::PostorderIterator(first_param_node_id) <
-                 last_param_iter);
-    do {
-      auto node_kind = context.parse_tree().node_kind(*last_param_iter);
-      if (node_kind == Parse::NodeKind::WhereExpr) {
-        // If we have a nested `where`, we need to see another `WhereOperand`
-        // before we find the one that matches our original `WhereExpr` node.
-        ++where_operands_to_skip;
-      } else if (node_kind == Parse::NodeKind::WhereOperand) {
-        --where_operands_to_skip;
-      }
-      --last_param_iter;
-      CARBON_CHECK(Parse::Tree::PostorderIterator(first_param_node_id) <
-                   last_param_iter);
-    } while (where_operands_to_skip > 0);
-  }
-
   return {
       .name_loc_id = Parse::NodeId::None,
       .name_id = SemIR::NameId::None,
@@ -315,9 +286,6 @@ static auto IsValidImplRedecl(Context& context, SemIR::Impl& new_impl,
 
   // TODO: Only allow redeclaration in a match_first/impl_priority block.
 
-  // TODO: Merge information from the new declaration into the old one as
-  // needed.
-
   return true;
 }
 
@@ -375,6 +343,8 @@ static auto BuildImplDecl(Context& context, Parse::AnyImplDeclId node_id,
   // Add the impl declaration.
   bool invalid_redeclaration = false;
   auto lookup_bucket_ref = context.impls().GetOrAddLookupBucket(impl_info);
+  // TODO: Detect two impl declarations with the same self type and interface,
+  // and issue an error if they don't match.
   for (auto prev_impl_id : lookup_bucket_ref) {
     if (MergeImplRedecl(context, impl_info, prev_impl_id)) {
       if (IsValidImplRedecl(context, impl_info, prev_impl_id)) {
@@ -392,15 +362,13 @@ static auto BuildImplDecl(Context& context, Parse::AnyImplDeclId node_id,
   if (!impl_decl.impl_id.has_value()) {
     impl_info.generic_id = BuildGeneric(context, impl_decl_id);
     impl_info.witness_id = ImplWitnessForDeclaration(context, impl_info);
-    AddConstantsToImplWitnessFromConstraint(
-        context, impl_info, impl_info.witness_id, impl_decl.impl_id);
+    AddConstantsToImplWitnessFromConstraint(context, impl_info,
+                                            impl_info.witness_id);
     FinishGenericDecl(context, impl_decl_id, impl_info.generic_id);
     impl_decl.impl_id = context.impls().Add(impl_info);
     lookup_bucket_ref.push_back(impl_decl.impl_id);
   } else {
     const auto& first_impl = context.impls().Get(impl_decl.impl_id);
-    AddConstantsToImplWitnessFromConstraint(
-        context, impl_info, first_impl.witness_id, impl_decl.impl_id);
     FinishGenericRedecl(context, impl_decl_id, first_impl.generic_id);
   }
 

toolchain/check/impl.cpp

@@ -32,16 +32,6 @@ static auto NoteAssociatedFunction(Context& context,
                function.name_id);
 }
 
-// Adds the location of the previous declaration to a diagnostic.
-static auto NotePreviousDecl(Context& context,
-                             Context::DiagnosticBuilder& builder,
-                             SemIR::ImplId impl_id) -> void {
-  CARBON_DIAGNOSTIC(ImplPreviousDeclHere, Note,
-                    "impl previously declared here");
-  const auto& impl = context.impls().Get(impl_id);
-  builder.Note(impl.latest_decl_id(), ImplPreviousDeclHere);
-}
-
 // Checks that `impl_function_id` is a valid implementation of the function
 // described in the interface as `interface_function_id`. Returns the value to
 // put into the corresponding slot in the witness table, which can be
@@ -88,6 +78,7 @@ static auto CheckAssociatedFunctionImplementation(
 }
 
 // Builds an initial empty witness.
+// TODO: Fill the witness with the rewrites from the declaration.
 auto ImplWitnessForDeclaration(Context& context, const SemIR::Impl& impl)
     -> SemIR::InstId {
   CARBON_CHECK(!impl.has_definition_started());
@@ -159,11 +150,10 @@ static auto WitnessAccessMatchesInterface(
   return false;
 }
 
+// TODO: Merge this function into `ImplWitnessForDeclaration`.
 auto AddConstantsToImplWitnessFromConstraint(Context& context,
                                              const SemIR::Impl& impl,
-                                             SemIR::InstId witness_id,
-                                             SemIR::ImplId prev_decl_id)
-    -> void {
+                                             SemIR::InstId witness_id) -> void {
   CARBON_CHECK(!impl.has_definition_started());
   CARBON_CHECK(witness_id.has_value());
   if (witness_id == SemIR::ErrorInst::SingletonInstId) {
@@ -229,16 +219,13 @@ auto AddConstantsToImplWitnessFromConstraint(Context& context,
     }
   }
 
-  // For each non-function associated constant, update witness entry.
+  // For each non-function associated constant, set the witness entry.
   for (auto index : llvm::seq(assoc_entities.size())) {
     auto decl_id =
         context.constant_values().GetConstantInstId(assoc_entities[index]);
     CARBON_CHECK(decl_id.has_value(), "Non-constant associated entity");
     if (auto decl =
             context.insts().TryGetAs<SemIR::AssociatedConstantDecl>(decl_id)) {
-      const auto& assoc_const =
-          context.associated_constants().Get(decl->assoc_const_id);
-      auto& witness_value = witness_block[index];
       auto rewrite_value = rewrite_values[index];
 
       // If the associated constant has a symbolic type, convert the rewrite
@@ -264,6 +251,8 @@ auto AddConstantsToImplWitnessFromConstraint(Context& context,
         // value was constant.
         if (!rewrite_value.is_constant() &&
             rewrite_value != SemIR::ErrorInst::SingletonConstantId) {
+          const auto& assoc_const =
+              context.associated_constants().Get(decl->assoc_const_id);
           CARBON_DIAGNOSTIC(
               AssociatedConstantNotConstantAfterConversion, Error,
               "associated constant {0} given value that is not constant "
@@ -276,39 +265,9 @@ auto AddConstantsToImplWitnessFromConstraint(Context& context,
         }
       }
 
-      if (witness_value.has_value() &&
-          witness_value != SemIR::ErrorInst::SingletonInstId) {
-        // TODO: Support just using the witness values if the redeclaration uses
-        // `where _`, per proposal #1084.
-        if (!rewrite_value.has_value()) {
-          CARBON_DIAGNOSTIC(AssociatedConstantMissingInRedecl, Error,
-                            "associated constant {0} given value in "
-                            "declaration but not redeclaration",
-                            SemIR::NameId);
-          auto builder = context.emitter().Build(
-              impl.latest_decl_id(), AssociatedConstantMissingInRedecl,
-              assoc_const.name_id);
-          NotePreviousDecl(context, builder, prev_decl_id);
-          builder.Emit();
-          continue;
-        }
-        auto witness_const_id = context.constant_values().Get(witness_value);
-        if (witness_const_id != rewrite_value &&
-            rewrite_value != SemIR::ErrorInst::SingletonConstantId) {
-          // TODO: Figure out how to print the two different values
-          CARBON_DIAGNOSTIC(
-              AssociatedConstantDifferentInRedecl, Error,
-              "redeclaration with different value for associated constant {0}",
-              SemIR::NameId);
-          auto builder = context.emitter().Build(
-              impl.latest_decl_id(), AssociatedConstantDifferentInRedecl,
-              assoc_const.name_id);
-          NotePreviousDecl(context, builder, prev_decl_id);
-          builder.Emit();
-          continue;
-        }
-      } else if (rewrite_value.has_value()) {
-        witness_value = context.constant_values().GetInstId(rewrite_value);
+      if (rewrite_value.has_value()) {
+        witness_block[index] =
+            context.constant_values().GetInstId(rewrite_value);
       }
     }
   }

toolchain/check/impl.h

@@ -16,9 +16,7 @@ auto ImplWitnessForDeclaration(Context& context, const SemIR::Impl& impl)
 
 auto AddConstantsToImplWitnessFromConstraint(Context& context,
                                              const SemIR::Impl& impl,
-                                             SemIR::InstId witness_id,
-                                             SemIR::ImplId prev_decl_id)
-    -> void;
+                                             SemIR::InstId witness_id) -> void;
 
 // Update `impl`'s witness at the start of a definition.
 auto ImplWitnessStartDefinition(Context& context, SemIR::Impl& impl) -> void;