From e7771c2f6dc2620970b79706f123e780e76a664e Mon Sep 17 00:00:00 2001 From: Dana Jansens Date: Tue, 30 Jun 2026 19:33:30 -0400 Subject: [PATCH] During identify replace `.Self` only in the initial facet type (#7436) When we find a named constraint during identity, we recurse into it. The specific args of the named constraint may contain references to `.Self` which can then make `.Self` appear inside the named constraint, which was making us replace `.Self` at multiple levels and incorrectly. The first specific argument replaces `Self` in the named constraint, and we pass in the self-type of the identify operation. This may contain `.Self` and we should _not_ be replacing the `.Self` references with the self-type that they are contained within. This led to infinite cycles. In the meantime, we have made the toolchain reject any ambiguous `.Self` from being constructed. So we know there is only one value of `.Self` around in a facet type. So now we replace `.Self` only in the top level facet type during identity. That means replacing `.Self` in the specifics of the named constraints that we recurse into. But we do _not_ replace `.Self` anymore inside those named constraints. This resolves the infinite loop. At the same time, when we are identifying an `impls` constraint from earlier in the same facet type, like `C impls Z(.Self)` we are identifying with a self-type of `C`. We want the output to use `C` as the self-type since we should get back an identified facet type that says `C impls Z(.Self)`. But we do _not_ want to replace the `.Self` there since we're inside a facet type and the `.Self` does not refer to `C`. So we parameterize `TryToIdentifyFacetType` to not replace `.Self` when identifying an `impls` constraint from the `where_stack()`. This resolves a large number of `fail_todo_` tests. --- toolchain/check/impl_lookup.cpp | 32 +++---- .../check/testdata/facet/early_impls.carbon | 90 +++++++------------ toolchain/check/type_completion.cpp | 76 ++++++++++------ toolchain/check/type_completion.h | 3 +- 4 files changed, 101 insertions(+), 100 deletions(-) diff --git a/toolchain/check/impl_lookup.cpp b/toolchain/check/impl_lookup.cpp index 92ed1379ec73..bf64194b3aa3 100644 --- a/toolchain/check/impl_lookup.cpp +++ b/toolchain/check/impl_lookup.cpp @@ -372,16 +372,13 @@ static auto CollectFacetWitnessSources( if (type_id != SemIR::TypeType::TypeId) { auto facet_type = context.types().GetAs(type_id); - // When we identify the facet type within a facet in the query, we also - // replace `.Self` with the `facet_const_id`, which has the same type as - // the facet type we're identifying. If that `.Self` replacement is in a - // `LookupImplWitness` instruction, it will evaluate and look for a final - // impl. When we find a generic `final impl` and try to deduce its - // parameters, we may end up trying to convert the `facet_const_id` which - // does an impl lookup and comes back here. If we replace `.Self` again, - // we just cycle indefinitely identifying and substituting - // `facet_const_id` into its type. We break that cycle here by preventing - // `final impl` lookups while replacing `.Self`. + // Identifying the facet type of `facet_const_id` causes `.Self` to be + // replaced with `facet_const_id`. The resulting `LookupImplWitness` + // evaluation searches for a `final impl`. The toolchain needs to deduce + // the result's parameters when a generic `final impl` is found. Deduction + // may convert the `facet_const_id`, which returns here and then we loop + // forever. We break that cycle here by preventing `final impl` lookups + // while replacing `.Self`. // // TODO: This prevents some legitimate code from working, as we can't find // some witnesses that involve concrete associated constants from a @@ -390,11 +387,13 @@ static auto CollectFacetWitnessSources( // facet type, remove that constraint from the type of `facet_const_id`. // Then each cycle back through to `facet_const_id` will have a smaller // type until it runs out of constraints. - context.impl_lookup_no_symbolic_final_lookups()++; - auto identified_id = - TryToIdentifyFacetType(context, loc_id, facet_const_id, facet_type, - allow_partially_identified); - context.impl_lookup_no_symbolic_final_lookups()--; + auto& no_symbolic_final_lookups = + context.impl_lookup_no_symbolic_final_lookups(); + ++no_symbolic_final_lookups; + auto identified_id = TryToIdentifyFacetType( + context, loc_id, facet_const_id, facet_type, + allow_partially_identified, /*subst_period_self=*/true); + --no_symbolic_final_lookups; if (identified_id.has_value()) { witnesses.push_back({.facet_const_id = facet_const_id, @@ -475,7 +474,8 @@ static auto CollectFacetWitnessSources( facet_type_const_id); auto identified_id = TryToIdentifyFacetType( context, loc_id, canon_self_const_id, facet_type, - /*allow_partially_identified=*/true); + /*allow_partially_identified=*/true, + /*subst_period_self=*/false); if (identified_id.has_value()) { witnesses.push_back({.facet_const_id = canon_self_const_id, .identified_facet_type_id = identified_id}); diff --git a/toolchain/check/testdata/facet/early_impls.carbon b/toolchain/check/testdata/facet/early_impls.carbon index a3f2d5297f6f..5b661a7618a4 100644 --- a/toolchain/check/testdata/facet/early_impls.carbon +++ b/toolchain/check/testdata/facet/early_impls.carbon @@ -58,7 +58,7 @@ class C(T:! type); fn F(_:! Z where C(.Self) impls Y and .Z1 = C(.Self).(Y.Y1)) {} -// --- fail_todo_early_class_impls_generic_interface.carbon +// --- early_class_impls_generic_interface.carbon library "[[@TEST_NAME]]"; interface Z { @@ -70,10 +70,6 @@ class C; // Needs to identify `C as Y(.Self)` without replacing this `.Self` with `C`, // since it refers to `T`. -// CHECK:STDERR: fail_todo_early_class_impls_generic_interface.carbon:[[@LINE+4]]:53: error: cannot convert type `C` into type implementing `Y(.Self)` [ConversionFailureTypeToFacet] -// CHECK:STDERR: fn F(unused T:! Z where C impls Y(.Self) and .Z1 = (C as Y(.Self))) {} -// CHECK:STDERR: ^~~~~~~~~~~~~ -// CHECK:STDERR: fn F(unused T:! Z where C impls Y(.Self) and .Z1 = (C as Y(.Self))) {} class D(V:! type); @@ -82,16 +78,9 @@ class D(V:! type); // `.Self` to `D` since it refers to `T`. But we _do_ need to replace the // `.Self` in the type of `U`, since it refers to `U` which is being replaced by // `D(.Self)` in this specific. -// CHECK:STDERR: fail_todo_early_class_impls_generic_interface.carbon:[[@LINE+7]]:71: error: cannot convert type `D(.Self)` into type implementing `Y(.Self)` [ConversionFailureTypeToFacet] -// CHECK:STDERR: fn G(unused T:! Z where D(.Self) impls Y(D(.Self)) and D(.Self) impls X(D(.Self))) {} -// CHECK:STDERR: ^~~~~~~~~~~ -// CHECK:STDERR: fail_todo_early_class_impls_generic_interface.carbon:[[@LINE-20]]:1: note: while deducing parameters of generic declared here [DeductionGenericHere] -// CHECK:STDERR: interface X(U:! Y(.Self)) {} -// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~ -// CHECK:STDERR: fn G(unused T:! Z where D(.Self) impls Y(D(.Self)) and D(.Self) impls X(D(.Self))) {} -// --- fail_todo_early_class_impls_generic_interface_with_member_designator.carbon +// --- early_class_impls_generic_interface_with_member_designator.carbon library "[[@TEST_NAME]]"; interface Z { @@ -103,13 +92,9 @@ interface Y(T:! type) { } class C; -// CHECK:STDERR: fail_todo_early_class_impls_generic_interface_with_member_designator.carbon:[[@LINE+4]]:43: error: cannot convert type `C` into type implementing `Y(.(Z.Z2))` [ConversionFailureTypeToFacet] -// CHECK:STDERR: fn F(_:! Z where C impls Y(.Z2) and .Z1 = C.(Y(.Z2).Y1)) {} -// CHECK:STDERR: ^~~~~~~~~~~~~ -// CHECK:STDERR: fn F(_:! Z where C impls Y(.Z2) and .Z1 = C.(Y(.Z2).Y1)) {} -// --- fail_todo_early_tuple_impls_generic_interface.carbon +// --- early_tuple_impls_generic_interface.carbon library "[[@TEST_NAME]]"; interface Z { @@ -123,33 +108,13 @@ interface X { } class C; -// CHECK:STDERR: fail_todo_early_tuple_impls_generic_interface.carbon:[[@LINE+4]]:46: error: cannot convert type `()` into type implementing `Y(.Self)` [ConversionFailureTypeToFacet] -// CHECK:STDERR: fn F(_:! Z where () impls Y(.Self) and .Z1 = ().(Y(.Self).Y1)) {} -// CHECK:STDERR: ^~~~~~~~~~~~~~~~ -// CHECK:STDERR: fn F(_:! Z where () impls Y(.Self) and .Z1 = ().(Y(.Self).Y1)) {} -// CHECK:STDERR: fail_todo_early_tuple_impls_generic_interface.carbon:[[@LINE+4]]:49: error: cannot convert type `(C,)` into type implementing `Y(.Self)` [ConversionFailureTypeToFacet] -// CHECK:STDERR: fn G(_:! Z where (C, ) impls Y(.Self) and .Z1 = (C, ).(Y(.Self).Y1)) {} -// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~ -// CHECK:STDERR: fn G(_:! Z where (C, ) impls Y(.Self) and .Z1 = (C, ).(Y(.Self).Y1)) {} -// TODO: Can merge with the next test once they both pass. - -// --- early_tuple_impls.carbon -library "[[@TEST_NAME]]"; - -interface Z { - let Z1:! type; -} -interface X { - let X1:! type; -} - fn H(_:! Z where (.Self, ) impls X and .Z1 = (.Self, ).(X.X1)) {} -// --- fail_todo_early_struct_impls.carbon +// --- early_struct_impls.carbon library "[[@TEST_NAME]]"; interface Z { @@ -163,13 +128,9 @@ interface X { } class C; -// CHECK:STDERR: fail_todo_early_struct_impls.carbon:[[@LINE+4]]:46: error: cannot convert type `{}` into type implementing `Y(.Self)` [ConversionFailureTypeToFacet] -// CHECK:STDERR: fn F(_:! Z where {} impls Y(.Self) and .Z1 = {}.(Y(.Self).Y1)) {} -// CHECK:STDERR: ^~~~~~~~~~~~~~~~ -// CHECK:STDERR: fn F(_:! Z where {} impls Y(.Self) and .Z1 = {}.(Y(.Self).Y1)) {} -// --- fail_todo_early_concrete_impls.carbon +// --- early_concrete_impls.carbon library "[[@TEST_NAME]]"; interface Z { @@ -183,10 +144,6 @@ interface Y(T:! type) { fn MakeIntLiteral() -> type = "int_literal.make_type"; alias IntLiteral = MakeIntLiteral(); -// CHECK:STDERR: fail_todo_early_concrete_impls.carbon:[[@LINE+4]]:54: error: cannot convert type `IntLiteral` into type implementing `Y(.Self)` [ConversionFailureTypeToFacet] -// CHECK:STDERR: fn F(_:! Z where IntLiteral impls Y(.Self) and .Z1 = IntLiteral.(Y(.Self).Y1)) {} -// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~ -// CHECK:STDERR: fn F(_:! Z where IntLiteral impls Y(.Self) and .Z1 = IntLiteral.(Y(.Self).Y1)) {} // --- early_impl_named_constraint.carbon @@ -244,7 +201,7 @@ constraint N(V:! type) { // CHECK:STDERR: fn F(_:! Z(.Self) where C impls N(.Z1) and .Z2 = (.Z1 as Y(.Self))) {} -// --- fail_todo_early_type_impls_nested_self_impls.carbon +// --- early_type_impls_nested_self_impls.carbon library "[[@TEST_NAME]]"; interface Z { @@ -265,11 +222,32 @@ constraint NY { // A lookup of `C(V).(Y.Y1) as X` requires us to see that the `.Y1 impls X` // constraint is visible through the `C(.Self) impls NY` constraint and that // `C(.Self)` is used as the implied `.Self` in `.Y1 impls X`. -// CHECK:STDERR: fail_todo_early_type_impls_nested_self_impls.carbon:[[@LINE+7]]:53: error: cannot convert type `C(.Self).(Y.Y1)` into type implementing `X` [ConversionFailureTypeToFacet] -// CHECK:STDERR: fn F(unused V:! Z where C(.Self) impls NY and .Z1 = D(C(.Self).(Y.Y1))) {} -// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~ -// CHECK:STDERR: fail_todo_early_type_impls_nested_self_impls.carbon:[[@LINE-12]]:9: note: initializing generic parameter `T` declared here [InitializingGenericParam] -// CHECK:STDERR: class D(T:! X); -// CHECK:STDERR: ^~~~~ -// CHECK:STDERR: fn F(unused V:! Z where C(.Self) impls NY and .Z1 = D(C(.Self).(Y.Y1))) {} + +// --- period_self_impls_named_constraint_used_in_later_constraint.carbon +library "[[@TEST_NAME]]"; + +interface X {} +interface Y { + let Y1:! type; +} + +constraint ConstraintForY { + require impls Y where .Y1 impls X; +} + +interface Z { + let Z1:! type; +} + +private constraint ConstraintForZ { + extend require impls Z where .Z1 impls ConstraintForY; +} + +// If we replace `.Self` inside the named constraint `ConstraintForY`, we end up +// replacing `.Self` in the `.Z1` from the specific args for the named +// constraint, which replaces `Self` in the named constraint. Doing this is +// incorrect and was leading to an infinite loop of `.Self` substitution. +fn F(_:! ConstraintForZ where + .Z1 impls ConstraintForY and + .Z1.(Y.Y1) == ()) {} diff --git a/toolchain/check/type_completion.cpp b/toolchain/check/type_completion.cpp index 789e23b12298..49963f45ce73 100644 --- a/toolchain/check/type_completion.cpp +++ b/toolchain/check/type_completion.cpp @@ -965,7 +965,8 @@ static auto GetSelfFacetValue(Context& context, SemIR::ConstantId self_const_id) static auto IdentifyFacetType(Context& context, SemIR::LocId loc_id, SemIR::ConstantId initial_self_const_id, const SemIR::FacetType& facet_type, - bool allow_partially_identified, bool diagnose) + bool allow_partially_identified, + bool initial_subst_period_self, bool diagnose) -> SemIR::IdentifiedFacetTypeId { // While partially identified facet types end up in the store of // IdentifiedFacetTypes, we don't try to construct a key to look for them @@ -986,18 +987,16 @@ static auto IdentifyFacetType(Context& context, SemIR::LocId loc_id, // Whether the impling of facet type should be considered as extending in // the resulting IdentifiedFacetType. bool extend; - // Whether we should replace `.Self` in the self type. This is false for the - // top-level self, since we'd be replacing parts of it with itself, which is - // cyclical. It becomes true when recursing into a `T impls ...` constraint - // where the self type is now something else. - bool subst_self; + // Whether we should replace `.Self` in the constraint. + bool subst_period_self; SemIR::ConstantId self; SemIR::FacetTypeId facet_type; }; // Work queue. llvm::SmallVector work = { - {true, false, initial_self_const_id, facet_type.facet_type_id}}; + {true, initial_subst_period_self, initial_self_const_id, + facet_type.facet_type_id}}; // Outputs for the IdentifiedFacetType. bool partially_identified = false; @@ -1010,30 +1009,34 @@ static auto IdentifyFacetType(Context& context, SemIR::LocId loc_id, while (!work.empty()) { SelfImplsFacetType next_impls = work.pop_back_val(); bool facet_type_extends = next_impls.extend; - auto subst_period_self_in_self = next_impls.subst_self; + auto subst_period_self = next_impls.subst_period_self; auto self_const_id = GetCanonicalFacetOrTypeValue(context, next_impls.self); const auto& facet_type_info = context.facet_types().Get(next_impls.facet_type); auto self_and_interface = [&](SemIR::SpecificInterface impls_interface) -> SemIR::IdentifiedFacetType::RequiredImpl { - auto self = subst_period_self_in_self - ? SubstPeriodSelf(context, loc_id, self_const_id, - period_self_replacement_id) - : self_const_id; - auto interface = SubstPeriodSelf(context, loc_id, impls_interface, - period_self_replacement_id); + auto self = self_const_id; + auto interface = subst_period_self + ? SubstPeriodSelf(context, loc_id, impls_interface, + period_self_replacement_id) + : impls_interface; return {self, interface}; }; auto type_and_interface = [&](SemIR::FacetTypeInfo::TypeImplsInterface impls) -> SemIR::IdentifiedFacetType::RequiredImpl { - auto self = SubstPeriodSelf( - context, loc_id, context.constant_values().Get(impls.self_type), - period_self_replacement_id); + auto self = + subst_period_self + ? SubstPeriodSelf(context, loc_id, + context.constant_values().Get(impls.self_type), + period_self_replacement_id) + : context.constant_values().Get(impls.self_type); auto interface = - SubstPeriodSelf(context, loc_id, impls.specific_interface, - period_self_replacement_id); + subst_period_self + ? SubstPeriodSelf(context, loc_id, impls.specific_interface, + period_self_replacement_id) + : impls.specific_interface; return {self, interface}; }; @@ -1065,6 +1068,11 @@ static auto IdentifyFacetType(Context& context, SemIR::LocId loc_id, auto self_facet = GetSelfFacetValue(context, self_const_id); for (auto extends : facet_type_info.extend_named_constraints) { + if (subst_period_self) { + extends = SubstPeriodSelf(context, loc_id, extends, + period_self_replacement_id); + } + const auto& constraint = context.named_constraints().Get(extends.named_constraint_id); @@ -1118,12 +1126,16 @@ static auto IdentifyFacetType(Context& context, SemIR::LocId loc_id, .GetInstAs(require_facet_type) .facet_type_id; bool extend = facet_type_extends && require.extend_self; - work.push_back( - {extend, subst_period_self_in_self, require_self, facet_type_id}); + work.push_back({extend, false, require_self, facet_type_id}); } } for (auto impls : facet_type_info.self_impls_named_constraints) { + if (subst_period_self) { + impls = + SubstPeriodSelf(context, loc_id, impls, period_self_replacement_id); + } + const auto& constraint = context.named_constraints().Get(impls.named_constraint_id); @@ -1175,14 +1187,21 @@ static auto IdentifyFacetType(Context& context, SemIR::LocId loc_id, context.constant_values() .GetInstAs(require_facet_type) .facet_type_id; - work.push_back( - {false, subst_period_self_in_self, require_self, facet_type_id}); + work.push_back({false, false, require_self, facet_type_id}); } } for (const auto& type_impls : facet_type_info.type_impls_named_constraints) { auto [self_type_inst_id, impls] = type_impls; + if (subst_period_self) { + self_type_inst_id = context.constant_values().GetInstId(SubstPeriodSelf( + context, loc_id, context.constant_values().Get(self_type_inst_id), + period_self_replacement_id)); + impls = + SubstPeriodSelf(context, loc_id, impls, period_self_replacement_id); + } + const auto& constraint = context.named_constraints().Get(impls.named_constraint_id); @@ -1237,7 +1256,7 @@ static auto IdentifyFacetType(Context& context, SemIR::LocId loc_id, context.constant_values() .GetInstAs(require_facet_type) .facet_type_id; - work.push_back({false, true, require_self, facet_type_id}); + work.push_back({false, false, require_self, facet_type_id}); } } } @@ -1250,10 +1269,12 @@ static auto IdentifyFacetType(Context& context, SemIR::LocId loc_id, auto TryToIdentifyFacetType(Context& context, SemIR::LocId loc_id, SemIR::ConstantId self_const_id, const SemIR::FacetType& facet_type, - bool allow_partially_identified) + bool allow_partially_identified, + bool subst_period_self) -> SemIR::IdentifiedFacetTypeId { return IdentifyFacetType(context, loc_id, self_const_id, facet_type, - allow_partially_identified, /*diagnose=*/false); + allow_partially_identified, subst_period_self, + /*diagnose=*/false); } auto RequireIdentifiedFacetType(Context& context, SemIR::LocId loc_id, @@ -1265,7 +1286,8 @@ auto RequireIdentifiedFacetType(Context& context, SemIR::LocId loc_id, Diagnostics::ContextScope scope(&context.emitter(), diagnostic_context); return IdentifyFacetType(context, loc_id, self_const_id, facet_type, - /*allow_partially_identified=*/false, diagnose); + /*allow_partially_identified=*/false, + /*initial_subst_period_self=*/true, diagnose); } } // namespace Carbon::Check diff --git a/toolchain/check/type_completion.h b/toolchain/check/type_completion.h index ddf9220807cb..f803680ad996 100644 --- a/toolchain/check/type_completion.h +++ b/toolchain/check/type_completion.h @@ -78,7 +78,8 @@ auto RequireConcreteType(Context& context, SemIR::TypeId type_id, auto TryToIdentifyFacetType(Context& context, SemIR::LocId loc_id, SemIR::ConstantId self_const_id, const SemIR::FacetType& facet_type, - bool allow_partially_identified) + bool allow_partially_identified, + bool subst_period_self) -> SemIR::IdentifiedFacetTypeId; // Requires the named constraints in the facet type to be complete, so that the