From 14998d6045d7e0e136d0fd36405412c22e3bc4b7 Mon Sep 17 00:00:00 2001 From: Dana Jansens Date: Thu, 11 Dec 2025 14:05:17 -0500 Subject: [PATCH] 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 | 1 - toolchain/check/impl.cpp | 147 ++++++++---------- .../check/testdata/impl/extend_final.carbon | 2 +- .../check/testdata/impl/extend_impl.carbon | 37 +++++ .../impl/fail_extend_impl_forall.carbon | 1 - .../impl/fail_extend_impl_scope.carbon | 4 +- .../impl/fail_extend_non_interface.carbon | 1 - ..._extend_partially_defined_interface.carbon | 1 - 8 files changed, 105 insertions(+), 89 deletions(-) diff --git a/toolchain/check/handle_impl.cpp b/toolchain/check/handle_impl.cpp index ee93d9496443..edc7bbe987eb 100644 --- a/toolchain/check/handle_impl.cpp +++ b/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; } } diff --git a/toolchain/check/impl.cpp b/toolchain/check/impl.cpp index 3af20aef4279..f04977b7dbde 100644 --- a/toolchain/check/impl.cpp +++ b/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,47 +378,34 @@ 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 { - 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(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); - return false; - } - // TODO: This is also valid in a mixin. - if (!TryAsClassScope(context, parent_scope_id)) { - 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; } - auto& parent_scope = context.name_scopes().Get(parent_scope_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; + } if (implicit_params_loc_id.has_value()) { CARBON_DIAGNOSTIC(ExtendImplForall, Error, @@ -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( + 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( - 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); } } diff --git a/toolchain/check/testdata/impl/extend_final.carbon b/toolchain/check/testdata/impl/extend_final.carbon index 52554a44165c..d56a50362420 100644 --- a/toolchain/check/testdata/impl/extend_final.carbon +++ b/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: diff --git a/toolchain/check/testdata/impl/extend_impl.carbon b/toolchain/check/testdata/impl/extend_impl.carbon index c28bc3553b8a..795de0a7e054 100644 --- a/toolchain/check/testdata/impl/extend_impl.carbon +++ b/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 diff --git a/toolchain/check/testdata/impl/fail_extend_impl_forall.carbon b/toolchain/check/testdata/impl/fail_extend_impl_forall.carbon index 65a9193d11e0..9a4b9f62fd85 100644 --- a/toolchain/check/testdata/impl/fail_extend_impl_forall.carbon +++ b/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 (), @C.as.GenericInterface.impl [concrete] // CHECK:STDOUT: %GenericInterface.impl_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: = complete_type_witness constants.%empty_struct_type [concrete = constants.%complete_type] // CHECK:STDOUT: complete_type_witness = %complete_type // 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 31fddae52282..2242e64cac41 100644 --- a/toolchain/check/testdata/impl/fail_extend_impl_scope.carbon +++ b/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: diff --git a/toolchain/check/testdata/impl/fail_extend_non_interface.carbon b/toolchain/check/testdata/impl/fail_extend_non_interface.carbon index c2ad8af8899e..0e598c434f2c 100644 --- a/toolchain/check/testdata/impl/fail_extend_non_interface.carbon +++ b/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..impl.%i32 // CHECK:STDOUT: has_error // CHECK:STDOUT: } // CHECK:STDOUT: diff --git a/toolchain/check/testdata/impl/fail_extend_partially_defined_interface.carbon b/toolchain/check/testdata/impl/fail_extend_partially_defined_interface.carbon index 1d633b9cf7a9..2ef11a6e176b 100644 --- a/toolchain/check/testdata/impl/fail_extend_partially_defined_interface.carbon +++ b/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: = 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: = complete_type_witness constants.%empty_struct_type [concrete = constants.%complete_type] // CHECK:STDOUT: complete_type_witness = %complete_type // CHECK:STDOUT: