[Concurrency,Distributed] ban inheriting Actor and DistActor explicitly by class

ktoso · ktoso · commit 085a1a98b7fd · 2021-06-24T11:52:51.000+09:00
diff --git a/docs/Diagnostics.md b/docs/Diagnostics.md
@@ -51,6 +51,7 @@ Clang also has a kind of diagnostic called a "remark", which represents informat
   - "...to silence this warning"
   - "...here" (for a purely locational note)
 
+- If possible, it is best to include the name of the type or function that has the error, e.g. "non-actor type 'Nope' cannot ..." is better than "non-actor type cannot ...". It helps developers relate the error message to the specific type the error is about, even if the error would highlight the appropriate line / function in other ways. 
 
 ### Locations and Highlights ###
 
diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def
@@ -4349,7 +4349,15 @@ ERROR(async_objc_dynamic_self,none,
       "asynchronous method returning 'Self' cannot be '@objc'", ())
 
 ERROR(actor_inheritance,none,
-      "actor types do not support inheritance", ())
+      "%select{actor|distributed actor}0 types do not support inheritance",
+      (bool))
+
+ERROR(actor_protocol_illegal_inheritance,none,
+      "non-actor type %0 cannot conform to the 'Actor' protocol",
+      (DeclName))
+ERROR(distributed_actor_protocol_illegal_inheritance,none,
+      "non-distributed actor type %0 cannot conform to the 'DistributedActor' protocol",
+      (DeclName))
 
 // FIXME: This diagnostic was temporarily downgraded from an error because
 // it spuriously triggers when building the Foundation module from its textual
diff --git a/include/swift/AST/TypeCheckRequests.h b/include/swift/AST/TypeCheckRequests.h
@@ -915,7 +915,7 @@ class IsDefaultActorRequest :
   bool isCached() const { return true; }
 };
 
-/// Determine whether the given class is an distributed actor.
+/// Determine whether the given class is a distributed actor.
 class IsDistributedActorRequest :
     public SimpleRequest<IsDistributedActorRequest,
         bool(NominalTypeDecl *),
diff --git a/lib/AST/Decl.cpp b/lib/AST/Decl.cpp
@@ -5167,7 +5167,8 @@ void ProtocolDecl::computeKnownProtocolKind() const {
   if (module != module->getASTContext().getStdlibModule() &&
       !module->getName().is("Foundation") &&
       !module->getName().is("_Differentiation") &&
-      !module->getName().is("_Concurrency")) {
+      !module->getName().is("_Concurrency") &&
+      !module->getName().is("_Distributed")) {
     const_cast<ProtocolDecl *>(this)->Bits.ProtocolDecl.KnownProtocol = 1;
     return;
   }
diff --git a/lib/AST/ProtocolConformance.cpp b/lib/AST/ProtocolConformance.cpp
@@ -1250,10 +1250,10 @@ void NominalTypeDecl::prepareConformanceTable() const {
 
   // Actor classes conform to the actor protocol.
   if (auto classDecl = dyn_cast<ClassDecl>(mutableThis)) {
-    if (classDecl->isActor())
-      addSynthesized(KnownProtocolKind::Actor);
     if (classDecl->isDistributedActor())
       addSynthesized(KnownProtocolKind::DistributedActor);
+    else if (classDecl->isActor())
+      addSynthesized(KnownProtocolKind::Actor);
   }
 
   // Global actors conform to the GlobalActor protocol.
diff --git a/lib/Sema/TypeCheckDeclPrimary.cpp b/lib/Sema/TypeCheckDeclPrimary.cpp
@@ -169,6 +169,14 @@ static void checkInheritanceClause(
     if (!inheritedTy || inheritedTy->hasError())
       continue;
 
+
+//    if (inheritedTy->isActorType()) {
+//      auto classDecl = dyn_cast<ClassDecl>(decl);
+//      if(isa<ProtocolDecl>(decl) || (classDecl && !classDecl->isExplicitActor())) {
+//        assert(false && "only explicit actor or protocol may conform to 'Actor' protocol");
+//      }
+//    }
+
     // For generic parameters and associated types, the GSB checks constraints;
     // however, we still want to fire off the requests to produce diagnostics
     // in some circular validation cases.
@@ -2396,9 +2404,11 @@ class DeclChecker : public DeclVisitor<DeclChecker> {
     if (auto superclass = CD->getSuperclassDecl()) {
       // Actors cannot have superclasses, nor can they be superclasses.
       if (CD->isActor() && !superclass->isNSObject())
-        CD->diagnose(diag::actor_inheritance);
+        CD->diagnose(diag::actor_inheritance,
+                     /*distributed=*/CD->isDistributedActor());
       else if (superclass->isActor())
-        CD->diagnose(diag::actor_inheritance);
+        CD->diagnose(diag::actor_inheritance,
+                     /*distributed=*/CD->isDistributedActor());
     }
 
     // Force lowering of stored properties.
diff --git a/lib/Sema/TypeCheckProtocol.cpp b/lib/Sema/TypeCheckProtocol.cpp
@@ -5851,6 +5851,31 @@ void TypeChecker::checkConformancesInContext(IterableDeclContext *idc) {
       }
     } else if (proto->isSpecificProtocol(KnownProtocolKind::Sendable)) {
       SendableConformance = conformance;
+    } else if (proto->isSpecificProtocol(KnownProtocolKind::DistributedActor)) {
+      if (auto classDecl = dyn_cast<ClassDecl>(nominal)) {
+        if (!classDecl->isDistributedActor()) {
+          if (classDecl->isActor()) {
+            dc->getSelfNominalTypeDecl()
+            ->diagnose(diag::distributed_actor_protocol_illegal_inheritance,
+                       dc->getSelfNominalTypeDecl()->getName())
+                       .fixItInsert(classDecl->getStartLoc(), "distributed ");
+          } else {
+            dc->getSelfNominalTypeDecl()
+            ->diagnose(diag::actor_protocol_illegal_inheritance,
+                       dc->getSelfNominalTypeDecl()->getName())
+                       .fixItReplace(nominal->getStartLoc(), "distributed actor");
+          }
+        }
+      }
+    } else if (proto->isSpecificProtocol(KnownProtocolKind::Actor)) {
+      if (auto classDecl = dyn_cast<ClassDecl>(nominal)) {
+        if (!classDecl->isExplicitActor()) {
+          dc->getSelfNominalTypeDecl()
+              ->diagnose(diag::actor_protocol_illegal_inheritance,
+                         dc->getSelfNominalTypeDecl()->getName())
+              .fixItReplace(nominal->getStartLoc(), "actor");
+        }
+      }
     } else if (proto->isSpecificProtocol(
                    KnownProtocolKind::UnsafeSendable)) {
       unsafeSendableConformance = conformance;
diff --git a/stdlib/public/Concurrency/MainActor.swift b/stdlib/public/Concurrency/MainActor.swift
@@ -15,7 +15,8 @@ import Swift
 /// A singleton actor whose executor is equivalent to the main
 /// dispatch queue.
 @available(SwiftStdlib 5.5, *)
-@globalActor public final actor MainActor: GlobalActor {
+@globalActor public final actor MainActor {
+//@globalActor public final actor MainActor: GlobalActor { // TODO: specialize this case?
   public static let shared = MainActor()
 
   @inlinable
diff --git a/stdlib/public/Distributed/ActorTransport.swift b/stdlib/public/Distributed/ActorTransport.swift
@@ -0,0 +1,47 @@
+//===----------------------------------------------------------------------===//
+//
+// This source file is part of the Swift.org open source project
+//
+// Copyright (c) 2020-2021 Apple Inc. and the Swift project authors
+// Licensed under Apache License v2.0 with Runtime Library Exception
+//
+// See https://swift.org/LICENSE.txt for license information
+// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
+//
+//===----------------------------------------------------------------------===//
+
+import Swift
+import _Concurrency
+
+@available(SwiftStdlib 5.5, *)
+public protocol ActorTransport: Sendable {
+    /// Resolve a local or remote actor address to a real actor instance, or throw if unable to.
+    /// The returned value is either a local actor or proxy to a remote actor.
+    func resolve<Act>(address: ActorAddress, as actorType: Act.Type)
+        throws -> ActorResolved<Act> where Act: DistributedActor
+
+    /// Create an `ActorAddress` for the passed actor type.
+    ///
+    /// This function is invoked by an distributed actor during its initialization,
+    /// and the returned address value is stored along with it for the time of its
+    /// lifetime.
+    ///
+    /// The address MUST uniquely identify the actor, and allow resolving it.
+    /// E.g. if an actor is created under address `addr1` then immediately invoking
+    /// `transport.resolve(address: addr1, as: Greeter.self)` MUST return a reference
+    /// to the same actor.
+    func assignAddress<Act>(_ actorType: Act.Type) -> ActorAddress
+        where Act: DistributedActor
+
+    func actorReady<Act>(_ actor: Act) where Act: DistributedActor
+
+    /// Called during actor deinit/destroy.
+    func resignAddress(_ address: ActorAddress)
+
+}
+
+@available(SwiftStdlib 5.5, *)
+public enum ActorResolved<Act: DistributedActor> {
+    case resolved(Act)
+    case makeProxy
+}
diff --git a/stdlib/public/Distributed/CMakeLists.txt b/stdlib/public/Distributed/CMakeLists.txt
@@ -15,6 +15,7 @@ set(swift_distributed_link_libraries
 
 add_swift_target_library(swift_Distributed ${SWIFT_STDLIB_LIBRARY_BUILD_TYPES} IS_STDLIB
   AssertDistributed.swift
+  ActorTransport.swift
   DistributedActor.swift
 
   SWIFT_MODULE_DEPENDS_LINUX Glibc
diff --git a/stdlib/public/Distributed/DistributedActor.swift b/stdlib/public/Distributed/DistributedActor.swift
@@ -13,6 +13,16 @@
 import Swift
 import _Concurrency
 
+// ==== Any Actor -------------------------------------------------------------
+
+/// Shared "base" protocol for both (local) `Actor` and (potentially remote)
+/// `DistributedActor`.
+///
+/// FIXME: !!! We'd need Actor to also conform to this, but don't want to add that conformance in _Concurrency yet.
+public protocol AnyActor: AnyObject {}
+
+// ==== Distributed Actor -----------------------------------------------------
+
 /// Common protocol to which all distributed actors conform implicitly.
 ///
 /// It is not possible to conform to this protocol manually explicitly.
@@ -24,7 +34,7 @@ import _Concurrency
 /// which involves enqueuing new partial tasks to be executed at some
 /// point.
 @available(SwiftStdlib 5.5, *)
-public protocol DistributedActor: Actor, Codable {
+public protocol DistributedActor: AnyActor, Codable {
 
     /// Creates new (local) distributed actor instance, bound to the passed transport.
     ///
@@ -91,48 +101,6 @@ extension DistributedActor {
         try container.encode(self.actorAddress)
     }
 }
-/******************************************************************************/
-/***************************** Actor Transport ********************************/
-/******************************************************************************/
-
-@available(SwiftStdlib 5.5, *)
-public protocol ActorTransport: Sendable {
-    /// Resolve a local or remote actor address to a real actor instance, or throw if unable to.
-    /// The returned value is either a local actor or proxy to a remote actor.
-    func resolve<Act>(address: ActorAddress, as actorType: Act.Type)
-    throws -> ActorResolved<Act> where Act: DistributedActor
-
-    /// Create an `ActorAddress` for the passed actor type.
-    ///
-    /// This function is invoked by an distributed actor during its initialization,
-    /// and the returned address value is stored along with it for the time of its
-    /// lifetime.
-    ///
-    /// The address MUST uniquely identify the actor, and allow resolving it.
-    /// E.g. if an actor is created under address `addr1` then immediately invoking
-    /// `transport.resolve(address: addr1, as: Greeter.self)` MUST return a reference
-    /// to the same actor.
-    func assignAddress<Act>(
-        _ actorType: Act.Type
-    ) -> ActorAddress
-        where Act: DistributedActor
-
-    func actorReady<Act>(
-        _ actor: Act
-    ) where Act: DistributedActor
-
-    /// Called during actor deinit/destroy.
-    func resignAddress(
-        _ address: ActorAddress
-    )
-
-}
-
-@available(SwiftStdlib 5.5, *)
-public enum ActorResolved<Act: DistributedActor> {
-    case resolved(Act)
-    case makeProxy
-}
 
 /******************************************************************************/
 /***************************** Actor Address **********************************/
diff --git a/test/Concurrency/actor_isolation.swift b/test/Concurrency/actor_isolation.swift
@@ -752,6 +752,20 @@ func outsideSomeClassWithInits() { // expected-note 3 {{add '@MainActor' to make
 // ----------------------------------------------------------------------
 // Actor protocols.
 // ----------------------------------------------------------------------
+
+@available(SwiftStdlib 5.5, *)
+actor A: Actor { // ok
+}
+
+@available(SwiftStdlib 5.5, *)
+class C: Actor, UnsafeSendable {
+  // expected-error@-1{{non-actor type 'C' cannot conform to the 'Actor' protocol}}
+  // expected-warning@-2{{'UnsafeSendable' is deprecated: Use @unchecked Sendable instead}}
+  nonisolated var unownedExecutor: UnownedSerialExecutor {
+    fatalError()
+  }
+}
+
 @available(SwiftStdlib 5.5, *)
 protocol P: Actor {
   func f()
diff --git a/test/Distributed/actor_protocols.swift b/test/Distributed/actor_protocols.swift
@@ -0,0 +1,83 @@
+// RUN: %target-typecheck-verify-swift -enable-experimental-distributed
+// REQUIRES: concurrency
+// REQUIRES: distributed
+
+import _Distributed
+
+// ==== -----------------------------------------------------------------------
+
+@available(SwiftStdlib 5.5, *)
+actor A: Actor {} // ok
+
+@available(SwiftStdlib 5.5, *)
+class C: Actor, UnsafeSendable {
+  // expected-error@-1{{non-actor type 'C' cannot conform to the 'Actor' protocol}} {{1-6=actor}}
+  // expected-warning@-2{{'UnsafeSendable' is deprecated: Use @unchecked Sendable instead}}
+  nonisolated var unownedExecutor: UnownedSerialExecutor {
+    fatalError()
+  }
+}
+
+@available(SwiftStdlib 5.5, *)
+struct S: Actor {
+  // expected-error@-1{{non-class type 'S' cannot conform to class protocol 'Actor'}}
+  nonisolated var unownedExecutor: UnownedSerialExecutor {
+    fatalError()
+  }
+}
+
+@available(SwiftStdlib 5.5, *)
+struct E: Actor {
+  // expected-error@-1{{non-class type 'E' cannot conform to class protocol 'Actor'}}
+  nonisolated var unownedExecutor: UnownedSerialExecutor {
+    fatalError()
+  }
+}
+
+// ==== -----------------------------------------------------------------------
+
+@available(SwiftStdlib 5.5, *)
+distributed actor DA: DistributedActor {} // ok
+
+@available(SwiftStdlib 5.5, *)
+actor A2: DistributedActor {
+  // expected-error@-1{{non-distributed actor type 'A2' cannot conform to the 'DistributedActor' protocol}} {{1-1=distributed }}
+  nonisolated var actorAddress: ActorAddress {
+    fatalError()
+  }
+  nonisolated var actorTransport: ActorTransport {
+    fatalError()
+  }
+
+  required init(transport: ActorTransport) {
+    fatalError()
+  }
+  required init(resolve address: ActorAddress, using transport: ActorTransport) throws {
+    fatalError()
+  }
+}
+
+@available(SwiftStdlib 5.5, *)
+class C2: DistributedActor {
+  // expected-error@-1{{non-actor type 'C2' cannot conform to the 'Actor' protocol}}
+  nonisolated var actorAddress: ActorAddress {
+    fatalError()
+  }
+  nonisolated var actorTransport: ActorTransport {
+    fatalError()
+  }
+
+  required init(transport: ActorTransport) {
+    fatalError()
+  }
+  required init(resolve address: ActorAddress, using transport: ActorTransport) throws {
+    fatalError()
+  }
+}
+
+@available(SwiftStdlib 5.5, *)
+struct S2: DistributedActor {
+  // expected-error@-1{{non-class type 'S2' cannot conform to class protocol 'DistributedActor'}}
+  // expected-error@-2{{non-class type 'S2' cannot conform to class protocol 'AnyActor'}}
+}
+
diff --git a/test/Distributed/distributed_actor_cannot_be_downcast_to_actor.swift b/test/Distributed/distributed_actor_cannot_be_downcast_to_actor.swift
@@ -0,0 +1,27 @@
+// RUN: %target-typecheck-verify-swift -enable-experimental-distributed
+// REQUIRES: concurrency
+// REQUIRES: distributed
+
+import _Distributed
+
+@available(SwiftStdlib 5.5, *)
+extension Actor {
+    func f() -> String { "Life is Study!" }
+}
+
+@available(SwiftStdlib 5.5, *)
+func g<A: Actor>(a: A) async { // expected-note{{where 'A' = 'MA'}}
+    print(await a.f())
+}
+
+@available(SwiftStdlib 5.5, *)
+distributed actor MA {
+}
+
+@available(SwiftStdlib 5.5, *)
+func h(ma: MA) async {
+    // this would have been a bug, a non distributed function might have been called here,
+    // so we must not allow for it, because if the actor was remote calling a non-distributed func
+    // would result in a hard crash (as there is no local actor to safely call the function on).
+    await g(a: ma) // expected-error{{global function 'g(a:)' requires that 'MA' conform to 'Actor'}}
+}
diff --git a/test/decl/class/actor/basic.swift b/test/decl/class/actor/basic.swift
diff --git a/test/decl/protocol/special/Actor.swift b/test/decl/protocol/special/Actor.swift
