Merge pull request #84822 from hamishknight/just-a-phase

[CS] Remove `ConstraintSystemPhase`

Author: hamishknight

include/swift/Sema/ConstraintSystem.h

@@ -2137,11 +2137,6 @@ class SyntacticElementTargetRewriter {
   virtual ~SyntacticElementTargetRewriter() = default;
 };
 
-enum class ConstraintSystemPhase {
-  ConstraintGeneration,
-  Solving
-};
-
 /// Retrieve the closure type from the constraint system.
 struct GetClosureType {
   ConstraintSystem &cs;
@@ -2203,9 +2198,6 @@ class ConstraintSystem {
   /// Do not set directly, call \c recordFailedConstraint instead.
   Constraint *failedConstraint = nullptr;
 
-  /// Current phase of the constraint system lifetime.
-  ConstraintSystemPhase Phase = ConstraintSystemPhase::ConstraintGeneration;
-
   /// The set of expressions for which we have generated constraints.
   llvm::SetVector<Expr *> InputExprs;
 
@@ -2683,30 +2675,6 @@ class ConstraintSystem {
   /// \c nullptr if no constraint has failed.
   Constraint *getFailedConstraint() const { return failedConstraint; }
 
-  ConstraintSystemPhase getPhase() const { return Phase; }
-
-  /// Move constraint system to a new phase of its lifetime.
-  void setPhase(ConstraintSystemPhase newPhase) {
-    if (Phase == newPhase)
-      return;
-
-#ifndef NDEBUG
-    switch (Phase) {
-    case ConstraintSystemPhase::ConstraintGeneration:
-      assert(newPhase == ConstraintSystemPhase::Solving);
-      break;
-
-    case ConstraintSystemPhase::Solving:
-      // We can come back to constraint generation phase while
-      // processing result builder body.
-      assert(newPhase == ConstraintSystemPhase::ConstraintGeneration);
-      break;
-    }
-#endif
-
-    Phase = newPhase;
-  }
-
   /// Check whether constraint system is in valid state e.g.
   /// has left-over active/inactive constraints which should
   /// have been simplified.
@@ -3914,7 +3882,7 @@ class ConstraintSystem {
     // Add this constraint to the constraint graph.
     CG.addConstraint(constraint);
 
-    if (isDebugMode() && getPhase() == ConstraintSystemPhase::Solving) {
+    if (isDebugMode() && solverState) {
       auto &log = llvm::errs();
       log.indent(solverState->getCurrentIndent() + 4) << "(added constraint: ";
       constraint->print(log, &getASTContext().SourceMgr,
@@ -3935,7 +3903,7 @@ class ConstraintSystem {
     if (solverState)
       recordChange(SolverTrail::Change::RetiredConstraint(where, constraint));
 
-    if (isDebugMode() && getPhase() == ConstraintSystemPhase::Solving) {
+    if (isDebugMode() && solverState) {
       auto &log = llvm::errs();
       log.indent(solverState->getCurrentIndent() + 4)
           << "(removed constraint: ";

lib/Sema/CSBindings.cpp

@@ -1828,9 +1828,7 @@ PotentialBindings::inferFromRelational(ConstraintSystem &CS,
     // Since inference now happens during constraint generation,
     // this hack should be allowed in both `Solving`
     // (during non-diagnostic mode) and `ConstraintGeneration` phases.
-    if (isGenericParameter(TypeVar) &&
-        (!CS.shouldAttemptFixes() ||
-         CS.getPhase() == ConstraintSystemPhase::ConstraintGeneration)) {
+    if (isGenericParameter(TypeVar) && !CS.inSalvageMode()) {
       type = fnTy->withExtInfo(fnTy->getExtInfo().withNoEscape(false));
     }
   }

lib/Sema/CSGen.cpp

@@ -944,7 +944,6 @@ namespace {
   class ConstraintGenerator : public ExprVisitor<ConstraintGenerator, Type> {
     ConstraintSystem &CS;
     DeclContext *CurDC;
-    ConstraintSystemPhase CurrPhase;
 
     /// A map from each UnresolvedMemberExpr to the respective (implicit) base
     /// found during our walk.
@@ -1158,6 +1157,33 @@ namespace {
       if (addedTypeVars)
         addedTypeVars->push_back(memberTy);
 
+      SmallVector<AnyFunctionType::Param, 8> params;
+      getMatchingParams(argList, params);
+
+      // Add the constraint that the index expression's type be convertible
+      // to the input type of the subscript operator.
+      auto addApplicableFn = [&]() {
+        CS.addApplicationConstraint(
+            FunctionType::get(params, outputTy), memberTy,
+            /*trailingClosureMatching=*/std::nullopt, CurDC, fnLocator);
+      };
+
+      // If we have a dynamic member base we need to add the applicable function
+      // first since solving the member constraint requires the constraint to be
+      // available since it may retire it. We can't yet do this in the general
+      // case since the simplifying of the applicable fn in CSGen is currently
+      // load-bearing for existential opening.
+      // FIXME: Once existential opening is no longer sensitive to solving
+      // order, we ought to be able to just always record the applicable fn as
+      // an unsolved constraint before the member.
+      auto hasDynamicMemberLookup = CS.simplifyType(baseTy)
+                                        ->getRValueType()
+                                        ->getMetatypeInstanceType()
+                                        ->eraseDynamicSelfType()
+                                        ->hasDynamicMemberLookupAttribute();
+      if (hasDynamicMemberLookup)
+        addApplicableFn();
+
       // FIXME: synthesizeMaterializeForSet() wants to statically dispatch to
       // a known subscript here. This might be cleaner if we split off a new
       // UnresolvedSubscriptExpr from SubscriptExpr.
@@ -1173,14 +1199,10 @@ namespace {
                                     /*outerAlternatives=*/{}, memberLocator);
       }
 
-      SmallVector<AnyFunctionType::Param, 8> params;
-      getMatchingParams(argList, params);
-
-      // Add the constraint that the index expression's type be convertible
-      // to the input type of the subscript operator.
-      CS.addApplicationConstraint(FunctionType::get(params, outputTy), memberTy,
-                                  /*trailingClosureMatching=*/std::nullopt,
-                                  CurDC, fnLocator);
+      // If we don't have a dynamic member, we add the application after the
+      // member, see the above comment.
+      if (!hasDynamicMemberLookup)
+        addApplicableFn();
 
       if (CS.performanceHacksEnabled()) {
         Type fixedOutputType =
@@ -1250,23 +1272,15 @@ namespace {
 
   public:
     ConstraintGenerator(ConstraintSystem &CS, DeclContext *DC)
-        : CS(CS), CurDC(DC ? DC : CS.DC), CurrPhase(CS.getPhase()) {
-      // Although constraint system is initialized in `constraint
-      // generation` phase, we have to set it here manually because e.g.
-      // result builders could generate constraints for its body
-      // in the middle of the solving.
-      CS.setPhase(ConstraintSystemPhase::ConstraintGeneration);
-
+        : CS(CS), CurDC(DC ? DC : CS.DC) {
       // Pick up the saved stack of pack expansions so we can continue
       // to handle pack element references inside the closure body.
       if (auto *ACE = dyn_cast<AbstractClosureExpr>(CurDC)) {
         OuterExpansions = CS.getCapturedExpansions(ACE);
       }
     }
 
-    virtual ~ConstraintGenerator() {
-      CS.setPhase(CurrPhase);
-    }
+    virtual ~ConstraintGenerator() = default;
 
     ConstraintSystem &getConstraintSystem() const { return CS; }
 

lib/Sema/CSSimplify.cpp

@@ -11457,22 +11457,6 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyMemberConstraint(
     if (result.actualBaseType)
       baseTy = result.actualBaseType;
 
-    // If only possible choice to refer to member is via keypath
-    // dynamic member dispatch, let's delay solving this constraint
-    // until constraint generation phase is complete, because
-    // subscript dispatch relies on presence of function application.
-    if (result.ViableCandidates.size() == 1) {
-      auto &choice = result.ViableCandidates.front();
-      if (Phase == ConstraintSystemPhase::ConstraintGeneration &&
-          choice.isKeyPathDynamicMemberLookup() &&
-          member.getBaseName().isSubscript()) {
-        // Let's move this constraint to the active
-        // list so it could be picked up right after
-        // constraint generation is done.
-        return formUnsolved(/*activate=*/true);
-      }
-    }
-
     generateOverloadConstraints(
         candidates, memberTy, result.ViableCandidates, useDC, locator,
         result.getFavoredIndex(), /*requiresFix=*/false,
@@ -13948,26 +13932,6 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyApplicableFnConstraint(
     if (instance2->isTypeVariableOrMember())
       return formUnsolved();
 
-    auto *argumentsLoc = getConstraintLocator(
-        outerLocator.withPathElement(ConstraintLocator::ApplyArgument));
-
-    auto *argumentList = getArgumentList(argumentsLoc);
-    assert(argumentList);
-
-    // Cannot simplify construction of callable types during constraint
-    // generation when trailing closures are present because such calls
-    // have special trailing closure matching semantics. It's unclear
-    // whether trailing arguments belong to `.init` or implicit
-    // `.callAsFunction` in this case.
-    //
-    // Note that the constraint has to be activate so that solver attempts
-    // once constraint generation is done.
-    if (getPhase() == ConstraintSystemPhase::ConstraintGeneration &&
-        argumentList->hasAnyTrailingClosures() &&
-        instance2->isCallAsFunctionType(DC)) {
-      return formUnsolved(/*activate=*/true);
-    }
-
     // Construct the instance from the input arguments.
     auto simplified = simplifyConstructionConstraint(
         instance2, func1, subflags,

lib/Sema/CSSolver.cpp

@@ -1577,8 +1577,6 @@ bool ConstraintSystem::solve(SmallVectorImpl<Solution> &solutions,
 void ConstraintSystem::solveImpl(SmallVectorImpl<Solution> &solutions) {
   assert(solverState);
 
-  setPhase(ConstraintSystemPhase::Solving);
-
   // If constraint system failed while trying to
   // genenerate constraints, let's stop right here.
   if (failedConstraint)
@@ -1798,12 +1796,6 @@ ConstraintSystem::filterDisjunction(
     // constraint, so instead let's keep the disjunction, but disable all
     // unviable choices.
     if (choice->getOverloadChoice().isKeyPathDynamicMemberLookup()) {
-      // Early simplification of the "keypath dynamic member lookup" choice
-      // is impossible because it requires constraints associated with
-      // subscript index expression to be present.
-      if (Phase == ConstraintSystemPhase::ConstraintGeneration)
-        return SolutionKind::Unsolved;
-
       for (auto *currentChoice : disjunction->getNestedConstraints()) {
         if (currentChoice->isDisabled())
           continue;

lib/Sema/ConstraintSystem.cpp

@@ -1808,10 +1808,8 @@ struct TypeSimplifier {
           // so the concrete dependent member type is considered a "hole" in
           // order to continue solving.
           auto memberTy = DependentMemberType::get(lookupBaseType, assocType);
-          if (CS.shouldAttemptFixes() &&
-              CS.getPhase() == ConstraintSystemPhase::Solving) {
+          if (CS.inSalvageMode())
             return PlaceholderType::get(CS.getASTContext(), memberTy);
-          }
 
           return memberTy;
         }

lib/Sema/TypeOfReference.cpp

@@ -2483,7 +2483,7 @@ void ConstraintSystem::bindOverloadType(const SelectedOverload &overload,
             return constraint->getKind() == ConstraintKind::ApplicableFunction;
           });
 
-      assert(constraints.size() == 1);
+      ASSERT(constraints.size() == 1);
       auto *applicableFn = constraints.front();
       retireConstraint(applicableFn);
 

test/Constraints/keypath_dynamic_member_lookup.swift

@@ -601,3 +601,18 @@ struct TestIssue56837 {
         _ = value[type: Int8.max]
     }
 }
+
+@dynamicMemberLookup
+class TestDynamicSelf {
+  struct S {
+    subscript() -> Int { 0 }
+  }
+  func foo() -> Self {
+    // Make sure we can do dynamic member lookup on a dynamic self.
+    _ = self[]
+    return self
+  }
+  subscript<T>(dynamicMember dynamicMember: KeyPath<S, T>) -> T {
+    fatalError()
+  }
+}

test/Constraints/opened_existentials_overload.swift

@@ -3,7 +3,7 @@
 protocol P {}
 
 func g(_: some P) {}
-// expected-note@-1 {{required by global function 'g' where 'some P' = 'any P'}}
+// expected-note@-1 2{{required by global function 'g' where 'some P' = 'any P'}}
 
 // rdar://problem/160389221
 func good(_ x: Array<any P>) {
@@ -28,3 +28,25 @@ func bad(_ x: Array<any P>) {
   // expected-error@-1 {{type 'any P' cannot conform to 'P'}}
   // expected-note@-2 {{only concrete types such as structs, enums and classes can conform to protocols}}
 }
+
+func testSubscript() {
+  struct S1<T> {
+    subscript() -> T { fatalError() }
+  }
+  func foo(_ xs: S1<any P>) {
+    g(xs[])
+  }
+
+  struct S2<T> {
+    subscript() -> Int { fatalError() }
+    subscript() -> T { fatalError() }
+  }
+  func foo(_ xs: S2<any P>) {
+    // FIXME: This should work, if you fix this you can also remove the
+    // dynamic member lookup hack in `addSubscriptConstraints`, we should always
+    // just add the applicable fn as an unsolved constraint before the member.
+    g(xs[])
+    // expected-error@-1 {{type 'any P' cannot conform to 'P'}}
+    // expected-note@-2 {{only concrete types such as structs, enums and classes can conform to protocols}}
+  }
+}