Consolidate error handling behaviour for `ApplyExtendImplAs` (Refactor Impl construction 4/7) (#6468)

Propagate error state in an `extend impl` declaration out to the
enclosing scope. We can do this generically in `ApplyExtendImplAs` so we
don't have to do it explicitly in other places.

Collapse `DiagnoseExtendImplOutsideClass` into `ApplyExtendImplAs` as it
had only the one caller and is very small, so this simplifies the code,
making `ApplyExtendImplAs` a clear set of diagnostics. And push the
construction of the SpecificConstant down into `ApplyExtendImplAs` so it
is only constructed if it's needed, instead of constructing it and
throwing it away in error cases.

Ensure any error in the declaration results in the witness being an
ErrorInst so the impl will not be used in impl lookup. This simplifies
some branches by combining them into a single if statement.

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

toolchain/check/handle_impl.cpp

@@ -94,7 +94,6 @@ auto HandleParseNode(Context& context, Parse::ImplTypeAsId node_id) -> bool {
       } else if (self_type.type_id != SemIR::ErrorInst::TypeId) {
         // Otherwise, the self-type is an error.
         diag.Emit();
-        class_scope->name_scope->set_has_error();
         self_type.inst_id = SemIR::ErrorInst::TypeInstId;
       }
     }

toolchain/check/impl.cpp

@@ -16,6 +16,7 @@
 #include "toolchain/check/interface.h"
 #include "toolchain/check/merge.h"
 #include "toolchain/check/name_lookup.h"
+#include "toolchain/check/name_scope.h"
 #include "toolchain/check/thunk.h"
 #include "toolchain/check/type.h"
 #include "toolchain/check/type_completion.h"
@@ -377,48 +378,35 @@ static auto IsValidImplRedecl(Context& context, SemIR::Impl& new_impl,
   return true;
 }
 
-static auto DiagnoseExtendImplOutsideClass(Context& context,
-                                           SemIR::LocId loc_id) -> void {
-  CARBON_DIAGNOSTIC(ExtendImplOutsideClass, Error,
-                    "`extend impl` can only be used in a class");
-  context.emitter().Emit(loc_id, ExtendImplOutsideClass);
-}
-
-// If the specified name scope corresponds to a class, returns the corresponding
-// class declaration.
-// TODO: Should this be somewhere more central?
-static auto TryAsClassScope(Context& context, SemIR::NameScopeId scope_id)
-    -> std::optional<SemIR::ClassDecl> {
-  if (!scope_id.has_value()) {
-    return std::nullopt;
-  }
-  auto& scope = context.name_scopes().Get(scope_id);
-  if (!scope.inst_id().has_value()) {
-    return std::nullopt;
-  }
-  return context.insts().TryGetAs<SemIR::ClassDecl>(scope.inst_id());
-}
-
 // 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, Parse::NodeId extend_node,
-                              SemIR::LocId loc_id, SemIR::ImplId impl_id,
-                              SemIR::LocId implicit_params_loc_id,
-                              SemIR::TypeInstId constraint_type_inst_id)
-    -> bool {
+static auto ApplyExtendImplAs(Context& context, SemIR::LocId loc_id,
+                              const SemIR::Impl& impl,
+                              Parse::NodeId extend_node,
+                              SemIR::LocId implicit_params_loc_id) -> bool {
   auto parent_scope_id = context.decl_name_stack().PeekParentScopeId();
-  if (!parent_scope_id.has_value()) {
-    DiagnoseExtendImplOutsideClass(context, loc_id);
+
+  // TODO: Also handle the parent scope being a mixin or an interface.
+  auto class_scope = TryAsClassScope(context, parent_scope_id);
+  if (!class_scope) {
+    if (impl.witness_id != SemIR::ErrorInst::InstId) {
+      CARBON_DIAGNOSTIC(
+          ExtendImplOutsideClass, Error,
+          "`extend impl` can only be used in an interface or class");
+      context.emitter().Emit(loc_id, ExtendImplOutsideClass);
+    }
     return false;
   }
-  // TODO: This is also valid in a mixin.
-  if (!TryAsClassScope(context, parent_scope_id)) {
-    DiagnoseExtendImplOutsideClass(context, loc_id);
+
+  auto& parent_scope = *class_scope->name_scope;
+
+  // An error was already diagnosed, but this is `extend impl as` inside a
+  // class, so propagate the error into the enclosing class scope.
+  if (impl.witness_id == SemIR::ErrorInst::InstId) {
+    parent_scope.set_has_error();
     return false;
   }
 
-  auto& parent_scope = context.name_scopes().Get(parent_scope_id);
-
   if (implicit_params_loc_id.has_value()) {
     CARBON_DIAGNOSTIC(ExtendImplForall, Error,
                       "cannot `extend` a parameterized `impl`");
@@ -427,30 +415,30 @@ static auto ApplyExtendImplAs(Context& context, Parse::NodeId extend_node,
     return false;
   }
 
-  const auto& impl = context.impls().Get(impl_id);
-
-  if (impl.witness_id == SemIR::ErrorInst::InstId) {
+  if (!RequireCompleteType(
+          context, context.types().GetTypeIdForTypeInstId(impl.constraint_id),
+          SemIR::LocId(impl.constraint_id), [&] {
+            CARBON_DIAGNOSTIC(ExtendImplAsIncomplete, Error,
+                              "`extend impl as` incomplete facet type {0}",
+                              InstIdAsType);
+            return context.emitter().Build(impl.latest_decl_id(),
+                                           ExtendImplAsIncomplete,
+                                           impl.constraint_id);
+          })) {
     parent_scope.set_has_error();
-  } else {
-    auto constraint_type_id =
-        context.types().GetTypeIdForTypeInstId(constraint_type_inst_id);
-    bool is_complete = RequireCompleteType(
-        context, constraint_type_id, SemIR::LocId(constraint_type_inst_id),
-        [&] {
-          CARBON_DIAGNOSTIC(ExtendImplAsIncomplete, Error,
-                            "`extend impl as` incomplete facet type {0}",
-                            InstIdAsType);
-          return context.emitter().Build(impl.latest_decl_id(),
-                                         ExtendImplAsIncomplete,
-                                         constraint_type_inst_id);
-        });
-    if (!is_complete) {
-      parent_scope.set_has_error();
-      return false;
-    }
+    return false;
   }
 
-  parent_scope.AddExtendedScope(constraint_type_inst_id);
+  if (!impl.generic_id.has_value()) {
+    parent_scope.AddExtendedScope(impl.constraint_id);
+  } else {
+    auto constraint_id_in_self_specific = AddTypeInst<SemIR::SpecificConstant>(
+        context, SemIR::LocId(impl.constraint_id),
+        {.type_id = SemIR::TypeType::TypeId,
+         .inst_id = impl.constraint_id,
+         .specific_id = context.generics().GetSelfSpecific(impl.generic_id)});
+    parent_scope.AddExtendedScope(constraint_id_in_self_specific);
+  }
   return true;
 }
 
@@ -521,16 +509,24 @@ auto GetOrAddImpl(Context& context, SemIR::LocId loc_id,
   // `Impl`.
 
   impl.generic_id = BuildGeneric(context, impl.latest_decl_id());
+
+  // Due to lack of an instruction to set to `ErrorInst`, an `InterfaceId::None`
+  // indicates that the interface could not be identified and an error was
+  // diagnosed. If there's any error in the construction of the impl, then the
+  // witness can't be constructed. We set it to `ErrorInst` to make the impl
+  // unusable for impl lookup.
+  if (!impl.interface.interface_id.has_value() ||
+      impl.self_id == SemIR::ErrorInst::TypeInstId ||
+      impl.constraint_id == SemIR::ErrorInst::TypeInstId) {
+    impl.witness_id = SemIR::ErrorInst::InstId;
+    // TODO: We might also want to mark that the name scope for the impl has an
+    // error -- at least once we start making name lookups within the impl also
+    // look into the facet (eg, so you can name associated constants from within
+    // the impl).
+  }
+
   if (impl.witness_id != SemIR::ErrorInst::InstId) {
-    if (impl.interface.interface_id.has_value()) {
-      impl.witness_id = ImplWitnessForDeclaration(context, impl, is_definition);
-    } else {
-      impl.witness_id = SemIR::ErrorInst::InstId;
-      // TODO: We might also want to mark that the name scope for the impl has
-      // an error -- at least once we start making name lookups within the
-      // impl also look into the facet (eg, so you can name associated
-      // constants from within the impl).
-    }
+    impl.witness_id = ImplWitnessForDeclaration(context, impl, is_definition);
   }
   FinishGenericDecl(context, SemIR::LocId(impl.latest_decl_id()),
                     impl.generic_id);
@@ -565,23 +561,10 @@ auto GetOrAddImpl(Context& context, SemIR::LocId loc_id,
 
   // For an `extend impl` declaration, mark the impl as extending this `impl`.
   if (extend_node.has_value()) {
-    auto self_type_id =
-        context.types().GetTypeIdForTypeInstId(stored_impl_info.self_id);
-    if (self_type_id != SemIR::ErrorInst::TypeId) {
-      auto constraint_id = impl.constraint_id;
-      if (stored_impl_info.generic_id.has_value()) {
-        constraint_id = AddTypeInst<SemIR::SpecificConstant>(
-            context, SemIR::LocId(constraint_id),
-            {.type_id = SemIR::TypeType::TypeId,
-             .inst_id = constraint_id,
-             .specific_id = context.generics().GetSelfSpecific(
-                 stored_impl_info.generic_id)});
-      }
-      if (!ApplyExtendImplAs(context, extend_node, loc_id, impl_id,
-                             implicit_params_loc_id, constraint_id)) {
-        // Don't allow the invalid impl to be used.
-        FillImplWitnessWithErrors(context, stored_impl_info);
-      }
+    if (!ApplyExtendImplAs(context, loc_id, stored_impl_info, extend_node,
+                           implicit_params_loc_id)) {
+      // Don't allow the invalid impl to be used.
+      FillImplWitnessWithErrors(context, stored_impl_info);
     }
   }
 

toolchain/check/testdata/impl/extend_final.carbon

@@ -48,7 +48,7 @@ interface Z {}
 
 class C {}
 
-// CHECK:STDERR: fail_final_extend_impl_outside_class.carbon:[[@LINE+4]]:1: error: `extend impl` can only be used in a class [ExtendImplOutsideClass]
+// CHECK:STDERR: fail_final_extend_impl_outside_class.carbon:[[@LINE+4]]:1: error: `extend impl` can only be used in an interface or class [ExtendImplOutsideClass]
 // CHECK:STDERR: extend final impl C as Z {}
 // CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~
 // CHECK:STDERR:

toolchain/check/testdata/impl/extend_impl.carbon

@@ -43,6 +43,36 @@ class C {
   // CHECK:STDERR:               ^~~~~~~~~~~
   // CHECK:STDERR:
   extend impl nonexistent as I {}
+
+  fn F() {
+    // The erroneous self-type for an `extend` causes the error to be propagated
+    // into the class scope, which prevents errors if we fail to find a name
+    // in that scope.
+    Self.A;
+  }
+}
+
+// --- fail_impl_nonexistent.carbon
+
+library "[[@TEST_NAME]]";
+
+interface I {}
+
+class C {
+  // CHECK:STDERR: fail_impl_nonexistent.carbon:[[@LINE+4]]:8: error: name `nonexistent` not found [NameNotFound]
+  // CHECK:STDERR:   impl nonexistent as I {}
+  // CHECK:STDERR:        ^~~~~~~~~~~
+  // CHECK:STDERR:
+  impl nonexistent as I {}
+
+  fn F() {
+    // The name lookup error still happens, since the `impl` is not `extend`.
+    // CHECK:STDERR: fail_impl_nonexistent.carbon:[[@LINE+4]]:5: error: member name `A` not found in `C` [MemberNameNotFoundInInstScope]
+    // CHECK:STDERR:     Self.A;
+    // CHECK:STDERR:     ^~~~~~
+    // CHECK:STDERR:
+    Self.A;
+  }
 }
 
 // --- fail_extend_impl_nonexistent_pointer.carbon
@@ -57,6 +87,13 @@ class C {
   // CHECK:STDERR:               ^~~~~~~~~~~
   // CHECK:STDERR:
   extend impl nonexistent* as I {}
+
+  fn F() {
+    // The erroneous self-type for an `extend` causes the error to be propagated
+    // into the class scope, which prevents errors if we fail to find a name
+    // in that scope.
+    Self.A;
+  }
 }
 
 // --- fail_extend_impl_nonexistent_outside_class.carbon

toolchain/check/testdata/impl/fail_extend_impl_forall.carbon

@@ -139,7 +139,6 @@ class C {
 // CHECK:STDOUT:   }
 // CHECK:STDOUT:   %GenericInterface.impl_witness_table = impl_witness_table (<error>), @C.as.GenericInterface.impl [concrete]
 // CHECK:STDOUT:   %GenericInterface.impl_witness: <witness> = impl_witness %GenericInterface.impl_witness_table, @C.as.GenericInterface.impl(constants.%T) [symbolic = @C.as.GenericInterface.impl.%GenericInterface.impl_witness (constants.%GenericInterface.impl_witness)]
-// CHECK:STDOUT:   %.loc24: type = specific_constant @C.as.GenericInterface.impl.%GenericInterface.type.loc24_54.2, @C.as.GenericInterface.impl(constants.%T) [symbolic = constants.%GenericInterface.type.6bb]
 // CHECK:STDOUT:   %complete_type: <witness> = complete_type_witness constants.%empty_struct_type [concrete = constants.%complete_type]
 // CHECK:STDOUT:   complete_type_witness = %complete_type
 // CHECK:STDOUT:

toolchain/check/testdata/impl/fail_extend_impl_scope.carbon

@@ -17,7 +17,7 @@ library "[[@TEST_NAME]]";
 
 interface I {}
 
-// CHECK:STDERR: fail_extend_impl_file_scope.carbon:[[@LINE+4]]:1: error: `extend impl` can only be used in a class [ExtendImplOutsideClass]
+// CHECK:STDERR: fail_extend_impl_file_scope.carbon:[[@LINE+4]]:1: error: `extend impl` can only be used in an interface or class [ExtendImplOutsideClass]
 // CHECK:STDERR: extend impl () as I {}
 // CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~
 // CHECK:STDERR:
@@ -29,7 +29,7 @@ library "[[@TEST_NAME]]";
 interface J {}
 
 fn F() {
-  // CHECK:STDERR: fail_extend_impl_function_scope.carbon:[[@LINE+4]]:3: error: `extend impl` can only be used in a class [ExtendImplOutsideClass]
+  // CHECK:STDERR: fail_extend_impl_function_scope.carbon:[[@LINE+4]]:3: error: `extend impl` can only be used in an interface or class [ExtendImplOutsideClass]
   // CHECK:STDERR:   extend impl {} as J {}
   // CHECK:STDERR:   ^~~~~~~~~~~~~~~~~~~~~
   // CHECK:STDERR:

toolchain/check/testdata/impl/fail_extend_non_interface.carbon

@@ -63,7 +63,6 @@ class C {
 // CHECK:STDOUT:
 // CHECK:STDOUT: !members:
 // CHECK:STDOUT:   .Self = constants.%C
-// CHECK:STDOUT:   extend @C.as.<error>.impl.%i32
 // CHECK:STDOUT:   has_error
 // CHECK:STDOUT: }
 // CHECK:STDOUT:

toolchain/check/testdata/impl/fail_extend_partially_defined_interface.carbon

@@ -74,7 +74,6 @@ interface I {
 // CHECK:STDOUT:     }
 // CHECK:STDOUT:     %I.impl_witness_table = impl_witness_table (), @C.as.I.impl [concrete]
 // CHECK:STDOUT:     %I.impl_witness: <witness> = impl_witness %I.impl_witness_table, @C.as.I.impl(constants.%Self) [symbolic = @C.as.I.impl.%I.impl_witness (constants.%I.impl_witness)]
-// CHECK:STDOUT:     %.loc24: type = specific_constant @C.as.I.impl.%I.ref, @C.as.I.impl(constants.%Self) [concrete = constants.%I.type]
 // CHECK:STDOUT:     %complete_type: <witness> = complete_type_witness constants.%empty_struct_type [concrete = constants.%complete_type]
 // CHECK:STDOUT:     complete_type_witness = %complete_type
 // CHECK:STDOUT: