diff --git a/toolchain/check/period_self.cpp b/toolchain/check/period_self.cpp index 9935d110270c..02ef7dca627d 100644 --- a/toolchain/check/period_self.cpp +++ b/toolchain/check/period_self.cpp @@ -150,11 +150,6 @@ class SubstPeriodSelfCallbacks : public SubstInstCallbacks { inst_id = GetReplacement(inst_id); } break; - case SubstPeriodSelfBehaviour::ExplicitOnly: - if (!is_implicit_self_in_desigator) { - inst_id = GetReplacement(inst_id); - } - break; } return FullySubstituted; } @@ -451,15 +446,13 @@ auto SubstPeriodSelfInFacetType(Context& context, SemIR::LocId loc_id, }; auto replace_rewrite = [&](SemIR::FacetTypeInfo::RewriteConstraint r) -> SemIR::FacetTypeInfo::RewriteConstraint { - // Designators in rewrite constraints are left alone so they can be found - // for rewrite constraint resolution. Using rewrites later requires further - // substitution of the `.Self` references inside designators. - // - // The LHS is always a simple designator since only that is allowed, so we - // just substitute the RHS. - auto rhs = SubstPeriodSelf( - context, loc_id, context.constant_values().Get(r.rhs_id), - period_self_replacement_id, SubstPeriodSelfBehaviour::ExplicitOnly); + // The LHS access instruction is not substituted so it keeps its `.Self`. + // This avoids evaluation replacing it with a concrete value from a final + // impl, as that would drop the association with the associated constant + // being rewritten. + auto rhs = SubstPeriodSelf(context, loc_id, + context.constant_values().Get(r.rhs_id), + period_self_replacement_id); return {r.lhs_id, context.constant_values().GetInstId(rhs)}; }; diff --git a/toolchain/check/period_self.h b/toolchain/check/period_self.h index 4c5e11d959d4..dc940a4f4622 100644 --- a/toolchain/check/period_self.h +++ b/toolchain/check/period_self.h @@ -23,7 +23,6 @@ auto MakePeriodSelfFacetValue(Context& context, SemIR::LocId loc_id, enum class SubstPeriodSelfBehaviour { ImplicitOnly, - ExplicitOnly, All, }; @@ -65,9 +64,10 @@ auto SubstPeriodSelf( // Replace `.Self` references with the self-type. The `facet_type_inst_id` must // be a `FacetType` instruction (or error). // -// The implicit `.Self` in designators is not replaced in rewrite constraints, -// to allow for rewrite constraint resolution to recognise the designators. -// Later use of rewrite constraints requires further `.Self` replacement. +// The `.Self` in the LHS of rewrite constraints is not replaced, to allow for +// rewrite constraint resolution to recognise the designators and avoid +// evaluation replacing them with a concrete value. Later use of rewrite +// constraints requires further `.Self` replacement. // // Unlike SubstPeriodSelf, which works with constant values and thus canonical // instructions, this operation can be done for non-canonical facet types. A new diff --git a/toolchain/check/testdata/named_constraint/require.carbon b/toolchain/check/testdata/named_constraint/require.carbon index aaa3a7ad3e37..f4b6a2014000 100644 --- a/toolchain/check/testdata/named_constraint/require.carbon +++ b/toolchain/check/testdata/named_constraint/require.carbon @@ -739,6 +739,22 @@ constraint N { require C.(Z(Self).Z1) impls Y; } +// --- fail_rewrite_cycle.carbon +library "[[@TEST_NAME]]"; + +interface Z { + let Z1:! type; + let Z2:! type; +} + +constraint N { + // CHECK:STDERR: fail_rewrite_cycle.carbon:[[@LINE+4]]:17: error: found cycle in facet type constraint for `.(Z.Z2)` [FacetTypeConstraintCycle] + // CHECK:STDERR: require impls Z where .Z1 = .Z2 and .Z2 = .Z1; + // CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + // CHECK:STDERR: + require impls Z where .Z1 = .Z2 and .Z2 = .Z1; +} + // CHECK:STDOUT: --- extend.carbon // CHECK:STDOUT: // CHECK:STDOUT: constants {