Merge pull request #59975 from DougGregor/cleanup-fix-behavior

Cleanup after recent refactoring for fix behaviors

Author: DougGregor

include/swift/Sema/CSFix.h

@@ -411,10 +411,10 @@ class ConstraintFix {
   FixKind Kind;
   ConstraintLocator *Locator;
 
+public:
   /// The behavior limit to apply to the diagnostics emitted.
-  FixBehavior fixBehavior;
+  const FixBehavior fixBehavior;
 
-public:
   ConstraintFix(ConstraintSystem &cs, FixKind kind, ConstraintLocator *locator,
                 FixBehavior fixBehavior = FixBehavior::Error)
       : CS(cs), Kind(kind), Locator(locator), fixBehavior(fixBehavior) {}
@@ -428,27 +428,22 @@ class ConstraintFix {
 
   FixKind getKind() const { return Kind; }
 
-  /// Whether it is still possible to "apply" a solution containing this kind
-  /// of fix to get a usable AST.
-  bool canApplySolution() const {
+  /// Whether this fix fatal for the constraint solver, meaning that it cannot
+  /// produce a usable type-checked AST.
+  bool isFatal() const {
     switch (fixBehavior) {
     case FixBehavior::AlwaysWarning:
     case FixBehavior::DowngradeToWarning:
     case FixBehavior::Suppress:
-      return true;
+      return false;
 
     case FixBehavior::Error:
-      return false;
+      return true;
     }
   }
 
-  /// Whether this kind of fix affects the solution score, and which score
-  /// it affects.
-  Optional<ScoreKind> affectsSolutionScore() const;
-
-  /// The diagnostic behavior limit that will be applied to any emitted
-  /// diagnostics.
-  FixBehavior diagfixBehavior() const { return fixBehavior; }
+  /// Determine the impact of this fix on the solution score, if any.
+  Optional<ScoreKind> impact() const;
 
   virtual std::string getName() const = 0;
 
@@ -804,6 +799,16 @@ class MarkGlobalActorFunction final : public ContextualMismatch {
                                          Type rhs, ConstraintLocator *locator,
                                          FixBehavior fixBehavior);
 
+  /// Try to apply this fix to the given types.
+  ///
+  /// \returns \c true if the fix cannot be applied and the solver must fail,
+  /// or \c false if the fix has been applied and the solver can continue.
+  static bool attempt(ConstraintSystem &cs,
+                      ConstraintKind constraintKind,
+                      FunctionType *fromType,
+                      FunctionType *toType,
+                      ConstraintLocatorBuilder locator);
+
   static bool classof(ConstraintFix *fix) {
     return fix->getKind() == FixKind::MarkGlobalActorFunction;
   }
@@ -855,6 +860,16 @@ class AddSendableAttribute final : public ContextualMismatch {
                                       ConstraintLocator *locator,
                                       FixBehavior fixBehavior);
 
+  /// Try to apply this fix to the given types.
+  ///
+  /// \returns \c true if the fix cannot be applied and the solver must fail,
+  /// or \c false if the fix has been applied and the solver can continue.
+  static bool attempt(ConstraintSystem &cs,
+                      ConstraintKind constraintKind,
+                      FunctionType *fromType,
+                      FunctionType *toType,
+                      ConstraintLocatorBuilder locator);
+
   static bool classof(ConstraintFix *fix) {
     return fix->getKind() == FixKind::AddSendableAttribute;
   }

include/swift/Sema/ConstraintSystem.h

@@ -3820,6 +3820,10 @@ class ConstraintSystem {
         });
   }
 
+  /// Determine whether the callee for the given locator is marked as
+  /// `@preconcurrency`.
+  bool hasPreconcurrencyCallee(ConstraintLocatorBuilder locator);
+
   /// Determine whether the given declaration is unavailable from the
   /// current context.
   bool isDeclUnavailable(const Decl *D,

lib/Sema/CSApply.cpp

@@ -8483,7 +8483,7 @@ bool ConstraintSystem::applySolutionFixes(const Solution &solution) {
 
         auto diagnosed =
             primaryFix->coalesceAndDiagnose(solution, secondaryFixes);
-        if (primaryFix->canApplySolution()) {
+        if (!primaryFix->isFatal()) {
           assert(diagnosed && "warnings should always be diagnosed");
           (void)diagnosed;
         } else {
@@ -9143,9 +9143,9 @@ Optional<SolutionApplicationTarget> ConstraintSystem::applySolution(
     bool diagnosedErrorsViaFixes = applySolutionFixes(solution);
     bool canApplySolution = true;
     for (const auto fix : solution.Fixes) {
-      if (!fix->canApplySolution())
+      if (fix->isFatal())
         canApplySolution = false;
-      if (fix->affectsSolutionScore() == SK_Fix && fix->canApplySolution())
+      if (fix->impact() == SK_Fix && !fix->isFatal())
         ++numResolvableFixes;
     }
 

lib/Sema/CSFix.cpp

@@ -17,6 +17,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "CSDiagnostics.h"
+#include "TypeCheckConcurrency.h"
 #include "swift/AST/Expr.h"
 #include "swift/AST/ParameterList.h"
 #include "swift/AST/Type.h"
@@ -37,7 +38,7 @@ using namespace constraints;
 
 ConstraintFix::~ConstraintFix() {}
 
-Optional<ScoreKind> ConstraintFix::affectsSolutionScore() const {
+Optional<ScoreKind> ConstraintFix::impact() const {
   switch (fixBehavior) {
   case FixBehavior::AlwaysWarning:
     return None;
@@ -243,10 +244,42 @@ MarkExplicitlyEscaping::create(ConstraintSystem &cs, Type lhs, Type rhs,
 bool MarkGlobalActorFunction::diagnose(const Solution &solution,
                                        bool asNote) const {
   DroppedGlobalActorFunctionAttr failure(
-      solution, getFromType(), getToType(), getLocator(), diagfixBehavior());
+      solution, getFromType(), getToType(), getLocator(), fixBehavior);
   return failure.diagnose(asNote);
 }
 
+/// The fix behavior to apply to a concurrency-related diagnostic.
+static Optional<FixBehavior>
+getConcurrencyFixBehavior(
+    ConstraintSystem &cs, ConstraintKind constraintKind,
+    ConstraintLocatorBuilder locator, bool forSendable) {
+  // We can only handle the downgrade for conversions.
+  switch (constraintKind) {
+  case ConstraintKind::Conversion:
+  case ConstraintKind::ArgumentConversion:
+    break;
+
+  default:
+    if (!cs.shouldAttemptFixes())
+      return None;
+
+    return FixBehavior::Error;
+  }
+
+  // For a @preconcurrency callee outside of a strict concurrency
+  // context, ignore.
+  if (cs.hasPreconcurrencyCallee(locator) &&
+      !contextRequiresStrictConcurrencyChecking(
+          cs.DC, GetClosureType{cs}, ClosureIsolatedByPreconcurrency{cs}))
+    return FixBehavior::Suppress;
+
+  // Otherwise, warn until Swift 6.
+  if (!cs.getASTContext().LangOpts.isSwiftVersionAtLeast(6))
+    return FixBehavior::DowngradeToWarning;
+
+  return FixBehavior::Error;
+}
+
 MarkGlobalActorFunction *
 MarkGlobalActorFunction::create(ConstraintSystem &cs, Type lhs, Type rhs,
                                 ConstraintLocator *locator,
@@ -259,11 +292,28 @@ MarkGlobalActorFunction::create(ConstraintSystem &cs, Type lhs, Type rhs,
       cs, lhs, rhs, locator, fixBehavior);
 }
 
+bool MarkGlobalActorFunction::attempt(ConstraintSystem &cs,
+                                      ConstraintKind constraintKind,
+                                      FunctionType *fromType,
+                                      FunctionType *toType,
+                                      ConstraintLocatorBuilder locator) {
+  auto fixBehavior = getConcurrencyFixBehavior(
+      cs, constraintKind, locator, /*forSendable=*/false);
+  if (!fixBehavior)
+    return true;
+
+  auto *fix = MarkGlobalActorFunction::create(
+      cs, fromType, toType, cs.getConstraintLocator(locator),
+      *fixBehavior);
+
+  return cs.recordFix(fix);
+}
+
 bool AddSendableAttribute::diagnose(const Solution &solution,
                                       bool asNote) const {
   AttributedFuncToTypeConversionFailure failure(
       solution, getFromType(), getToType(), getLocator(),
-      AttributedFuncToTypeConversionFailure::Concurrent, diagfixBehavior());
+      AttributedFuncToTypeConversionFailure::Concurrent, fixBehavior);
   return failure.diagnose(asNote);
 }
 
@@ -280,6 +330,22 @@ AddSendableAttribute::create(ConstraintSystem &cs,
   return new (cs.getAllocator()) AddSendableAttribute(
       cs, fromType, toType, locator, fixBehavior);
 }
+
+bool AddSendableAttribute::attempt(ConstraintSystem &cs,
+                                   ConstraintKind constraintKind,
+                                   FunctionType *fromType,
+                                   FunctionType *toType,
+                                   ConstraintLocatorBuilder locator) {
+  auto fixBehavior = getConcurrencyFixBehavior(
+      cs, constraintKind, locator, /*forSendable=*/true);
+  if (!fixBehavior)
+    return true;
+
+  auto *fix = AddSendableAttribute::create(
+      cs, fromType, toType, cs.getConstraintLocator(locator), *fixBehavior);
+  return cs.recordFix(fix);
+}
+
 bool RelabelArguments::diagnose(const Solution &solution, bool asNote) const {
   LabelingFailure failure(solution, getLocator(), getLabels());
   return failure.diagnose(asNote);
@@ -1639,7 +1705,7 @@ bool TreatEphemeralAsNonEphemeral::diagnose(const Solution &solution,
                                             bool asNote) const {
   NonEphemeralConversionFailure failure(solution, getLocator(), getFromType(),
                                         getToType(), ConversionKind,
-                                        diagfixBehavior());
+                                        fixBehavior);
   return failure.diagnose(asNote);
 }
 
@@ -2233,7 +2299,7 @@ IgnoreDefaultExprTypeMismatch::create(ConstraintSystem &cs, Type argType,
 bool AddExplicitExistentialCoercion::diagnose(const Solution &solution,
                                               bool asNote) const {
   MissingExplicitExistentialCoercion failure(solution, ErasedResultType,
-                                             getLocator(), diagfixBehavior());
+                                             getLocator(), fixBehavior);
   return failure.diagnose(asNote);
 }
 

lib/Sema/CSSimplify.cpp

@@ -2655,14 +2655,10 @@ static bool fixExtraneousArguments(ConstraintSystem &cs,
       /*impact=*/numExtraneous * 2);
 }
 
-bool hasPreconcurrencyCallee(ConstraintSystem *cs,
-                             ConstraintLocatorBuilder locator) {
-  if (cs->getASTContext().isSwiftVersionAtLeast(6))
-    // Swift 6 mode does not reduce errors to warnings.
-    return false;
-
-  auto calleeLocator = cs->getCalleeLocator(cs->getConstraintLocator(locator));
-  auto calleeOverload = cs->findSelectedOverloadFor(calleeLocator);
+bool ConstraintSystem::hasPreconcurrencyCallee(
+    ConstraintLocatorBuilder locator) {
+  auto calleeLocator = getCalleeLocator(getConstraintLocator(locator));
+  auto calleeOverload = findSelectedOverloadFor(calleeLocator);
   if (!calleeOverload || !calleeOverload->choice.isDecl())
     return false;
 
@@ -2716,48 +2712,12 @@ ConstraintSystem::matchFunctionTypes(FunctionType *func1, FunctionType *func2,
       increaseScore(SK_SyncInAsync);
   }
 
-  /// The behavior limit to apply to a concurrency check.
-  auto getConcurrencyFixBehavior = [&](
-        bool forSendable
-      ) -> Optional<FixBehavior> {
-        // We can only handle the downgrade for conversions.
-        switch (kind) {
-        case ConstraintKind::Conversion:
-        case ConstraintKind::ArgumentConversion:
-          break;
-
-        default:
-          if (!shouldAttemptFixes())
-            return None;
-
-          return FixBehavior::Error;
-        }
-
-        // For a @preconcurrency callee outside of a strict concurrency
-        // context, ignore.
-        if (hasPreconcurrencyCallee(this, locator) &&
-            !contextRequiresStrictConcurrencyChecking(DC, GetClosureType{*this}, ClosureIsolatedByPreconcurrency{*this}))
-          return FixBehavior::Suppress;
-
-        // Otherwise, warn until Swift 6.
-        if (!getASTContext().LangOpts.isSwiftVersionAtLeast(6))
-          return FixBehavior::DowngradeToWarning;
-
-        return FixBehavior::Error;
-  };
-
   // A @Sendable function can be a subtype of a non-@Sendable function.
   if (func1->isSendable() != func2->isSendable()) {
     // Cannot add '@Sendable'.
     if (func2->isSendable() || kind < ConstraintKind::Subtype) {
-      if (auto fixBehavior = getConcurrencyFixBehavior(true)) {
-        auto *fix = AddSendableAttribute::create(
-            *this, func1, func2, getConstraintLocator(locator), *fixBehavior);
-        if (recordFix(fix))
-          return getTypeMatchFailure(locator);
-      } else {
+      if (AddSendableAttribute::attempt(*this, kind, func1, func2, locator))
         return getTypeMatchFailure(locator);
-      }
     }
   }
 
@@ -2785,16 +2745,8 @@ ConstraintSystem::matchFunctionTypes(FunctionType *func1, FunctionType *func2,
         return getTypeMatchFailure(locator);
     } else if (func1->getGlobalActor() && !func2->isAsync()) {
       // Cannot remove a global actor from a synchronous function.
-      if (auto fixBehavior = getConcurrencyFixBehavior(false)) {
-        auto *fix = MarkGlobalActorFunction::create(
-            *this, func1, func2, getConstraintLocator(locator),
-            *fixBehavior);
-
-        if (recordFix(fix))
-          return getTypeMatchFailure(locator);
-      } else {
+      if (MarkGlobalActorFunction::attempt(*this, kind, func1, func2, locator))
         return getTypeMatchFailure(locator);
-      }
     } else if (kind < ConstraintKind::Subtype) {
       return getTypeMatchFailure(locator);
     }
@@ -9839,7 +9791,7 @@ bool ConstraintSystem::resolveClosure(TypeVariableType *typeVar,
   auto *inferredClosureType = getClosureType(closure);
 
   // Note if this closure is isolated by preconcurrency.
-  if (hasPreconcurrencyCallee(this, locator))
+  if (hasPreconcurrencyCallee(locator))
     preconcurrencyClosures.insert(closure);
 
   // Let's look through all optionals associated with contextual
@@ -12711,8 +12663,8 @@ bool ConstraintSystem::recordFix(ConstraintFix *fix, unsigned impact) {
   // Record the fix.
 
   // If this should affect the solution score, do so.
-  if (auto scoreKind = fix->affectsSolutionScore())
-    increaseScore(*scoreKind, impact);
+  if (auto impactScoreKind = fix->impact())
+    increaseScore(*impactScoreKind, impact);
 
   // If we've made the current solution worse than the best solution we've seen
   // already, stop now.
@@ -12735,7 +12687,7 @@ bool ConstraintSystem::recordFix(ConstraintFix *fix, unsigned impact) {
     // Fixes that don't affect the score shouldn't be considered because even
     // if such a fix is recorded at that anchor this should not
     // have any affect in the recording of any other fix.
-    if (!fix->affectsSolutionScore())
+    if (!fix->impact())
       continue;
 
     anchors.insert(fix->getAnchor());

lib/Sema/ConstraintSystem.cpp

@@ -4621,7 +4621,7 @@ bool ConstraintSystem::diagnoseAmbiguityWithFixes(
   // let's diagnose this as regular ambiguity.
   if (llvm::all_of(solutions, [](const Solution &solution) {
         return llvm::all_of(solution.Fixes, [](const ConstraintFix *fix) {
-          return fix->canApplySolution();
+          return !fix->isFatal();
         });
       })) {
     return diagnoseAmbiguity(solutions);
@@ -4643,7 +4643,7 @@ bool ConstraintSystem::diagnoseAmbiguityWithFixes(
       // source of ambiguity or failures.
       // Ignore warnings in favor of actual error fixes,
       // because they are not the source of ambiguity/failures.
-      if (!fix->affectsSolutionScore())
+      if (!fix->impact())
         continue;
 
       fixes.insert({&solution, fix});