Switch handling of errors in impls to not build a type structure (#5881)

Per discussion at
#5875 (comment),
a different approach to the same solution.

A key difference is that whereas #5875 would build a `TypeStructure`
containing `ConcreteType{error}`, this instead just returns nothing.
This means impls with errors can't be compared in the same way, though
I'm not sure how much impact that'll really have (I've added a test here
to show a case where it seemed interesting to see what effect it'd have,
and it seems to have none).

---------

Co-authored-by: Dana Jansens <[email protected]>

Author: jonmeow

toolchain/check/impl_lookup.cpp

@@ -537,17 +537,20 @@ static auto CollectCandidateImplsForQuery(
     // Build the type structure used for choosing the best the candidate.
     auto type_structure =
         BuildTypeStructure(context, impl.self_id, impl.interface);
+    if (!type_structure) {
+      continue;
+    }
     // TODO: We can skip the comparison here if the `impl_interface_const_id` is
     // not symbolic, since when the interface and specific ids match, and they
     // aren't symbolic, the structure will be identical.
     if (!query_type_structure.CompareStructure(
             TypeStructure::CompareTest::IsEqualToOrMoreSpecificThan,
-            type_structure)) {
+            *type_structure)) {
       continue;
     }
 
     candidate_impls.push_back(
-        {id, impl.definition_id, std::move(type_structure)});
+        {id, impl.definition_id, std::move(*type_structure)});
   }
 
   auto compare = [](auto& lhs, auto& rhs) -> bool {
@@ -607,6 +610,9 @@ auto EvalLookupSingleImplWitness(Context& context, SemIR::LocId loc_id,
   auto query_type_structure = BuildTypeStructure(
       context, context.constant_values().GetInstId(query_self_const_id),
       query_specific_interface);
+  if (!query_type_structure) {
+    return EvalImplLookupResult::MakeNone();
+  }
   bool query_is_concrete =
       QueryIsConcrete(context, query_self_const_id, query_specific_interface);
 
@@ -621,7 +627,7 @@ auto EvalLookupSingleImplWitness(Context& context, SemIR::LocId loc_id,
   // not be concrete in this case, so only final impls can produce a concrete
   // witness for this query.
   auto candidate_impls = CollectCandidateImplsForQuery(
-      context, self_facet_provides_witness, query_type_structure,
+      context, self_facet_provides_witness, *query_type_structure,
       query_specific_interface);
 
   for (const auto& candidate : candidate_impls) {

toolchain/check/impl_validation.cpp

@@ -34,7 +34,7 @@ struct ImplInfo {
   bool is_local;
   // If imported, the IR from which the `impl` decl was imported.
   SemIR::ImportIRId ir_id;
-  TypeStructure type_structure;
+  std::optional<TypeStructure> type_structure;
 };
 
 }  // namespace
@@ -47,7 +47,7 @@ static auto GetIRId(Context& context, SemIR::InstId owning_inst_id)
   return GetCanonicalImportIRInst(context, owning_inst_id).ir_id();
 }
 
-auto GetImplInfo(Context& context, SemIR::ImplId impl_id) -> ImplInfo {
+static auto GetImplInfo(Context& context, SemIR::ImplId impl_id) -> ImplInfo {
   const auto& impl = context.impls().Get(impl_id);
   auto ir_id = GetIRId(context, impl.first_owning_decl_id);
   return {.impl_id = impl_id,
@@ -271,6 +271,11 @@ static auto ValidateImplsForInterface(Context& context,
 
     auto impls_before = llvm::drop_end(impls, num_impls - split_point);
     for (const auto& impl_a : impls_before) {
+      // Don't diagnose structures that contain errors.
+      if (!impl_a.type_structure || !impl_b.type_structure) {
+        continue;
+      }
+
       // Only enforce rules when at least one of the impls was written in this
       // file.
       if (!impl_a.is_local && !impl_b.is_local) {
@@ -304,9 +309,9 @@ static auto ValidateImplsForInterface(Context& context,
             did_diagnose_unmatchable_non_final_impl_with_final_impl = true;
           }
         }
-      } else if (impl_a.type_structure.CompareStructure(
+      } else if (impl_a.type_structure->CompareStructure(
                      TypeStructure::CompareTest::HasOverlap,
-                     impl_b.type_structure)) {
+                     *impl_b.type_structure)) {
         // =====================================================================
         // Rules between two overlapping final impls.
         // =====================================================================

toolchain/check/testdata/impl/basic.carbon

@@ -60,6 +60,56 @@ fn F() {
   C.(I.Op)();
 }
 
+// --- fail_conflicting_bad_impls.carbon
+
+library "[[@TEST_NAME]]";
+
+interface I {}
+
+class C(T:! type) {}
+
+// CHECK:STDERR: fail_conflicting_bad_impls.carbon:[[@LINE+4]]:12: error: name `invalid` not found [NameNotFound]
+// CHECK:STDERR: final impl invalid as I {}
+// CHECK:STDERR:            ^~~~~~~
+// CHECK:STDERR:
+final impl invalid as I {}
+// CHECK:STDERR: fail_conflicting_bad_impls.carbon:[[@LINE+11]]:12: error: name `invalid` not found [NameNotFound]
+// CHECK:STDERR: final impl invalid as I {}
+// CHECK:STDERR:            ^~~~~~~
+// CHECK:STDERR:
+// CHECK:STDERR: fail_conflicting_bad_impls.carbon:[[@LINE+7]]:1: error: redefinition of `impl <error> as I` [ImplRedefinition]
+// CHECK:STDERR: final impl invalid as I {}
+// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~
+// CHECK:STDERR: fail_conflicting_bad_impls.carbon:[[@LINE-8]]:1: note: previous definition was here [ImplPreviousDefinition]
+// CHECK:STDERR: final impl invalid as I {}
+// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~
+// CHECK:STDERR:
+final impl invalid as I {}
+
+// CHECK:STDERR: fail_conflicting_bad_impls.carbon:[[@LINE+4]]:12: error: name `alsoinvalid` not found [NameNotFound]
+// CHECK:STDERR: final impl alsoinvalid as I {}
+// CHECK:STDERR:            ^~~~~~~~~~~
+// CHECK:STDERR:
+final impl alsoinvalid as I {}
+
+// CHECK:STDERR: fail_conflicting_bad_impls.carbon:[[@LINE+4]]:14: error: name `invalid` not found [NameNotFound]
+// CHECK:STDERR: final impl C(invalid) as I {}
+// CHECK:STDERR:              ^~~~~~~
+// CHECK:STDERR:
+final impl C(invalid) as I {}
+// CHECK:STDERR: fail_conflicting_bad_impls.carbon:[[@LINE+11]]:14: error: name `invalid` not found [NameNotFound]
+// CHECK:STDERR: final impl C(invalid) as I {}
+// CHECK:STDERR:              ^~~~~~~
+// CHECK:STDERR:
+// CHECK:STDERR: fail_conflicting_bad_impls.carbon:[[@LINE+7]]:1: error: redefinition of `impl <error> as I` [ImplRedefinition]
+// CHECK:STDERR: final impl C(invalid) as I {}
+// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
+// CHECK:STDERR: fail_conflicting_bad_impls.carbon:[[@LINE-8]]:1: note: previous definition was here [ImplPreviousDefinition]
+// CHECK:STDERR: final impl C(invalid) as I {}
+// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
+// CHECK:STDERR:
+final impl C(invalid) as I {}
+
 // CHECK:STDOUT: --- basic.carbon
 // CHECK:STDOUT:
 // CHECK:STDOUT: constants {

toolchain/check/type_structure.cpp

@@ -155,7 +155,8 @@ class TypeStructureBuilder {
   explicit TypeStructureBuilder(Context* context) : context_(context) {}
 
   auto Run(SemIR::InstId self_inst_id,
-           SemIR::SpecificInterface interface_constraint) -> TypeStructure {
+           SemIR::SpecificInterface interface_constraint)
+      -> std::optional<TypeStructure> {
     structure_.clear();
     symbolic_type_indices_.clear();
     concrete_types_.clear();
@@ -172,7 +173,7 @@ class TypeStructureBuilder {
   }
 
  private:
-  auto Build(SemIR::TypeIterator type_iter) -> TypeStructure;
+  auto Build(SemIR::TypeIterator type_iter) -> std::optional<TypeStructure>;
 
   // Append a structural element to the TypeStructure being built.
   auto AppendStructuralConcrete(TypeStructure::ConcreteType type) -> void {
@@ -202,7 +203,7 @@ class TypeStructureBuilder {
 
 // Builds the type structure and returns it.
 auto TypeStructureBuilder::Build(SemIR::TypeIterator type_iter)
-    -> TypeStructure {
+    -> std::optional<TypeStructure> {
   while (true) {
     using Step = SemIR::TypeIterator::Step;
     CARBON_KIND_SWITCH(type_iter.Next().any) {
@@ -217,6 +218,9 @@ auto TypeStructureBuilder::Build(SemIR::TypeIterator type_iter)
         AppendStructuralConcreteCloseParen();
         break;
       }
+      case CARBON_KIND(Step::Error _): {
+        return std::nullopt;
+      }
       case CARBON_KIND(Step::ConcreteType concrete): {
         AppendStructuralConcrete(concrete.type_id);
         break;
@@ -291,7 +295,8 @@ auto TypeStructureBuilder::Build(SemIR::TypeIterator type_iter)
 }
 
 auto BuildTypeStructure(Context& context, SemIR::InstId self_inst_id,
-                        SemIR::SpecificInterface interface) -> TypeStructure {
+                        SemIR::SpecificInterface interface)
+    -> std::optional<TypeStructure> {
   TypeStructureBuilder builder(&context);
   return builder.Run(self_inst_id, interface);
 }

toolchain/check/type_structure.h

@@ -180,8 +180,12 @@ class TypeStructure : public Printable<TypeStructure> {
 //
 // Given `impl C as Z {}` the `self_const_id` would be a `C` and the interface
 // constraint would be `Z`.
+//
+// Returns nullopt if an ErrorInst is encountered in the self type or facet
+// value.
 auto BuildTypeStructure(Context& context, SemIR::InstId self_inst_id,
-                        SemIR::SpecificInterface interface) -> TypeStructure;
+                        SemIR::SpecificInterface interface)
+    -> std::optional<TypeStructure>;
 
 }  // namespace Carbon::Check
 

toolchain/sem_ir/type_iterator.cpp

@@ -79,7 +79,6 @@ auto TypeIterator::Next() -> Step {
 
       case SemIR::AssociatedEntityType::Kind:
       case SemIR::BoolType::Kind:
-      case SemIR::ErrorInst::Kind:
       case SemIR::FacetType::Kind:
       case SemIR::FloatType::Kind:
       case SemIR::FunctionType::Kind:
@@ -167,6 +166,10 @@ auto TypeIterator::Next() -> Step {
           return Step::StructStart{.type_id = type_id};
         }
       }
+
+      case SemIR::ErrorInst::Kind:
+        return Step::Error();
+
       default:
         CARBON_FATAL("Unhandled type instruction {0}", inst_id);
     }

toolchain/sem_ir/type_iterator.h

@@ -187,14 +187,16 @@ class TypeIterator::Step {
   struct End {};
   // Iteration is complete.
   struct Done {};
+  // Iteration found an error.
+  struct Error {};
 
   // Each step is one of these.
   using Any =
       std::variant<ConcreteType, SymbolicType, TemplateType, ConcreteValue,
                    SymbolicValue, StructFieldName, ClassStartOnly,
                    StructStartOnly, TupleStartOnly, InterfaceStartOnly,
                    ClassStart, StructStart, TupleStart, InterfaceStart,
-                   IntStart, ArrayStart, PointerStart, End, Done>;
+                   IntStart, ArrayStart, PointerStart, End, Done, Error>;
 
   template <class T>
   auto Is() const -> bool {