Merge pull request #70257 from hborla/isolated-default-values-upcoming-flag

[SE-0411] Promote `IsolatedDefaultValues` from an experimental feature to an upcoming feature.

Author: hborla

CHANGELOG.md

@@ -5,6 +5,28 @@
 
 ## Swift 5.11
 
+* [SE-0411][]:
+
+  Default value expressions can now have the same isolation as the enclosing
+  function or the corresponding stored property:
+
+  ```swift
+  @MainActor
+  func requiresMainActor() -> Int { ... }
+
+  class C {
+    @MainActor
+    var x: Int = requiresMainActor()
+  }
+
+  @MainActor func defaultArg(value: Int = requiresMainActor()) { ... }
+  ```
+
+  For isolated default values of stored properties, the implicit initialization
+  only happens in the body of an `init` with the same isolation. This closes
+  an important data-race safety hole where global-actor-isolated default values
+  could inadvertently run synchronously from outside the actor.
+
 * [SE-0413][]:
 
   Functions can now specify the type of error that they throw as part of the
@@ -9904,6 +9926,7 @@ using the `.dynamicType` member to retrieve the type of an expression should mig
 [SE-0394]: https://github.com/apple/swift-evolution/blob/main/proposals/0394-swiftpm-expression-macros.md
 [SE-0397]: https://github.com/apple/swift-evolution/blob/main/proposals/0397-freestanding-declaration-macros.md
 [SE-0407]: https://github.com/apple/swift-evolution/blob/main/proposals/0407-member-macro-conformances.md
+[SE-0411]: https://github.com/apple/swift-evolution/blob/main/proposals/0411-isolated-default-values.md
 [SE-0413]: https://github.com/apple/swift-evolution/blob/main/proposals/0413-typed-throws.md
 [#64927]: <https://github.com/apple/swift/issues/64927>
 [#42697]: <https://github.com/apple/swift/issues/42697>

include/swift/Basic/Features.def

@@ -121,6 +121,7 @@ UPCOMING_FEATURE(DeprecateApplicationMain, 383, 6)
 UPCOMING_FEATURE(ImportObjcForwardDeclarations, 384, 6)
 UPCOMING_FEATURE(DisableOutwardActorInference, 401, 6)
 UPCOMING_FEATURE(InternalImportsByDefault, 409, 6)
+UPCOMING_FEATURE(IsolatedDefaultValues, 411, 6)
 UPCOMING_FEATURE(GlobalConcurrency, 412, 6)
 UPCOMING_FEATURE(FullTypedThrows, 413, 6)
 
@@ -219,9 +220,6 @@ EXPERIMENTAL_FEATURE(StrictConcurrency, true)
 /// Region Based Isolation testing using the TransferNonSendable pass
 EXPERIMENTAL_FEATURE(RegionBasedIsolation, false)
 
-/// Allow default values to require isolation at the call-site.
-EXPERIMENTAL_FEATURE(IsolatedDefaultValues, false)
-
 /// Enable extended callbacks (with additional parameters) to be used when the
 /// "playground transform" is enabled.
 EXPERIMENTAL_FEATURE(PlaygroundExtendedCallbacks, true)

lib/Frontend/CompilerInvocation.cpp

@@ -880,6 +880,7 @@ static bool ParseLangArgs(LangOptions &Opts, ArgList &Args,
   if (Opts.hasFeature(Feature::CompleteConcurrency)) {
     Opts.StrictConcurrencyLevel = StrictConcurrency::Complete;
     Opts.enableFeature(Feature::DisableOutwardActorInference);
+    Opts.enableFeature(Feature::IsolatedDefaultValues);
     Opts.enableFeature(Feature::GlobalConcurrency);
   }
 
@@ -1019,6 +1020,11 @@ static bool ParseLangArgs(LangOptions &Opts, ArgList &Args,
     Opts.StrictConcurrencyLevel = StrictConcurrency::Minimal;
   }
 
+  // StrictConcurrency::Complete enables all data-race safety features.
+  if (Opts.StrictConcurrencyLevel == StrictConcurrency::Complete) {
+    Opts.enableFeature(Feature::IsolatedDefaultValues);
+  }
+
   Opts.WarnImplicitOverrides =
     Args.hasArg(OPT_warn_implicit_overrides);
 

lib/SILGen/SILGenApply.cpp

@@ -2718,6 +2718,28 @@ class DelayedArgument {
     return LV().Loc;
   }
 
+  bool isDefaultArg() const {
+    return Kind == DefaultArgument;
+  }
+
+  SILLocation getDefaultArgLoc() const {
+    assert(isDefaultArg());
+    auto storage = Value.get<DefaultArgumentStorage>(Kind);
+    return storage.loc;
+  }
+
+  llvm::Optional<ActorIsolation> getIsolation() const {
+    if (!isDefaultArg())
+      return llvm::None;
+
+    auto storage = Value.get<DefaultArgumentStorage>(Kind);
+    if (!storage.implicitlyAsync)
+      return llvm::None;
+
+    auto callee = storage.defaultArgsOwner.getDecl();
+    return getActorIsolation(callee);
+  }
+
   void emit(SILGenFunction &SGF, SmallVectorImpl<ManagedValue> &args,
             size_t &argIndex) {
     switch (Kind) {
@@ -2943,6 +2965,31 @@ static void emitDelayedArguments(SILGenFunction &SGF,
                          MutableArrayRef<SmallVector<ManagedValue, 4>> args) {
   assert(!delayedArgs.empty());
 
+  // If any of the delayed arguments are isolated default arguments,
+  // argument evaluation happens in the following order:
+  //
+  //   1. Left-to-right evalution of explicit r-value arguments
+  //   2. Left-to-right evaluation of formal access arguments
+  //   3. Hop to the callee's isolation domain
+  //   4. Left-to-right evaluation of default arguments
+
+  // So, if any delayed arguments are isolated, all default arguments
+  // are collected during the first pass over the delayed arguments,
+  // and emitted separately after a hop to the callee's isolation domain.
+
+  llvm::Optional<ActorIsolation> defaultArgIsolation;
+  for (auto &arg : delayedArgs) {
+    if (auto isolation = arg.getIsolation()) {
+      defaultArgIsolation = isolation;
+      break;
+    }
+  }
+
+  SmallVector<std::tuple<
+      /*delayedArgIt*/decltype(delayedArgs)::iterator,
+      /*siteArgsIt*/decltype(args)::iterator,
+      /*index*/size_t>, 2> isolatedArgs;
+
   SmallVector<std::pair<SILValue, SILLocation>, 4> emittedInoutArgs;
   auto delayedNext = delayedArgs.begin();
 
@@ -2951,7 +2998,8 @@ static void emitDelayedArguments(SILGenFunction &SGF,
   // wherever there's a delayed argument to insert.
   //
   // Note that this also begins the formal accesses in evaluation order.
-  for (auto &siteArgs : args) {
+  for (auto argsIt = args.begin(); argsIt != args.end(); ++argsIt) {
+    auto &siteArgs = *argsIt;
     // NB: siteArgs.size() may change during iteration
     for (size_t i = 0; i < siteArgs.size(); ) {
       auto &siteArg = siteArgs[i];
@@ -2964,6 +3012,15 @@ static void emitDelayedArguments(SILGenFunction &SGF,
       assert(delayedNext != delayedArgs.end());
       auto &delayedArg = *delayedNext;
 
+      if (defaultArgIsolation && delayedArg.isDefaultArg()) {
+        isolatedArgs.push_back(std::make_tuple(delayedNext, argsIt, i));
+        if (++delayedNext == delayedArgs.end()) {
+          goto done;
+        } else {
+          continue;
+        }
+      }
+
       // Emit the delayed argument and replace it in the arguments array.
       delayedArg.emit(SGF, siteArgs, i);
 
@@ -2984,6 +3041,45 @@ static void emitDelayedArguments(SILGenFunction &SGF,
 
 done:
 
+  if (defaultArgIsolation) {
+    assert(SGF.F.isAsync());
+    assert(!isolatedArgs.empty());
+
+    auto &firstArg = *std::get<0>(isolatedArgs[0]);
+    auto loc = firstArg.getDefaultArgLoc();
+
+    SILValue executor;
+    switch (*defaultArgIsolation) {
+    case ActorIsolation::GlobalActor:
+    case ActorIsolation::GlobalActorUnsafe:
+      executor = SGF.emitLoadGlobalActorExecutor(
+          defaultArgIsolation->getGlobalActor());
+      break;
+
+    case ActorIsolation::ActorInstance:
+      llvm_unreachable("default arg cannot be actor instance isolated");
+
+    case ActorIsolation::Unspecified:
+    case ActorIsolation::Nonisolated:
+    case ActorIsolation::NonisolatedUnsafe:
+      llvm_unreachable("Not isolated");
+    }
+
+    // Hop to the target isolation domain once to evaluate all
+    // default arguments.
+    SGF.emitHopToTargetExecutor(loc, executor);
+
+    size_t argsEmitted = 0;
+    for (auto &isolatedArg : isolatedArgs) {
+      auto &delayedArg = *std::get<0>(isolatedArg);
+      auto &siteArgs = *std::get<1>(isolatedArg);
+      auto argIndex = std::get<2>(isolatedArg) + argsEmitted;
+      auto origIndex = argIndex;
+      delayedArg.emit(SGF, siteArgs, argIndex);
+      argsEmitted += (argIndex - origIndex);
+    }
+  }
+
   // Check to see if we have multiple inout arguments which obviously
   // alias.  Note that we could do this in a later SILDiagnostics pass
   // as well: this would be stronger (more equivalences exposed) but

lib/SILGen/SILGenExpr.cpp

@@ -2647,24 +2647,8 @@ SILGenFunction::emitApplyOfDefaultArgGenerator(SILLocation loc,
   emitCaptures(loc, generator, CaptureEmission::ImmediateApplication,
                captures);
 
-  // The default argument might require the callee's isolation. If so,
-  // make sure to emit an actor hop.
-  //
-  // FIXME: Instead of hopping back and forth for each individual isolated
-  // default argument, we should emit one hop for all default arguments if
-  // any of them are isolated, and immediately enter the function after.
-  llvm::Optional<ActorIsolation> implicitActorHopTarget = llvm::None;
-  if (implicitlyAsync) {
-    auto *param = getParameterAt(defaultArgsOwner.getDecl(), destIndex);
-    auto isolation = param->getInitializerIsolation();
-    if (isolation.isActorIsolated()) {
-      implicitActorHopTarget = isolation;
-    }
-  }
-
   return emitApply(std::move(resultPtr), std::move(argScope), loc, fnRef, subs,
-                   captures, calleeTypeInfo, ApplyOptions(), C,
-                   implicitActorHopTarget);
+                   captures, calleeTypeInfo, ApplyOptions(), C, llvm::None);
 }
 
 RValue SILGenFunction::emitApplyOfStoredPropertyInitializer(

lib/Sema/CodeSynthesis.cpp

@@ -337,7 +337,8 @@ static ConstructorDecl *createImplicitConstructor(NominalTypeDecl *decl,
   ctor->setSynthesized();
   ctor->setAccess(accessLevel);
 
-  if (ctx.LangOpts.hasFeature(Feature::IsolatedDefaultValues)) {
+  if (ctx.LangOpts.hasFeature(Feature::IsolatedDefaultValues) &&
+      !decl->isActor()) {
     // If any of the type's actor-isolated properties:
     //   1. Have non-Sendable type, or
     //   2. Have an isolated initial value

test/Concurrency/actor_isolation.swift

@@ -765,15 +765,14 @@ actor LocalFunctionIsolatedActor {
 @available(SwiftStdlib 5.1, *)
 actor LazyActor {
     var v: Int = 0
-    // expected-note@-1 6 {{property declared here}}
 
     let l: Int = 0
 
     lazy var l11: Int = { v }()
     lazy var l12: Int = v
     lazy var l13: Int = { self.v }()
     lazy var l14: Int = self.v
-    lazy var l15: Int = { [unowned self] in self.v }() // expected-error{{actor-isolated property 'v' can not be referenced from a non-isolated context}}
+    lazy var l15: Int = { [unowned self] in self.v }()
 
     lazy var l21: Int = { l }()
     lazy var l22: Int = l
@@ -782,15 +781,15 @@ actor LazyActor {
     lazy var l25: Int = { [unowned self] in self.l }()
 
     nonisolated lazy var l31: Int = { v }()
-    // expected-error@-1 {{actor-isolated property 'v' can not be referenced from a non-isolated context}}
+    // expected-error@-1 {{actor-isolated default value in a nonisolated context}}
     nonisolated lazy var l32: Int = v
-    // expected-error@-1 {{actor-isolated property 'v' can not be referenced from a non-isolated context}}
+    // expected-error@-1 {{actor-isolated default value in a nonisolated context}}
     nonisolated lazy var l33: Int = { self.v }()
-    // expected-error@-1 {{actor-isolated property 'v' can not be referenced from a non-isolated context}}
+    // expected-error@-1 {{actor-isolated default value in a nonisolated context}}
     nonisolated lazy var l34: Int = self.v
-    // expected-error@-1 {{actor-isolated property 'v' can not be referenced from a non-isolated context}}
+    // expected-error@-1 {{actor-isolated default value in a nonisolated context}}
     nonisolated lazy var l35: Int = { [unowned self] in self.v }()
-    // expected-error@-1 {{actor-isolated property 'v' can not be referenced from a non-isolated context}}
+    // expected-error@-1 {{actor-isolated default value in a nonisolated context}}
 
     nonisolated lazy var l41: Int = { l }()
     nonisolated lazy var l42: Int = l

test/Concurrency/global_actor_inference.swift

@@ -1,8 +1,8 @@
 // RUN: %empty-directory(%t)
 
 // RUN: %target-swift-frontend -emit-module -emit-module-path %t/other_global_actor_inference.swiftmodule -module-name other_global_actor_inference -strict-concurrency=complete %S/Inputs/other_global_actor_inference.swift
-// RUN: %target-swift-frontend -I %t -disable-availability-checking %s -emit-sil -o /dev/null -verify
-// RUN: %target-swift-frontend -I %t -disable-availability-checking %s -emit-sil -o /dev/null -verify -strict-concurrency=targeted
+// RUN: %target-swift-frontend -I %t -disable-availability-checking %s -emit-sil -o /dev/null -verify -verify-additional-prefix minimal-targeted-
+// RUN: %target-swift-frontend -I %t -disable-availability-checking %s -emit-sil -o /dev/null -verify -strict-concurrency=targeted -verify-additional-prefix minimal-targeted-
 // RUN: %target-swift-frontend -I %t -disable-availability-checking %s -emit-sil -o /dev/null -verify -strict-concurrency=complete -verify-additional-prefix complete-tns-
 // RUN: %target-swift-frontend -I %t -disable-availability-checking %s -emit-sil -o /dev/null -verify -strict-concurrency=complete -enable-experimental-feature RegionBasedIsolation -verify-additional-prefix complete-tns-
 
@@ -437,13 +437,15 @@ actor WrapperActorBad2<Wrapped: Sendable> {
 struct WrapperWithMainActorDefaultInit {
   var wrappedValue: Int { fatalError() }
 
-  @MainActor init() {} // expected-note 2 {{calls to initializer 'init()' from outside of its actor context are implicitly asynchronous}}
+  @MainActor init() {} // expected-note {{calls to initializer 'init()' from outside of its actor context are implicitly asynchronous}}
+  // expected-minimal-targeted-note@-1 {{calls to initializer 'init()' from outside of its actor context are implicitly asynchronous}}
 }
 
 actor ActorWithWrapper {
   @WrapperOnActor var synced: Int = 0
   // expected-note@-1 3{{property declared here}}
-  @WrapperWithMainActorDefaultInit var property: Int // expected-error {{call to main actor-isolated initializer 'init()' in a synchronous actor-isolated context}}
+  @WrapperWithMainActorDefaultInit var property: Int // expected-minimal-targeted-error {{call to main actor-isolated initializer 'init()' in a synchronous actor-isolated context}}
+  // expected-complete-tns-error@-1 {{main actor-isolated default value in a actor-isolated context}}
   func f() {
     _ = synced // expected-error{{main actor-isolated property 'synced' can not be referenced on a different actor instance}}
     _ = $synced // expected-error{{global actor 'SomeGlobalActor'-isolated property '$synced' can not be referenced on a different actor instance}}
@@ -557,8 +559,9 @@ struct WrapperOnUnsafeActor<Wrapped> {
   }
 }
 
+// HasWrapperOnUnsafeActor gets an inferred @MainActor attribute.
 struct HasWrapperOnUnsafeActor {
-  @WrapperOnUnsafeActor var synced: Int = 0
+  @WrapperOnUnsafeActor var synced: Int = 0 // expected-complete-tns-error {{global actor 'OtherGlobalActor'-isolated default value in a main actor-isolated context}}
   // expected-note @-1 3{{property declared here}}
   // expected-complete-tns-note @-2 3{{property declared here}}
 
@@ -643,11 +646,11 @@ func acceptAsyncSendableClosureInheriting<T>(@_inheritActorContext _: @Sendable
 
 // defer bodies inherit global actor-ness
 @MainActor
-var statefulThingy: Bool = false // expected-note {{var declared here}}
+var statefulThingy: Bool = false // expected-minimal-targeted-note {{var declared here}}
 // expected-complete-tns-error @-1 {{top-level code variables cannot have a global actor}}
 
 @MainActor
-func useFooInADefer() -> String { // expected-note {{calls to global function 'useFooInADefer()' from outside of its actor context are implicitly asynchronous}}
+func useFooInADefer() -> String { // expected-minimal-targeted-note {{calls to global function 'useFooInADefer()' from outside of its actor context are implicitly asynchronous}}
   defer {
     statefulThingy = true
   }
@@ -677,9 +680,11 @@ class Cutter {
 
 @SomeGlobalActor
 class Butter {
-  var a = useFooInADefer() // expected-error {{call to main actor-isolated global function 'useFooInADefer()' in a synchronous global actor 'SomeGlobalActor'-isolated context}}
+  var a = useFooInADefer() // expected-minimal-targeted-error {{call to main actor-isolated global function 'useFooInADefer()' in a synchronous global actor 'SomeGlobalActor'-isolated context}}
+  // expected-complete-tns-error@-1 {{main actor-isolated default value in a global actor 'SomeGlobalActor'-isolated context}}
 
-  nonisolated let b = statefulThingy // expected-error {{main actor-isolated var 'statefulThingy' can not be referenced from a non-isolated context}}
+  nonisolated let b = statefulThingy // expected-minimal-targeted-error {{main actor-isolated var 'statefulThingy' can not be referenced from a non-isolated context}}
+  // expected-complete-tns-error@-1 {{main actor-isolated default value in a nonisolated context}}
 
   var c: Int = {
     return getGlobal7()