From 570660109600524318a32bd80f922baedbe1eec0 Mon Sep 17 00:00:00 2001 From: Dana Jansens Date: Wed, 13 May 2026 14:49:02 -0400 Subject: [PATCH] Replace `.Self` in the self type of `T(.Self) impls NamedConstraint` when identifying a facet type (#7204) Given this example function: ```carbon fn F(T:! Z where .Self imps Y(.Self) and E(.Self) impls X(.Self)) { T as Y(T); E(T) as X(T); } ``` When we identify the facet type of `T`, we replace `.Self` with `T`. However in the initial loop, the self _is_ `T` so we don't want to substitute `.Self` instances inside `T` with itself, as that creates cycles. When we see a `type impls...` constraint such as `E(.Self)` we now have a different self-type so we can, and want to, substitute the `.Self` in it with `T`. This was already being done, unless the RHS of the `impls` was a named constraint. In that case we forgot that we were in a `type impls...` constraint, and avoided replacing `.Self`. Now the algorithm remembers and correctly replaces `.Self` for `type impls named constraint` constraints in a facet type. --- .../facet/validate_impl_constraints.carbon | 38 +++++++++++++++++++ toolchain/check/type_completion.cpp | 32 +++++++++++----- 2 files changed, 60 insertions(+), 10 deletions(-) diff --git a/toolchain/check/testdata/facet/validate_impl_constraints.carbon b/toolchain/check/testdata/facet/validate_impl_constraints.carbon index 7bb4063f244e..82e6a479a95f 100644 --- a/toolchain/check/testdata/facet/validate_impl_constraints.carbon +++ b/toolchain/check/testdata/facet/validate_impl_constraints.carbon @@ -452,3 +452,41 @@ fn F(T:! Z) { T.Z1 as Y; C(T.Z1) as Y; } + +// --- period_self_in_type_impls_named_constraint.carbon +library "[[@TEST_NAME]]"; + +interface Z { + let Z1:! type; +} +interface Y {} + +constraint GivesY { + require impls Y; +} + +class C(T:! type); + +fn F(T:! Z where C(.Self) impls GivesY) { + C(T) as Y; +} + +// --- impl_witness_access_impls_named_constraint.carbon +library "[[@TEST_NAME]]"; + +interface Z { + let Z1:! type; +} +interface Y {} + +constraint GivesY { + require impls Y; +} + +// This type places a `.Self` (in the .Z1) into the identified facet type as +// `.Self impls Y` through the named constraint. It ensures that the `.Self` is +// correctly replaced by `T` in order to compare equal with `T.Z1` in impl +// lookup. +fn F(T:! Z where .Z1 impls GivesY) { + T.Z1 as Y; +} diff --git a/toolchain/check/type_completion.cpp b/toolchain/check/type_completion.cpp index 18abb9a7e872..789e23b12298 100644 --- a/toolchain/check/type_completion.cpp +++ b/toolchain/check/type_completion.cpp @@ -983,14 +983,21 @@ static auto IdentifyFacetType(Context& context, SemIR::LocId loc_id, } struct SelfImplsFacetType { + // 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; SemIR::ConstantId self; SemIR::FacetTypeId facet_type; }; // Work queue. llvm::SmallVector work = { - {true, initial_self_const_id, facet_type.facet_type_id}}; + {true, false, initial_self_const_id, facet_type.facet_type_id}}; // Outputs for the IdentifiedFacetType. bool partially_identified = false; @@ -1003,17 +1010,20 @@ 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 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 interface) + auto self_and_interface = [&](SemIR::SpecificInterface impls_interface) -> SemIR::IdentifiedFacetType::RequiredImpl { - // Note that we subst `.Self` in the interface, but we do not in the - // self type here, as that would be cyclical, replacing part of the self - // type with itself. - return {self_const_id, SubstPeriodSelf(context, loc_id, interface, - period_self_replacement_id)}; + 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); + return {self, interface}; }; auto type_and_interface = [&](SemIR::FacetTypeInfo::TypeImplsInterface impls) @@ -1108,7 +1118,8 @@ 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, require_self, facet_type_id}); + work.push_back( + {extend, subst_period_self_in_self, require_self, facet_type_id}); } } @@ -1164,7 +1175,8 @@ static auto IdentifyFacetType(Context& context, SemIR::LocId loc_id, context.constant_values() .GetInstAs(require_facet_type) .facet_type_id; - work.push_back({false, require_self, facet_type_id}); + work.push_back( + {false, subst_period_self_in_self, require_self, facet_type_id}); } } @@ -1225,7 +1237,7 @@ static auto IdentifyFacetType(Context& context, SemIR::LocId loc_id, context.constant_values() .GetInstAs(require_facet_type) .facet_type_id; - work.push_back({false, require_self, facet_type_id}); + work.push_back({false, true, require_self, facet_type_id}); } } }