Merge pull request #79717 from xedin/objc-handlers-are-execution-caller

[Sema/SILGen] Import ObjC async functions as `nonisolated(nonsending)` by …

Author: xedin

lib/SIL/IR/SILFunctionType.cpp

@@ -1706,8 +1706,12 @@ class DestructureInputs {
 
     // If we are an async function that is unspecified or nonisolated, insert an
     // isolated parameter if AsyncCallerExecution is enabled.
+    //
+    // NOTE: The parameter is not inserted for async functions imported
+    // from ObjC because they are handled in a special way that doesn't
+    // require it.
     if (IsolationInfo && IsolationInfo->isCallerIsolationInheriting() &&
-        extInfoBuilder.isAsync()) {
+        extInfoBuilder.isAsync() && !Foreign.async) {
       auto actorProtocol = TC.Context.getProtocol(KnownProtocolKind::Actor);
       auto actorType =
           ExistentialType::get(actorProtocol->getDeclaredInterfaceType());

lib/SILGen/SILGenBridging.cpp

@@ -1373,11 +1373,16 @@ emitObjCThunkArguments(SILGenFunction &SGF, SILLocation loc, SILDeclRef thunk,
   auto inputs = objcFnTy->getParameters();
   auto nativeInputs = swiftFnTy->getParameters();
   auto fnConv = SGF.silConv.getFunctionConventions(swiftFnTy);
-  assert(nativeInputs.size() == bridgedFormalTypes.size());
-  assert(nativeInputs.size() == nativeFormalTypes.size());
+  bool nativeInputsHasImplicitIsolatedParam = false;
+  if (auto param = swiftFnTy->maybeGetIsolatedParameter())
+    nativeInputsHasImplicitIsolatedParam = param->hasOption(SILParameterInfo::ImplicitLeading);
+  assert(nativeInputs.size() - unsigned(nativeInputsHasImplicitIsolatedParam) == bridgedFormalTypes.size());
+  assert(nativeInputs.size() - unsigned(nativeInputsHasImplicitIsolatedParam) == nativeFormalTypes.size());
   assert(inputs.size() ==
            nativeInputs.size() + unsigned(foreignError.has_value())
-                               + unsigned(foreignAsync.has_value()));
+                               + unsigned(foreignAsync.has_value()) -
+         unsigned(nativeInputsHasImplicitIsolatedParam));
+
   for (unsigned i = 0, e = inputs.size(); i < e; ++i) {
     SILType argTy = SGF.getSILType(inputs[i], objcFnTy);
     SILValue arg = SGF.F.begin()->createFunctionArgument(argTy);
@@ -1423,13 +1428,13 @@ emitObjCThunkArguments(SILGenFunction &SGF, SILLocation loc, SILDeclRef thunk,
            + unsigned(foreignAsync.has_value())
         == objcFnTy->getParameters().size() &&
          "objc inputs don't match number of arguments?!");
-  assert(bridgedArgs.size() == swiftFnTy->getParameters().size() &&
+  assert(bridgedArgs.size() == swiftFnTy->getParameters().size() - bool(nativeInputsHasImplicitIsolatedParam) &&
          "swift inputs don't match number of arguments?!");
   assert((foreignErrorSlot || !foreignError) &&
          "didn't find foreign error slot");
 
   // Bridge the input types.
-  assert(bridgedArgs.size() == nativeInputs.size());
+  assert(bridgedArgs.size() == nativeInputs.size() - bool(nativeInputsHasImplicitIsolatedParam));
   for (unsigned i = 0, size = bridgedArgs.size(); i < size; ++i) {
     // Consider the bridged values to be "call results" since they're coming
     // from potentially nil-unsound ObjC callers.
@@ -1656,6 +1661,31 @@ void SILGenFunction::emitNativeToForeignThunk(SILDeclRef thunk) {
   // we will deallocate it too early.
   Scope argScope(Cleanups, CleanupLocation(loc));
 
+  // See if our native function has an implicitly isolated parameter. In such a
+  // case, we need to pass in the isolation.
+  if (auto isolatedParameter = substTy->maybeGetIsolatedParameter();
+      isolatedParameter && isolatedParameter->hasOption(SILParameterInfo::ImplicitLeading)) {
+    assert(F.isAsync() && "Can only be async");
+    assert(isolation && "No isolation?!");
+    switch (isolation->getKind()) {
+    case ActorIsolation::Unspecified:
+    case ActorIsolation::Nonisolated:
+    case ActorIsolation::NonisolatedUnsafe:
+    case ActorIsolation::CallerIsolationInheriting:
+      args.push_back(emitNonIsolatedIsolation(loc).getValue());
+      break;
+    case ActorIsolation::ActorInstance:
+      llvm::report_fatal_error("Should never see this");
+      break;
+    case ActorIsolation::GlobalActor:
+      args.push_back(emitLoadGlobalActorExecutor(isolation->getGlobalActor()));
+      break;
+    case ActorIsolation::Erased:
+      llvm::report_fatal_error("Should never see this");
+      break;
+    }
+  }
+
   // Bridge the arguments.
   SILValue foreignErrorSlot;
   SILValue foreignAsyncSlot;
@@ -2117,7 +2147,25 @@ void SILGenFunction::emitForeignToNativeThunk(SILDeclRef thunk) {
     indirectResult = F.begin()->createFunctionArgument(
         nativeConv.getSingleSILResultType(F.getTypeExpansionContext()));
   }
-  
+
+  // Before we do anything, see if our function type is async and has an
+  // implicit isolated parameter. In such a case, we need to implicitly insert
+  // it here before we insert other parameters.
+  //
+  // NOTE: We do not jump to it or do anything further since as mentioned above,
+  // we might switch to the callee's actor as part of making the call... but we
+  // don't need to do anything further than that because we're going to
+  // immediately return.
+  bool hasImplicitIsolatedParameter = false;
+  if (auto isolatedParameter = nativeFnTy->maybeGetIsolatedParameter();
+      isolatedParameter && nativeFnTy->isAsync() &&
+      isolatedParameter->hasOption(SILParameterInfo::ImplicitLeading)) {
+    auto loweredTy = getLoweredTypeForFunctionArgument(
+        isolatedParameter->getArgumentType(&F));
+    F.begin()->createFunctionArgument(loweredTy);
+    hasImplicitIsolatedParameter = true;
+  }
+
   // Forward the arguments.
   SmallVector<SILValue, 8> params;
 
@@ -2182,9 +2230,12 @@ void SILGenFunction::emitForeignToNativeThunk(SILDeclRef thunk) {
         getParameterTypes(nativeCI.LoweredType.getParams(), hasSelfParam);
 
       for (unsigned nativeParamIndex : indices(params)) {
+        // Adjust the parameter if we inserted an implicit isolated parameter.
+        unsigned nativeFnTyParamIndex = nativeParamIndex + hasImplicitIsolatedParameter;
+
         // Bring the parameter to +1.
         auto paramValue = params[nativeParamIndex];
-        auto thunkParam = nativeFnTy->getParameters()[nativeParamIndex];
+        auto thunkParam = nativeFnTy->getParameters()[nativeFnTyParamIndex];
         // TODO: Could avoid a retain if the bridged parameter is also +0 and
         // doesn't require a bridging conversion.
         ManagedValue param;

lib/Sema/TypeCheckConcurrency.cpp

@@ -5840,13 +5840,41 @@ computeDefaultInferredActorIsolation(ValueDecl *value) {
     // Use the overridden decl's isolation as the default isolation for this
     // decl.
     auto isolation = getOverriddenIsolationFor(value);
+
+    // If this is an override of an async completion handler, mark
+    // it `@concurrent` instead of inferring `nonisolated(nonsending)`
+    // to preserve pre-SE-0461 behavior.
+    if (isolation.isCallerIsolationInheriting() &&
+        overriddenValue->hasClangNode()) {
+      if (auto *AFD = dyn_cast<AbstractFunctionDecl>(overriddenValue)) {
+        if (AFD->getForeignAsyncConvention()) {
+          return {{ActorIsolation::forNonisolated(/*unsafe=*/false),
+                   IsolationSource(overriddenValue, IsolationSource::Override)},
+                  overriddenValue,
+                  isolation};
+        }
+      }
+    }
+
     return {{isolation,
              IsolationSource(overriddenValue, IsolationSource::Override)},
             overriddenValue,
             isolation}; // use the overridden decl's iso as the default
                         // isolation for this decl.
   }
 
+  // Asynchronous variants for functions imported from ObjC are
+  // `nonisolated(nonsending)` by default.
+  if (value->hasClangNode()) {
+    if (auto *AFD = dyn_cast<AbstractFunctionDecl>(value)) {
+      if (!isa<ProtocolDecl>(AFD->getDeclContext()) &&
+          AFD->getForeignAsyncConvention()) {
+        return {
+            {ActorIsolation::forCallerIsolationInheriting(), {}}, nullptr, {}};
+      }
+    }
+  }
+
   // We did not find anything special, return unspecified.
   return {{ActorIsolation::forUnspecified(), {}}, nullptr, {}};
 }

test/Concurrency/nonisolated_nonsending_objc_async_by_default.swift

@@ -0,0 +1,56 @@
+// RUN: %empty-directory(%t)
+// RUN: %empty-directory(%t/src)
+// RUN: split-file %s %t/src
+
+// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk) -typecheck %t/src/main.swift \
+// RUN:   -import-objc-header %t/src/Test.h \
+// RUN:   -swift-version 6 \
+// RUN:   -module-name main -I %t -verify
+
+// REQUIRES: objc_interop
+
+//--- Test.h
+#define MAIN_ACTOR __attribute__((__swift_attr__("@MainActor")))
+
+#pragma clang assume_nonnull begin
+
+@import Foundation;
+
+@interface Test : NSObject
+- (void)loadWithCompletionHandler:(void (^)(void)) completionHandler;
+@end
+
+MAIN_ACTOR
+@interface TestIsolated : NSObject
+- (void)loadWithCompletionHandler:(void (^)(void)) completionHandler;
+@end
+
+#pragma clang assume_nonnull end
+
+//--- main.swift
+
+class OverrideTest1 : Test {
+  override func load() async {}
+}
+
+class OverrideTest2 : TestIsolated {
+  override func load() async {}
+}
+
+func test(t: Test, i: TestIsolated) async throws {
+  let fn = t.load // nonisolated(nonsending) () async -> Void
+
+  let _: @isolated(any) () async -> Void = fn
+  // expected-error@-1 {{cannot convert value of type 'nonisolated(nonsending) () async -> Void' to specified type '@isolated(any) () async -> Void'}}
+
+  let isolatedFn = i.load
+  let _: () -> Void = isolatedFn
+  // expected-error@-1 {{invalid conversion from 'async' function of type '@MainActor @Sendable () async -> Void' to synchronous function type '() -> Void'}}
+}
+
+func testOverrides(o1: OverrideTest1, o2: OverrideTest2) {
+  let _: () -> Void = o1.load
+  // expected-error@-1 {{invalid conversion from 'async' function of type '() async -> ()' to synchronous function type '() -> Void'}}
+  let _: () -> Void = o2.load
+  // expected-error@-1 {{invalid conversion from 'async' function of type '@MainActor @Sendable () async -> ()' to synchronous function type '() -> Void'}}
+}