Merge pull request #42136 from ktoso/wip-identifiable-synthesis-fix

[Distributed] ID synthesis must be eager, or we run into issues in real projects

Author: swift-ci

include/swift/AST/Decl.h

@@ -4345,6 +4345,7 @@ enum class KnownDerivableProtocolKind : uint8_t {
   Decodable,
   AdditiveArithmetic,
   Differentiable,
+  Identifiable,
   Actor,
   DistributedActor,
   DistributedActorSystem,

include/swift/AST/KnownProtocols.def

@@ -60,6 +60,7 @@
 
 PROTOCOL(Actor)
 PROTOCOL(Sequence)
+PROTOCOL(Identifiable)
 PROTOCOL(IteratorProtocol)
 PROTOCOL(RawRepresentable)
 PROTOCOL(Equatable)

lib/AST/Decl.cpp

@@ -5622,6 +5622,8 @@ Optional<KnownDerivableProtocolKind>
     return KnownDerivableProtocolKind::AdditiveArithmetic;
   case KnownProtocolKind::Differentiable:
     return KnownDerivableProtocolKind::Differentiable;
+  case KnownProtocolKind::Identifiable:
+    return KnownDerivableProtocolKind::Identifiable;
   case KnownProtocolKind::Actor:
     return KnownDerivableProtocolKind::Actor;
   case KnownProtocolKind::DistributedActor:

lib/IRGen/GenMeta.cpp

@@ -5366,6 +5366,7 @@ SpecialProtocol irgen::getSpecialProtocolID(ProtocolDecl *P) {
   case KnownProtocolKind::AdditiveArithmetic:
   case KnownProtocolKind::Differentiable:
   case KnownProtocolKind::FloatingPoint:
+  case KnownProtocolKind::Identifiable:
   case KnownProtocolKind::Actor:
   case KnownProtocolKind::DistributedActor:
   case KnownProtocolKind::DistributedActorSystem:

lib/Sema/DerivedConformanceDistributedActor.cpp

@@ -24,10 +24,27 @@
 
 using namespace swift;
 
+bool DerivedConformance::canDeriveIdentifiable(
+    NominalTypeDecl *nominal, DeclContext *dc) {
+  // we only synthesize for concrete 'distributed actor' decls (which are class)
+  if (!isa<ClassDecl>(nominal))
+    return false;
+
+  auto &C = nominal->getASTContext();
+  if (!C.getLoadedModule(C.Id_Distributed))
+    return false;
+
+  return nominal->isDistributedActor();
+}
+
 bool DerivedConformance::canDeriveDistributedActor(
     NominalTypeDecl *nominal, DeclContext *dc) {
+  auto &C = nominal->getASTContext();
   auto classDecl = dyn_cast<ClassDecl>(nominal);
-  return classDecl && classDecl->isDistributedActor() && dc == nominal;
+
+  return C.getLoadedModule(C.Id_Distributed) &&
+         classDecl && classDecl->isDistributedActor() &&
+         dc == nominal;
 }
 
 bool DerivedConformance::canDeriveDistributedActorSystem(
@@ -500,6 +517,26 @@ deriveDistributedActorType_ActorSystem(
   return defaultDistributedActorSystemTypeDecl->getDeclaredInterfaceType();
 }
 
+static Type
+deriveDistributedActorType_ID(
+    DerivedConformance &derived) {
+  if (!derived.Nominal->isDistributedActor())
+    return nullptr;
+
+  // Look for a type DefaultDistributedActorSystem within the parent context.
+  auto systemTy = getDistributedActorSystemType(derived.Nominal);
+
+  // There is no known actor system type, so fail to synthesize.
+  if (!systemTy || systemTy->hasError())
+    return nullptr;
+
+  if (auto systemNominal = systemTy->getAnyNominal()) {
+    return getDistributedActorSystemActorIDType(systemNominal);
+  }
+
+  return nullptr;
+}
+
 /******************************************************************************/
 /**************************** ENTRY POINTS ************************************/
 /******************************************************************************/
@@ -538,6 +575,10 @@ std::pair<Type, TypeDecl *> DerivedConformance::deriveDistributedActor(
     return std::make_pair(deriveDistributedActorType_ActorSystem(*this), nullptr);
   }
 
+  if (assocType->getName() == Context.Id_ID) {
+    return std::make_pair(deriveDistributedActorType_ID(*this), nullptr);
+  }
+
   Context.Diags.diagnose(assocType->getLoc(),
                          diag::broken_distributed_actor_requirement);
   return std::make_pair(Type(), nullptr);

lib/Sema/DerivedConformances.cpp

@@ -77,6 +77,9 @@ bool DerivedConformance::derivesProtocolConformance(DeclContext *DC,
 
   if (*derivableKind == KnownDerivableProtocolKind::Actor)
     return canDeriveActor(DC, Nominal);
+
+  if (*derivableKind == KnownDerivableProtocolKind::Identifiable)
+    return canDeriveIdentifiable(Nominal, DC);
   if (*derivableKind == KnownDerivableProtocolKind::DistributedActor)
     return canDeriveDistributedActor(Nominal, DC);
   if (*derivableKind == KnownDerivableProtocolKind::DistributedActorSystem)

lib/Sema/DerivedConformances.h

@@ -323,6 +323,10 @@ class DerivedConformance {
   /// \returns the derived member, which will also be added to the type.
   ValueDecl *deriveDecodable(ValueDecl *requirement);  
 
+  /// Identifiable may need to have the `ID` type witness synthesized explicitly
+  static bool canDeriveIdentifiable(NominalTypeDecl *nominal,
+                                    DeclContext *dc);
+
   /// Whether we can derive the given DistributedActor requirement in the given context.
   static bool canDeriveDistributedActor(NominalTypeDecl *nominal,
                                         DeclContext *dc);

lib/Sema/TypeCheckProtocol.cpp

@@ -6911,6 +6911,15 @@ ValueDecl *TypeChecker::deriveProtocolRequirement(DeclContext *DC,
   case KnownDerivableProtocolKind::Differentiable:
     return derived.deriveDifferentiable(Requirement);
 
+  case KnownDerivableProtocolKind::Identifiable:
+    if (derived.Nominal->isDistributedActor()) {
+      return derived.deriveDistributedActor(Requirement);
+    } else {
+      // No synthesis is required for other types; we should only end up
+      // attempting synthesis if the nominal was a distributed actor.
+      llvm_unreachable("Identifiable is synthesized for distributed actors");
+    }
+
   case KnownDerivableProtocolKind::DistributedActor:
     return derived.deriveDistributedActor(Requirement);
   case KnownDerivableProtocolKind::DistributedActorSystem:
@@ -6947,6 +6956,12 @@ TypeChecker::deriveTypeWitness(DeclContext *DC,
     return derived.deriveDifferentiable(AssocType);
   case KnownProtocolKind::DistributedActor:
     return derived.deriveDistributedActor(AssocType);
+  case KnownProtocolKind::Identifiable:
+    // Identifiable only has derivation logic for distributed actors,
+    // because how it depends on the ActorSystem the actor is associated with.
+    // If the nominal wasn't a distributed actor, we should not end up here,
+    // but either way, then we'd return null (fail derivation).
+    return derived.deriveDistributedActor(AssocType);
   default:
     return std::make_pair(nullptr, nullptr);
   }

lib/Sema/TypeCheckProtocol.h

@@ -1198,6 +1198,20 @@ class AssociatedTypeInference {
                 ConformanceChecker &checker,
                 SmallVectorImpl<InferredTypeWitnessesSolution> &solutions);
 
+  /// We may need to determine a type witness, regardless of the existence of a
+  /// default value for it, e.g. when a 'distributed actor' is looking up its
+  /// 'ID', the default defined in an extension for 'Identifiable' would be
+  /// located using the lookup resolve. This would not be correct, since the
+  /// type actually must be based on the associated 'ActorSystem'.
+  ///
+  /// TODO(distributed): perhaps there is a better way to avoid this mixup?
+  ///   Note though that this issue seems to only manifest in "real" builds
+  ///   involving multiple files/modules, and not in tests within the Swift
+  ///   project itself.
+  bool canAttemptEagerTypeWitnessDerivation(
+      ConformanceChecker &checker,
+      AssociatedTypeDecl *assocType);
+
 public:
   /// Describes a mapping from associated type declarations to their
   /// type witnesses (as interface types).

lib/Sema/TypeCheckProtocolInference.cpp

@@ -2128,6 +2128,20 @@ bool AssociatedTypeInference::diagnoseAmbiguousSolutions(
   return false;
 }
 
+bool AssociatedTypeInference::canAttemptEagerTypeWitnessDerivation(
+    ConformanceChecker &checker,
+    AssociatedTypeDecl *assocType) {
+
+  /// Rather than locating the TypeID via the default implementation of
+  /// Identifiable, we need to find the type based on the associated ActorSystem
+  if (checker.Adoptee->isDistributedActor() &&
+      assocType->getProtocol()->isSpecificProtocol(KnownProtocolKind::Identifiable)) {
+    return true;
+  }
+
+  return false;
+}
+
 auto AssociatedTypeInference::solve(ConformanceChecker &checker)
     -> Optional<InferredTypeWitnesses> {
   // Track when we are checking type witnesses.
@@ -2142,6 +2156,16 @@ auto AssociatedTypeInference::solve(ConformanceChecker &checker)
     if (conformance->hasTypeWitness(assocType))
       continue;
 
+    if (canAttemptEagerTypeWitnessDerivation(checker, assocType)) {
+      auto derivedType = computeDerivedTypeWitness(assocType);
+      if (derivedType.first) {
+        checker.recordTypeWitness(assocType,
+                                  derivedType.first->mapTypeOutOfContext(),
+                                  derivedType.second);
+        continue;
+      }
+    }
+
     // Try to resolve this type witness via name lookup, which is the
     // most direct mechanism, overriding all others.
     switch (checker.resolveTypeWitnessViaLookup(assocType)) {