Reimplement isolation checking for overrides in terms of actor references.

DougGregor · DougGregor · commit 663eb0e610fc · 2022-04-15T18:06:33.000-07:00
As with other refactorings to use `ActorReferenceResult`, maintain the
existing behavior even where it is technically wrong.
diff --git a/lib/Sema/TypeCheckConcurrency.cpp b/lib/Sema/TypeCheckConcurrency.cpp
@@ -3396,34 +3396,79 @@ static bool checkClassGlobalActorIsolation(
   return true;
 }
 
+namespace {
+  /// Describes the result of checking override isolation.
+  enum class OverrideIsolationResult {
+    /// The override is permitted.
+    Allowed,
+    /// The override is permitted, but requires a Sendable check.
+    Sendable,
+    /// The override is not permitted.
+    Disallowed,
+  };
+}
+
+/// Return the isolation of the declaration overridden by this declaration,
+/// in the context of the
+static ActorIsolation getOverriddenIsolationFor(ValueDecl *value) {
+  auto overridden = value->getOverriddenDecl();
+  assert(overridden && "Doesn't have an overridden declaration");
+
+  auto isolation = getActorIsolation(overridden);
+  if (!isolation.requiresSubstitution())
+    return isolation;
+
+  SubstitutionMap subs;
+  if (Type selfType = value->getDeclContext()->getSelfInterfaceType()) {
+    subs = selfType->getMemberSubstitutionMap(
+        value->getModuleContext(), overridden);
+  }
+  return isolation.subst(subs);
+}
+
 /// Generally speaking, the isolation of the decl that overrides
 /// must match the overridden decl. But there are a number of exceptions,
 /// e.g., the decl that overrides can be nonisolated.
-/// \param newIso the isolation of the overriding declaration.
-static bool validOverrideIsolation(ActorIsolation newIso,
-                                   ValueDecl *overriddenDecl,
-                                   ActorIsolation overriddenIso) {
-  // If the isolation matches, we're done.
-  if (newIso == overriddenIso)
-    return true;
-
-  // If the overriding declaration is non-isolated, it's okay.
-  if (newIso.isIndependent() || newIso.isUnspecified())
-    return true;
+/// \param isolation the isolation of the overriding declaration.
+static OverrideIsolationResult validOverrideIsolation(
+    ValueDecl *value, ActorIsolation isolation,
+    ValueDecl *overridden, ActorIsolation overriddenIsolation) {
+  ConcreteDeclRef valueRef(value);
+  auto declContext = value->getInnermostDeclContext();
+  if (auto genericEnv = declContext->getGenericEnvironmentOfContext()) {
+    valueRef = ConcreteDeclRef(
+        value, genericEnv->getForwardingSubstitutionMap());
+  }
 
-  // If both are actor-instance isolated, we're done. This wasn't caught by
-  // the equality case above because the nominal type describing the actor
-  // will differ when we're overriding.
-  if (newIso.getKind() == overriddenIso.getKind() &&
-      newIso.getKind() == ActorIsolation::ActorInstance)
-    return true;
+  auto refResult = ActorReferenceResult::forReference(
+      valueRef, SourceLoc(), declContext, None, None,
+      isolation, overriddenIsolation);
+  switch (refResult) {
+  case ActorReferenceResult::SameConcurrencyDomain:
+    return OverrideIsolationResult::Allowed;
+
+  case ActorReferenceResult::ExitsActorToNonisolated:
+    return OverrideIsolationResult::Sendable;
+
+  case ActorReferenceResult::EntersActor:
+    // It's okay to enter the actor when the overridden declaration is
+    // asynchronous (because it will do the switch) or is accessible from
+    // anywhere.
+    if (isAsyncDecl(overridden) ||
+        isAccessibleAcrossActors(
+            overridden, refResult.isolation, declContext)) {
+      // FIXME: Perform Sendable checking here because we're entering an
+      // actor.
+      return OverrideIsolationResult::Allowed;
+    }
 
-  // If the overridden declaration is from Objective-C with no actor annotation,
-  // allow it.
-  if (overriddenDecl->hasClangNode() && !overriddenIso)
-    return true;
+    // If the overridden declaration is from Objective-C with no actor
+    // annotation, allow it.
+    if (overridden->hasClangNode() && !overriddenIsolation)
+      return OverrideIsolationResult::Allowed;
 
-  return false;
+    return OverrideIsolationResult::Disallowed;
+  }
 }
 
 ActorIsolation ActorIsolationRequest::evaluate(
@@ -3485,20 +3530,11 @@ ActorIsolation ActorIsolationRequest::evaluate(
 
   // Look for and remember the overridden declaration's isolation.
   Optional<ActorIsolation> overriddenIso;
-  ValueDecl *overriddenValue = nullptr;
-  if ( (overriddenValue = value->getOverriddenDecl()) ) {
-    auto iso = getActorIsolation(overriddenValue);
-    SubstitutionMap subs;
-
-    if (Type selfType = value->getDeclContext()->getSelfInterfaceType()) {
-      subs = selfType->getMemberSubstitutionMap(
-          value->getModuleContext(), overriddenValue);
-    }
-    iso = iso.subst(subs);
-
+  ValueDecl *overriddenValue = value->getOverriddenDecl();
+  if (overriddenValue) {
     // use the overridden decl's iso as the default isolation for this decl.
-    defaultIsolation = iso;
-    overriddenIso = iso;
+    defaultIsolation = getOverriddenIsolationFor(value);
+    overriddenIso = defaultIsolation;
   }
 
   // Function used when returning an inferred isolation.
@@ -3509,8 +3545,16 @@ ActorIsolation ActorIsolationRequest::evaluate(
     if (overriddenValue) {
       // if the inferred isolation is not valid, then carry-over the overridden
       // declaration's isolation as this decl's inferred isolation.
-      if (!validOverrideIsolation(inferred, overriddenValue, *overriddenIso))
+      switch (validOverrideIsolation(
+                  value, inferred, overriddenValue, *overriddenIso)) {
+      case OverrideIsolationResult::Allowed:
+      case OverrideIsolationResult::Sendable:
+        break;
+
+      case OverrideIsolationResult::Disallowed:
         inferred = *overriddenIso;
+        break;
+      }
     }
 
     // Add an implicit attribute to capture the actor isolation that was
@@ -3779,29 +3823,36 @@ void swift::checkOverrideActorIsolation(ValueDecl *value) {
   if (!overridden)
     return;
 
-  // Determine the actor isolation of this declaration.
+  // Determine the actor isolation of the overriding function.
   auto isolation = getActorIsolation(value);
+  
+  // Determine the actor isolation of the overridden function.
+  auto overriddenIsolation = getOverriddenIsolationFor(value);
+  switch (validOverrideIsolation(
+              value, isolation, overridden, overriddenIsolation)) {
+  case OverrideIsolationResult::Allowed:
+    return;
 
-  // Determine the actor isolation of the overridden function.=
-  auto overriddenIsolation = getActorIsolation(overridden);
-
-  if (overriddenIsolation.requiresSubstitution()) {
-    SubstitutionMap subs;
-    if (Type selfType = value->getDeclContext()->getSelfInterfaceType()) {
-      subs = selfType->getMemberSubstitutionMap(
-          value->getModuleContext(), overridden);
-    }
+  case OverrideIsolationResult::Sendable:
+    // FIXME: Do the Sendable check.
+    return;
 
-    overriddenIsolation = overriddenIsolation.subst(subs);
+  case OverrideIsolationResult::Disallowed:
+    // Diagnose below.
+    break;
   }
 
-  if (validOverrideIsolation(isolation, overridden, overriddenIsolation))
-    return;
-
   // Isolation mismatch. Diagnose it.
+  DiagnosticBehavior behavior = DiagnosticBehavior::Unspecified;
+  if (overridden->hasClangNode() && !overriddenIsolation) {
+    behavior = SendableCheckContext(value->getInnermostDeclContext())
+        .defaultDiagnosticBehavior();
+  }
+
   value->diagnose(
       diag::actor_isolation_override_mismatch, isolation,
-      value->getDescriptiveKind(), value->getName(), overriddenIsolation);
+      value->getDescriptiveKind(), value->getName(), overriddenIsolation)
+    .limitBehavior(behavior);
   overridden->diagnose(diag::overridden_here);
 }
 
@@ -4796,14 +4847,6 @@ ActorReferenceResult ActorReferenceResult::forExitsActorToNonisolated(
   return ActorReferenceResult{ExitsActorToNonisolated, None, isolation};
 }
 
-ActorReferenceResult ActorReferenceResult::forReference(
-    ConcreteDeclRef declRef, SourceLoc declRefLoc, const DeclContext *fromDC,
-    Optional<VarRefUseEnv> useKind,
-    Optional<ReferencedActor> actorInstance) {
-  return forReference(declRef, declRefLoc, fromDC, useKind, actorInstance,
-                      getInnermostIsolatedContext(fromDC));
-}
-
 // Determine if two actor isolation contexts are considered to be equivalent.
 static bool equivalentIsolationContexts(
     const ActorIsolation &lhs, const ActorIsolation &rhs) {
@@ -4822,11 +4865,26 @@ ActorReferenceResult ActorReferenceResult::forReference(
     ConcreteDeclRef declRef, SourceLoc declRefLoc, const DeclContext *fromDC,
     Optional<VarRefUseEnv> useKind,
     Optional<ReferencedActor> actorInstance,
-    ActorIsolation contextIsolation) {
-  // Compute the isolation of the declaration, adjusted for references.
-  auto declIsolation = getActorIsolationForReference(declRef.getDecl(), fromDC);
-  if (auto subs = declRef.getSubstitutions())
-    declIsolation = declIsolation.subst(subs);
+    Optional<ActorIsolation> knownDeclIsolation,
+    Optional<ActorIsolation> knownContextIsolation) {
+  // If not provided, compute the isolation of the declaration, adjusted
+  // for references.
+  ActorIsolation declIsolation = ActorIsolation::forUnspecified();
+  if (knownDeclIsolation) {
+    declIsolation = *knownDeclIsolation;
+  } else {
+    declIsolation = getActorIsolationForReference(declRef.getDecl(), fromDC);
+    if (declIsolation.requiresSubstitution())
+      declIsolation = declIsolation.subst(declRef.getSubstitutions());
+  }
+
+  // Compute the isolation of the context, if not provided.
+  ActorIsolation contextIsolation = ActorIsolation::forUnspecified();
+  if (knownContextIsolation) {
+    contextIsolation = *knownContextIsolation;
+  } else {
+    contextIsolation = getInnermostIsolatedContext(fromDC);
+  }
 
   // When the declaration is not actor-isolated, it can always be accessed
   // directly.
diff --git a/lib/Sema/TypeCheckConcurrency.h b/lib/Sema/TypeCheckConcurrency.h
@@ -238,26 +238,9 @@ struct ActorReferenceResult {
       SourceLoc declRefLoc,
       const DeclContext *fromDC,
       Optional<VarRefUseEnv> useKind = None,
-      Optional<ReferencedActor> actorInstance = None);
-
-  /// Determine what happens when referencing the given declaration from the
-  /// given declaration context.
-  ///
-  ///
-  /// \param declRef The declaration that is being referenced.
-  ///
-  /// \param fromDC The declaration context from which the reference occurs.
-  ///
-  /// \param actorInstance When not \c None, the actor instance value that is
-  /// provided when referencing the declaration. This can be either the base
-  /// of a member access or a parameter passed to a function.
-  static ActorReferenceResult forReference(
-      ConcreteDeclRef declRef,
-      SourceLoc declRefLoc,
-      const DeclContext *fromDC,
-      Optional<VarRefUseEnv> useKind,
-      Optional<ReferencedActor> actorInstance,
-      ActorIsolation contextIsolation);
+      Optional<ReferencedActor> actorInstance = None,
+      Optional<ActorIsolation> knownDeclIsolation = None,
+      Optional<ActorIsolation> knownContextIsolation = None);
 
   operator Kind() const { return kind; }
 };
diff --git a/lib/Sema/TypeCheckProtocol.cpp b/lib/Sema/TypeCheckProtocol.cpp
@@ -2956,7 +2956,7 @@ bool ConformanceChecker::checkActorIsolation(
 
   auto refResult = ActorReferenceResult::forReference(
       getConcreteWitness(), witness->getLoc(), DC, None, None,
-      requirementIsolation);
+      None, requirementIsolation);
   bool sameConcurrencyDomain = false;
   switch (refResult) {
   case ActorReferenceResult::SameConcurrencyDomain:
