Merge pull request #62153 from kavon/lose-mainactor-94462333

Allow a global-actor to be dropped for some non-async functions.

Author: kavon

include/swift/AST/ActorIsolation.h

@@ -238,6 +238,26 @@ bool isSameActorIsolated(ValueDecl *value, DeclContext *dc);
 /// Determines whether this function's body uses flow-sensitive isolation.
 bool usesFlowSensitiveIsolation(AbstractFunctionDecl const *fn);
 
+/// Check if it is safe for the \c globalActor qualifier to be removed from
+/// \c ty, when the function value of that type is isolated to that actor.
+///
+/// In general this is safe in a narrow but common case: a global actor
+/// qualifier can be dropped from a function type while in a DeclContext
+/// isolated to that same actor, as long as the value is not Sendable.
+///
+/// \param dc the innermost context in which the cast to remove the global actor
+///           is happening.
+/// \param globalActor global actor that was dropped from \c ty.
+/// \param ty a function type where \c globalActor was removed from it.
+/// \param getClosureActorIsolation function that knows how to produce accurate
+///        information about the isolation of a closure.
+/// \return true if it is safe to drop the global-actor qualifier.
+bool safeToDropGlobalActor(
+                DeclContext *dc, Type globalActor, Type ty,
+                llvm::function_ref<ClosureActorIsolation(AbstractClosureExpr *)>
+                    getClosureActorIsolation =
+                        _getRef__AbstractClosureExpr_getActorIsolation());
+
 void simple_display(llvm::raw_ostream &out, const ActorIsolation &state);
 
 } // end namespace swift

include/swift/Sema/CSFix.h

@@ -858,7 +858,7 @@ class MarkGlobalActorFunction final : public ContextualMismatch {
   }
 
 public:
-  std::string getName() const override { return "add @escaping"; }
+  std::string getName() const override { return "add global actor"; }
 
   bool diagnose(const Solution &solution, bool asNote = false) const override;
 

include/swift/Sema/ConstraintSystem.h

@@ -2750,7 +2750,7 @@ struct GetClosureType {
   Type operator()(const AbstractClosureExpr *expr) const;
 };
 
-/// Retrieve the closure type from the constraint system.
+/// Retrieve the closure's preconcurrency status from the constraint system.
 struct ClosureIsolatedByPreconcurrency {
   ConstraintSystem &cs;
 

lib/Sema/CSSimplify.cpp

@@ -2920,6 +2920,68 @@ bool ConstraintSystem::hasPreconcurrencyCallee(
   return calleeOverload->choice.getDecl()->preconcurrency();
 }
 
+/// Determines whether a DeclContext is preconcurrency, using information
+/// tracked by the solver to aid in answering that.
+static bool isPreconcurrency(ConstraintSystem &cs, DeclContext *dc) {
+  if (auto *decl = dc->getAsDecl())
+    return decl->preconcurrency();
+
+  if (auto *ce = dyn_cast<ClosureExpr>(dc)) {
+    return ClosureIsolatedByPreconcurrency{cs}(ce);
+  }
+
+  if (auto *autoClos = dyn_cast<AutoClosureExpr>(dc)) {
+    return isPreconcurrency(cs, autoClos->getParent());
+  }
+
+  llvm_unreachable("unhandled DeclContext kind in isPreconcurrency");
+}
+
+/// A thin wrapper around \c swift::safeToDropGlobalActor that provides the
+/// ability to infer the isolation of a closure. Not perfect but mostly works.
+static bool okToRemoveGlobalActor(ConstraintSystem &cs,
+                                  DeclContext *dc,
+                                  Type globalActor, Type ty) {
+  auto findGlobalActorForClosure = [&](AbstractClosureExpr *ace) -> ClosureActorIsolation {
+    // FIXME: Because the actor isolation checking happens after constraint
+    // solving, the closure expression does not yet have its actor isolation
+    // set, i.e., the code that would call
+    // `AbstractClosureExpr::setActorIsolation` hasn't run yet.
+    // So, I expect the existing isolation to always be set to the default.
+    // If the assertion below starts tripping, then this ad-hoc inference
+    // is no longer needed!
+    auto existingIso = ace->getActorIsolation();
+    if (existingIso != ClosureActorIsolation()) {
+      assert(false && "somebody set the closure's isolation already?");
+      return existingIso;
+    }
+
+    // Otherwise, do an ad-hoc inference of the closure's isolation.
+
+    // see if the closure's type has isolation.
+    if (auto closType = GetClosureType{cs}(ace)) {
+      if (auto fnTy = closType->getAs<AnyFunctionType>()) {
+        if (auto globActor = fnTy->getGlobalActor()) {
+          return ClosureActorIsolation::forGlobalActor(globActor,
+                                                       isPreconcurrency(cs, ace));
+        }
+      }
+    }
+
+    // otherwise, check for an explicit annotation
+    if (auto *ce = dyn_cast<ClosureExpr>(ace)) {
+      if (auto globActor = getExplicitGlobalActor(ce)) {
+        return ClosureActorIsolation::forGlobalActor(globActor,
+                                                     isPreconcurrency(cs, ce));
+      }
+    }
+
+    return existingIso;
+  };
+
+  return safeToDropGlobalActor(dc, globalActor, ty, findGlobalActorForClosure);
+}
+
 ConstraintSystem::TypeMatchResult
 ConstraintSystem::matchFunctionTypes(FunctionType *func1, FunctionType *func2,
                                      ConstraintKind kind, TypeMatchOptions flags,
@@ -2999,10 +3061,30 @@ ConstraintSystem::matchFunctionTypes(FunctionType *func1, FunctionType *func2,
                                ConstraintKind::Equal, subflags, locator);
       if (result == SolutionKind::Error)
         return getTypeMatchFailure(locator);
+
     } else if (func1->getGlobalActor() && !func2->isAsync()) {
-      // Cannot remove a global actor from a synchronous function.
-      if (MarkGlobalActorFunction::attempt(*this, kind, func1, func2, locator))
+      // Cannot remove a global actor from a synchronous function in mismatched
+      // DeclContext.
+      //
+      // FIXME: the ConstraintSystem's DeclContext is not always precise enough
+      // to give an accurate answer. We want the innermost DeclContext that
+      // contains the expression associated with the constraint locator.
+      // Sometimes the ConstraintSystem only has the enclosing function, and not
+      // the inner closure containing the expression. To workaround this,
+      // `ActorIsolationChecker::checkFunctionConversion`, has extra checking of
+      // function conversions specifically to account for this false positive.
+      // This means we may have duplicate diagnostics emitted.
+      if (okToRemoveGlobalActor(*this, DC, func1->getGlobalActor(), func2)) {
+        // FIXME: this is a bit of a hack to workaround multiple solutions
+        // because in these contexts it's valid to both add or remove the actor
+        // from these function types. At least with the score increases, we
+        // can bias the solver to pick the solution with fewer conversions.
+        increaseScore(SK_FunctionConversion);
+
+      } else if (MarkGlobalActorFunction::attempt(*this, kind, func1, func2, locator)) {
         return getTypeMatchFailure(locator);
+      }
+
     } else if (kind < ConstraintKind::Subtype) {
       return getTypeMatchFailure(locator);
     } else {
@@ -3011,6 +3093,7 @@ ConstraintSystem::matchFunctionTypes(FunctionType *func1, FunctionType *func2,
       // is a function conversion going on here, let's increase the score to
       // avoid ambiguity when solver can also match a global actor matching
       // function type.
+      // FIXME: there may be a better way. see https://github.com/apple/swift/pull/62514
       increaseScore(SK_FunctionConversion);
     }
   }

lib/Sema/TypeCheckConcurrency.cpp

@@ -1526,6 +1526,41 @@ bool swift::isAsyncDecl(ConcreteDeclRef declRef) {
   return false;
 }
 
+bool swift::safeToDropGlobalActor(
+    DeclContext *dc, Type globalActor, Type ty,
+    llvm::function_ref<ClosureActorIsolation(AbstractClosureExpr *)>
+        getClosureActorIsolation) {
+  auto funcTy = ty->getAs<AnyFunctionType>();
+  if (!funcTy)
+    return false;
+
+  // can't add a different global actor
+  if (auto otherGA = funcTy->getGlobalActor()) {
+    assert(otherGA->getCanonicalType() != globalActor->getCanonicalType()
+           && "not even dropping the actor?");
+    return false;
+  }
+
+  // We currently allow unconditional dropping of global actors from
+  // async function types, despite this confusing Sendable checking
+  // in light of SE-338.
+  if (funcTy->isAsync())
+    return true;
+
+  // fundamentally cannot be sendable if we want to drop isolation info
+  if (funcTy->isSendable())
+    return false;
+
+  // finally, must be in a context with matching isolation.
+  auto dcIsolation = getActorIsolationOfContext(dc, getClosureActorIsolation);
+  if (dcIsolation.isGlobalActor())
+    if (dcIsolation.getGlobalActor()->getCanonicalType()
+        == globalActor->getCanonicalType())
+      return true;
+
+  return false;
+}
+
 static FuncDecl *findAnnotatableFunction(DeclContext *dc) {
   auto fn = dyn_cast<FuncDecl>(dc);
   if (!fn) return nullptr;
@@ -1737,6 +1772,41 @@ namespace {
       return false;
     }
 
+    /// Some function conversions synthesized by the constraint solver may not
+    /// be correct AND the solver doesn't know, so we must emit a diagnostic.
+    void checkFunctionConversion(FunctionConversionExpr *funcConv) {
+      auto subExprType = funcConv->getSubExpr()->getType();
+      if (auto fromType = subExprType->getAs<FunctionType>()) {
+        if (auto fromActor = fromType->getGlobalActor()) {
+          if (auto toType = funcConv->getType()->getAs<FunctionType>()) {
+
+            // ignore some kinds of casts, as they're diagnosed elsewhere.
+            if (toType->hasGlobalActor() || toType->isAsync())
+              return;
+
+            auto dc = const_cast<DeclContext*>(getDeclContext());
+            if (!safeToDropGlobalActor(dc, fromActor, toType)) {
+            // FIXME: this diagnostic is sometimes a duplicate of one emitted
+            // by the constraint solver. Difference is the solver doesn't use
+            // warnUntilSwiftVersion, which appends extra text on the end.
+            // So, I'm making the messages exactly the same so IDEs will
+            // hopefully ignore the second diagnostic!
+
+            // otherwise, it's not a safe cast.
+            dc->getASTContext()
+                .Diags
+                .diagnose(funcConv->getLoc(),
+                          diag::converting_func_loses_global_actor, fromType,
+                          toType, fromActor)
+                .limitBehavior(dc->getASTContext().isSwiftVersionAtLeast(6)
+                                   ? DiagnosticBehavior::Error
+                                   : DiagnosticBehavior::Warning);
+            }
+          }
+        }
+      }
+    }
+
     /// Check closure captures for Sendable violations.
     void checkClosureCaptures(AbstractClosureExpr *closure) {
       SmallVector<CapturedValue, 2> captures;
@@ -1994,6 +2064,11 @@ namespace {
         }
       }
 
+      // The constraint solver may not have chosen legal casts.
+      if (auto funcConv = dyn_cast<FunctionConversionExpr>(expr)) {
+        checkFunctionConversion(funcConv);
+      }
+
       return Action::Continue(expr);
     }
 

test/Concurrency/actor_call_implicitly_async.swift

@@ -298,7 +298,8 @@ func blender(_ peeler : () -> Void) {
   // expected-warning@-1 3{{non-sendable type 'Any' passed in call to global actor 'BananaActor'-isolated function cannot cross actor boundary}}
 
   blender((peelBanana))
-  // expected-warning@-1{{converting function value of type '@BananaActor () -> ()' to '() -> Void' loses global actor 'BananaActor'}}
+  // expected-warning@-1 2{{converting function value of type '@BananaActor () -> ()' to '() -> Void' loses global actor 'BananaActor'}}
+
   await wisk(peelBanana)
   // expected-warning@-1{{non-sendable type 'Any' passed in call to global actor 'BananaActor'-isolated function cannot cross actor boundary}}
 

test/Concurrency/actor_isolation.swift

@@ -463,7 +463,7 @@ func testGlobalActorClosures() {
     return 17
   }
 
-  acceptConcurrentClosure { @SomeGlobalActor in 5 } // expected-warning{{converting function value of type '@SomeGlobalActor @Sendable () -> Int' to '@Sendable () -> Int' loses global actor 'SomeGlobalActor'}}
+  acceptConcurrentClosure { @SomeGlobalActor in 5 } // expected-warning 2{{converting function value of type '@SomeGlobalActor @Sendable () -> Int' to '@Sendable () -> Int' loses global actor 'SomeGlobalActor'}}
 }
 
 @available(SwiftStdlib 5.1, *)