Don't add isolation attributes for deinit when they were inferred from overriden declaration.

But if isolation was inferred from class isolation and matches with isolation of the overriden declaration, attributes are added.

Author: nickolas-pohilets

lib/Sema/TypeCheckConcurrency.cpp

@@ -5008,6 +5008,13 @@ static std::optional<unsigned> getIsolatedParamIndex(ValueDecl *value) {
   return std::nullopt;
 }
 
+static bool belongsToActor(ValueDecl *value) {
+  if (auto nominal = value->getDeclContext()->getSelfNominalTypeDecl()) {
+    return nominal->isAnyActor();
+  }
+  return false;
+}
+
 /// Verifies rules about `isolated` parameters for the given decl. There is more
 /// checking about these in TypeChecker::checkParameterList.
 ///
@@ -5041,75 +5048,53 @@ static void checkDeclWithIsolatedParameter(ValueDecl *value) {
   }
 }
 
-ActorIsolation ActorIsolationRequest::evaluate(
-    Evaluator &evaluator, ValueDecl *value) const {
-  auto &ctx = value->getASTContext();
-
-  const bool hasIsolatedSelf =
-      evaluateOrDefault(evaluator, HasIsolatedSelfRequest{value}, false);
-
-  auto addAttributesForActorIsolation = [&](ActorIsolation isolation) {
-    ASTContext &ctx = value->getASTContext();
-    switch (isolation) {
-    case ActorIsolation::Nonisolated:
-    case ActorIsolation::NonisolatedUnsafe: {
-      value->getAttrs().add(new (ctx) NonisolatedAttr(
-          isolation == ActorIsolation::NonisolatedUnsafe, /*implicit=*/true));
-      break;
-    }
-    case ActorIsolation::GlobalActor: {
-      auto typeExpr = TypeExpr::createImplicit(isolation.getGlobalActor(), ctx);
-      auto attr = CustomAttr::create(ctx, SourceLoc(), typeExpr, /*implicit=*/true);
-      value->getAttrs().add(attr);
+static void addAttributesForActorIsolation(ValueDecl *value,
+                                           ActorIsolation isolation) {
+  ASTContext &ctx = value->getASTContext();
+  switch (isolation) {
+  case ActorIsolation::Nonisolated:
+  case ActorIsolation::NonisolatedUnsafe: {
+    value->getAttrs().add(new (ctx) NonisolatedAttr(
+        isolation == ActorIsolation::NonisolatedUnsafe, /*implicit=*/true));
+    break;
+  }
+  case ActorIsolation::GlobalActor: {
+    auto typeExpr = TypeExpr::createImplicit(isolation.getGlobalActor(), ctx);
+    auto attr =
+        CustomAttr::create(ctx, SourceLoc(), typeExpr, /*implicit=*/true);
+    value->getAttrs().add(attr);
 
-      if (isolation.preconcurrency() && !value->getAttrs().hasAttribute<PreconcurrencyAttr>()) {
-        auto preconcurrency = new (ctx) PreconcurrencyAttr(/*isImplicit*/true);
-        value->getAttrs().add(preconcurrency);
-      }
-      break;
+    if (isolation.preconcurrency() &&
+        !value->getAttrs().hasAttribute<PreconcurrencyAttr>()) {
+      auto preconcurrency = new (ctx) PreconcurrencyAttr(/*isImplicit*/ true);
+      value->getAttrs().add(preconcurrency);
     }
+    break;
+  }
     case ActorIsolation::Erased:
       llvm_unreachable("cannot add attributes for erased isolation");
     case ActorIsolation::ActorInstance: {
       // Nothing to do. Default value for actors.
-      assert(hasIsolatedSelf);
+      assert(belongsToActor(value));
       break;
     }
     case ActorIsolation::Unspecified: {
       // Nothing to do. Default value for non-actors.
-      assert(!hasIsolatedSelf);
+      assert(!belongsToActor(value));
       break;
     }
     }
-  };
-
-  auto isolationFromAttr = getIsolationFromAttributes(value);
-
-  // No need to isolate implicit deinit, unless there is already an isolated one
-  // in the superclass
-  if (isa<DestructorDecl>(value)) {
-    if (value->isImplicit() && !isolationFromAttr) {
-      ValueDecl *overriddenValue = value->getOverriddenDeclOrSuperDeinit();
-      ActorIsolation isolation = ActorIsolation::forUnspecified();
-      if (overriddenValue) {
-        isolation = getOverriddenIsolationFor(value);
-      }
-
-      if (hasIsolatedSelf && isolation.isUnspecified()) {
-        // Don't use 'unspecified' for actors, use 'nonisolated' instead.
-        // To force generation of the 'nonisolated' attribute in SIL and
-        // .swiftmodule
-        isolation = ActorIsolation::forNonisolated(false);
-      }
+}
 
-      addAttributesForActorIsolation(isolation);
-      return isolation;
-    }
-  }
+static bool isImplicitDeinit(ValueDecl *value) {
+  return isa<DestructorDecl>(value) && value->isImplicit();
+}
 
+ActorIsolation ActorIsolationRequest::evaluate(Evaluator &evaluator,
+                                               ValueDecl *value) const {
   // If this declaration has actor-isolated "self", it's isolated to that
   // actor.
-  if (hasIsolatedSelf) {
+  if (evaluateOrDefault(evaluator, HasIsolatedSelfRequest{value}, false)) {
     auto actor = value->getDeclContext()->getSelfNominalTypeDecl();
     assert(actor && "could not find the actor that 'self' is isolated to");
 
@@ -5120,7 +5105,7 @@ ActorIsolation ActorIsolationRequest::evaluate(
         actor->getDeclContext()->isModuleScopeContext() &&
         actor->getDeclContext()->getParentModule()->getABIName().is("Swift")) {
       auto isolation = ActorIsolation::forNonisolated(false);
-      addAttributesForActorIsolation(isolation);
+      addAttributesForActorIsolation(value, isolation);
       return isolation;
     }
     return ActorIsolation::forActorInstanceSelf(value);
@@ -5144,6 +5129,8 @@ ActorIsolation ActorIsolationRequest::evaluate(
       return ActorIsolation::forActorInstanceParameter(param, *paramIdx);
   }
 
+  auto isolationFromAttr = getIsolationFromAttributes(value);
+
   // Diagnose global state that is not either immutable plus Sendable or
   // isolated to a global actor.
   auto checkGlobalIsolation = [var = dyn_cast<VarDecl>(value)](
@@ -5200,6 +5187,7 @@ ActorIsolation ActorIsolationRequest::evaluate(
     return isolation;
   };
 
+  ASTContext &ctx = value->getASTContext();
   if (isolationFromAttr && isolationFromAttr->preconcurrency() &&
       !value->getAttrs().hasAttribute<PreconcurrencyAttr>()) {
     auto preconcurrency =
@@ -5303,14 +5291,14 @@ ActorIsolation ActorIsolationRequest::evaluate(
         }
 
         // Add nonisolated attribute
-        addAttributesForActorIsolation(inferred);
+        addAttributesForActorIsolation(value, inferred);
         break;
 
       case ActorIsolation::Erased:
         llvm_unreachable("cannot infer erased isolation");
       case ActorIsolation::GlobalActor: {
         // Add global actor attribute
-        addAttributesForActorIsolation(inferred);
+        addAttributesForActorIsolation(value, inferred);
         break;
       }
 
@@ -5455,8 +5443,14 @@ ActorIsolation ActorIsolationRequest::evaluate(
     // If the declaration is in a nominal type (or extension thereof) that
     // has isolation, use that.
     if (auto selfTypeDecl = value->getDeclContext()->getSelfNominalTypeDecl()) {
-      if (auto selfTypeIsolation = getActorIsolation(selfTypeDecl))
-        return inferredIsolation(selfTypeIsolation, onlyGlobal);
+      if (auto selfTypeIsolation = getActorIsolation(selfTypeDecl)) {
+        if (isImplicitDeinit(value)) {
+          return inferredIsolation(ActorIsolation::forUnspecified(),
+                                   onlyGlobal);
+        } else {
+          return inferredIsolation(selfTypeIsolation, onlyGlobal);
+        }
+      }
     }
   }
 
@@ -5552,6 +5546,24 @@ bool HasIsolatedSelfRequest::evaluate(
       return false;
   }
 
+  if (isImplicitDeinit(value)) {
+    // Actors don't have inheritance (except inheriting from NSObject),
+    // but check for it anyway, just in case it will be re-introduced later.
+    ValueDecl *overriddenValue = value->getOverriddenDeclOrSuperDeinit();
+    if (overriddenValue) {
+      ActorIsolation isolation = getOverriddenIsolationFor(value);
+      if (isolation.isActorIsolated()) {
+        return isolation.getKind() == ActorIsolation::ActorInstance;
+      }
+    }
+
+    // No need to isolate implicit deinit, unless there is already an isolated
+    // one in the superclass
+    addAttributesForActorIsolation(value,
+                                   ActorIsolation::forNonisolated(false));
+    return false;
+  }
+
   return true;
 }
 

test/Concurrency/deinit_isolation_import/test.swift

@@ -54,7 +54,8 @@ class ProbeExplicit_RoundtripNonisolated: RoundtripNonisolated {
 }
 
 // CHECK-LABEL: @objc @_inheritsConvenienceInitializers class ProbeImplicit_RoundtripIsolated : RoundtripIsolated {
-// CHECK: @objc @MainActor deinit
+// Note: Type-checked as isolated, but no @MainActor attribute, because attributes are not added for overriding members
+// CHECK: @objc deinit
 // CHECK: }
 // CHECK-SYMB: ProbeImplicit_RoundtripIsolated.__isolated_deallocating_deinit
 // CHECK-SYMB-NEXT: // Isolation: global_actor. type: MainActor
@@ -65,7 +66,7 @@ class ProbeExplicit_RoundtripNonisolated: RoundtripNonisolated {
 class ProbeImplicit_RoundtripIsolated: RoundtripIsolated {}
 
 // CHECK-LABEL: @objc @_inheritsConvenienceInitializers class ProbeExplicit_RoundtripIsolated : RoundtripIsolated {
-// Note: Not @MainActor attribute, because attributes are not added for overriding members
+// Note: Type-checked as isolated, but no @MainActor attribute, because attributes are not added for overriding members
 // CHECK: @objc deinit
 // CHECK: }
 // CHECK-SYMB: ProbeExplicit_RoundtripIsolated.__isolated_deallocating_deinit
@@ -183,7 +184,8 @@ class ProbeExplicit_DerivedIsolatedClass: DerivedIsolatedClass {
 }
 
 // CHECK-LABEL: @objc @_inheritsConvenienceInitializers class ProbeImplicit_BaseIsolatedDealloc : BaseIsolatedDealloc {
-// CHECK: @objc @MainActor @preconcurrency deinit
+// Note: Type-checked as isolated, but no @MainActor attribute, because attributes are not added for overriding members
+// CHECK: @objc deinit
 // CHECK: }
 // CHECK-SYMB-NOT: ProbeImplicit_BaseIsolatedDealloc.__isolated_deallocating_deinit
 // CHECK-SYMB-NOT: @$s4test33ProbeImplicit_BaseIsolatedDeallocCfZ
@@ -193,6 +195,7 @@ class ProbeExplicit_DerivedIsolatedClass: DerivedIsolatedClass {
 class ProbeImplicit_BaseIsolatedDealloc: BaseIsolatedDealloc {}
 
 // CHECK-LABEL: @objc @_inheritsConvenienceInitializers class ProbeExplicit_BaseIsolatedDealloc : BaseIsolatedDealloc {
+// Note: Type-checked as isolated, but no @MainActor attribute, because attributes are not added for overriding members
 // CHECK: @objc deinit
 // CHECK: }
 // CHECK-SYMB-NOT: ProbeExplicit_BaseIsolatedDealloc.__isolated_deallocating_deinit
@@ -213,7 +216,8 @@ class ProbeAnother_BaseIsolatedDealloc: BaseIsolatedDealloc{
 #endif
 
 // CHECK-LABEL: @objc @_inheritsConvenienceInitializers class ProbeImplicit_DerivedIsolatedDealloc : DerivedIsolatedDealloc {
-// CHECK: @objc @MainActor @preconcurrency deinit
+// Note: Type-checked as isolated, but no @MainActor attribute, because attributes are not added for overriding members
+// CHECK: @objc deinit
 // CHECK: }
 // CHECK-SYMB-NOT: ProbeImplicit_DerivedIsolatedDealloc.__isolated_deallocating_deinit
 // CHECK-SYMB-NOT: @$s4test36ProbeImplicit_DerivedIsolatedDeallocCfZ