respect nonisolated on an async actor initializer

Previously, we were ignoring the explicit `nonisolated`
attribute on an actor initializer. This only has a
noticable effect if the initializer is `async`: an actor
hop is being emitted even though isolation was explicitly
not requested.

While we internally treat the isolation of an unadorned
actor initializer as if it were `nonisolated`, if the
programmer explicitly requests it, then it should be honored
in an async init too. Thus, this patch tells definite initialization
to skip the insertion of the hop, instead treating a `nonisolated`
initializer as though it has the escaping-use restriction.

resolves rdar://84164173

Author: kavon

include/swift/AST/ActorIsolation.h

@@ -106,6 +106,8 @@ class ActorIsolation {
   operator Kind() const { return getKind(); }
 
   bool isUnspecified() const { return kind == Unspecified; }
+  
+  bool isIndependent() const { return kind == Independent; }
 
   NominalTypeDecl *getActor() const {
     assert(getKind() == ActorInstance || getKind() == DistributedActorInstance);

include/swift/AST/DiagnosticsSIL.def

@@ -168,14 +168,12 @@ ERROR(variable_defer_use_uninit,none,
 ERROR(self_closure_use_uninit,none,
       "'self' captured by a closure before all members were initialized", ())
 
-/// false == sync; true == global-actor isolated
-ERROR(self_use_actor_init,none,
-      "this use of actor 'self' %select{can only|cannot}0 appear in "
-      "%select{an async|a global-actor isolated}0 initializer",
-      (bool))
-ERROR(self_disallowed_actor_init,none,
-      "actor 'self' %select{can only|cannot}0 %1 from "
-      "%select{an async|a global-actor isolated}0 initializer",
+ERROR(self_disallowed_nonisolated_actor_init,none,
+      "%select{|this use of }0actor 'self' cannot %2 %select{from|in}0 "
+      "%select{a non-isolated, designated|a global-actor isolated}1 initializer",
+      (bool, bool, StringRef))
+ERROR(self_disallowed_plain_actor_init,none,
+      "%select{|this use of }0actor 'self' can only %1 %select{from|in}0 an async initializer",
       (bool, StringRef))
 NOTE(actor_convenience_init,none,
      "convenience initializers allow non-isolated use of 'self' once "

lib/SILOptimizer/Mandatory/DefiniteInitialization.cpp

@@ -388,9 +388,10 @@ namespace {
 
   enum class ActorInitKind {
     None,           // not an actor init
-    Plain,          // synchronous, not isolated to global-actor
-    PlainAsync,     // asynchronous, not isolated to global-actor
-    GlobalActorIsolated  // isolated to global-actor (sync or async).
+    Plain,          // synchronous initializer
+    PlainAsync,     // asynchronous initializer
+    GlobalActorIsolated,  // isolated to global-actor (any async-ness)
+    NonIsolated           // explicitly 'nonisolated' (any async-ness)
   };
 
   /// LifetimeChecker - This is the main heavy lifting for definite
@@ -509,10 +510,18 @@ namespace {
     /// written out. Otherwise, the None initializer kind will be written out.
     bool isRestrictedActorInitSelf(ActorInitKind *kind = nullptr) const;
 
+    /// Emits a diagnostic to flag an illegal use of self in an actor init.
+    /// The message is tuned based on the input arguments.
+    /// \param ProblemDesc fills in the blank for "cannot ___ in an actor init"
+    /// \param suggestConvenienceInit if true, will emit a note about
+    ///                               convenience inits
+    /// \param specifyThisUse if true, precedes the message with "this use of"
+    ///                       to be more precise about the problem.
     void reportIllegalUseForActorInit(const DIMemoryUse &Use,
                                       ActorInitKind ActorKind,
                                       StringRef ProblemDesc,
-                                      bool suggestConvenienceInit) const;
+                                      bool suggestConvenienceInit,
+                                      bool specifyThisUse) const;
 
     void handleLoadUseFailureForActorInit(const DIMemoryUse &Use,
                                           ActorInitKind ActorKind) const;
@@ -1450,7 +1459,8 @@ void LifetimeChecker::handleInOutUse(const DIMemoryUse &Use) {
   // 'self' cannot be passed 'inout' from some kinds of actor initializers.
   if (isRestrictedActorInitSelf(&ActorKind))
     reportIllegalUseForActorInit(Use, ActorKind, "be passed 'inout'",
-                                 /*suggestConvenienceInit=*/false);
+                                 /*suggestConvenienceInit=*/false,
+                                 /*specifyThisUse=*/false);
 
   // One additional check: 'let' properties may never be passed inout, because
   // they are only allowed to have their initial value set, not a subsequent
@@ -1626,7 +1636,8 @@ void LifetimeChecker::handleEscapeUse(const DIMemoryUse &Use) {
     // no escaping uses of 'self' are allowed in restricted actor inits.
     if (isRestrictedActorInitSelf(&ActorKind))
       reportIllegalUseForActorInit(Use, ActorKind, "be captured by a closure",
-                                   /*suggestConvenienceInit=*/true);
+                                   /*suggestConvenienceInit=*/true,
+                                   /*specifyThisUse=*/false);
 
     return;
   }
@@ -2018,16 +2029,39 @@ bool LifetimeChecker::isRestrictedActorInitSelf(ActorInitKind *kind) const {
       *kind = k;
     return isRestricted;
   };
+  
+  // Determines whether the constructor was explicitly marked as `nonisolated`
+  auto isExplicitlyNonIsolated = [] (ConstructorDecl const* ctor) -> bool {
+      if (auto *attr = ctor->getAttrs().getAttribute<NonisolatedAttr>())
+        return !attr->isImplicit();
+      return false;
+    };
 
-  // Currently: being synchronous, or global-actor isolated, means the actor's
-  // self is restricted within the init.
+  // Currently: being synchronous, or having an explicitly specified isolation,
+  // means the actor's self is restricted within the init.
   if (auto *ctor = TheMemory.getActorInitSelf()) {
-    if (getActorIsolation(ctor).isGlobalActor())  // global-actor isolated?
-      return result(ActorInitKind::GlobalActorIsolated, true);
-    else if (!ctor->hasAsync()) // synchronous?
-      return result(ActorInitKind::Plain, true);
-    else
-      return result(ActorInitKind::PlainAsync, false);
+    auto isolation = getActorIsolation(ctor);
+    switch (isolation.getKind()) {
+      case ActorIsolation::Unspecified:
+        llvm_unreachable("actor decl with unspecified isolation?");
+
+      case ActorIsolation::Independent:
+        if (isExplicitlyNonIsolated(ctor))
+          return result(ActorInitKind::NonIsolated, true);
+
+        llvm_unreachable("implicitly nonisolated actor init?");
+
+      case ActorIsolation::ActorInstance:
+      case ActorIsolation::DistributedActorInstance:
+        if (ctor->hasAsync())
+          return result(ActorInitKind::PlainAsync, false);
+        else
+          return result(ActorInitKind::Plain, true);
+
+      case ActorIsolation::GlobalActor:
+      case ActorIsolation::GlobalActorUnsafe:
+        return result(ActorInitKind::GlobalActorIsolated, true);
+    };
   }
 
   return result(ActorInitKind::None, false);
@@ -2037,25 +2071,47 @@ void LifetimeChecker::reportIllegalUseForActorInit(
                                            const DIMemoryUse &Use,
                                            ActorInitKind ActorKind,
                                            StringRef ProblemDesc,
-                                           bool suggestConvenienceInit) const {
+                                           bool suggestConvenienceInit,
+                                           bool specifyThisUse) const {
+
+  ConstructorDecl *asyncNonIsoCtor = nullptr;
+                                             
   switch(ActorKind) {
   case ActorInitKind::None:
   case ActorInitKind::PlainAsync:
     llvm::report_fatal_error("this actor init is never problematic!");
 
   case ActorInitKind::Plain:
-    diagnose(Module, Use.Inst->getLoc(), diag::self_disallowed_actor_init,
-             false, ProblemDesc)
+    diagnose(Module, Use.Inst->getLoc(), diag::self_disallowed_plain_actor_init,
+             specifyThisUse, ProblemDesc)
       .warnUntilSwiftVersion(6);
     break;
 
-  case ActorInitKind::GlobalActorIsolated:
-    diagnose(Module, Use.Inst->getLoc(), diag::self_disallowed_actor_init,
-             true, ProblemDesc)
-      .warnUntilSwiftVersion(6);
+  case ActorInitKind::NonIsolated:
+    if (auto *ctor = TheMemory.getActorInitSelf())
+      if (ctor->hasAsync())
+        asyncNonIsoCtor = ctor;
+    
+    LLVM_FALLTHROUGH;
+    
+  case ActorInitKind::GlobalActorIsolated: {
+    bool isGlobalInstead = ActorKind == ActorInitKind::GlobalActorIsolated;
+    auto diag = diagnose(Module, Use.Inst->getLoc(),
+             diag::self_disallowed_nonisolated_actor_init, specifyThisUse,
+             isGlobalInstead, ProblemDesc);
+    
+    // Emit a fix-it to remove `nonisolated`.
+    // For an async init, it would fix their program.
+    if (asyncNonIsoCtor) {
+      auto attr = asyncNonIsoCtor->getAttrs().getAttribute<NonisolatedAttr>();
+      diag.fixItRemove(attr->getRange());
+    }
+    
+    diag.warnUntilSwiftVersion(6);
     break;
   }
-
+  };
+                                             
   if (suggestConvenienceInit)
     diagnose(Module, Use.Inst->getLoc(), diag::actor_convenience_init);
 }
@@ -2088,28 +2144,15 @@ void LifetimeChecker::handleLoadUseFailureForActorInit(
     return;
 
   // Everything else is disallowed!
-  switch(ActorKind) {
-  case ActorInitKind::None:
-  case ActorInitKind::PlainAsync:
-    llvm::report_fatal_error("this actor init is never problematic!");
-
-  case ActorInitKind::Plain:
-    diagnose(Module, Use.Inst->getLoc(), diag::self_use_actor_init, false)
-      .warnUntilSwiftVersion(6);
-    break;
-
-  case ActorInitKind::GlobalActorIsolated:
-    diagnose(Module, Use.Inst->getLoc(), diag::self_use_actor_init, true)
-      .warnUntilSwiftVersion(6);
-    break;
-  }
-
+                            
   // We cannot easily determine which argument in the call the use of 'self'
   // appears in. If we could, then we could determine whether the callee
   // is 'isolated' to that parameter, in order to avoid suggesting a convenience
-  // init in those cases. Thus, the phrasing of the note should be informative.
-  if (isa<ApplyInst>(Inst))
-    diagnose(Module, Use.Inst->getLoc(), diag::actor_convenience_init);
+  // init in those cases. For now, just always suggest it if it's an apply.
+  bool suggestConvenience = isa<ApplyInst>(Inst);
+                                             
+  reportIllegalUseForActorInit(Use, ActorKind, "appear", suggestConvenience,
+                               /*specifyThisUse=*/true);
 }
 
 /// Check and diagnose various failures when a load use is not fully

test/Concurrency/actor_definite_init.swift

@@ -403,4 +403,25 @@ actor EscapeArtist {
 
     func isolatedMethod() { x += 1 }
     nonisolated func nonisolated() {}
-}
+}
+
+@available(SwiftStdlib 5.5, *)
+actor Ahmad {
+  func f() {}
+  
+  nonisolated init(v1: Void) {
+    Task.detached { await self.f() } // expected-warning {{actor 'self' cannot be captured by a closure from a non-isolated, designated initializer}}
+                                     // expected-note@-1 {{convenience initializers allow non-isolated use of 'self' once initialized}}
+    
+    f()   // expected-warning {{this use of actor 'self' cannot appear in a non-isolated, designated initializer}}
+          // expected-note@-1 {{convenience initializers allow non-isolated use of 'self' once initialized}}
+  }
+  
+  nonisolated init(v2: Void) async {
+    Task.detached { await self.f() } // expected-warning {{actor 'self' cannot be captured by a closure from a non-isolated, designated initializer}} {{3-15=}}
+    // expected-note@-1 {{convenience initializers allow non-isolated use of 'self' once initialized}}
+    
+    f()   // expected-warning {{this use of actor 'self' cannot appear in a non-isolated, designated initializer}}
+    // expected-note@-1 {{convenience initializers allow non-isolated use of 'self' once initialized}}
+  }
+}

test/Concurrency/actor_definite_init_swift6.swift

@@ -402,3 +402,24 @@ actor EscapeArtist {
     func isolatedMethod() { x += 1 }
     nonisolated func nonisolated() {}
 }
+
+@available(SwiftStdlib 5.5, *)
+actor Ahmad {
+  func f() {}
+  
+  nonisolated init(v1: Void) {
+    Task.detached { await self.f() } // expected-error {{actor 'self' cannot be captured by a closure from a non-isolated, designated initializer}}
+    // expected-note@-1 {{convenience initializers allow non-isolated use of 'self' once initialized}}
+    
+    f()   // expected-error {{this use of actor 'self' cannot appear in a non-isolated, designated initializer}}
+    // expected-note@-1 {{convenience initializers allow non-isolated use of 'self' once initialized}}
+  }
+  
+  nonisolated init(v2: Void) async {
+    Task.detached { await self.f() } // expected-error {{actor 'self' cannot be captured by a closure from a non-isolated, designated initializer}} {{3-15=}}
+    // expected-note@-1 {{convenience initializers allow non-isolated use of 'self' once initialized}}
+    
+    f()   // expected-error {{this use of actor 'self' cannot appear in a non-isolated, designated initializer}}
+    // expected-note@-1 {{convenience initializers allow non-isolated use of 'self' once initialized}}
+  }
+}

test/SILOptimizer/definite_init_actor.swift

@@ -243,3 +243,12 @@ actor SomeActor {
     // CHECK: } // end sil function '$s4test9SomeActorCACyYacfc'
     init() async {}
 }
+
+actor Ahmad {
+  var x: Int = 0
+  
+  // CHECK-LABEL: sil hidden @$s4test5AhmadCACyYacfc : $@convention(method) @async (@owned Ahmad) -> @owned Ahmad {
+  // CHECK:           store {{%[0-9]+}} to {{%[0-9]+}} : $*Int
+  // CHECK: } // end sil function '$s4test5AhmadCACyYacfc'
+  nonisolated init() async {} // no hop should appear here because of explicit nonisolated marking.
+}