Merge pull request #40388 from DougGregor/swift-6-stricter-concurrency-checking

DougGregor · web-flow · commit 91fb149faf80 · 2021-12-03T08:40:46.000-08:00
Swift 6 stricter concurrency checking
diff --git a/include/swift/AST/ActorIsolation.h b/include/swift/AST/ActorIsolation.h
@@ -132,6 +132,9 @@ class ActorIsolation {
 
   friend bool operator==(const ActorIsolation &lhs,
                          const ActorIsolation &rhs) {
+    if (lhs.isGlobalActor() && rhs.isGlobalActor())
+      return areTypesEqual(lhs.globalActor, rhs.globalActor);
+
     if (lhs.kind != rhs.kind)
       return false;
 
@@ -146,7 +149,7 @@ class ActorIsolation {
 
     case GlobalActor:
     case GlobalActorUnsafe:
-      return areTypesEqual(lhs.globalActor, rhs.globalActor);
+      llvm_unreachable("Global actors handled above");
     }
   }
 
diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def
@@ -4550,10 +4550,6 @@ ERROR(actor_isolated_witness,none,
 ERROR(distributed_actor_isolated_witness,none,
      "distributed actor-isolated %0 %1 cannot be used to satisfy a protocol requirement",
      (DescriptiveDeclKind, DeclName))
-ERROR(global_actor_isolated_requirement,none,
-      "%0 %1 must be isolated to the global actor %2 to satisfy corresponding "
-      "requirement from protocol %3",
-      (DescriptiveDeclKind, DeclName, Type, Identifier))
 ERROR(global_actor_isolated_witness,none,
       "%0 %1 isolated to global actor %2 can not satisfy corresponding "
       "requirement from protocol %3",
diff --git a/lib/SILGen/SILGenProlog.cpp b/lib/SILGen/SILGenProlog.cpp
@@ -560,34 +560,30 @@ void SILGenFunction::emitProlog(CaptureInfo captureInfo,
         dyn_cast_or_null<AbstractFunctionDecl>(FunctionDC->getAsDecl())) {
     auto actorIsolation = getActorIsolation(funcDecl);
     switch (actorIsolation.getKind()) {
-      case ActorIsolation::Unspecified:
-      case ActorIsolation::Independent:
-        // If this is an async function that has an isolated parameter, hop
-        // to it.
-        if (F.isAsync()) {
-          for (auto param : *funcDecl->getParameters()) {
-            if (param->isIsolated()) {
-              auto loc = RegularLocation::getAutoGeneratedLocation(F.getLocation());
-              Type actorType = param->getType();
-              RValue actorInstanceRV = emitRValueForDecl(
-                  loc, param, actorType, AccessSemantics::Ordinary);
-              ManagedValue actorInstance =
-                  std::move(actorInstanceRV).getScalarValue();
-              ExpectedExecutor = emitLoadActorExecutor(loc, actorInstance);
-              break;
-            }
+    case ActorIsolation::Unspecified:
+    case ActorIsolation::Independent:
+      // If this is an async function that has an isolated parameter, hop
+      // to it.
+      if (F.isAsync()) {
+        for (auto param : *funcDecl->getParameters()) {
+          if (param->isIsolated()) {
+            auto loc = RegularLocation::getAutoGeneratedLocation(F.getLocation());
+            Type actorType = param->getType();
+            RValue actorInstanceRV = emitRValueForDecl(
+                loc, param, actorType, AccessSemantics::Ordinary);
+            ManagedValue actorInstance =
+                std::move(actorInstanceRV).getScalarValue();
+            ExpectedExecutor = emitLoadActorExecutor(loc, actorInstance);
+            break;
           }
         }
-
-        break;
-
-      case ActorIsolation::GlobalActorUnsafe:
-        break;
-
-      case ActorIsolation::DistributedActorInstance: {
-        // TODO: perhaps here we can emit our special handling to make a message?
-        LLVM_FALLTHROUGH;
       }
+      break;
+
+    case ActorIsolation::DistributedActorInstance: {
+      // TODO: perhaps here we can emit our special handling to make a message?
+      LLVM_FALLTHROUGH;
+    }
 
     case ActorIsolation::ActorInstance: {
       assert(selfParam && "no self parameter for ActorInstance isolation");
@@ -605,6 +601,7 @@ void SILGenFunction::emitProlog(CaptureInfo captureInfo,
     }
 
     case ActorIsolation::GlobalActor:
+    case ActorIsolation::GlobalActorUnsafe:
       if (F.isAsync() || wantDataRaceChecks) {
         ExpectedExecutor =
           emitLoadGlobalActorExecutor(actorIsolation.getGlobalActor());
diff --git a/lib/Sema/CSSimplify.cpp b/lib/Sema/CSSimplify.cpp
@@ -2171,8 +2171,7 @@ ConstraintSystem::matchFunctionTypes(FunctionType *func1, FunctionType *func2,
 
   /// Whether to downgrade to a concurrency warning.
   auto isConcurrencyWarning = [&] {
-    if (contextUsesConcurrencyFeatures(DC) ||
-        Context.LangOpts.isSwiftVersionAtLeast(6))
+    if (contextRequiresStrictConcurrencyChecking(DC))
       return false;
 
     switch (kind) {
diff --git a/lib/Sema/TypeCheckConcurrency.cpp b/lib/Sema/TypeCheckConcurrency.cpp
@@ -605,7 +605,7 @@ static bool shouldDiagnoseExistingDataRaces(const DeclContext *dc) {
   if (dc->getParentModule()->isConcurrencyChecked())
     return true;
 
-  return contextUsesConcurrencyFeatures(dc);
+  return contextRequiresStrictConcurrencyChecking(dc);
 }
 
 /// Determine the default diagnostic behavior for this language mode.
@@ -3518,7 +3518,13 @@ void swift::checkOverrideActorIsolation(ValueDecl *value) {
   if (isolation == overriddenIsolation)
     return;
 
-  // If both are actor-instance isolated, we're done.
+  // If the overriding declaration is non-isolated, it's okay.
+  if (isolation.isIndependent() || isolation.isUnspecified())
+    return;
+
+  // 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 (isolation.getKind() == overriddenIsolation.getKind() &&
       (isolation.getKind() == ActorIsolation::ActorInstance ||
        isolation.getKind() == ActorIsolation::DistributedActorInstance))
@@ -3529,64 +3535,18 @@ void swift::checkOverrideActorIsolation(ValueDecl *value) {
   if (overridden->hasClangNode() && !overriddenIsolation)
     return;
 
-  // If the overridden declaration uses an unsafe global actor, we can do
-  // anything except be actor-isolated or have a different global actor.
-  if (overriddenIsolation == ActorIsolation::GlobalActorUnsafe) {
-    switch (isolation) {
-    case ActorIsolation::Independent:
-    case ActorIsolation::Unspecified:
-      return;
-
-    case ActorIsolation::ActorInstance:
-    case ActorIsolation::DistributedActorInstance:
-      // Diagnose below.
-      break;
-
-    case ActorIsolation::GlobalActor:
-    case ActorIsolation::GlobalActorUnsafe:
-      // The global actors don't match; diagnose it.
-      if (overriddenIsolation.getGlobalActor()->isEqual(
-              isolation.getGlobalActor()))
-        return;
-
-      // Diagnose below.
-      break;
-    }
-  }
-
-  // If the overriding declaration uses an unsafe global actor, we can do
-  // anything that doesn't actively conflict with the overridden isolation.
-  if (isolation == ActorIsolation::GlobalActorUnsafe) {
-    switch (overriddenIsolation) {
-    case ActorIsolation::Unspecified:
-      return;
-
-    case ActorIsolation::ActorInstance:
-    case ActorIsolation::DistributedActorInstance:
-    case ActorIsolation::Independent:
-      // Diagnose below.
-      break;
-
-    case ActorIsolation::GlobalActor:
-    case ActorIsolation::GlobalActorUnsafe:
-      // The global actors don't match; diagnose it.
-      if (overriddenIsolation.getGlobalActor()->isEqual(
-              isolation.getGlobalActor()))
-        return;
-
-      // Diagnose below.
-      break;
-    }
-  }
-
   // Isolation mismatch. Diagnose it.
   value->diagnose(
       diag::actor_isolation_override_mismatch, isolation,
       value->getDescriptiveKind(), value->getName(), overriddenIsolation);
   overridden->diagnose(diag::overridden_here);
 }
 
-bool swift::contextUsesConcurrencyFeatures(const DeclContext *dc) {
+bool swift::contextRequiresStrictConcurrencyChecking(const DeclContext *dc) {
+  // If Swift >= 6, everything uses strict concurrency checking.
+  if (dc->getASTContext().LangOpts.isSwiftVersionAtLeast(6))
+    return true;
+
   while (!dc->isModuleScopeContext()) {
     if (auto closure = dyn_cast<AbstractClosureExpr>(dc)) {
       // A closure with an explicit global actor or nonindependent
@@ -4071,7 +4031,7 @@ Type swift::adjustVarTypeForConcurrency(
   if (!var->predatesConcurrency())
     return type;
 
-  if (contextUsesConcurrencyFeatures(dc))
+  if (contextRequiresStrictConcurrencyChecking(dc))
     return type;
 
   bool isLValue = false;
@@ -4193,7 +4153,7 @@ AnyFunctionType *swift::adjustFunctionTypeForConcurrency(
     unsigned numApplies, bool isMainDispatchQueue) {
   // Apply unsafe concurrency features to the given function type.
   fnType = applyUnsafeConcurrencyToFunctionType(
-      fnType, decl, contextUsesConcurrencyFeatures(dc), numApplies,
+      fnType, decl, contextRequiresStrictConcurrencyChecking(dc), numApplies,
       isMainDispatchQueue);
 
   Type globalActorType;
@@ -4208,7 +4168,7 @@ AnyFunctionType *swift::adjustFunctionTypeForConcurrency(
     case ActorIsolation::GlobalActorUnsafe:
       // Only treat as global-actor-qualified within code that has adopted
       // Swift Concurrency features.
-      if (!contextUsesConcurrencyFeatures(dc))
+      if (!contextRequiresStrictConcurrencyChecking(dc))
         return fnType;
 
       LLVM_FALLTHROUGH;
@@ -4250,7 +4210,7 @@ AnyFunctionType *swift::adjustFunctionTypeForConcurrency(
 }
 
 bool swift::completionContextUsesConcurrencyFeatures(const DeclContext *dc) {
-  return contextUsesConcurrencyFeatures(dc);
+  return contextRequiresStrictConcurrencyChecking(dc);
 }
 
 AbstractFunctionDecl const *swift::isActorInitOrDeInitContext(
diff --git a/lib/Sema/TypeCheckConcurrency.h b/lib/Sema/TypeCheckConcurrency.h
@@ -211,9 +211,10 @@ class ActorIsolationRestriction {
 /// overridden declaration.
 void checkOverrideActorIsolation(ValueDecl *value);
 
-/// Determine whether the given context uses concurrency features, such
-/// as async functions or actors.
-bool contextUsesConcurrencyFeatures(const DeclContext *dc);
+/// Determine whether the given context requires strict concurrency checking,
+/// e.g., because it uses concurrency features directly or because it's in
+/// code where strict checking has been enabled.
+bool contextRequiresStrictConcurrencyChecking(const DeclContext *dc);
 
 /// Diagnose the presence of any non-sendable types when referencing a
 /// given declaration from a particular declaration context.
diff --git a/lib/Sema/TypeCheckProtocol.cpp b/lib/Sema/TypeCheckProtocol.cpp
@@ -2868,6 +2868,8 @@ bool ConformanceChecker::checkActorIsolation(
   // unsuitable as a witness.
   bool isCrossActor = false;
   bool witnessIsUnsafe = false;
+  DiagnosticBehavior behavior = SendableCheckContext(
+      witness->getInnermostDeclContext()).defaultDiagnosticBehavior();
   Type witnessGlobalActor;
   switch (auto witnessRestriction =
               ActorIsolationRestriction::forDeclaration(
@@ -3091,8 +3093,6 @@ bool ConformanceChecker::checkActorIsolation(
   }
 
   case ActorIsolationRestriction::Unsafe:
-    break;
-
   case ActorIsolationRestriction::Unrestricted:
     // The witness is completely unrestricted, so ignore any annotations on
     // the requirement.
@@ -3155,53 +3155,34 @@ bool ConformanceChecker::checkActorIsolation(
   // However, we allow this case when the requirement was imported, because
   // it might not have been annotated.
   if (witnessGlobalActor && !requirementGlobalActor) {
-    // If the requirement was imported from Objective-C, it may not have been
-    // annotated appropriately. Allow the mismatch.
+    // If the requirement was imported from Objective-C, downgrade to a warning
+    // because it may not have been annotated appropriately.
     if (requirement->hasClangNode())
-      return false;
-
-    // If the witness is "unsafe", allow the mismatch.
-    if (witnessIsUnsafe)
-      return false;
+      behavior = std::max(behavior, DiagnosticBehavior::Warning);
 
     witness->diagnose(
         diag::global_actor_isolated_witness, witness->getDescriptiveKind(),
-        witness->getName(), witnessGlobalActor, Proto->getName());
+        witness->getName(), witnessGlobalActor, Proto->getName())
+      .limitBehavior(behavior);
     requirement->diagnose(diag::decl_declared_here, requirement->getName());
-    return true;
+    return behavior == DiagnosticBehavior::Unspecified;
   }
 
-  // If the requirement has a global actor but the witness does not, we have
-  // an isolation error.
+  // If the requirement has a global actor but the witness does not, it's
+  // fine.
   if (requirementGlobalActor && !witnessGlobalActor) {
-    // If the requirement's global actor was "unsafe", we allow this.
-    if (requirementIsUnsafe)
-      return false;
-
-    if (isCrossActor) {
-      return diagnoseNonSendableTypesInReference(
-        witness, DC, witness->getLoc(),
-        ConcurrentReferenceKind::CrossActor);
-    }
-
-    witness->diagnose(
-        diag::global_actor_isolated_requirement, witness->getDescriptiveKind(),
-        witness->getName(), requirementGlobalActor, Proto->getName())
-      .fixItInsert(
-        witness->getAttributeInsertionLoc(/*forModifier=*/false),
-      "@" + requirementGlobalActor.getString());
-    requirement->diagnose(diag::decl_declared_here, requirement->getName());
-    return true;
+    return false;
   }
 
   // Both have global actors but they differ, so this is an isolation error.
   assert(!witnessGlobalActor->isEqual(requirementGlobalActor));
   witness->diagnose(
       diag::global_actor_isolated_requirement_witness_conflict,
       witness->getDescriptiveKind(), witness->getName(), witnessGlobalActor,
-      Proto->getName(), requirementGlobalActor);
+      Proto->getName(), requirementGlobalActor)
+    .limitBehavior(behavior);
   requirement->diagnose(diag::decl_declared_here, requirement->getName());
-  return true;
+  return behavior == DiagnosticBehavior::Unspecified;
 }
 
 bool ConformanceChecker::checkObjCTypeErasedGenerics(
diff --git a/test/Concurrency/actor_isolation_unsafe.swift b/test/Concurrency/actor_isolation_unsafe.swift
@@ -30,7 +30,7 @@ struct S3_P1: P1 {
 }
 
 struct S4_P1: P1 {
-  @SomeGlobalActor func onMainActor() { } // expected-error{{instance method 'onMainActor()' isolated to global actor 'SomeGlobalActor' can not satisfy corresponding requirement from protocol 'P1' isolated to global actor 'MainActor'}}
+  @SomeGlobalActor func onMainActor() { } // expected-warning{{instance method 'onMainActor()' isolated to global actor 'SomeGlobalActor' can not satisfy corresponding requirement from protocol 'P1' isolated to global actor 'MainActor'}}
 }
 
 @MainActor(unsafe)
diff --git a/test/Concurrency/global_actor_inference.swift b/test/Concurrency/global_actor_inference.swift
@@ -47,7 +47,7 @@ protocol P1 {
 }
 
 protocol P2 {
-  @SomeGlobalActor func method1() // expected-note {{'method1()' declared here}}
+  @SomeGlobalActor func method1()
   func method2()
 }
 
@@ -146,14 +146,12 @@ class C5 {
 }
 
 protocol P3 {
-  @OtherGlobalActor func method1() // expected-note{{'method1()' declared here}}
+  @OtherGlobalActor func method1()
   func method2()
 }
 
 class C6: P2, P3 {
   func method1() { }
-    // expected-error@-1{{instance method 'method1()' must be isolated to the global actor 'SomeGlobalActor' to satisfy corresponding requirement from protocol 'P2'}}
-    // expected-error@-2{{instance method 'method1()' must be isolated to the global actor 'OtherGlobalActor' to satisfy corresponding requirement from protocol 'P3'}}
   func method2() { }
 
   func testMethod() {
diff --git a/test/Concurrency/predates_concurrency_swift6.swift b/test/Concurrency/predates_concurrency_swift6.swift
diff --git a/test/SILGen/check_executor.swift b/test/SILGen/check_executor.swift
diff --git a/test/decl/class/actor/global_actor_conformance.swift b/test/decl/class/actor/global_actor_conformance.swift

Original file line number	Diff line number	Diff line change
`@@ -132,6 +132,9 @@ class ActorIsolation {`
`132`	`132`
`133`	`133`	`friend bool operator==(const ActorIsolation &lhs,`
`134`	`134`	`const ActorIsolation &rhs) {`
	`135`	`+ if (lhs.isGlobalActor() && rhs.isGlobalActor())`
	`136`	`+ return areTypesEqual(lhs.globalActor, rhs.globalActor);`
	`137`	`+`
`135`	`138`	`if (lhs.kind != rhs.kind)`
`136`	`139`	`return false;`
`137`	`140`
`@@ -146,7 +149,7 @@ class ActorIsolation {`
`146`	`149`
`147`	`150`	`case GlobalActor:`
`148`	`151`	`case GlobalActorUnsafe:`
`149`		`- return areTypesEqual(lhs.globalActor, rhs.globalActor);`
	`152`	`+ llvm_unreachable("Global actors handled above");`
`150`	`153`	`}`
`151`	`154`	`}`
`152`	`155`
Original file line number	Diff line number	Diff line change
`@@ -30,7 +30,7 @@ struct S3_P1: P1 {`
`30`	`30`	`}`
`31`	`31`
`32`	`32`	`struct S4_P1: P1 {`
`33`		`- @SomeGlobalActor func onMainActor() { } // expected-error{{instance method 'onMainActor()' isolated to global actor 'SomeGlobalActor' can not satisfy corresponding requirement from protocol 'P1' isolated to global actor 'MainActor'}}`
	`33`	`+ @SomeGlobalActor func onMainActor() { } // expected-warning{{instance method 'onMainActor()' isolated to global actor 'SomeGlobalActor' can not satisfy corresponding requirement from protocol 'P1' isolated to global actor 'MainActor'}}`
`34`	`34`	`}`
`35`	`35`
`36`	`36`	`@MainActor(unsafe)`
Original file line number	Diff line number	Diff line change
`@@ -47,7 +47,7 @@ protocol P1 {`
`47`	`47`	`}`
`48`	`48`
`49`	`49`	`protocol P2 {`
`50`		`- @SomeGlobalActor func method1() // expected-note {{'method1()' declared here}}`
	`50`	`+ @SomeGlobalActor func method1()`
`51`	`51`	`func method2()`
`52`	`52`	`}`
`53`	`53`
`@@ -146,14 +146,12 @@ class C5 {`
`146`	`146`	`}`
`147`	`147`
`148`	`148`	`protocol P3 {`
`149`		`- @OtherGlobalActor func method1() // expected-note{{'method1()' declared here}}`
	`149`	`+ @OtherGlobalActor func method1()`
`150`	`150`	`func method2()`
`151`	`151`	`}`
`152`	`152`
`153`	`153`	`class C6: P2, P3 {`
`154`	`154`	`func method1() { }`
`155`		`- // expected-error@-1{{instance method 'method1()' must be isolated to the global actor 'SomeGlobalActor' to satisfy corresponding requirement from protocol 'P2'}}`
`156`		`- // expected-error@-2{{instance method 'method1()' must be isolated to the global actor 'OtherGlobalActor' to satisfy corresponding requirement from protocol 'P3'}}`
`157`	`155`	`func method2() { }`
`158`	`156`
`159`	`157`	`func testMethod() {`