ImplWitness (#4679)

* Change `InterfaceWitness` -> `ImplWitness` * Include a `SpecificId` in the `ImplWitness`. This allows the `InstBlock` it contains to have its own identity, allowing it to be changed as the impl is processed. Evaluation only updates the specific. * Create the `ImplWitness` at the start of the impl definition. In the future, this will be populated with the values of non-function associated constants. For now, it starts full of invalid instruction ids. * Implements the model suggested in #4672 . Note that the non-SemIR testdata changes are to these file: * `toolchain/check/testdata/impl/lookup/fail_todo_undefined_impl.carbon` * `toolchain/check/testdata/struct/import.carbon` * `toolchain/check/testdata/tuple/import.carbon` The last two are due to an import of generics bug exposed by this PR, which will be fixed in a follow-on. --------- Co-authored-by: Josh L <[email protected]> Co-authored-by: Richard Smith <[email protected]>
carbon-language · Jan 2, 2025 · c5fd8f4 · c5fd8f4
1 parent 4a7aefe
commit c5fd8f4
Show file tree

Hide file tree

Showing 256 changed files with 3,058 additions and 2,614 deletions.
diff --git a/toolchain/check/check_unit.cpp b/toolchain/check/check_unit.cpp
@@ -8,6 +8,7 @@
 #include "toolchain/base/pretty_stack_trace_function.h"
 #include "toolchain/check/generic.h"
 #include "toolchain/check/handle.h"
+#include "toolchain/check/impl.h"
 #include "toolchain/check/import.h"
 #include "toolchain/check/import_ref.h"
 #include "toolchain/check/node_id_traversal.h"
@@ -397,6 +398,7 @@ auto CheckUnit::CheckRequiredDefinitions() -> void {
       case CARBON_KIND(SemIR::ImplDecl impl_decl): {
         auto& impl = context_.impls().Get(impl_decl.impl_id);
         if (!impl.is_defined()) {
+          FillImplWitnessWithErrors(context_, impl);
           CARBON_DIAGNOSTIC(MissingImplDefinition, Error,
                             "impl declared but not defined");
           emitter_.Emit(decl_inst_id, MissingImplDefinition);

diff --git a/toolchain/check/context.cpp b/toolchain/check/context.cpp
@@ -1284,7 +1284,7 @@ class TypeCompleter {
 
   template <typename InstT>
     requires(InstT::Kind.template IsAnyOf<SemIR::BindSymbolicName,
-                                          SemIR::InterfaceWitnessAccess>())
+                                          SemIR::ImplWitnessAccess>())
   auto BuildValueReprForInst(SemIR::TypeId type_id, InstT /*inst*/) const
       -> SemIR::ValueRepr {
     // For symbolic types, we arbitrarily pick a copy representation.

diff --git a/toolchain/check/eval.cpp b/toolchain/check/eval.cpp
@@ -7,6 +7,7 @@
 #include "toolchain/base/kind_switch.h"
 #include "toolchain/check/diagnostic_helpers.h"
 #include "toolchain/check/generic.h"
+#include "toolchain/check/import_ref.h"
 #include "toolchain/diagnostics/diagnostic_emitter.h"
 #include "toolchain/diagnostics/format_providers.h"
 #include "toolchain/sem_ir/builtin_function_kind.h"
@@ -1565,9 +1566,13 @@ static auto TryEvalInstInContext(EvalContext& eval_context,
       return RebuildIfFieldsAreConstant(
           eval_context, inst,
           &SemIR::GenericInterfaceType::enclosing_specific_id);
-    case SemIR::InterfaceWitness::Kind:
+    case SemIR::ImplWitness::Kind:
+      // We intentionally don't replace the `elements_id` field here. We want to
+      // track that specific InstBlock in particular, not coalesce blocks with
+      // the same members. That block may get updated, and we want to pick up
+      // those changes.
       return RebuildIfFieldsAreConstant(eval_context, inst,
-                                        &SemIR::InterfaceWitness::elements_id);
+                                        &SemIR::ImplWitness::specific_id);
     case CARBON_KIND(SemIR::IntType int_type): {
       return RebuildAndValidateIfFieldsAreConstant(
           eval_context, inst,
@@ -1742,11 +1747,48 @@ static auto TryEvalInstInContext(EvalContext& eval_context,
 
     // The elements of a constant aggregate can be accessed.
     case SemIR::ClassElementAccess::Kind:
-    case SemIR::InterfaceWitnessAccess::Kind:
     case SemIR::StructAccess::Kind:
     case SemIR::TupleAccess::Kind:
       return PerformAggregateAccess(eval_context, inst);
 
+    case CARBON_KIND(SemIR::ImplWitnessAccess access_inst): {
+      // This is PerformAggregateAccess followed by GetConstantInSpecific.
+      Phase phase = Phase::Template;
+      if (ReplaceFieldWithConstantValue(eval_context, &access_inst,
+                                        &SemIR::ImplWitnessAccess::witness_id,
+                                        &phase)) {
+        if (auto witness = eval_context.insts().TryGetAs<SemIR::ImplWitness>(
+                access_inst.witness_id)) {
+          auto elements = eval_context.inst_blocks().Get(witness->elements_id);
+          auto index = static_cast<size_t>(access_inst.index.index);
+          CARBON_CHECK(index < elements.size(), "Access out of bounds.");
+          // `Phase` is not used here. If this element is a template constant,
+          // then so is the result of indexing, even if the aggregate also
+          // contains a symbolic context.
+
+          auto element = elements[index];
+          if (!element.is_valid()) {
+            // TODO: Perhaps this should be a `{}` value with incomplete type?
+            CARBON_DIAGNOSTIC(ImplAccessMemberBeforeComplete, Error,
+                              "accessing member from impl before the end of "
+                              "its definition");
+            // TODO: Add note pointing to the impl declaration.
+            eval_context.emitter().Emit(eval_context.GetDiagnosticLoc(inst_id),
+                                        ImplAccessMemberBeforeComplete);
+            return SemIR::ErrorInst::SingletonConstantId;
+          }
+          LoadImportRef(eval_context.context(), element);
+          return GetConstantValueInSpecific(eval_context.sem_ir(),
+                                            witness->specific_id, element);
+        } else {
+          CARBON_CHECK(phase != Phase::Template,
+                       "Failed to evaluate template constant {0} arg0: {1}",
+                       inst, eval_context.insts().Get(access_inst.witness_id));
+        }
+        return MakeConstantResult(eval_context.context(), access_inst, phase);
+      }
+      return MakeNonConstantResult(phase);
+    }
     case CARBON_KIND(SemIR::ArrayIndex index): {
       return PerformArrayIndex(eval_context, index);
     }

diff --git a/toolchain/check/generic.cpp b/toolchain/check/generic.cpp
@@ -15,6 +15,11 @@
 
 namespace Carbon::Check {
 
+static auto MakeSelfSpecificId(Context& context, SemIR::GenericId generic_id)
+    -> SemIR::SpecificId;
+static auto ResolveSpecificDeclaration(Context& context, SemIRLoc loc,
+                                       SemIR::SpecificId specific_id) -> void;
+
 auto StartGenericDecl(Context& context) -> void {
   context.generic_region_stack().Push();
 }
@@ -315,15 +320,19 @@ auto DiscardGenericDecl(Context& context) -> void {
   context.generic_region_stack().Pop();
 }
 
-auto FinishGenericDecl(Context& context, SemIR::InstId decl_id)
-    -> SemIR::GenericId {
+auto BuildGeneric(Context& context, SemIR::InstId decl_id) -> SemIR::GenericId {
   auto all_bindings =
       context.scope_stack().compile_time_bindings_stack().PeekAllValues();
 
   if (all_bindings.empty()) {
     CARBON_CHECK(context.generic_region_stack().PeekDependentInsts().empty(),
-                 "Have dependent instructions but no compile time bindings are "
-                 "in scope.");
+                 "Have dependent instruction {0} in declaration {1} but no "
+                 "compile time bindings are in scope.",
+                 context.insts().Get(context.generic_region_stack()
+                                         .PeekDependentInsts()
+                                         .front()
+                                         .inst_id),
+                 context.insts().Get(decl_id));
     context.generic_region_stack().Pop();
     return SemIR::GenericId::Invalid;
   }
@@ -333,18 +342,38 @@ auto FinishGenericDecl(Context& context, SemIR::InstId decl_id)
   // collection can have items added to it by import resolution while we are
   // building this generic.
   auto bindings_id = context.inst_blocks().Add(all_bindings);
-  auto generic_id = context.generics().Add(
+
+  SemIR::GenericId generic_id = context.generics().Add(
       SemIR::Generic{.decl_id = decl_id,
                      .bindings_id = bindings_id,
                      .self_specific_id = SemIR::SpecificId::Invalid});
+  // MakeSelfSpecificId could cause something to be imported, which would
+  // invalidate the return value of `context.generics().Get(generic_id)`.
+  auto self_specific_id = MakeSelfSpecificId(context, generic_id);
+  context.generics().Get(generic_id).self_specific_id = self_specific_id;
+  return generic_id;
+}
 
+auto FinishGenericDecl(Context& context, SemIRLoc loc,
+                       SemIR::GenericId generic_id) -> void {
+  if (!generic_id.is_valid()) {
+    return;
+  }
   auto decl_block_id = MakeGenericEvalBlock(
       context, generic_id, SemIR::GenericInstIndex::Region::Declaration);
   context.generic_region_stack().Pop();
   context.generics().Get(generic_id).decl_block_id = decl_block_id;
 
-  auto self_specific_id = MakeSelfSpecific(context, decl_id, generic_id);
-  context.generics().Get(generic_id).self_specific_id = self_specific_id;
+  ResolveSpecificDeclaration(context, loc,
+                             context.generics().GetSelfSpecific(generic_id));
+}
+
+auto BuildGenericDecl(Context& context, SemIR::InstId decl_id)
+    -> SemIR::GenericId {
+  SemIR::GenericId generic_id = BuildGeneric(context, decl_id);
+  if (generic_id.is_valid()) {
+    FinishGenericDecl(context, decl_id, generic_id);
+  }
   return generic_id;
 }
 
@@ -373,26 +402,34 @@ auto FinishGenericDefinition(Context& context, SemIR::GenericId generic_id)
   context.generic_region_stack().Pop();
 }
 
-auto MakeSpecific(Context& context, SemIRLoc loc, SemIR::GenericId generic_id,
-                  SemIR::InstBlockId args_id) -> SemIR::SpecificId {
-  auto specific_id = context.specifics().GetOrAdd(generic_id, args_id);
-
+static auto ResolveSpecificDeclaration(Context& context, SemIRLoc loc,
+                                       SemIR::SpecificId specific_id) -> void {
   // If this is the first time we've formed this specific, evaluate its decl
   // block to form information about the specific.
   if (!context.specifics().Get(specific_id).decl_block_id.is_valid()) {
+    // Set a placeholder value as the decl block ID so we won't attempt to
+    // recursively resolve the same specific.
+    context.specifics().Get(specific_id).decl_block_id =
+        SemIR::InstBlockId::Empty;
+
     auto decl_block_id =
         TryEvalBlockForSpecific(context, loc, specific_id,
                                 SemIR::GenericInstIndex::Region::Declaration);
     // Note that TryEvalBlockForSpecific may reallocate the list of specifics,
     // so re-lookup the specific here.
     context.specifics().Get(specific_id).decl_block_id = decl_block_id;
   }
+}
 
+auto MakeSpecific(Context& context, SemIRLoc loc, SemIR::GenericId generic_id,
+                  SemIR::InstBlockId args_id) -> SemIR::SpecificId {
+  auto specific_id = context.specifics().GetOrAdd(generic_id, args_id);
+  ResolveSpecificDeclaration(context, loc, specific_id);
   return specific_id;
 }
 
-auto MakeSelfSpecific(Context& context, SemIRLoc loc,
-                      SemIR::GenericId generic_id) -> SemIR::SpecificId {
+static auto MakeSelfSpecificId(Context& context, SemIR::GenericId generic_id)
+    -> SemIR::SpecificId {
   if (!generic_id.is_valid()) {
     return SemIR::SpecificId::Invalid;
   }
@@ -407,16 +444,23 @@ auto MakeSelfSpecific(Context& context, SemIRLoc loc,
     arg_ids.push_back(context.constant_values().GetConstantInstId(arg_id));
   }
   auto args_id = context.inst_blocks().AddCanonical(arg_ids);
+  return context.specifics().GetOrAdd(generic_id, args_id);
+}
 
+auto MakeSelfSpecific(Context& context, SemIRLoc loc,
+                      SemIR::GenericId generic_id) -> SemIR::SpecificId {
   // Build a corresponding specific.
+  SemIR::SpecificId specific_id = MakeSelfSpecificId(context, generic_id);
   // TODO: This could be made more efficient. We don't need to perform
   // substitution here; we know we want identity mappings for all constants and
   // types. We could also consider not storing the mapping at all in this case.
-  return MakeSpecific(context, loc, generic_id, args_id);
+  ResolveSpecificDeclaration(context, loc, specific_id);
+  return specific_id;
 }
 
 auto ResolveSpecificDefinition(Context& context, SemIRLoc loc,
                                SemIR::SpecificId specific_id) -> bool {
+  // TODO: Handle recursive resolution of the same generic definition.
   auto& specific = context.specifics().Get(specific_id);
   auto generic_id = specific.generic_id;
   CARBON_CHECK(generic_id.is_valid(), "Specific with no generic ID");

diff --git a/toolchain/check/generic.h b/toolchain/check/generic.h
@@ -24,7 +24,15 @@ auto DiscardGenericDecl(Context& context) -> void;
 // Finish processing a potentially generic declaration and produce a
 // corresponding generic object. Returns SemIR::GenericId::Invalid if this
 // declaration is not actually generic.
-auto FinishGenericDecl(Context& context, SemIR::InstId decl_id)
+auto BuildGeneric(Context& context, SemIR::InstId decl_id) -> SemIR::GenericId;
+
+// Builds eval block for the declaration.
+auto FinishGenericDecl(Context& context, SemIRLoc loc,
+                       SemIR::GenericId generic_id) -> void;
+
+// BuildGeneric() and FinishGenericDecl() combined. Normally you would call this
+// function unless the caller has work to do between the two steps.
+auto BuildGenericDecl(Context& context, SemIR::InstId decl_id)
     -> SemIR::GenericId;
 
 // Merge a redeclaration of an entity that might be a generic into the original

diff --git a/toolchain/check/handle_class.cpp b/toolchain/check/handle_class.cpp
@@ -237,7 +237,7 @@ static auto BuildClassDecl(Context& context, Parse::AnyClassDeclId node_id,
     // TODO: If this is an invalid redeclaration of a non-class entity or there
     // was an error in the qualifier, we will have lost track of the class name
     // here. We should keep track of it even if the name is invalid.
-    class_info.generic_id = FinishGenericDecl(context, class_decl_id);
+    class_info.generic_id = BuildGenericDecl(context, class_decl_id);
     class_decl.class_id = context.classes().Add(class_info);
     if (class_info.has_parameters()) {
       class_decl.type_id = context.GetGenericClassType(

diff --git a/toolchain/check/handle_function.cpp b/toolchain/check/handle_function.cpp
@@ -261,7 +261,7 @@ static auto BuildFunctionDecl(Context& context,
     if (function_info.is_extern && context.IsImplFile()) {
       DiagnoseExternRequiresDeclInApiFile(context, node_id);
     }
-    function_info.generic_id = FinishGenericDecl(context, decl_id);
+    function_info.generic_id = BuildGenericDecl(context, decl_id);
     function_decl.function_id = context.functions().Add(function_info);
   } else {
     FinishGenericRedecl(context, decl_id, function_info.generic_id);

diff --git a/toolchain/check/handle_impl.cpp b/toolchain/check/handle_impl.cpp
@@ -241,8 +241,6 @@ static auto MergeImplRedecl(Context& context, SemIR::Impl& new_impl,
                             SemIR::ImplId prev_impl_id) -> bool {
   auto& prev_impl = context.impls().Get(prev_impl_id);
 
-  // TODO: Following #3763, disallow redeclarations in different scopes.
-
   // If the parameters aren't the same, then this is not a redeclaration of this
   // `impl`. Keep looking for a prior declaration without issuing a diagnostic.
   if (!CheckRedeclParamsMatch(context, DeclParams(new_impl),
@@ -343,8 +341,9 @@ static auto BuildImplDecl(Context& context, Parse::AnyImplDeclId node_id,
 
   // Create a new impl if this isn't a valid redeclaration.
   if (!impl_decl.impl_id.is_valid()) {
-    impl_info.generic_id = FinishGenericDecl(context, impl_decl_id);
+    impl_info.generic_id = BuildGeneric(context, impl_decl_id);
     impl_info.witness_id = ImplWitnessForDeclaration(context, impl_info);
+    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 {
@@ -416,6 +415,9 @@ auto HandleParseNode(Context& context, Parse::ImplDefinitionStartId node_id)
       context.generics().GetSelfSpecific(impl_info.generic_id));
   StartGenericDefinition(context);
 
+  if (!impl_info.is_defined()) {
+    ImplWitnessStartDefinition(context, impl_info);
+  }
   context.inst_block_stack().Push();
   context.node_stack().Push(node_id, impl_id);
 
@@ -439,7 +441,7 @@ auto HandleParseNode(Context& context, Parse::ImplDefinitionId /*node_id*/)
 
   auto& impl_info = context.impls().Get(impl_id);
   if (!impl_info.is_defined()) {
-    impl_info.witness_id = BuildImplWitness(context, impl_info);
+    FinishImplWitness(context, impl_info);
     impl_info.defined = true;
   }
 

diff --git a/toolchain/check/handle_interface.cpp b/toolchain/check/handle_interface.cpp
@@ -115,7 +115,7 @@ static auto BuildInterfaceDecl(Context& context,
     // there was an error in the qualifier, we will have lost track of the
     // interface name here. We should keep track of it even if the name is
     // invalid.
-    interface_info.generic_id = FinishGenericDecl(context, interface_decl_id);
+    interface_info.generic_id = BuildGenericDecl(context, interface_decl_id);
     interface_decl.interface_id = context.interfaces().Add(interface_info);
     if (interface_info.has_parameters()) {
       interface_decl.type_id = context.GetGenericInterfaceType(