Change SemIR formed for 'as' errors (#5792)

Direct use of an error means we can delay looking at the actual type,
and also use the `self_id` of the stored `ImplInfo` directly.

Note, I'm assuming we should be okay with this not always being a
`NameRef`. In error cases though, it seems like we shouldn't mind it
being an error instead of a name reference to an error.

toolchain/check/handle_impl.cpp

@@ -98,26 +98,28 @@ static auto GetDefaultSelfType(Context& context) -> SemIR::TypeId {
 
 auto HandleParseNode(Context& context, Parse::DefaultSelfImplAsId node_id)
     -> bool {
-  auto self_type_id = GetDefaultSelfType(context);
-  if (!self_type_id.has_value()) {
+  auto self_inst_id = SemIR::TypeInstId::None;
+
+  if (auto self_type_id = GetDefaultSelfType(context);
+      self_type_id.has_value()) {
+    // Build the implicit access to the enclosing `Self`.
+    // TODO: Consider calling `HandleNameAsExpr` to build this implicit `Self`
+    // expression. We've already done the work to check that the enclosing
+    // context is a class and found its `Self`, so additionally performing an
+    // unqualified name lookup would be redundant work, but would avoid
+    // duplicating the handling of the `Self` expression.
+    self_inst_id = AddTypeInst(
+        context, node_id,
+        SemIR::NameRef{.type_id = SemIR::TypeType::TypeId,
+                       .name_id = SemIR::NameId::SelfType,
+                       .value_id = context.types().GetInstId(self_type_id)});
+  } else {
     CARBON_DIAGNOSTIC(ImplAsOutsideClass, Error,
                       "`impl as` can only be used in a class");
     context.emitter().Emit(node_id, ImplAsOutsideClass);
-    self_type_id = SemIR::ErrorInst::TypeId;
+    self_inst_id = SemIR::ErrorInst::TypeInstId;
   }
 
-  // Build the implicit access to the enclosing `Self`.
-  // TODO: Consider calling `HandleNameAsExpr` to build this implicit `Self`
-  // expression. We've already done the work to check that the enclosing context
-  // is a class and found its `Self`, so additionally performing an unqualified
-  // name lookup would be redundant work, but would avoid duplicating the
-  // handling of the `Self` expression.
-  auto self_inst_id = AddTypeInst(
-      context, node_id,
-      SemIR::NameRef{.type_id = SemIR::TypeType::TypeId,
-                     .name_id = SemIR::NameId::SelfType,
-                     .value_id = context.types().GetInstId(self_type_id)});
-
   // There's no need to push `Self` into scope here, because we can find it in
   // the parent class scope.
   context.node_stack().Push(node_id, self_inst_id);
@@ -397,7 +399,6 @@ static auto BuildImplDecl(Context& context, Parse::AnyImplDeclId node_id,
       context.node_stack().PopExprWithNodeId();
   auto [self_type_node, self_type_inst_id] =
       context.node_stack().PopWithNodeId<Parse::NodeCategory::ImplAs>();
-  auto self_type_id = context.types().GetTypeIdForTypeInstId(self_type_inst_id);
   // Pop the `impl` introducer and any `forall` parameters as a "name".
   auto name = PopImplIntroducerAndParamsAsNameComponent(context, node_id);
   auto decl_block_id = context.inst_block_stack().Pop();
@@ -502,24 +503,26 @@ static auto BuildImplDecl(Context& context, Parse::AnyImplDeclId node_id,
   ReplaceInstBeforeConstantUse(context, impl_decl_id, impl_decl);
 
   // For an `extend impl` declaration, mark the impl as extending this `impl`.
-  if (self_type_id != SemIR::ErrorInst::TypeId &&
-      introducer.modifier_set.HasAnyOf(KeywordModifierSet::Extend)) {
+  if (introducer.modifier_set.HasAnyOf(KeywordModifierSet::Extend)) {
     auto& stored_impl_info = context.impls().Get(impl_decl.impl_id);
-
-    auto extend_node = introducer.modifier_node_id(ModifierOrder::Extend);
-    if (stored_impl_info.generic_id.has_value()) {
-      constraint_type_inst_id = AddTypeInst<SemIR::SpecificConstant>(
-          context, SemIR::LocId(constraint_type_inst_id),
-          {.type_id = SemIR::TypeType::TypeId,
-           .inst_id = constraint_type_inst_id,
-           .specific_id = context.generics().GetSelfSpecific(
-               stored_impl_info.generic_id)});
-    }
-    if (!ExtendImpl(context, extend_node, node_id, impl_decl.impl_id,
-                    self_type_node, self_type_id, name.implicit_params_loc_id,
-                    constraint_type_inst_id, constraint_type_id)) {
-      // Don't allow the invalid impl to be used.
-      FillImplWitnessWithErrors(context, stored_impl_info);
+    auto self_type_id =
+        context.types().GetTypeIdForTypeInstId(stored_impl_info.self_id);
+    if (self_type_id != SemIR::ErrorInst::TypeId) {
+      auto extend_node = introducer.modifier_node_id(ModifierOrder::Extend);
+      if (stored_impl_info.generic_id.has_value()) {
+        constraint_type_inst_id = AddTypeInst<SemIR::SpecificConstant>(
+            context, SemIR::LocId(constraint_type_inst_id),
+            {.type_id = SemIR::TypeType::TypeId,
+             .inst_id = constraint_type_inst_id,
+             .specific_id = context.generics().GetSelfSpecific(
+                 stored_impl_info.generic_id)});
+      }
+      if (!ExtendImpl(context, extend_node, node_id, impl_decl.impl_id,
+                      self_type_node, self_type_id, name.implicit_params_loc_id,
+                      constraint_type_inst_id, constraint_type_id)) {
+        // Don't allow the invalid impl to be used.
+        FillImplWitnessWithErrors(context, stored_impl_info);
+      }
     }
   }
 

toolchain/check/testdata/impl/extend_impl.carbon

@@ -3,8 +3,6 @@
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 // INCLUDE-FILE: toolchain/testing/testdata/min_prelude/none.carbon
-// TODO: Add ranges and switch to "--dump-sem-ir-ranges=only".
-// EXTRA-ARGS: --dump-sem-ir-ranges=if-present
 //
 // AUTOUPDATE
 // TIP: To test this file alone, run:
@@ -12,71 +10,66 @@
 // TIP: To dump output, run:
 // TIP:   bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/check/testdata/impl/extend_impl.carbon
 
+// --- extend_impl.carbon
+
+library "[[@TEST_NAME]]";
+
 interface HasF {
   fn F();
 }
 
+//@dump-sem-ir-begin
 class C {
   extend impl as HasF {
     fn F() {}
   }
 }
+//@dump-sem-ir-end
 
 fn G(c: C) {
   C.F();
   c.F();
 }
 
+// --- fail_extend_impl_nonexistent.carbon
+
+library "[[@TEST_NAME]]";
+
+interface I {}
+
+class C {
+  // CHECK:STDERR: fail_extend_impl_nonexistent.carbon:[[@LINE+4]]:15: error: name `nonexistent` not found [NameNotFound]
+  // CHECK:STDERR:   extend impl nonexistent* as I {}
+  // CHECK:STDERR:               ^~~~~~~~~~~
+  // CHECK:STDERR:
+  extend impl nonexistent* as I {}
+}
+
+// --- fail_extend_impl_nonexistent_outside_class.carbon
+
+library "[[@TEST_NAME]]";
+
+interface I {}
+// CHECK:STDERR: fail_extend_impl_nonexistent_outside_class.carbon:[[@LINE+4]]:13: error: name `nonexistent` not found [NameNotFound]
+// CHECK:STDERR: extend impl nonexistent* as I {}
+// CHECK:STDERR:             ^~~~~~~~~~~
+// CHECK:STDERR:
+extend impl nonexistent* as I {}
+
 // CHECK:STDOUT: --- extend_impl.carbon
 // CHECK:STDOUT:
 // CHECK:STDOUT: constants {
 // CHECK:STDOUT:   %HasF.type: type = facet_type <@HasF> [concrete]
-// CHECK:STDOUT:   %Self: %HasF.type = bind_symbolic_name Self, 0 [symbolic]
-// CHECK:STDOUT:   %F.type.b7b: type = fn_type @F.1 [concrete]
-// CHECK:STDOUT:   %empty_tuple.type: type = tuple_type () [concrete]
-// CHECK:STDOUT:   %F.f50: %F.type.b7b = struct_value () [concrete]
-// CHECK:STDOUT:   %HasF.assoc_type: type = assoc_entity_type @HasF [concrete]
-// CHECK:STDOUT:   %assoc0: %HasF.assoc_type = assoc_entity element0, @HasF.%F.decl [concrete]
 // CHECK:STDOUT:   %C: type = class_type @C [concrete]
 // CHECK:STDOUT:   %HasF.impl_witness: <witness> = impl_witness @C.%HasF.impl_witness_table [concrete]
 // CHECK:STDOUT:   %F.type.a65: type = fn_type @F.2 [concrete]
 // CHECK:STDOUT:   %F.ad8: %F.type.a65 = struct_value () [concrete]
-// CHECK:STDOUT:   %HasF.facet: %HasF.type = facet_value %C, (%HasF.impl_witness) [concrete]
 // CHECK:STDOUT:   %empty_struct_type: type = struct_type {} [concrete]
 // CHECK:STDOUT:   %complete_type: <witness> = complete_type_witness %empty_struct_type [concrete]
-// CHECK:STDOUT:   %pattern_type: type = pattern_type %C [concrete]
-// CHECK:STDOUT:   %G.type: type = fn_type @G [concrete]
-// CHECK:STDOUT:   %G: %G.type = struct_value () [concrete]
-// CHECK:STDOUT:   %.a44: type = fn_type_with_self_type %F.type.b7b, %HasF.facet [concrete]
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
 // CHECK:STDOUT: file {
-// CHECK:STDOUT:   package: <namespace> = namespace [concrete] {
-// CHECK:STDOUT:     .HasF = %HasF.decl
-// CHECK:STDOUT:     .C = %C.decl
-// CHECK:STDOUT:     .G = %G.decl
-// CHECK:STDOUT:   }
-// CHECK:STDOUT:   %HasF.decl: type = interface_decl @HasF [concrete = constants.%HasF.type] {} {}
 // CHECK:STDOUT:   %C.decl: type = class_decl @C [concrete = constants.%C] {} {}
-// CHECK:STDOUT:   %G.decl: %G.type = fn_decl @G [concrete = constants.%G] {
-// CHECK:STDOUT:     %c.patt: %pattern_type = binding_pattern c [concrete]
-// CHECK:STDOUT:     %c.param_patt: %pattern_type = value_param_pattern %c.patt, call_param0 [concrete]
-// CHECK:STDOUT:   } {
-// CHECK:STDOUT:     %c.param: %C = value_param call_param0
-// CHECK:STDOUT:     %C.ref.loc25: type = name_ref C, file.%C.decl [concrete = constants.%C]
-// CHECK:STDOUT:     %c: %C = bind_name c, %c.param
-// CHECK:STDOUT:   }
-// CHECK:STDOUT: }
-// CHECK:STDOUT:
-// CHECK:STDOUT: interface @HasF {
-// CHECK:STDOUT:   %Self: %HasF.type = bind_symbolic_name Self, 0 [symbolic = constants.%Self]
-// CHECK:STDOUT:   %F.decl: %F.type.b7b = fn_decl @F.1 [concrete = constants.%F.f50] {} {}
-// CHECK:STDOUT:   %assoc0: %HasF.assoc_type = assoc_entity element0, %F.decl [concrete = constants.%assoc0]
-// CHECK:STDOUT:
-// CHECK:STDOUT: !members:
-// CHECK:STDOUT:   .Self = %Self
-// CHECK:STDOUT:   .F = %assoc0
-// CHECK:STDOUT:   witness = (%F.decl)
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
 // CHECK:STDOUT: impl @impl: %Self.ref as %HasF.ref {
@@ -105,29 +98,8 @@ fn G(c: C) {
 // CHECK:STDOUT:   extend @impl.%HasF.ref
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
-// CHECK:STDOUT: generic fn @F.1(@HasF.%Self: %HasF.type) {
-// CHECK:STDOUT:   fn();
-// CHECK:STDOUT: }
-// CHECK:STDOUT:
 // CHECK:STDOUT: fn @F.2() {
 // CHECK:STDOUT: !entry:
 // CHECK:STDOUT:   return
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
-// CHECK:STDOUT: fn @G(%c.param: %C) {
-// CHECK:STDOUT: !entry:
-// CHECK:STDOUT:   %C.ref.loc26: type = name_ref C, file.%C.decl [concrete = constants.%C]
-// CHECK:STDOUT:   %F.ref.loc26: %HasF.assoc_type = name_ref F, @HasF.%assoc0 [concrete = constants.%assoc0]
-// CHECK:STDOUT:   %impl.elem0.loc26: %.a44 = impl_witness_access constants.%HasF.impl_witness, element0 [concrete = constants.%F.ad8]
-// CHECK:STDOUT:   %F.call.loc26: init %empty_tuple.type = call %impl.elem0.loc26()
-// CHECK:STDOUT:   %c.ref: %C = name_ref c, %c
-// CHECK:STDOUT:   %F.ref.loc27: %HasF.assoc_type = name_ref F, @HasF.%assoc0 [concrete = constants.%assoc0]
-// CHECK:STDOUT:   %impl.elem0.loc27: %.a44 = impl_witness_access constants.%HasF.impl_witness, element0 [concrete = constants.%F.ad8]
-// CHECK:STDOUT:   %F.call.loc27: init %empty_tuple.type = call %impl.elem0.loc27()
-// CHECK:STDOUT:   return
-// CHECK:STDOUT: }
-// CHECK:STDOUT:
-// CHECK:STDOUT: specific @F.1(constants.%Self) {}
-// CHECK:STDOUT:
-// CHECK:STDOUT: specific @F.1(constants.%HasF.facet) {}
-// CHECK:STDOUT:

toolchain/check/testdata/impl/fail_extend_impl_scope.carbon

@@ -220,7 +220,6 @@ fn F() {
 // CHECK:STDOUT:   %Zero.decl: %Zero.type.822 = fn_decl @Zero.1 [concrete = constants.%Zero.d14] {} {}
 // CHECK:STDOUT:   %assoc0: %Z.assoc_type = assoc_entity element0, %Zero.decl [concrete = constants.%assoc0.534]
 // CHECK:STDOUT:   impl_decl @impl.6b5 [concrete] {} {
-// CHECK:STDOUT:     %Self.ref: type = name_ref Self, <error> [concrete = <error>]
 // CHECK:STDOUT:     %Z.ref: type = name_ref Z, file.%Z.decl [concrete = constants.%Z.type]
 // CHECK:STDOUT:   }
 // CHECK:STDOUT:
@@ -237,7 +236,7 @@ fn F() {
 // CHECK:STDOUT:   %Zero.type: type = fn_type @Zero.2, @impl.6b5(%Self) [symbolic = %Zero.type (constants.%Zero.type.db4)]
 // CHECK:STDOUT:   %Zero: @impl.6b5.%Zero.type (%Zero.type.db4) = struct_value () [symbolic = %Zero (constants.%Zero.8fb)]
 // CHECK:STDOUT:
-// CHECK:STDOUT:   impl: %Self.ref as %Z.ref {
+// CHECK:STDOUT:   impl: <error> as %Z.ref {
 // CHECK:STDOUT:     %Zero.decl: @impl.6b5.%Zero.type (%Zero.type.db4) = fn_decl @Zero.2 [symbolic = @impl.6b5.%Zero (constants.%Zero.8fb)] {} {}
 // CHECK:STDOUT:
 // CHECK:STDOUT:   !members:

toolchain/check/testdata/impl/fail_impl_as_scope.carbon

@@ -129,7 +129,6 @@ class X {
 // CHECK:STDOUT:   %I.decl: type = interface_decl @I [concrete = constants.%I.type] {} {}
 // CHECK:STDOUT:   %G.decl: %G.type = fn_decl @G [concrete = constants.%G] {} {}
 // CHECK:STDOUT:   impl_decl @impl [concrete] {} {
-// CHECK:STDOUT:     %Self.ref: type = name_ref Self, <error> [concrete = <error>]
 // CHECK:STDOUT:     %I.ref: type = name_ref I, file.%I.decl [concrete = constants.%I.type]
 // CHECK:STDOUT:   }
 // CHECK:STDOUT:   %F.decl: %F.type = fn_decl @F [concrete = constants.%F] {} {}
@@ -143,7 +142,7 @@ class X {
 // CHECK:STDOUT:   witness = ()
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
-// CHECK:STDOUT: impl @impl: %Self.ref as %I.ref {
+// CHECK:STDOUT: impl @impl: <error> as %I.ref {
 // CHECK:STDOUT: !members:
 // CHECK:STDOUT:   witness = <error>
 // CHECK:STDOUT: }
@@ -200,7 +199,7 @@ class X {
 // CHECK:STDOUT:   witness = ()
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
-// CHECK:STDOUT: impl @impl: %Self.ref as %J.ref {
+// CHECK:STDOUT: impl @impl: <error> as %J.ref {
 // CHECK:STDOUT: !members:
 // CHECK:STDOUT:   witness = <error>
 // CHECK:STDOUT: }
@@ -213,7 +212,6 @@ class X {
 // CHECK:STDOUT: fn @F() {
 // CHECK:STDOUT: !entry:
 // CHECK:STDOUT:   impl_decl @impl [concrete] {} {
-// CHECK:STDOUT:     %Self.ref: type = name_ref Self, <error> [concrete = <error>]
 // CHECK:STDOUT:     %J.ref: type = name_ref J, file.%J.decl [concrete = constants.%J.type]
 // CHECK:STDOUT:   }
 // CHECK:STDOUT:   %G.ref: %G.type = name_ref G, file.%G.decl [concrete = constants.%G]
@@ -303,7 +301,6 @@ class X {
 // CHECK:STDOUT:   }
 // CHECK:STDOUT:   %assoc1: %Z.assoc_type = assoc_entity element1, %Method.decl [concrete = constants.%assoc1]
 // CHECK:STDOUT:   impl_decl @impl.6b5 [concrete] {} {
-// CHECK:STDOUT:     %Self.ref: type = name_ref Self, <error> [concrete = <error>]
 // CHECK:STDOUT:     %Z.ref: type = name_ref Z, file.%Z.decl [concrete = constants.%Z.type]
 // CHECK:STDOUT:   }
 // CHECK:STDOUT:
@@ -323,7 +320,7 @@ class X {
 // CHECK:STDOUT:   %Method.type: type = fn_type @Method.2, @impl.6b5(%Self) [symbolic = %Method.type (constants.%Method.type.163)]
 // CHECK:STDOUT:   %Method: @impl.6b5.%Method.type (%Method.type.163) = struct_value () [symbolic = %Method (constants.%Method.84d)]
 // CHECK:STDOUT:
-// CHECK:STDOUT:   impl: %Self.ref as %Z.ref {
+// CHECK:STDOUT:   impl: <error> as %Z.ref {
 // CHECK:STDOUT:     %Zero.decl: @impl.6b5.%Zero.type (%Zero.type.db4) = fn_decl @Zero.2 [symbolic = @impl.6b5.%Zero (constants.%Zero.8fb)] {} {}
 // CHECK:STDOUT:     %Method.decl: @impl.6b5.%Method.type (%Method.type.163) = fn_decl @Method.2 [symbolic = @impl.6b5.%Method (constants.%Method.84d)] {
 // CHECK:STDOUT:       %self.patt: @Method.2.%pattern_type (%pattern_type.a40) = binding_pattern self [concrete]
@@ -551,7 +548,6 @@ class X {
 // CHECK:STDOUT:
 // CHECK:STDOUT: impl @impl.3b4: %Self.ref as %A.ref {
 // CHECK:STDOUT:   impl_decl @impl.4cc [concrete] {} {
-// CHECK:STDOUT:     %Self.ref: type = name_ref Self, <error> [concrete = <error>]
 // CHECK:STDOUT:     %C.ref: type = name_ref C, file.%C.decl [concrete = constants.%C.type]
 // CHECK:STDOUT:   }
 // CHECK:STDOUT:   %B.decl: %B.type.d47 = fn_decl @B.2 [concrete = constants.%B.4af] {} {}
@@ -563,7 +559,7 @@ class X {
 // CHECK:STDOUT:   witness = @X.%A.impl_witness
 // CHECK:STDOUT: }
 // CHECK:STDOUT:
-// CHECK:STDOUT: impl @impl.4cc: %Self.ref as %C.ref {
+// CHECK:STDOUT: impl @impl.4cc: <error> as %C.ref {
 // CHECK:STDOUT: !members:
 // CHECK:STDOUT:   witness = <error>
 // CHECK:STDOUT: }