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

It's ok to drop the global-actor qualifier `@G` from a function's type if:

- the cast is happening in a context isolated to global-actor `G`
- the function value will not be `@Sendable`
- the function value is not `async`

It's primarily safe to drop the attribute because we're already in the
same isolation domain. So it's OK to simply drop the global-actor
if we prevent the value from later leaving that isolation domain.
This means we no longer need to warn about code like this:

```
@mainactor func doIt(_ x: [Int], _ f: @mainactor (Int) -> ()) {
  x.forEach(f)
// warning: converting function value of type '@mainactor (Int) -> ()' to '(Int) throws -> Void' loses global actor 'MainActor'
}
```

NOTE: this implementation is a bit gross in that the constraint solver
might emit false warnings about casts it introduced that are actually
safe. This is mainly because closure isolation is only fully determined
after constraint solving. See the FIXME's for more details.

resolves rdar://94462333

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, *)