downgrade actor deinit accesses to warnings in more contexts

previous downgrade did not properly account for accesses within
closures, because it was done hastily.

Author: kavon

include/swift/AST/DiagnosticsSema.def

@@ -4500,9 +4500,10 @@ NOTE(note_distributed_actor_system_conformance_missing_adhoc_requirement,none,
 ERROR(override_implicit_unowned_executor,none,
       "cannot override an actor's 'unownedExecutor' property that wasn't "
       "explicitly defined", ())
-ERROR(actor_isolated_from_deinit,none,
-      "actor-isolated %0 %1 can not be referenced from a non-isolated deinit",
-      (DescriptiveDeclKind, DeclName))
+ERROR(actor_isolated_from_decl,none,
+      "actor-isolated %0 %1 can not be referenced from a non-isolated "
+      "%select{deinit|autoclosure|closure}2",
+      (DescriptiveDeclKind, DeclName, unsigned))
 ERROR(actor_isolated_non_self_reference,none,
         "actor-isolated %0 %1 can not be "
         "%select{referenced|mutated|used 'inout'}2 "

lib/Sema/TypeCheckConcurrency.cpp

@@ -2686,6 +2686,52 @@ namespace {
           });
     }
 
+    static bool isStoredProperty(ValueDecl const *member) {
+      if (auto *var = dyn_cast<VarDecl>(member))
+        if (var->hasStorage() && var->isInstanceMember())
+          return true;
+      return false;
+    }
+
+    /// an ad-hoc check specific to member isolation checking.
+    static bool memberAccessWasAllowedInSwift5(DeclContext const *refCxt,
+                                               ValueDecl const *member,
+                                               SourceLoc memberLoc) {
+      // no need for this in Swift 6+
+      if (refCxt->getASTContext().isSwiftVersionAtLeast(6))
+        return false;
+
+      // In Swift 5, we were allowing all members to be referenced from a
+      // deinit, nested within a wide variety of contexts.
+      if (auto oldFn = isActorInitOrDeInitContext(refCxt)) {
+        if (isa<DestructorDecl>(oldFn) && member->isInstanceMember()) {
+          auto &diags = refCxt->getASTContext().Diags;
+
+          // if the context in which we consider the access matches between
+          // old and new, and its a stored property, then skip the warning
+          // because it will still be allowed in Swift 6.
+          if (!(refCxt == oldFn && isStoredProperty(member))) {
+            unsigned cxtKind = 0; // deinit
+
+            // try to get a better name for this context.
+            if (isa<AutoClosureExpr>(refCxt)) {
+              cxtKind = 1;
+            } else if (isa<AbstractClosureExpr>(refCxt)) {
+              cxtKind = 2;
+            }
+
+            diags.diagnose(memberLoc, diag::actor_isolated_from_decl,
+                           member->getDescriptiveKind(),
+                           member->getName(),
+                           cxtKind).warnUntilSwiftVersion(6);
+          }
+          return true;
+        }
+      }
+
+      return false;
+    }
+
     /// To support flow-isolation, some member accesses in inits / deinits
     /// must be permitted, despite the isolation of 'self' not being
     /// correct in Sema.
@@ -2697,7 +2743,7 @@ namespace {
     ///
     /// \returns true iff the member access is permitted in Sema because it will
     /// be verified later by flow-isolation.
-    bool checkedByFlowIsolation(DeclContext const *refCxt,
+    static bool checkedByFlowIsolation(DeclContext const *refCxt,
                                 ReferencedActor &baseActor,
                                 ValueDecl const *member,
                                 SourceLoc memberLoc) {
@@ -2716,7 +2762,7 @@ namespace {
       while (true) {
         fnDecl = dyn_cast_or_null<AbstractFunctionDecl>(refCxt->getAsDecl());
         if (!fnDecl)
-          return false;
+          break;
 
         // go up one level if this context is a defer.
         if (auto *d = dyn_cast<FuncDecl>(fnDecl)) {
@@ -2728,25 +2774,15 @@ namespace {
         break;
       }
 
+      if (memberAccessWasAllowedInSwift5(refCxt, member, memberLoc))
+        return true; // then permit it now.
+
       if (!usesFlowSensitiveIsolation(fnDecl))
         return false;
 
       // Stored properties are definitely OK.
-      if (auto *var = dyn_cast<VarDecl>(member))
-        if (var->hasStorage() && var->isInstanceMember())
-          return true;
-
-      // In Swift 5, we were allowing all members to be referenced from a
-      // deinit, but that will not be valid in Swift 6+, so warn about it.
-      if (!refCxt->getASTContext().isSwiftVersionAtLeast(6)) {
-        if (isa<DestructorDecl>(fnDecl) && member->isInstanceMember()) {
-          auto &diags = refCxt->getASTContext().Diags;
-          diags.diagnose(memberLoc, diag::actor_isolated_from_deinit,
-                         member->getDescriptiveKind(),
-                         member->getName()).warnUntilSwiftVersion(6);
+      if (isStoredProperty(member))
           return true;
-        }
-      }
 
       return false;
     }

test/Concurrency/actor_isolation.swift

@@ -811,7 +811,7 @@ extension SomeClassInActor.ID {
 // ----------------------------------------------------------------------
 @available(SwiftStdlib 5.1, *)
 actor SomeActorWithInits {
-  var mutableState: Int = 17 // expected-note 3 {{mutation of this property is only permitted within the actor}}
+  var mutableState: Int = 17 // expected-note 2 {{mutation of this property is only permitted within the actor}}
   var otherMutableState: Int
   let nonSendable: SomeClass
 
@@ -911,16 +911,16 @@ actor SomeActorWithInits {
 
     let _ = {
       defer {
-        isolated() // expected-error{{actor-isolated instance method 'isolated()' can not be referenced from a non-isolated context}}
-        mutableState += 1  // expected-error{{actor-isolated property 'mutableState' can not be mutated from a non-isolated context}}
+        isolated() // expected-warning{{actor-isolated instance method 'isolated()' can not be referenced from a non-isolated closure; this is an error in Swift 6}}
+        mutableState += 1  // expected-warning{{actor-isolated property 'mutableState' can not be referenced from a non-isolated closure; this is an error in Swift 6}}
         nonisolated()
       }
       nonisolated()
     }()
   }
 
 
-  func isolated() { } // expected-note 8 {{calls to instance method 'isolated()' from outside of its actor context are implicitly asynchronous}}
+  func isolated() { } // expected-note 7 {{calls to instance method 'isolated()' from outside of its actor context are implicitly asynchronous}}
   nonisolated func nonisolated() {}
 }
 
@@ -1344,3 +1344,15 @@ final class MainActorInit: Sendable {
 
   func f() {}
 }
+
+actor DunkTracker {
+  private var lebron: Int?
+  private var curry: Int?
+
+  deinit {
+    // expected-warning@+1 {{actor-isolated property 'curry' can not be referenced from a non-isolated autoclosure; this is an error in Swift 6}}
+    if lebron != nil || curry != nil {
+      
+    }
+  }
+}