Don't generate isolating destructor if dealloc was explicitly isolated in ObjC

nickolas-pohilets · nickolas-pohilets · commit 95079aff06c2 · 2024-07-11T13:09:06.000+02:00
diff --git a/include/swift/AST/Decl.h b/include/swift/AST/Decl.h
@@ -8762,6 +8762,10 @@ class DestructorDecl : public AbstractFunctionDecl {
   /// Retrieve the Objective-C selector for destructors.
   ObjCSelector getObjCSelector() const;
 
+  /// Retrives destructor decl from the superclass, or nil if there is no
+  /// superclass
+  DestructorDecl *getSuperDeinit() const;
+
   static bool classof(const Decl *D) {
     return D->getKind() == DeclKind::Destructor;
   }
diff --git a/lib/AST/Decl.cpp b/lib/AST/Decl.cpp
@@ -3273,12 +3273,8 @@ ValueDecl *ValueDecl::getOverriddenDeclOrSuperDeinit() const {
   if (auto overridden = getOverriddenDecl()) {
     return overridden;
   }
-  if (isa<DestructorDecl>(this)) {
-    if (auto classDecl = dyn_cast<ClassDecl>(getDeclContext())) {
-      if (auto superclass = classDecl->getSuperclassDecl()) {
-        return superclass->getDestructor();
-      }
-    }
+  if (auto dtor = dyn_cast<DestructorDecl>(this)) {
+    return dtor->getSuperDeinit();
   }
   return nullptr;
 }
@@ -10794,6 +10790,16 @@ ObjCSelector DestructorDecl::getObjCSelector() const {
   return ObjCSelector(ctx, 0, ctx.Id_dealloc);
 }
 
+DestructorDecl *DestructorDecl::getSuperDeinit() const {
+  auto declContext = getDeclContext()->getImplementedObjCContext();
+  if (auto classDecl = dyn_cast<ClassDecl>(declContext)) {
+    if (auto superclass = classDecl->getSuperclassDecl()) {
+      return superclass->getDestructor();
+    }
+  }
+  return nullptr;
+}
+
 SourceRange FuncDecl::getSourceRange() const {
   SourceLoc StartLoc = getStartLoc();
 
diff --git a/lib/SILGen/SILGen.cpp b/lib/SILGen/SILGen.cpp
@@ -1114,8 +1114,7 @@ void SILGenModule::emitFunctionDefinition(SILDeclRef constant, SILFunction *f) {
   }
   case SILDeclRef::Kind::Deallocator: {
     auto *dd = cast<DestructorDecl>(constant.getDecl());
-    auto ai = swift::getActorIsolation(dd);
-    if (ai.isActorIsolated()) {
+    if (SILGenFunction::shouldEmitIsolatingDestructor(dd)) {
       auto loc = RegularLocation::getAutoGeneratedLocation(dd);
       preEmitFunction(constant, f, loc);
       PrettyStackTraceSILFunction X("silgen emitIsolatingDestructor", f);
@@ -1574,7 +1573,8 @@ bool SILGenModule::requiresIVarDestroyer(ClassDecl *cd) {
 void SILGenModule::emitObjCAllocatorDestructor(ClassDecl *cd,
                                                DestructorDecl *dd) {
 
-  const bool isActorIsolated = getActorIsolation(dd).isActorIsolated();
+  const bool isActorIsolated =
+      SILGenFunction::shouldEmitIsolatingDestructor(dd);
 
   // Emit the isolated deallocating destructor.
   // If emitted, it implements actual deallocating and deallocating destructor
@@ -1663,7 +1663,7 @@ void SILGenModule::emitDestructor(ClassDecl *cd, DestructorDecl *dd) {
   // Emit the isolated deallocating destructor.
   // If emitted, it implements actual deallocating and deallocating destructor
   // only switches executor
-  if (getActorIsolation(dd).isActorIsolated()) {
+  if (SILGenFunction::shouldEmitIsolatingDestructor(dd)) {
     SILDeclRef deallocator(dd, SILDeclRef::Kind::IsolatedDeallocator);
     emitFunctionDefinition(deallocator,
                            getFunction(deallocator, ForDefinition));
diff --git a/lib/SILGen/SILGenDestructor.cpp b/lib/SILGen/SILGenDestructor.cpp
@@ -295,6 +295,28 @@ static AccessorDecl *getUnownedExecutorGetter(ASTContext &ctx,
   return nullptr;
 }
 
+bool SILGenFunction::shouldEmitIsolatingDestructor(DestructorDecl *dd) {
+  auto ai = swift::getActorIsolation(dd);
+  if (!ai.isActorIsolated()) {
+    return false;
+  }
+  DestructorDecl *firstIsolated = dd;
+  while (true) {
+    DestructorDecl *next = firstIsolated->getSuperDeinit();
+    if (!next)
+      break;
+    auto ai = swift::getActorIsolation(next);
+    if (!ai.isActorIsolated())
+      break;
+    firstIsolated = next;
+  }
+
+  // If isolation was intoduced in ObjC code, then we assume that ObjC code also
+  // overrides retain/release to make sure that dealloc is called on the correct
+  // executor in the first place.
+  return firstIsolated->getClangNode().isNull();
+}
+
 void SILGenFunction::emitIsolatingDestructor(DestructorDecl *dd) {
   MagicFunctionName = DeclName(SGM.M.getASTContext().getIdentifier("deinit"));
 
diff --git a/lib/SILGen/SILGenFunction.h b/lib/SILGen/SILGenFunction.h
@@ -828,6 +828,9 @@ class LLVM_LIBRARY_VISIBILITY SILGenFunction
   /// definite initialization.
   bool isCtorWithHopsInjectedByDefiniteInit();
 
+  /// Checks if isolating destructor is needed.
+  static bool shouldEmitIsolatingDestructor(DestructorDecl *dd);
+
   /// Generates code for a struct constructor.
   /// This allocates the new 'self' value, emits the
   /// body code, then returns the final initialized 'self'.
diff --git a/test/Concurrency/deinit_isolation_import/test.swift b/test/Concurrency/deinit_isolation_import/test.swift
@@ -20,6 +20,10 @@
 // Note: intentionally importing Alpha implicitly
 import Beta
 
+@globalActor final actor AnotherActor {
+  static let shared = AnotherActor()
+}
+
 @MainActor
 func isolatedFunc() {} // expected-note 3{{calls to global function 'isolatedFunc()' from outside of its actor context are implicitly asynchronous}}
 
@@ -181,8 +185,8 @@ class ProbeExplicit_DerivedIsolatedClass: DerivedIsolatedClass {
 // CHECK-LABEL: @objc @_inheritsConvenienceInitializers class ProbeImplicit_BaseIsolatedDealloc : BaseIsolatedDealloc {
 // CHECK: @objc @MainActor @preconcurrency deinit
 // CHECK: }
-// CHECK-SYMB: // ProbeImplicit_BaseIsolatedDealloc.__isolated_deallocating_deinit
-// CHECK-SYMB: sil private [ossa] @$s4test33ProbeImplicit_BaseIsolatedDeallocCfZ : $@convention(thin) (@owned ProbeImplicit_BaseIsolatedDealloc) -> () {
+// CHECK-SYMB-NOT: ProbeImplicit_BaseIsolatedDealloc.__isolated_deallocating_deinit
+// CHECK-SYMB-NOT: @$s4test33ProbeImplicit_BaseIsolatedDeallocCfZ
 // CHECK-SYMB: // ProbeImplicit_BaseIsolatedDealloc.__deallocating_deinit
 // CHECK-SYMB-NEXT: // Isolation: nonisolated
 // CHECK-SYMB-NEXT: sil hidden [ossa] @$s4test33ProbeImplicit_BaseIsolatedDeallocCfD : $@convention(method) (@owned ProbeImplicit_BaseIsolatedDealloc) -> () {
@@ -191,9 +195,8 @@ class ProbeImplicit_BaseIsolatedDealloc: BaseIsolatedDealloc {}
 // CHECK-LABEL: @objc @_inheritsConvenienceInitializers class ProbeExplicit_BaseIsolatedDealloc : BaseIsolatedDealloc {
 // CHECK: @objc deinit
 // CHECK: }
-// CHECK-SYMB: // ProbeExplicit_BaseIsolatedDealloc.__isolated_deallocating_deinit
-// CHECK-SYMB-NEXT: // Isolation: global_actor. type: MainActor
-// CHECK-SYMB-NEXT: sil private [ossa] @$s4test33ProbeExplicit_BaseIsolatedDeallocCfZ : $@convention(thin) (@owned ProbeExplicit_BaseIsolatedDealloc) -> () {
+// CHECK-SYMB-NOT: ProbeExplicit_BaseIsolatedDealloc.__isolated_deallocating_deinit
+// CHECK-SYMB-NOT: @$s4test33ProbeExplicit_BaseIsolatedDeallocCfZ
 // CHECK-SYMB: // ProbeExplicit_BaseIsolatedDealloc.__deallocating_deinit
 // CHECK-SYMB-NEXT: // Isolation: nonisolated
 // CHECK-SYMB-NEXT: sil hidden [ossa] @$s4test33ProbeExplicit_BaseIsolatedDeallocCfD : $@convention(method) (@owned ProbeExplicit_BaseIsolatedDealloc) -> () {
@@ -203,12 +206,17 @@ class ProbeExplicit_BaseIsolatedDealloc: BaseIsolatedDealloc {
     }
 }
 
+#if !SILGEN
+class ProbeAnother_BaseIsolatedDealloc: BaseIsolatedDealloc{
+    @AnotherActor deinit {} // expected-error {{global actor 'AnotherActor'-isolated deinitializer 'deinit' has different actor isolation from main actor-isolated overridden declaration}}
+}
+#endif
+
 // CHECK-LABEL: @objc @_inheritsConvenienceInitializers class ProbeImplicit_DerivedIsolatedDealloc : DerivedIsolatedDealloc {
 // CHECK: @objc @MainActor @preconcurrency deinit
 // CHECK: }
-// CHECK-SYMB: // ProbeImplicit_DerivedIsolatedDealloc.__isolated_deallocating_deinit
-// CHECK-SYMB-NEXT: // Isolation: global_actor. type: MainActor
-// CHECK-SYMB: sil private [ossa] @$s4test36ProbeImplicit_DerivedIsolatedDeallocCfZ : $@convention(thin) (@owned ProbeImplicit_DerivedIsolatedDealloc) -> () {
+// CHECK-SYMB-NOT: ProbeImplicit_DerivedIsolatedDealloc.__isolated_deallocating_deinit
+// CHECK-SYMB-NOT: @$s4test36ProbeImplicit_DerivedIsolatedDeallocCfZ
 // CHECK-SYMB: // ProbeImplicit_DerivedIsolatedDealloc.__deallocating_deinit
 // CHECK-SYMB-NEXT: // Isolation: nonisolated
 // CHECK-SYMB-NEXT: sil hidden [ossa] @$s4test36ProbeImplicit_DerivedIsolatedDeallocCfD : $@convention(method) (@owned ProbeImplicit_DerivedIsolatedDealloc) -> () {
@@ -217,9 +225,8 @@ class ProbeImplicit_DerivedIsolatedDealloc: DerivedIsolatedDealloc {}
 // CHECK-LABEL: @objc @_inheritsConvenienceInitializers class ProbeExplicit_DerivedIsolatedDealloc : DerivedIsolatedDealloc {
 // CHECK: @objc deinit
 // CHECK: }
-// CHECK-SYMB: // ProbeExplicit_DerivedIsolatedDealloc.__isolated_deallocating_deinit
-// CHECK-SYMB-NEXT: // Isolation: global_actor. type: MainActor
-// CHECK-SYMB-NEXT: sil private [ossa] @$s4test36ProbeExplicit_DerivedIsolatedDeallocCfZ : $@convention(thin) (@owned ProbeExplicit_DerivedIsolatedDealloc) -> () {
+// CHECK-SYMB-NOT: ProbeExplicit_DerivedIsolatedDealloc.__isolated_deallocating_deinit
+// CHECK-SYMB-NOT: @$s4test36ProbeExplicit_DerivedIsolatedDeallocCfZ
 // CHECK-SYMB: // ProbeExplicit_DerivedIsolatedDealloc.__deallocating_deinit
 // CHECK-SYMB-NEXT: // Isolation: nonisolated
 // CHECK-SYMB-NEXT: sil hidden [ossa] @$s4test36ProbeExplicit_DerivedIsolatedDeallocCfD : $@convention(method) (@owned ProbeExplicit_DerivedIsolatedDealloc) -> () {
@@ -229,6 +236,12 @@ class ProbeExplicit_DerivedIsolatedDealloc: DerivedIsolatedDealloc {
     }
 }
 
+#if !SILGEN
+class ProbeAnother_DerivedIsolatedDealloc: DerivedIsolatedDealloc{
+    @AnotherActor deinit {} // expected-error {{global actor 'AnotherActor'-isolated deinitializer 'deinit' has different actor isolation from main actor-isolated overridden declaration}}
+}
+#endif
+
 // CHECK-LABEL: @objc @_inheritsConvenienceInitializers class ProbeImplicit_DeallocIsolatedFromProtocol : DeallocIsolatedFromProtocol {
 // CHECK: @objc deinit
 // CHECK: }
@@ -239,6 +252,10 @@ class ProbeExplicit_DerivedIsolatedDealloc: DerivedIsolatedDealloc {
 // CHECK-SYMB-NEXT: sil hidden [ossa] @$s4test41ProbeImplicit_DeallocIsolatedFromProtocolCfD : $@convention(method) (@owned ProbeImplicit_DeallocIsolatedFromProtocol) -> () {
 class ProbeImplicit_DeallocIsolatedFromProtocol: DeallocIsolatedFromProtocol {}
 
+class ProbeAnother_DeallocIsolatedFromProtocol: DeallocIsolatedFromProtocol {
+    @AnotherActor deinit {} // ok, base is not isolated
+}
+
 // CHECK-LABEL: @objc @_inheritsConvenienceInitializers class ProbeImplicit_DeallocIsolatedFromCategory : DeallocIsolatedFromCategory {
 // CHECK: @objc deinit
 // CHECK: }
@@ -249,6 +266,10 @@ class ProbeImplicit_DeallocIsolatedFromProtocol: DeallocIsolatedFromProtocol {}
 // CHECK-SYMB-NEXT: sil hidden [ossa] @$s4test41ProbeImplicit_DeallocIsolatedFromCategoryCfD : $@convention(method) (@owned ProbeImplicit_DeallocIsolatedFromCategory) -> () {
 class ProbeImplicit_DeallocIsolatedFromCategory: DeallocIsolatedFromCategory {}
 
+class ProbeAnother_DeallocIsolatedFromCategory: DeallocIsolatedFromCategory {
+    @AnotherActor deinit {} // ok, base is not isolated
+}
+
 // CHECK-LABEL: @objc @_inheritsConvenienceInitializers class ProbeImplicit_DeallocIsolatedFromExtension : DeallocIsolatedFromExtension {
 // CHECK: @objc deinit
 // CHECK: }
@@ -258,3 +279,7 @@ class ProbeImplicit_DeallocIsolatedFromCategory: DeallocIsolatedFromCategory {}
 // CHECK-SYMB-NEXT: // Isolation: nonisolated
 // CHECK-SYMB-NEXT: sil hidden [ossa] @$s4test42ProbeImplicit_DeallocIsolatedFromExtensionCfD : $@convention(method) (@owned ProbeImplicit_DeallocIsolatedFromExtension) -> () {
 class ProbeImplicit_DeallocIsolatedFromExtension: DeallocIsolatedFromExtension {}
+
+class ProbeAnother_DeallocIsolatedFromExtension: DeallocIsolatedFromExtension {
+    @AnotherActor deinit {} // ok, base is not isolated
+}
