Handle all isolation checking for function calls in one place

Isolation checking for calls had two separate implementation places:
one that looked at the declaration being called (for member
declarations) and one that worked on the actual call expression. Unify
on the latter implementation, which is more general and has access to
the specific call arguments. Improve diagnostics here somewher so we
don't regress in that area.

This refactoring shouldn't change the actual semantics, but it makes
upcoming semantic changes easier.

Author: DougGregor

include/swift/AST/DiagnosticsSema.def

@@ -5239,9 +5239,9 @@ WARNING(non_sendable_param_type,none,
         "in parameter of superclass method overridden by %4 %2 %3|"
         "in parameter of %4 '@objc' %2 %3}1 cannot cross actor boundary",
         (Type, unsigned, DescriptiveDeclKind, DeclName, ActorIsolation))
-WARNING(non_sendable_call_param_type,none,
-        "non-sendable type %0 passed in %select{implicitly asynchronous |}1"
-        "call to %2 function cannot cross actor boundary",
+WARNING(non_sendable_call_argument,none,
+        "passing argument of non-sendable type %0 %select{into %2 context|"
+        "outside of %2 context}1 may introduce data races",
         (Type, bool, ActorIsolation))
 WARNING(non_sendable_result_type,none,
         "non-sendable type %0 returned by %select{call to %4 %2 %3|"

lib/Sema/TypeCheckConcurrency.cpp

@@ -2177,11 +2177,6 @@ namespace {
       }
 
       if (auto apply = dyn_cast<ApplyExpr>(expr)) {
-        applyStack.push_back(apply);  // record this encounter
-
-        // Check the call itself.
-        (void)checkApply(apply);
-
         // If this is a call to a partial apply thunk, decompose it to check it
         // like based on the original written syntax, e.g., "self.method".
         if (auto partialApply = decomposePartialApplyThunk(
@@ -2195,45 +2190,31 @@ namespace {
 
             partialApply->base->walk(*this);
 
-            // manual clean-up since normal traversal is skipped
-            assert(applyStack.back() == apply);
-            applyStack.pop_back();
-
             return Action::SkipChildren(expr);
           }
         }
-      }
-
-      // NOTE: SelfApplyExpr is a subtype of ApplyExpr
-      if (auto call = dyn_cast<SelfApplyExpr>(expr)) {
-        Expr *fn = call->getFn()->getValueProvidingExpr();
-        if (auto memberRef = findReference(fn)) {
-          checkReference(
-              call->getBase(), memberRef->first, memberRef->second,
-              /*partialApply=*/None, call);
 
-          call->getBase()->walk(*this);
+        applyStack.push_back(apply);  // record this encounter
 
+        if (isa<SelfApplyExpr>(apply)) {
+          // Self applications are checked as part of the outer call.
+          // However, we look for inout issues here.
           if (applyStack.size() >= 2) {
             ApplyExpr *outerCall = applyStack[applyStack.size() - 2];
             if (isAsyncCall(outerCall)) {
               // This call is a partial application within an async call.
               // If the partial application take a value inout, it is bad.
               if (InOutExpr *inoutArg = dyn_cast<InOutExpr>(
-                      call->getBase()->getSemanticsProvidingExpr()))
+                     apply->getArgs()->getExpr(0)->getSemanticsProvidingExpr()))
                 diagnoseInOutArg(outerCall, inoutArg, true);
             }
           }
-
-          // manual clean-up since normal traversal is skipped
-          assert(applyStack.back() == dyn_cast<ApplyExpr>(expr));
-          applyStack.pop_back();
-
-          return Action::SkipChildren(expr);
+        } else {
+          // Check the call itself.
+          (void)checkApply(apply);
         }
       }
 
-
       if (auto keyPath = dyn_cast<KeyPathExpr>(expr))
         checkKeyPathExpr(keyPath);
 
@@ -2797,26 +2778,69 @@ namespace {
         return *contextIsolation;
       };
 
-      // If the function type is global-actor-qualified, determine whether
-      // we are within that global actor already.
+      // Default the call options to allow promotion to async, if it will be
+      // warranted.
+      ActorReferenceResult::Options callOptions;
+      if (!fnType->getExtInfo().isAsync())
+        callOptions |= ActorReferenceResult::Flags::AsyncPromotion;
+
+      // Determine from the callee whether actor isolation is unsatisfied.
       Optional<ActorIsolation> unsatisfiedIsolation;
+      bool mayExitToNonisolated = true;
+      auto calleeDecl = apply->getCalledValue(/*skipFunctionConversions=*/true);
       if (Type globalActor = fnType->getGlobalActor()) {
+        // If the function type is global-actor-qualified, determine whether
+        // we are within that global actor already.
         if (!(getContextIsolation().isGlobalActor() &&
             getContextIsolation().getGlobalActor()->isEqual(globalActor))) {
           unsatisfiedIsolation = ActorIsolation::forGlobalActor(
               globalActor, /*unsafe=*/false);
         }
-      }
 
-      if (isa<SelfApplyExpr>(apply) && !unsatisfiedIsolation)
+        mayExitToNonisolated = false;
+      } else if (auto selfApplyFn = dyn_cast<SelfApplyExpr>(
+                    apply->getFn()->getValueProvidingExpr())) {
+        // If we're calling a member function, check whether the function
+        // itself is isolated.
+        auto memberFn = selfApplyFn->getFn()->getValueProvidingExpr();
+        if (auto memberRef = findReference(memberFn)) {
+          auto isolatedActor = getIsolatedActor(selfApplyFn->getBase());
+          auto result = ActorReferenceResult::forReference(
+              memberRef->first, selfApplyFn->getLoc(), getDeclContext(),
+              kindOfUsage(memberRef->first.getDecl(), selfApplyFn),
+              isolatedActor, None, None, getClosureActorIsolation);
+          switch (result) {
+          case ActorReferenceResult::SameConcurrencyDomain:
+            break;
+
+          case ActorReferenceResult::ExitsActorToNonisolated:
+            unsatisfiedIsolation = ActorIsolation::forIndependent();
+            break;
+
+          case ActorReferenceResult::EntersActor:
+            unsatisfiedIsolation = result.isolation;
+            break;
+          }
+
+          callOptions = result.options;
+          mayExitToNonisolated = false;
+          calleeDecl = memberRef->first.getDecl();
+        }
+      } else if (calleeDecl &&
+                 calleeDecl->getAttrs()
+                     .hasAttribute<UnsafeInheritExecutorAttr>()) {
         return false;
+      }
 
       // Check for isolated parameters.
+      bool anyIsolatedParameters = false;
       for (unsigned paramIdx : range(fnType->getNumParams())) {
         // We only care about isolated parameters.
         if (!fnType->getParams()[paramIdx].isIsolated())
           continue;
 
+        anyIsolatedParameters = true;
+
         auto *args = apply->getArgs();
         if (paramIdx >= args->size())
           continue;
@@ -2840,14 +2864,22 @@ namespace {
         break;
       }
 
+      // If we're calling an async function that's nonisolated, and we're in
+      // an isolated context, then we're exiting the actor context.
+      if (mayExitToNonisolated && !anyIsolatedParameters && fnType->isAsync() &&
+          getContextIsolation().isActorIsolated())
+        unsatisfiedIsolation = ActorIsolation::forIndependent();
+
       // If there was no unsatisfied actor isolation, we're done.
       if (!unsatisfiedIsolation)
         return false;
 
+      bool requiresAsync =
+          callOptions.contains(ActorReferenceResult::Flags::AsyncPromotion);
+
       // If we are not in an asynchronous context, complain.
-      if (!getDeclContext()->isAsyncContext()) {
-        if (auto calleeDecl = apply->getCalledValue(
-                /*skipFunctionConversions=*/true)) {
+      if (requiresAsync && !getDeclContext()->isAsyncContext()) {
+        if (calleeDecl) {
           ctx.Diags.diagnose(
               apply->getLoc(), diag::actor_isolated_call_decl,
               *unsatisfiedIsolation,
@@ -2873,8 +2905,8 @@ namespace {
         return true;
       }
 
-      // Mark as implicitly async.
-      if (!fnType->getExtInfo().isAsync() && unsatisfiedIsolation) {
+      // If needed, mark as implicitly async.
+      if (requiresAsync) {
         apply->setImplicitlyAsync(*unsatisfiedIsolation);
       }
 
@@ -2891,11 +2923,13 @@ namespace {
             argLoc = arg.getStartLoc();
         }
 
+        bool isExiting = !unsatisfiedIsolation->isActorIsolated();
+        ActorIsolation diagnoseIsolation = isExiting ? getContextIsolation()
+                                                     : *unsatisfiedIsolation;
         if (diagnoseNonSendableTypes(
                 param.getParameterType(), getDeclContext(), argLoc,
-                diag::non_sendable_call_param_type,
-                apply->isImplicitlyAsync().has_value(),
-                *unsatisfiedIsolation))
+                diag::non_sendable_call_argument,
+                isExiting, diagnoseIsolation))
           return true;
       }
 
@@ -3088,6 +3122,16 @@ namespace {
         return false;
 
       auto decl = declRef.getDecl();
+
+      // If this declaration is a callee from the enclosing application,
+      // it's already been checked via the call.
+      if (!applyStack.empty()) {
+        auto immediateCallee =
+            applyStack.back()->getCalledValue(/*skipFunctionConversions=*/true);
+        if (decl == immediateCallee)
+          return false;
+      }
+
       Optional<ReferencedActor> isolatedActor;
       if (base)
         isolatedActor.emplace(getIsolatedActor(base));
@@ -5498,7 +5542,7 @@ ActorReferenceResult ActorReferenceResult::forReference(
       declIsolation = declIsolation.subst(declRef.getSubstitutions());
   }
 
-  // If the entity we are referencing is not a value, we're in thesame
+  // If the entity we are referencing is not a value, we're in the same
   // concurrency domain.
   if (isNonValueReference(declRef.getDecl()))
     return forSameConcurrencyDomain(declIsolation);

test/Concurrency/actor_call_implicitly_async.swift

@@ -244,7 +244,7 @@ func regularFunc() {
 
   _ = a.deposit //expected-error{{actor-isolated instance method 'deposit' can not be partially applied}}
 
-  _ = a.deposit(1)  // expected-error{{actor-isolated instance method 'deposit' can not be referenced from a non-isolated context}}
+  _ = a.deposit(1)  // expected-error{{call to actor-isolated instance method 'deposit' in a synchronous nonisolated context}}
 }
 
 
@@ -289,29 +289,29 @@ func blender(_ peeler : () -> Void) {
 
 
   await wisk({})
-  // expected-warning@-1{{non-sendable type 'Any' passed in call to global actor 'BananaActor'-isolated function cannot cross actor boundary}}
+  // expected-warning@-1{{passing argument of non-sendable type 'Any' into global actor 'BananaActor'-isolated context may introduce data races}}
   await wisk(1)
-  // expected-warning@-1{{non-sendable type 'Any' passed in call to global actor 'BananaActor'-isolated function cannot cross actor boundary}}
+  // expected-warning@-1{{passing argument of non-sendable type 'Any' into global actor 'BananaActor'-isolated context may introduce data races}}
   await (peelBanana)()
   await (((((peelBanana)))))()
   await (((wisk)))((wisk)((wisk)(1)))
-  // expected-warning@-1 3{{non-sendable type 'Any' passed in call to global actor 'BananaActor'-isolated function cannot cross actor boundary}}
+  // expected-warning@-1 3{{passing argument of non-sendable type 'Any' into global actor 'BananaActor'-isolated context may introduce data races}}
 
   blender((peelBanana))
   // 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}}
+  // expected-warning@-1{{passing argument of non-sendable type 'Any' into global actor 'BananaActor'-isolated context may introduce data races}}
 
   await wisk(wisk)
-  // expected-warning@-1{{non-sendable type 'Any' passed in call to global actor 'BananaActor'-isolated function cannot cross actor boundary}}
+  // expected-warning@-1{{passing argument of non-sendable type 'Any' into global actor 'BananaActor'-isolated context may introduce data races}}
   await (((wisk)))(((wisk)))
-  // expected-warning@-1{{non-sendable type 'Any' passed in call to global actor 'BananaActor'-isolated function cannot cross actor boundary}}
+  // expected-warning@-1{{passing argument of non-sendable type 'Any' into global actor 'BananaActor'-isolated context may introduce data races}}
 
-  // expected-warning@+1 {{non-sendable type 'Any' passed in call to global actor 'BananaActor'-isolated function cannot cross actor boundary}}
+  // expected-warning@+1 {{passing argument of non-sendable type 'Any' into global actor 'BananaActor'-isolated context may introduce data races}}
   await {wisk}()(1)
 
-  // expected-warning@+1 {{non-sendable type 'Any' passed in call to global actor 'BananaActor'-isolated function cannot cross actor boundary}}
+  // expected-warning@+1 {{passing argument of non-sendable type 'Any' into global actor 'BananaActor'-isolated context may introduce data races}}
   await (true ? wisk : {n in return})(1)
 }
 
@@ -368,14 +368,14 @@ actor Calculator {
   let calc = Calculator()
 
   let _ = (await calc.addCurried(1))(2)
-  // expected-warning@-1{{non-sendable type '(Int) -> Int' returned by implicitly asynchronous call to actor-isolated instance method 'addCurried' cannot cross actor boundary}}
+  // expected-warning@-1{{non-sendable type '(Int) -> Int' returned by call to actor-isolated function cannot cross actor boundary}}
   // expected-note@-2{{a function type must be marked '@Sendable' to conform to 'Sendable'}}
   let _ = await (await calc.addCurried(1))(2) // expected-warning{{no 'async' operations occur within 'await' expression}}
-  // expected-warning@-1{{non-sendable type '(Int) -> Int' returned by implicitly asynchronous call to actor-isolated instance method 'addCurried' cannot cross actor boundary}}
+  // expected-warning@-1{{non-sendable type '(Int) -> Int' returned by call to actor-isolated function cannot cross actor boundary}}
   // expected-note@-2{{a function type must be marked '@Sendable' to conform to 'Sendable'}}
 
   let plusOne = await calc.addCurried(await calc.add(0, 1))
-  // expected-warning@-1{{non-sendable type '(Int) -> Int' returned by implicitly asynchronous call to actor-isolated instance method 'addCurried' cannot cross actor boundary}}
+  // expected-warning@-1{{non-sendable type '(Int) -> Int' returned by call to actor-isolated function cannot cross actor boundary}}
   // expected-note@-2{{a function type must be marked '@Sendable' to conform to 'Sendable'}}
   let _ = plusOne(2)
 }