Merge pull request #83315 from DougGregor/default-isolation-extensions

[SE-0466] Extensions and members thereof can apply default isolation

Author: DougGregor

include/swift/Basic/Features.def

@@ -334,6 +334,10 @@ EXPERIMENTAL_FEATURE(ConsumeSelfInDeinit, true)
 
 EXPERIMENTAL_FEATURE(AccessLevelOnImport, true)
 
+/// Disable the special behavior of of explicit 'nonisolated' vs. being
+/// implicitly nonisolated.
+EXPERIMENTAL_FEATURE(NoExplicitNonIsolated, true)
+
 /// Enables a module to allow non resilient access from other
 /// modules within a package if built from source.
 EXPERIMENTAL_FEATURE(AllowNonResilientAccessInPackage, false)

lib/AST/FeatureSet.cpp

@@ -125,6 +125,7 @@ UNINTERESTING_FEATURE(MacrosOnImports)
 UNINTERESTING_FEATURE(NonisolatedNonsendingByDefault)
 UNINTERESTING_FEATURE(KeyPathWithMethodMembers)
 UNINTERESTING_FEATURE(ImportMacroAliases)
+UNINTERESTING_FEATURE(NoExplicitNonIsolated)
 
 // TODO: Return true for inlinable function bodies with module selectors in them
 UNINTERESTING_FEATURE(ModuleSelector)

lib/Frontend/CompilerInvocation.cpp

@@ -1852,8 +1852,10 @@ static bool ParseLangArgs(LangOptions &Opts, ArgList &Args,
     Opts.DefaultIsolationBehavior = DefaultIsolation::Nonisolated;
   }
 
-  if (Opts.DefaultIsolationBehavior == DefaultIsolation::MainActor)
+  if (Opts.DefaultIsolationBehavior == DefaultIsolation::MainActor) {
     Opts.enableFeature(Feature::InferIsolatedConformances);
+    Opts.enableFeature(Feature::NoExplicitNonIsolated);
+  }
 
 #if !defined(NDEBUG) && SWIFT_ENABLE_EXPERIMENTAL_PARSER_VALIDATION
   /// Enable round trip parsing via the new swift parser unless it is disabled

lib/Sema/TypeCheckConcurrency.cpp

@@ -5282,6 +5282,33 @@ getIsolationFromAttributes(const Decl *decl, bool shouldDiagnose = true,
   llvm_unreachable("Forgot about an attribute?");
 }
 
+/// Determine the default isolation for the given declaration context.
+static DefaultIsolation getDefaultIsolationForContext(const DeclContext *dc) {
+  // Check whether there is a file-specific setting.
+  if (auto *sourceFile = dc->getParentSourceFile()) {
+    if (auto defaultIsolationInFile = sourceFile->getDefaultIsolation())
+      return defaultIsolationInFile.value();
+  }
+
+  // If we're in the main module, check the language option.
+  ASTContext &ctx = dc->getASTContext();
+  if (dc->getParentModule() == ctx.MainModule)
+    return ctx.LangOpts.DefaultIsolationBehavior;
+
+  // Otherwise, default to nonisolated.
+  return DefaultIsolation::Nonisolated;
+}
+
+/// Determines whether explicit 'nonisolated' is different from 'unspecified'
+/// in the given context.
+static bool explicitNonisolatedIsSpecial(const DeclContext *dc) {
+  ASTContext &ctx = dc->getASTContext();
+  if (ctx.LangOpts.hasFeature(Feature::NoExplicitNonIsolated))
+    return false;
+
+  return getDefaultIsolationForContext(dc) == DefaultIsolation::Nonisolated;
+}
+
 /// Infer isolation from witnessed protocol requirements.
 static std::optional<InferredActorIsolation>
 getIsolationFromWitnessedRequirements(ValueDecl *value) {
@@ -5298,11 +5325,13 @@ getIsolationFromWitnessedRequirements(ValueDecl *value) {
   if (dc->getSelfProtocolDecl())
     return std::nullopt;
 
-  // Prevent isolation inference from requirements if the conforming type
-  // has an explicit `nonisolated` attribute.
-  if (auto *NTD = dc->getSelfNominalTypeDecl()) {
-    if (NTD->getAttrs().hasAttribute<NonisolatedAttr>())
-      return std::nullopt;
+  if (explicitNonisolatedIsSpecial(dc)) {
+    // Prevent isolation inference from requirements if the conforming type
+    // has an explicit `nonisolated` attribute.
+    if (auto *NTD = dc->getSelfNominalTypeDecl()) {
+      if (NTD->getAttrs().hasAttribute<NonisolatedAttr>())
+        return std::nullopt;
+    }
   }
 
   // Walk through each of the conformances in this context, collecting any
@@ -5360,8 +5389,13 @@ getIsolationFromWitnessedRequirements(ValueDecl *value) {
       }
 
       case ActorIsolation::GlobalActor:
+        break;
+
       case ActorIsolation::Nonisolated:
       case ActorIsolation::NonisolatedUnsafe:
+        if (!explicitNonisolatedIsSpecial(requirement->getDeclContext()))
+          continue;
+
         break;
       }
 
@@ -5468,7 +5502,8 @@ getIsolationFromConformances(NominalTypeDecl *nominal) {
     case ActorIsolation::NonisolatedUnsafe:
       break;
     case ActorIsolation::Nonisolated:
-      if (inferredIsolation.source.kind == IsolationSource::Kind::Explicit) {
+      if (inferredIsolation.source.kind == IsolationSource::Kind::Explicit &&
+          explicitNonisolatedIsSpecial(nominal)) {
         if (!foundIsolation) {
           // We found an explicitly 'nonisolated' protocol.
           foundIsolation = {
@@ -6131,12 +6166,27 @@ computeDefaultInferredActorIsolation(ValueDecl *value) {
             return {};
 
           if (dc != dyn_cast<DeclContext>(value)) {
+            // If the nominal type is global-actor-isolated, there's nothing
+            // more to look for.
             if (getActorIsolation(nominal).isMainActor())
               break;
 
-            return {};
+            // If this is an extension of a nonisolated type, its isolation
+            // is independent of the type.
+            if (auto ext = dyn_cast<ExtensionDecl>(dc)) {
+              // If there were isolation attributes on the extension, respect
+              // them.
+              if (getIsolationFromAttributes(ext).has_value())
+                return {};
+
+              // Keep looking.
+            } else {
+              // The type is nonisolated, so its members are nonisolated.
+              return {};
+            }
           }
         }
+
         dc = dc->getParent();
       }
 
@@ -6166,12 +6216,8 @@ computeDefaultInferredActorIsolation(ValueDecl *value) {
       return {};
     };
 
-    DefaultIsolation defaultIsolation = ctx.LangOpts.DefaultIsolationBehavior;
-    if (auto *SF = value->getDeclContext()->getParentSourceFile()) {
-      if (auto defaultIsolationInFile = SF->getDefaultIsolation())
-        defaultIsolation = defaultIsolationInFile.value();
-    }
-
+    DefaultIsolation defaultIsolation =
+        getDefaultIsolationForContext(value->getDeclContext());
     // If we are required to use main actor... just use that.
     if (defaultIsolation == DefaultIsolation::MainActor)
       if (auto result =
@@ -6248,6 +6294,36 @@ computeDefaultInferredActorIsolation(ValueDecl *value) {
   return {{ActorIsolation::forUnspecified(), {}}, nullptr, {}};
 }
 
+/// Determines when the given "self" isolation should override default
+/// isolation.
+static bool shouldSelfIsolationOverrideDefault(
+    ASTContext &ctx, const DeclContext *dc,
+    const ActorIsolation &selfIsolation) {
+  switch (selfIsolation) {
+  case ActorIsolation::ActorInstance:
+  case ActorIsolation::Erased:
+  case ActorIsolation::GlobalActor:
+    // Actor isolation always overrides.
+    return true;
+
+  case ActorIsolation::Unspecified:
+    // Unspecified isolation never overrides.
+    return false;
+
+  case ActorIsolation::Nonisolated:
+  case ActorIsolation::NonisolatedUnsafe:
+  case ActorIsolation::CallerIsolationInheriting:
+    // Explicit nonisolated used to overwrite default isolation all the time,
+    // but under NoExplicitNonIsolated it doesn't affect extensions.
+    if (isa<NominalTypeDecl>(dc))
+      return true;
+
+    /// Only allow explicit nonisolated to override the default when it's
+    /// treated as special.
+    return explicitNonisolatedIsSpecial(dc);
+  }
+}
+
 static InferredActorIsolation computeActorIsolation(Evaluator &evaluator,
                                                     ValueDecl *value) {
   // If this declaration has actor-isolated "self", it's isolated to that
@@ -6580,7 +6656,8 @@ static InferredActorIsolation computeActorIsolation(Evaluator &evaluator,
     // has isolation, use that.
     if (auto selfTypeDecl = value->getDeclContext()->getSelfNominalTypeDecl()) {
       auto selfTypeIsolation = getInferredActorIsolation(selfTypeDecl);
-      if (selfTypeIsolation.isolation) {
+      if (shouldSelfIsolationOverrideDefault(
+              ctx, value->getDeclContext(), selfTypeIsolation.isolation)) {
         auto isolation = selfTypeIsolation.isolation;
 
         if (ctx.LangOpts.hasFeature(Feature::NonisolatedNonsendingByDefault) &&
@@ -8426,6 +8503,19 @@ ActorIsolation swift::inferConformanceIsolation(
   // isolated to a global actor, we may use the conforming type's isolation.
   auto nominalIsolation = getActorIsolation(nominal);
   if (!nominalIsolation.isGlobalActor()) {
+    // If we are in an extension of the type, we might still infer an
+    // isolated conformance depending on default isolation and on the extension
+    // itself.
+    if (auto ext = dyn_cast<ExtensionDecl>(dc)) {
+      // If there's an isolation-related attribute on the extension, use it.
+      if (auto attrIsolation = getIsolationFromAttributes(ext))
+        return *attrIsolation;
+
+      // If we're defaulting to main-actor isolation, use that.
+      if (getDefaultIsolationForContext(dc) == DefaultIsolation::MainActor)
+        return ActorIsolation::forMainActor(ctx);
+    }
+
     return ActorIsolation::forNonisolated(false);
   }
 

test/Concurrency/Inputs/implicit_nonisolated_things.swift

@@ -0,0 +1,7 @@
+public protocol ImportedSendableProto: Sendable { }
+
+@MainActor public struct ImportedStruct: ImportedSendableProto { }
+
+public struct ImportedOtherStruct { }
+
+public protocol ImportedP { }

test/Concurrency/assume_mainactor.swift

@@ -227,24 +227,6 @@ actor MyActor2 {
   print("123")
 }
 
-// https://github.com/swiftlang/swift/issues/82168 - used to fail
-nonisolated protocol P {
-  associatedtype AT
-  static var at: AT { get }
-}
-
-nonisolated struct KP<R: P, V> {
-  init(keyPath: KeyPath<R, V>) {}
-}
-
-struct S: P {
-  let p: Int
-  struct AT {
-    let kp = KP(keyPath: \S.p)
-  }
-  static let at = AT() // used to fail here
-}
-
 nonisolated func localDeclIsolation() async {
   struct Local {
     static func f() {}

test/Concurrency/assume_mainactor_typechecker_errors.swift

@@ -1,11 +1,15 @@
-// RUN: %target-swift-frontend -swift-version 5 -emit-sil -default-isolation MainActor %s -verify -verify-additional-prefix swift5-
-// RUN: %target-swift-frontend -swift-version 6 -emit-sil -default-isolation MainActor %s -verify -verify-additional-prefix swift6-
+// RUN: %empty-directory(%t)
+// RUN: %target-swift-frontend -emit-module -swift-version 6 -module-name implicit_nonisolated_things -o %t/implicit_nonisolated_things.swiftmodule %S/Inputs/implicit_nonisolated_things.swift
+// RUN: %target-swift-frontend -I %t -swift-version 5 -emit-sil -default-isolation MainActor %s -verify -verify-additional-prefix swift5-
+// RUN: %target-swift-frontend -I %t  -swift-version 6 -emit-sil -default-isolation MainActor %s -verify -verify-additional-prefix swift6-
 
 // READ THIS! This test is meant to check the specific isolation when
 // `-default-isolation` is set to `MainActor` in combination with validating
 // behavior around explicitly non-Sendable types that trigger type checker
 // specific errors. Please do not put other types of tests in here.
 
+import implicit_nonisolated_things
+
 // Fake Sendable Data
 class SendableData : @unchecked Sendable {}
 // expected-swift5-note@-1 {{calls to initializer 'init()' from outside of its actor context are implicitly asynchronous}}
@@ -79,3 +83,98 @@ func testTaskDetached() async {
     await k.doSomething()
   }
 }
+
+// @MainActor
+extension Int {
+  func memberOfInt() { } // expected-note 3{{calls to instance method 'memberOfInt()' from outside of its actor context are implicitly asynchronous}}
+}
+
+nonisolated func testMemberOfInt(i: Int) {
+  i.memberOfInt() // expected-swift5-warning{{call to main actor-isolated instance method 'memberOfInt()' in a synchronous nonisolated context}}
+  // expected-swift6-error@-1{{call to main actor-isolated instance method 'memberOfInt()' in a synchronous nonisolated context}}
+}
+
+protocol SendableProto: Sendable { }
+
+struct MyStruct: SendableProto { }
+
+extension MyStruct: CustomStringConvertible {
+  var description: String {
+    17.memberOfInt() // okay, on main actor
+    return "hello"
+  }
+}
+
+nonisolated struct MyOtherStruct { }
+
+extension MyOtherStruct {
+  func f() {
+    17.memberOfInt() // okay, on main actor
+  }
+}
+
+nonisolated
+extension MyOtherStruct {
+  func g() {
+    17.memberOfInt() // expected-swift5-warning{{call to main actor-isolated instance method 'memberOfInt()' in a synchronous nonisolated context}}
+  // expected-swift6-error@-1{{call to main actor-isolated instance method 'memberOfInt()' in a synchronous nonisolated context}}
+  }
+}
+
+nonisolated protocol P {
+  func g()
+}
+
+struct MyP: P {
+  func g() {
+    17.memberOfInt() // okay, on main actor
+  }
+}
+
+// Above tests for imported types
+extension ImportedStruct: @retroactive CustomStringConvertible {
+  public var description: String {
+    17.memberOfInt() // okay, on main actor
+    return "hello"
+  }
+}
+
+extension ImportedOtherStruct {
+  func f() {
+    17.memberOfInt() // okay, on main actor
+  }
+}
+
+nonisolated
+extension ImportedOtherStruct {
+  func g() {
+    17.memberOfInt() // expected-swift5-warning{{call to main actor-isolated instance method 'memberOfInt()' in a synchronous nonisolated context}}
+  // expected-swift6-error@-1{{call to main actor-isolated instance method 'memberOfInt()' in a synchronous nonisolated context}}
+  }
+}
+
+struct MyImportedP: ImportedP {
+  func g() {
+    17.memberOfInt() // okay, on main actor
+  }
+}
+
+// https://github.com/swiftlang/swift/issues/82168 -
+nonisolated protocol OtherP {
+  associatedtype AT
+  static var at: AT { get }
+}
+
+nonisolated struct KP<R: OtherP, V> {
+  init(keyPath: KeyPath<R, V>) {}
+}
+
+struct S: OtherP {
+  let p: Int
+  struct AT {
+    let kp = KP(keyPath: \S.p)
+  }
+
+  // FIXME: This should not be an error.
+  static let at = AT() // expected-swift6-error{{'S.AT' cannot be constructed because it has no accessible initializers}}
+}