Remove the ExplicitOnly option from SubstPeriodSelf (#7460)

The implicit/explicit `.Self` concept is a heuristic at best, so we
should avoid relying on it. We now only have one value of `.Self` in a
facet type (we have banned nested `where` on the RHS of a `where`). So
we don't need to preserve any `.Self` for the purpose of disambiguation,
and we can subst all `.Self` on the the RHS of a rewrite constraint.

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)};
   };
 

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

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 {