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: