Rewrite the type of nonisolated(nonsending) closures.

rjmccall · rjmccall · commit 92b9857e6ab6 · 2025-07-28T22:20:38.000-04:00
The constraint solver does not reliably give closures a function type
that includes `nonisolated(noncaller)`, even when the immediate context
requires a conversion to such a type. We were trying to work around this
in SILGen, but the peephole only kicked in if the types matched exactly,
so a contextual conversion that e.g. added `throws` was still emitting
the closure as `@concurrent`, which is of course the wrong semantics.
It's relatively easy to avoid all this by just rewriting the closure's
type to include `nonisolated(nonsending)` at a point where we can reliably
decide that, and then SILGen doesn't have to peephole anything for
correctness.

Fixes rdar://155313349
diff --git a/lib/SILGen/SILGenExpr.cpp b/lib/SILGen/SILGenExpr.cpp
@@ -498,9 +498,6 @@ namespace {
     emitFunctionCvtFromExecutionCallerToGlobalActor(FunctionConversionExpr *E,
                                                     SGFContext C);
 
-    RValue emitFunctionCvtForNonisolatedNonsendingClosureExpr(
-        FunctionConversionExpr *E, SGFContext C);
-
     RValue visitActorIsolationErasureExpr(ActorIsolationErasureExpr *E,
                                           SGFContext C);
     RValue visitExtractFunctionIsolationExpr(ExtractFunctionIsolationExpr *E,
@@ -2035,44 +2032,6 @@ RValueEmitter::emitFunctionCvtToExecutionCaller(FunctionConversionExpr *e,
   return RValue(SGF, e, destType, result);
 }
 
-RValue RValueEmitter::emitFunctionCvtForNonisolatedNonsendingClosureExpr(
-    FunctionConversionExpr *E, SGFContext C) {
-  // The specific AST pattern for this looks as follows:
-  //
-  //   (function_conversion_expr type="nonisolated(nonsending) () async -> Void"
-  //      (closure_expr type="() async -> ()" isolated_to_caller_isolation))
-  CanAnyFunctionType destType =
-      cast<FunctionType>(E->getType()->getCanonicalType());
-  auto subExpr = E->getSubExpr()->getSemanticsProvidingExpr();
-
-  // If we do not have a closure or if that closure is not caller isolation
-  // inheriting, bail.
-  auto *closureExpr = dyn_cast<ClosureExpr>(subExpr);
-  if (!closureExpr ||
-      !closureExpr->getActorIsolation().isCallerIsolationInheriting())
-    return RValue();
-
-  // Then grab our closure type... make sure it is non isolated and then make
-  // sure it is the same as our destType but with nonisolated.
-  CanAnyFunctionType closureType =
-      cast<FunctionType>(closureExpr->getType()->getCanonicalType());
-  if (!closureType->getIsolation().isNonIsolated() ||
-      closureType !=
-          destType->withIsolation(FunctionTypeIsolation::forNonIsolated())
-              ->getCanonicalType())
-    return RValue();
-
-  // NOTE: This is a partial inline of getClosureTypeInfo. We do this so we have
-  // more control and make this change less viral in the compiler for 6.2.
-  auto newExtInfo = closureType->getExtInfo().withIsolation(
-      FunctionTypeIsolation::forNonIsolatedCaller());
-  closureType = closureType.withExtInfo(newExtInfo);
-  auto info = SGF.getFunctionTypeInfo(closureType);
-
-  auto closure = emitClosureReference(closureExpr, info);
-  return RValue(SGF, closureExpr, destType, closure);
-}
-
 RValue RValueEmitter::emitFunctionCvtFromExecutionCallerToGlobalActor(
     FunctionConversionExpr *e, SGFContext C) {
   // We are pattern matching a conversion sequence like the following:
@@ -2185,26 +2144,7 @@ RValue RValueEmitter::visitFunctionConversionExpr(FunctionConversionExpr *e,
   // convention.
   auto subExpr = e->getSubExpr()->getSemanticsProvidingExpr();
 
-  // Before we go any further into emitting the convert function expr, see if
-  // our SubExpr is a ClosureExpr with the exact same type as our
-  // FunctionConversionExpr except with the FunctionConversionExpr adding
-  // nonisolated(nonsending). Then see if the ClosureExpr itself (even though it
-  // is not nonisolated(nonsending) typed is considered to have
-  // nonisolated(nonsending) isolation. In such a case, emit the closure
-  // directly. We are going to handle it especially in closure emission to work
-  // around the missing information in the type.
-  //
-  // DISCUSSION: We need to do this here since in the Expression TypeChecker we
-  // do not have access to capture information when we would normally want to
-  // mark the closure type as being nonisolated(nonsending). As a result, we
-  // cannot know if the nonisolated(nonsending) should be overridden by for
-  // example an actor that is captured by the closure. So to work around this in
-  // Sema, we still mark the ClosureExpr as having the appropriate isolation
-  // even though its type does not have it... and then we work around this here
-  // and also in getClosureTypeInfo.
-  if (destType->getIsolation().isNonIsolatedCaller())
-    if (auto rv = emitFunctionCvtForNonisolatedNonsendingClosureExpr(e, C))
-      return rv;
+
 
   // Look through `as` type ascriptions that don't induce bridging too.
   while (auto subCoerce = dyn_cast<CoerceExpr>(subExpr)) {
diff --git a/lib/Sema/TypeCheckConcurrency.cpp b/lib/Sema/TypeCheckConcurrency.cpp
@@ -3329,7 +3329,22 @@ namespace {
       }
 
       if (auto *closure = dyn_cast<AbstractClosureExpr>(expr)) {
-        closure->setActorIsolation(determineClosureIsolation(closure));
+        auto isolation = determineClosureIsolation(closure);
+        closure->setActorIsolation(isolation);
+
+        // There is a case in which the constraint solver cannot decide
+        // that a closure is `nonisolated(nonsending)` because it cannot
+        // analyze the captures, but the closure isolation logic can.
+        // Rewrite the closure type at this point.
+        if (isolation.isCallerIsolationInheriting()) {
+          auto fnType = closure->getType()->castTo<AnyFunctionType>();
+          if (!fnType->getIsolation().isNonIsolatedCaller()) {
+            fnType = fnType->withIsolation(
+              FunctionTypeIsolation::forNonIsolatedCaller());
+            closure->setType(fnType);
+          }
+        }
+
         checkLocalCaptures(closure);
         contextStack.push_back(closure);
         return Action::Continue(expr);
diff --git a/test/Concurrency/attr_execution/attr_execution.swift b/test/Concurrency/attr_execution/attr_execution.swift
@@ -18,7 +18,7 @@ func callerTest() async {}
 struct Test {
   // CHECK-LABEL: // closure #1 in variable initialization expression of Test.x
   // CHECK: // Isolation: caller_isolation_inheriting
-  // CHECK: sil private [ossa] @$s14attr_execution4TestV1xyyYaYCcvpfiyyYacfU_ : $@convention(thin) @async (@sil_isolated @sil_implicit_leading_param @guaranteed Optional<any Actor>) -> ()
+  // CHECK: sil private [ossa] @$s14attr_execution4TestV1xyyYaYCcvpfiyyYaYCcfU_ : $@convention(thin) @async (@sil_isolated @sil_implicit_leading_param @guaranteed Optional<any Actor>) -> ()
   var x: () async -> Void = {}
 
   // CHECK-LABEL: // Test.test()
@@ -67,15 +67,15 @@ func takesClosure(fn: () async -> Void) {
 }
 
 // CHECK-LABEL: sil hidden [ossa] @$s14attr_execution11testClosureyyF : $@convention(thin) () -> ()
-// CHECK:  [[CLOSURE:%.*]] = function_ref @$s14attr_execution11testClosureyyFyyYaXEfU_ : $@convention(thin) @async (@sil_isolated @sil_implicit_leading_param @guaranteed Optional<any Actor>) -> ()
+// CHECK:  [[CLOSURE:%.*]] = function_ref @$s14attr_execution11testClosureyyFyyYaYCXEfU_ : $@convention(thin) @async (@sil_isolated @sil_implicit_leading_param @guaranteed Optional<any Actor>) -> ()
 // CHECK:  [[THUNKED_CLOSURE:%.*]] = thin_to_thick_function %0 to $@noescape @async @callee_guaranteed (@sil_isolated @sil_implicit_leading_param @guaranteed Optional<any Actor>) -> ()
 // CHECK:  [[TAKES_CLOSURE:%.*]] = function_ref @$s14attr_execution12takesClosure2fnyyyYaYCXE_tF : $@convention(thin) (@guaranteed @noescape @async @callee_guaranteed (@sil_isolated @sil_implicit_leading_param @guaranteed Optional<any Actor>) -> ()) -> ()
 // CHECK:  apply [[TAKES_CLOSURE]]([[THUNKED_CLOSURE]])
 // CHECK: } // end sil function '$s14attr_execution11testClosureyyF'
 
 // CHECK-LABEL: // closure #1 in testClosure()
 // CHECK: // Isolation: caller_isolation_inheriting
-// CHECK: sil private [ossa] @$s14attr_execution11testClosureyyFyyYaXEfU_ : $@convention(thin) @async (@sil_isolated @sil_implicit_leading_param @guaranteed Optional<any Actor>) -> ()
+// CHECK: sil private [ossa] @$s14attr_execution11testClosureyyFyyYaYCXEfU_ : $@convention(thin) @async (@sil_isolated @sil_implicit_leading_param @guaranteed Optional<any Actor>) -> ()
 func testClosure() {
   takesClosure {
   }
diff --git a/test/Concurrency/attr_execution/conversions_silgen.swift b/test/Concurrency/attr_execution/conversions_silgen.swift
@@ -263,15 +263,6 @@ public func testConcurrentCallerLocalVariables(_ x: @escaping @concurrent () asy
 
 // CHECK: } // end sil function '$s21attr_execution_silgen22globalActorConversionsyyyyYac_yyYaYCctYaF'
 
-// FIVE-LABEL: sil shared [transparent] [serialized] [reabstraction_thunk] [ossa] @$sScA_pSgIegHgIL_IegH_TRScMTU : $@convention(thin) @async (@guaranteed @async @callee_guaranteed (@sil_isolated @sil_implicit_leading_param @guaranteed Optional<any Actor>) -> ()) -> () {
-// FIVE: bb0([[FUNC:%.*]] : @guaranteed
-// FIVE:   [[ACTOR:%.*]] = apply {{%.*}}({{%.*}}) : $@convention(method) (@thick MainActor.Type) -> @owned MainActor
-// FIVE:   [[E:%.*]] = init_existential_ref [[ACTOR]] : $MainActor : $MainActor, $any Actor
-// FIVE:   [[E_OPT:%.*]] = enum $Optional<any Actor>, #Optional.some!enumelt, [[E]]
-// FIVE:   hop_to_executor [[E_OPT]]
-// FIVE:   apply [[FUNC]]([[E_OPT]]) : $@async @callee_guaranteed (@sil_isolated @sil_implicit_leading_param @guaranteed Optional<any Actor>) -> ()
-// FIVE: } // end sil function '$sScA_pSgIegHgIL_IegH_TRScMTU'
-
 // SIX-LABEL: sil shared [transparent] [serialized] [reabstraction_thunk] [ossa] @$sScA_pSgIetHgIL_IeghH_TRScMTU : $@convention(thin) @Sendable @async (@convention(thin) @async (@sil_isolated @sil_implicit_leading_param @guaranteed Optional<any Actor>) -> ()) -> () {
 // SIX: bb0([[FUNC:%.*]] : $@convention(thin) @async (@sil_isolated
 // SIX:   [[ACTOR:%.*]] = apply {{%.*}}({{%.*}}) : $@convention(method) (@thick MainActor.Type) -> @owned MainActor
@@ -515,7 +506,7 @@ func conversionsFromSyncToAsync(_ x: @escaping @Sendable (NonSendableKlass) -> V
 }
 
 func testThatClosuresAssumeIsolation(fn: inout nonisolated(nonsending) (Int) async -> Void) {
-  // CHECK-LABEL: sil private [ossa] @$s21attr_execution_silgen31testThatClosuresAssumeIsolation2fnyySiYaYCcz_tFyyYacfU_ : $@convention(thin) @async (@sil_isolated @sil_implicit_leading_param @guaranteed Optional<any Actor>) -> () {
+  // CHECK-LABEL: sil private [ossa] @$s21attr_execution_silgen31testThatClosuresAssumeIsolation2fnyySiYaYCcz_tFyyYaYCcfU_ : $@convention(thin) @async (@sil_isolated @sil_implicit_leading_param @guaranteed Optional<any Actor>) -> () {
   // CHECK: bb0([[EXECUTOR:%.*]] : @guaranteed $Optional<any Actor>):
   // CHECK: hop_to_executor [[EXECUTOR]]
   let _: nonisolated(nonsending) () async -> Void = {
@@ -524,31 +515,24 @@ func testThatClosuresAssumeIsolation(fn: inout nonisolated(nonsending) (Int) asy
 
   func testParam(_: nonisolated(nonsending) () async throws -> Void) {}
 
-  // CHECK-LABEL: sil private [ossa] @$s21attr_execution_silgen31testThatClosuresAssumeIsolation2fnyySiYaYCcz_tFyyYaXEfU0_ : $@convention(thin) @async (@sil_isolated @sil_implicit_leading_param @guaranteed Optional<any Actor>) -> () {
+  // CHECK-LABEL: sil private [ossa] @$s21attr_execution_silgen31testThatClosuresAssumeIsolation2fnyySiYaYCcz_tFyyYaYCXEfU0_ : $@convention(thin) @async (@sil_isolated @sil_implicit_leading_param @guaranteed Optional<any Actor>) -> @error any Error {
   // CHECK: bb0([[EXECUTOR:%.*]] : @guaranteed $Optional<any Actor>):
   // CHECK: hop_to_executor [[EXECUTOR]]
-  // CHECK: } // end sil function '$s21attr_execution_silgen31testThatClosuresAssumeIsolation2fnyySiYaYCcz_tFyyYaXEfU0_'
+  // CHECK: } // end sil function '$s21attr_execution_silgen31testThatClosuresAssumeIsolation2fnyySiYaYCcz_tFyyYaYCXEfU0_'
+  testParam { 42 }
 
-  // FIVE-LABEL: sil shared [transparent] [serialized] [reabstraction_thunk] [ossa] @$sScA_pSgIetHgIL_IegH_TR : $@convention(thin) @async (@convention(thin) @async (@sil_isolated @sil_implicit_leading_param @guaranteed Optional<any Actor>) -> ()) -> () {
-  // FIVE: bb0([[FUNC:%.*]] : $@convention(thin) @async (@sil_isolated @sil_implicit_leading_param @guaranteed Optional<any Actor>) -> ()):
-  // FIVE:   [[ACTOR:%.*]] = enum $Optional<any Actor>, #Optional.none!enumelt
-  // FIVE:   hop_to_executor [[ACTOR]]
-  // FIVE:   apply [[FUNC]]([[ACTOR]])
-  // FIVE: } // end sil function '$sScA_pSgIetHgIL_IegH_TR'
+  // CHECK-LABEL: sil private [ossa] @$s21attr_execution_silgen31testThatClosuresAssumeIsolation2fnyySiYaYCcz_tFyyYaXEfU1_ : $@convention(thin) @async () -> ()
+  // CHECK: [[GENERIC_EXECUTOR:%.*]] = enum $Optional<Builtin.Executor>, #Optional.none!enumelt
+  // CHECK: hop_to_executor [[GENERIC_EXECUTOR]]
+  testParam { @concurrent in 42 }
 
   // CHECK-LABEL: sil shared [transparent] [serialized] [reabstraction_thunk] [ossa] @$sIgH_ScA_pSgs5Error_pIegHgILzo_TR : $@convention(thin) @async (@sil_isolated @sil_implicit_leading_param @guaranteed Optional<any Actor>, @guaranteed @noescape @async @callee_guaranteed () -> ()) -> @error any Error {
   // CHECK: bb0([[ACTOR:%.*]] : @guaranteed $Optional<any Actor>, [[FUNC:%.*]] : @guaranteed $@noescape @async @callee_guaranteed () -> ()):
   // CHECK:   apply [[FUNC]]()
   // CHECK:   hop_to_executor [[ACTOR]]
   // CHECK: } // end sil function '$sIgH_ScA_pSgs5Error_pIegHgILzo_TR'
-  testParam { 42 }
 
-  // CHECK-LABEL: sil private [ossa] @$s21attr_execution_silgen31testThatClosuresAssumeIsolation2fnyySiYaYCcz_tFyyYaXEfU1_ : $@convention(thin) @async () -> ()
-  // CHECK: [[GENERIC_EXECUTOR:%.*]] = enum $Optional<Builtin.Executor>, #Optional.none!enumelt
-  // CHECK: hop_to_executor [[GENERIC_EXECUTOR]]
-  testParam { @concurrent in 42 }
-
-  // CHECK-LABEL: sil private [ossa] @$s21attr_execution_silgen31testThatClosuresAssumeIsolation2fnyySiYaYCcz_tFySiYacfU2_ : $@convention(thin) @async (@sil_isolated @sil_implicit_leading_param @guaranteed Optional<any Actor>, Int) -> () {
+  // CHECK-LABEL: sil private [ossa] @$s21attr_execution_silgen31testThatClosuresAssumeIsolation2fnyySiYaYCcz_tFySiYaYCcfU2_ : $@convention(thin) @async (@sil_isolated @sil_implicit_leading_param @guaranteed Optional<any Actor>, Int) -> () {
   // CHECK: bb0([[EXECUTOR:%.*]] : @guaranteed $Optional<any Actor>, %1 : $Int):
   // CHECK: hop_to_executor [[EXECUTOR]]
   fn = { _ in }
@@ -588,3 +572,44 @@ func testClosuresDontAssumeGlobalActorWithMarkedAsConcurrent() {
   test { @Sendable @concurrent in
   }
 }
+
+nonisolated(nonsending)
+public func takesCallerIsolatedThrowingFunction<T>(
+  _ operation: nonisolated(nonsending) () async throws -> T
+) async rethrows -> T {
+  try await operation()
+}
+
+func observe() {}
+
+// Test that we emit closures with nonisolated(nonsending) isolation without
+// introducing an intermediate @concurrent closure function.
+func testConvertToThrowing(isolation: isolated (any Actor)? = #isolation) async {
+  // CHECK-LABEL: sil hidden [ossa] @$s21attr_execution_silgen21testConvertToThrowing9isolationyScA_pSgYi_tYaF :
+  // CHECK:         [[ACTOR_COPY:%.*]] = copy_value %0
+  // CHECK-NEXT:    [[ACTOR_BORROW:%.*]] = begin_borrow [[ACTOR_COPY]]
+  // CHECK-NEXT:    hop_to_executor [[ACTOR_BORROW]]
+  // CHECK:         [[CLOSURE:%.*]] = function_ref @$s21attr_execution_silgen21testConvertToThrowing9isolationyScA_pSgYi_tYaFyyYaYCXEfU_ :
+  // CHECK-NEXT:    [[CLOSURE_VALUE:%.*]] = thin_to_thick_function [[CLOSURE]] to
+  // CHECK-NEXT:    // function_ref
+  // CHECK-NEXT:    [[FN:%.*]] = function_ref
+  // CHECK-NEXT:    try_apply [[FN]]<()>({{%.*}}, {{%.*}}, [[CLOSURE_VALUE]]) {{.*}}, normal bb1, error bb2
+  // CHECK:       bb1(
+  //   This hop is unnecessary because nonisolated(nonsending) should
+  //   preserve isolation on return.
+  // CHECK-NEXT:    hop_to_executor [[ACTOR_BORROW]]
+
+  // CHECK-LABEL: sil private [ossa] @$s21attr_execution_silgen21testConvertToThrowing9isolationyScA_pSgYi_tYaFyyYaYCXEfU_ : $@convention(thin) @async @substituted <τ_0_0> (@sil_isolated @sil_implicit_leading_param @guaranteed Optional<any Actor>) -> (@out τ_0_0, @error any Error) for <()>
+  // CHECK:      bb0(
+  // CHECK-NEXT:   debug_value
+  //   This hop is unnecessary because nonisolated(nonsending) should
+  //   ensure isolation before call.
+  // CHECK-NEXT:   hop_to_executor %1
+  // CHECK-NEXT:   // function_ref observe()
+  // CHECK-NEXT:   [[FN:%.*]] = function_ref @$s21attr_execution_silgen7observeyyF :
+  // CHECK-NEXT:   apply [[FN]]()
+
+  await takesCallerIsolatedThrowingFunction {
+    observe()
+  }
+}
