From 2bb8e9884988ef0724eb97e10962d616ea35d278 Mon Sep 17 00:00:00 2001 From: Jon Ross-Perkins Date: Thu, 10 Jul 2025 14:56:03 -0700 Subject: [PATCH] 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 | 69 +++++++------- .../check/testdata/impl/extend_impl.carbon | 90 +++++++------------ .../impl/fail_extend_impl_scope.carbon | 3 +- .../testdata/impl/fail_impl_as_scope.carbon | 12 +-- 4 files changed, 72 insertions(+), 102 deletions(-) diff --git a/toolchain/check/handle_impl.cpp b/toolchain/check/handle_impl.cpp index 9fb1f17e184a..505bea2b2b2e 100644 --- a/toolchain/check/handle_impl.cpp +++ b/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(); - 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( - 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( + 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); + } } } diff --git a/toolchain/check/testdata/impl/extend_impl.carbon b/toolchain/check/testdata/impl/extend_impl.carbon index 54fb1683d875..76ff4504128a 100644 --- a/toolchain/check/testdata/impl/extend_impl.carbon +++ b/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: = 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: = 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 [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: diff --git a/toolchain/check/testdata/impl/fail_extend_impl_scope.carbon b/toolchain/check/testdata/impl/fail_extend_impl_scope.carbon index 47d2d4a8d93a..2f0478092f01 100644 --- a/toolchain/check/testdata/impl/fail_extend_impl_scope.carbon +++ b/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, [concrete = ] // 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: 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: diff --git a/toolchain/check/testdata/impl/fail_impl_as_scope.carbon b/toolchain/check/testdata/impl/fail_impl_as_scope.carbon index 6bba10213f7e..16c665008cc0 100644 --- a/toolchain/check/testdata/impl/fail_impl_as_scope.carbon +++ b/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, [concrete = ] // 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: as %I.ref { // CHECK:STDOUT: !members: // CHECK:STDOUT: witness = // 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: as %J.ref { // CHECK:STDOUT: !members: // CHECK:STDOUT: witness = // 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, [concrete = ] // 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, [concrete = ] // 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: 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, [concrete = ] // 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: as %C.ref { // CHECK:STDOUT: !members: // CHECK:STDOUT: witness = // CHECK:STDOUT: }