[Concurrency] Suggest adding a method, when mutating actor property cross isolation (#75922)

Author: ktoso

include/swift/AST/ActorIsolation.h

@@ -91,9 +91,7 @@ class ActorIsolation {
 
   ActorIsolation(Kind kind, Expr *actor, unsigned parameterIndex);
 
-  ActorIsolation(Kind kind, Type globalActor)
-      : globalActor(globalActor), kind(kind), isolatedByPreconcurrency(false),
-        silParsed(false), parameterIndex(0) {}
+  ActorIsolation(Kind kind, Type globalActor);
 
 public:
   // No-argument constructor needed for DenseMap use in PostfixCompletion.cpp
@@ -203,7 +201,6 @@ class ActorIsolation {
   }
 
   NominalTypeDecl *getActor() const;
-  NominalTypeDecl *getActorOrNullPtr() const;
 
   VarDecl *getActorInstance() const;
 

include/swift/AST/DiagnosticsSema.def

@@ -5456,6 +5456,9 @@ ERROR(distributed_actor_isolated_non_self_reference,none,
       "distributed actor-isolated %kind0 can not be accessed from a "
       "nonisolated context",
       (const ValueDecl *))
+NOTE(note_consider_method_for_isolated_property_mutation,none,
+     "consider declaring an isolated method on %0 to perform the mutation",
+     (const NominalTypeDecl *))
 ERROR(conflicting_stored_property_isolation,none,
       "%select{default|memberwise}0 initializer for %1 cannot be both %2 and %3",
       (bool, Type, ActorIsolation, ActorIsolation))

lib/AST/Decl.cpp

@@ -11659,6 +11659,10 @@ ActorIsolation::ActorIsolation(Kind kind, Expr *actor,
     : actorInstance(actor), kind(kind), isolatedByPreconcurrency(false),
       silParsed(false), parameterIndex(parameterIndex) {}
 
+ActorIsolation::ActorIsolation(Kind kind, Type globalActor)
+    : globalActor(globalActor), kind(kind), isolatedByPreconcurrency(false),
+      silParsed(false), parameterIndex(0) {}
+
 ActorIsolation
 ActorIsolation::forActorInstanceParameter(Expr *actor,
                                           unsigned parameterIndex) {
@@ -11709,51 +11713,29 @@ ActorIsolation ActorIsolation::forActorInstanceSelf(NominalTypeDecl *selfDecl) {
 }
 
 NominalTypeDecl *ActorIsolation::getActor() const {
-  assert(getKind() == ActorInstance ||
-         getKind() == GlobalActor);
+  assert(getKind() == ActorInstance || getKind() == GlobalActor);
 
   if (silParsed)
     return nullptr;
 
-  Type actorType;
-
-  if (auto *instance = actorInstance.dyn_cast<VarDecl *>()) {
-    actorType = instance->getTypeInContext();
-  } else if (auto *instance = actorInstance.dyn_cast<Expr *>()) {
-    actorType = instance->getType();
-  }
-
-  if (actorType) {
-    if (auto wrapped = actorType->getOptionalObjectType()) {
-      actorType = wrapped;
-    }
-    return actorType
-        ->getReferenceStorageReferent()->getAnyActor();
+  if (getKind() == GlobalActor) {
+    return getGlobalActor()->getAnyNominal();
   }
 
-  return actorInstance.get<NominalTypeDecl *>();
-}
-
-NominalTypeDecl *ActorIsolation::getActorOrNullPtr() const {
-  if (getKind() != ActorInstance || getKind() != GlobalActor)
-    return nullptr;
-
-  if (silParsed)
-    return nullptr;
-
   Type actorType;
 
   if (auto *instance = actorInstance.dyn_cast<VarDecl *>()) {
     actorType = instance->getTypeInContext();
-  } else if (auto *instance = actorInstance.dyn_cast<Expr *>()) {
-    actorType = instance->getType();
+  } else if (auto *expr = actorInstance.dyn_cast<Expr *>()) {
+    actorType = expr->getType();
   }
 
   if (actorType) {
     if (auto wrapped = actorType->getOptionalObjectType()) {
       actorType = wrapped;
     }
-    return actorType->getReferenceStorageReferent()->getAnyActor();
+    return actorType
+        ->getReferenceStorageReferent()->getAnyActor();
   }
 
   return actorInstance.get<NominalTypeDecl *>();
@@ -11793,7 +11775,10 @@ bool ActorIsolation::isDistributedActor() const {
   if (silParsed)
     return false;
 
-  return getKind() == ActorInstance && getActor()->isDistributedActor();
+  if (getKind() != ActorInstance)
+    return false;
+
+  return getActor()->isDistributedActor();
 }
 
 bool ActorIsolation::isEqual(const ActorIsolation &lhs,

lib/Sema/TypeCheckConcurrency.cpp

@@ -1823,6 +1823,54 @@ static bool wasLegacyEscapingUseRestriction(AbstractFunctionDecl *fn) {
   return !(fn->hasAsync()); // basic case: not async = had restriction.
 }
 
+/// Note that while a direct access to the actor isolated property is not legal
+/// you may want to consider introducing an accessing method for a mutation.
+static void
+maybeNoteMutatingMethodSuggestion(ASTContext &C,
+                                  ValueDecl const *member,
+                                  SourceLoc memberLoc,
+                                  DeclContext const *refCxt,
+                                  ActorIsolation isolation,
+                                  std::optional<VarRefUseEnv> useKind) {
+  if (!member || !isa<VarDecl>(member))
+      return; // we only offer the note property mutations
+
+  if (!(isolation.getKind() == ActorIsolation::Kind::ActorInstance ||
+        isolation.getKind() == ActorIsolation::Kind::GlobalActor)) {
+      return;
+  }
+
+  if (useKind != VarRefUseEnv::Mutating) {
+      // This note is tailored for the 'mutating' access, i.e. when
+      // attempting to mutate a property, they should instead make an actor method to perform the mutation. Reading properties does not have the same restriction.
+      return;
+  }
+
+  if (!refCxt->isAsyncContext()) {
+      // don't suggest creating method when in sync context, as we won't be able
+      // to invoke it anyway so this would not be helpful to suggest
+      return;
+  }
+
+  if (auto actor = isolation.getActor()) {
+      NominalTypeDecl *actorTy =
+        actor;
+//          (isolation.getKind() == swift::ActorIsolation::ActorInstance ?
+//)
+
+      C.Diags.diagnose(
+          memberLoc,
+          diag::note_consider_method_for_isolated_property_mutation,
+          actor);
+//      } else if (isolation.getKind() == swift::ActorIsolation::GlobalActor) {
+//        C.Diags.diagnose(
+//            memberLoc,
+//            diag::note_consider_method_for_global_actor_isolated_property_mutation,
+//            isolation.getGlobalActor());
+//      }
+  }
+}
+
 /// Note that the given actor member is isolated.
 static void noteIsolatedActorMember(ValueDecl const *decl,
                                     std::optional<VarRefUseEnv> useKind) {
@@ -1944,20 +1992,23 @@ static bool memberAccessHasSpecialPermissionInSwift5(
     }
 
     // Otherwise, it's definitely going to be illegal, so warn and permit.
-    auto &diags = refCxt->getASTContext().Diags;
+    auto &C = refCxt->getASTContext();
+    auto &diags = C.Diags;
     auto useKindInt = static_cast<unsigned>(
         useKind.value_or(VarRefUseEnv::Read));
 
+    auto isolation = getActorIsolation(const_cast<ValueDecl *>(member));
     diags.diagnose(
         memberLoc, diag::actor_isolated_non_self_reference,
         member,
         useKindInt,
         baseActor.kind + 1,
         baseActor.globalActor,
-        getActorIsolation(const_cast<ValueDecl *>(member)))
+        isolation)
     .warnUntilSwiftVersion(6);
 
     noteIsolatedActorMember(member, useKind);
+    maybeNoteMutatingMethodSuggestion(C, member, memberLoc, refCxt, isolation, useKind);
     return true;
   }
 
@@ -4438,6 +4489,8 @@ namespace {
               refKind + 1, refGlobalActor,
               result.isolation)
           .warnUntilSwiftVersionIf(preconcurrencyContext, 6);
+          maybeNoteMutatingMethodSuggestion(ctx, decl, loc, getDeclContext(), result.isolation,
+                                            kindOfUsage(decl, context).value_or(VarRefUseEnv::Read));
 
           if (derivedConformanceType) {
             auto *decl = dyn_cast<ValueDecl>(getDeclContext()->getAsDecl());

test/Concurrency/Inputs/GlobalVariables.swift

@@ -2,7 +2,7 @@ public struct Globals {
   public static let integerConstant = 0
   public static var integerMutable = 0
 
-  public static nonisolated(unsafe) let nonisolatedUnsafeIntegerConstant = 0
+  public static let nonisolatedUnsafeIntegerConstant = 0
   public static nonisolated(unsafe) var nonisolatedUnsafeIntegerMutable = 0
 
   @MainActor public static var actorInteger = 0

test/Concurrency/actor_call_implicitly_async.swift

@@ -232,7 +232,9 @@ func anotherAsyncFunc() async {
 
   _ = b.balance // expected-error {{actor-isolated instance method 'balance()' can not be partially applied}}
 
-  a.owner = "cat" // expected-error{{actor-isolated property 'owner' can not be mutated from a nonisolated context}}
+  // expected-error@+2{{actor-isolated property 'owner' can not be mutated from a nonisolated context}}
+  // expected-note@+1{{consider declaring an isolated method on 'BankAccount' to perform the mutation}}
+  a.owner = "cat"
   // expected-error@+1{{expression is 'async' but is not marked with 'await'}} {{7-7=await }} expected-note@+1{{property access is 'async'}}
   _ = b.owner
   _ = await b.owner == "cat"
@@ -279,7 +281,9 @@ func blender(_ peeler : () -> Void) {
   var money = await dollarsInBananaStand
   money -= 1200
 
-  dollarsInBananaStand = money // expected-error{{global actor 'BananaActor'-isolated var 'dollarsInBananaStand' can not be mutated from global actor 'OrangeActor'}}
+  // expected-error@+2{{global actor 'BananaActor'-isolated var 'dollarsInBananaStand' can not be mutated from global actor 'OrangeActor'}}
+  // expected-note@+1{{consider declaring an isolated method on 'BananaActor' to perform the mutation}}
+  dollarsInBananaStand = money
 
   // FIXME: these two errors seem a bit redundant.
   // expected-error@+2 {{actor-isolated var 'dollarsInBananaStand' cannot be passed 'inout' to implicitly 'async' function call}}

test/Concurrency/actor_existentials.swift

@@ -31,12 +31,16 @@ func from_isolated_existential2(_ x: isolated any P) async {
 func from_nonisolated(_ x: any P) async {
   await x.f()
   x.prop += 1 // expected-error {{actor-isolated property 'prop' can not be mutated from a nonisolated context}}
+  // expected-note@-1 {{consider declaring an isolated method on 'P' to perform the mutation}}
   x.prop = 100 // expected-error {{actor-isolated property 'prop' can not be mutated from a nonisolated context}}
+  // expected-note@-1 {{consider declaring an isolated method on 'P' to perform the mutation}}
 }
 
 func from_concrete(_ x: A) async {
   x.prop += 1 // expected-error {{actor-isolated property 'prop' can not be mutated from a nonisolated context}}
+  // expected-note@-1 {{consider declaring an isolated method on 'A' to perform the mutation}}
   x.prop = 100 // expected-error {{actor-isolated property 'prop' can not be mutated from a nonisolated context}}
+  // expected-note@-1 {{consider declaring an isolated method on 'A' to perform the mutation}}
 }
 
 func from_isolated_concrete(_ x: isolated A) async {
@@ -52,8 +56,9 @@ actor Act {
 nonisolated let act = Act()
 
 func bad() async {
-    // expected-warning@+2 {{no 'async' operations occur within 'await' expression}}
-    // expected-error@+1 {{actor-isolated property 'i' can not be mutated from a nonisolated context}}
+    // expected-warning@+3 {{no 'async' operations occur within 'await' expression}}
+    // expected-error@+2 {{actor-isolated property 'i' can not be mutated from a nonisolated context}}
+    // expected-note@+1 {{consider declaring an isolated method on 'Act' to perform the mutation}}
     await act.i = 666
 }
 
@@ -63,12 +68,14 @@ protocol Proto: Actor {
 extension Act: Proto {}
 
 func good() async {
-    // expected-warning@+2 {{no 'async' operations occur within 'await' expression}}
-    // expected-error@+1 {{actor-isolated property 'i' can not be mutated from a nonisolated context}}
+    // expected-warning@+3 {{no 'async' operations occur within 'await' expression}}
+    // expected-error@+2 {{actor-isolated property 'i' can not be mutated from a nonisolated context}}
+    // expected-note@+1 {{consider declaring an isolated method on 'Proto' to perform the mutation}}
     await (act as any Proto).i = 42
     let aIndirect: any Proto = act
 
-    // expected-warning@+2 {{no 'async' operations occur within 'await' expression}}
-    // expected-error@+1 {{actor-isolated property 'i' can not be mutated from a nonisolated context}}
+    // expected-warning@+3 {{no 'async' operations occur within 'await' expression}}
+    // expected-error@+2 {{actor-isolated property 'i' can not be mutated from a nonisolated context}}
+    // expected-note@+1 {{consider declaring an isolated method on 'Proto' to perform the mutation}}
     await aIndirect.i = 777
 }

test/Concurrency/actor_isolation.swift

@@ -98,18 +98,23 @@ func checkAsyncPropertyAccess() async {
   let act = MyActor()
   let _ : Int = await act.mutable + act.mutable
   act.mutable += 1  // expected-error {{actor-isolated property 'mutable' can not be mutated from a nonisolated context}}
+  // expected-note@-1{{consider declaring an isolated method on 'MyActor' to perform the mutation}}
 
   act.superState += 1 // expected-error {{actor-isolated property 'superState' can not be mutated from a nonisolated context}}
+  // expected-note@-1{{consider declaring an isolated method on 'MySuperActor' to perform the mutation}}
 
   act.text[0].append("hello") // expected-error{{actor-isolated property 'text' can not be mutated from a nonisolated context}}
+  // expected-note@-1{{consider declaring an isolated method on 'MyActor' to perform the mutation}}
 
   // this is not the same as the above, because Array is a value type
   var arr = await act.text
   arr[0].append("hello")
 
   act.text.append("no") // expected-error{{actor-isolated property 'text' can not be mutated from a nonisolated context}}
+  // expected-note@-1{{consider declaring an isolated method on 'MyActor' to perform the mutation}}
 
   act.text[0] += "hello" // expected-error{{actor-isolated property 'text' can not be mutated from a nonisolated context}}
+  // expected-note@-1{{consider declaring an isolated method on 'MyActor' to perform the mutation}}
 
   _ = act.point  // expected-warning{{non-sendable type 'Point' of property 'point' cannot exit actor-isolated context}}
   // expected-warning@-1 {{expression is 'async' but is not marked with 'await'}}
@@ -269,9 +274,11 @@ extension MyActor {
     // expected-note@-1{{property access is 'async'}}
     _ = await otherActor.mutable
     otherActor.mutable = 0  // expected-error{{actor-isolated property 'mutable' can not be mutated on a nonisolated actor instance}}
+    // expected-note@-1{{consider declaring an isolated method on 'MyActor' to perform the mutation}}
     acceptInout(&otherActor.mutable)  // expected-error{{actor-isolated property 'mutable' can not be used 'inout' on a nonisolated actor instance}}
-    // expected-error@+2{{actor-isolated property 'mutable' can not be mutated on a nonisolated actor instance}}
-    // expected-warning@+1{{no 'async' operations occur within 'await' expression}}
+    // expected-error@+3{{actor-isolated property 'mutable' can not be mutated on a nonisolated actor instance}}
+    // expected-warning@+2{{no 'async' operations occur within 'await' expression}}
+    // expected-note@+1{{consider declaring an isolated method on 'MyActor' to perform the mutation}}
     await otherActor.mutable = 0
 
     _ = otherActor.synchronous()
@@ -443,8 +450,10 @@ actor Crystal {
     _ = await globActorVar + globActorProp
 
     globActorProp = 20 // expected-error {{global actor 'SomeGlobalActor'-isolated property 'globActorProp' can not be mutated on a different actor instance}}
+    // expected-note@-1{{consider declaring an isolated method on 'SomeGlobalActor' to perform the mutation}}
 
     globActorVar = 30 // expected-error {{global actor 'SomeGlobalActor'-isolated property 'globActorVar' can not be mutated on a different actor instance}}
+    // expected-note@-1{{consider declaring an isolated method on 'SomeGlobalActor' to perform the mutation}}
 
     // expected-error@+2 {{global actor 'SomeGlobalActor'-isolated property 'globActorVar' can not be used 'inout' on a different actor instance}}
     // expected-error@+1 {{actor-isolated property 'globActorVar' cannot be passed 'inout' to implicitly 'async' function call}}
@@ -864,7 +873,7 @@ extension SomeClassInActor.ID {
 @available(SwiftStdlib 5.1, *)
 actor SomeActorWithInits {
   // expected-note@+2 2 {{property declared here}}
-  // expected-note@+1 3 {{mutation of this property is only permitted within the actor}}
+  // expected-note@+1 4 {{mutation of this property is only permitted within the actor}}
   var mutableState: Int = 17
   var otherMutableState: Int
   // expected-note@+1 {{mutation of this property is only permitted within the actor}}
@@ -896,6 +905,14 @@ actor SomeActorWithInits {
     }
     local()
 
+    func localAsync() async {
+      isolated() // expected-warning{{actor-isolated instance method 'isolated()' can not be referenced from a nonisolated context; this is an error in the Swift 6 language mode}}
+      mutableState += 1 // expected-warning{{actor-isolated property 'mutableState' can not be mutated from a nonisolated context; this is an error in the Swift 6 language mode}}
+      // expected-note@-1{{consider declaring an isolated method on 'SomeActorWithInits' to perform the mutation}}
+      nonisolated()
+    }
+    Task { await localAsync() }
+
     let _ = {
       defer {
         isolated() // expected-warning{{actor-isolated instance method 'isolated()' can not be referenced from a nonisolated context; this is an error in the Swift 6 language mode}}
@@ -990,8 +1007,7 @@ actor SomeActorWithInits {
     }()
   }
 
-
-  func isolated() { } // expected-note 9 {{calls to instance method 'isolated()' from outside of its actor context are implicitly asynchronous}}
+  func isolated() { } // expected-note 10 {{calls to instance method 'isolated()' from outside of its actor context are implicitly asynchronous}}
   nonisolated func nonisolated() {}
 }
 
@@ -1346,6 +1362,7 @@ actor Butterfly {
   nonisolated init(cookies: Void) async {
     self.init()
     self.flapsPerSec += 1 // expected-error {{actor-isolated property 'flapsPerSec' can not be mutated from a nonisolated context}}
+    // expected-note@-1 {{consider declaring an isolated method on 'Butterfly' to perform the mutation}}
   }
 
   init(brownies: Void) {

test/Concurrency/toplevel/async-5-top-level.swift

@@ -25,6 +25,7 @@ func nonIsolatedAsync() async {
     // expected-warning@-1:5 {{main actor-isolated var 'a' can not be mutated from a nonisolated context}}
     // expected-warning@-2:9 {{expression is 'async' but is not marked with 'await'}}{{9-9=await }}
     // expected-note@-3:9 {{property access is 'async'}}
+    // expected-note@-4 {{consider declaring an isolated method on 'MainActor' to perform the mutation}}
 }
 
 @MainActor

test/Concurrency/toplevel/async-6-top-level.swift

@@ -24,6 +24,7 @@ func nonIsolatedAsync() async {
     // expected-error@-1:5 {{main actor-isolated var 'a' can not be mutated from a nonisolated context}}
     // expected-error@-2:9 {{expression is 'async' but is not marked with 'await'}}
     // expected-note@-3:9 {{property access is 'async'}}
+    // expected-note@-4 {{consider declaring an isolated method on 'MainActor' to perform the mutation}}
 }
 
 @MainActor

test/Concurrency/toplevel/no-async-6-top-level.swift

@@ -26,6 +26,7 @@ func nonIsolatedAsync() async {
   a = a + 10 // expected-error{{main actor-isolated var 'a' can not be mutated from a nonisolated context}}
   // expected-note@-1{{property access is 'async'}}
   // expected-error@-2{{expression is 'async' but is not marked with 'await'}}
+  // expected-note@-3{{consider declaring an isolated method on 'MainActor' to perform the mutation}}
 }
 
 @MainActor