Merge pull request #40868 from kavon/drop-redundant-isolation

Author: swift-ci

include/swift/AST/Decl.h

@@ -5240,6 +5240,10 @@ class VarDecl : public AbstractStorageDecl {
   /// Is this an "async let" property?
   bool isAsyncLet() const;
 
+  /// Is this a stored property that will _not_ trigger any user-defined code
+  /// upon any kind of access?
+  bool isOrdinaryStoredProperty() const;
+
   Introducer getIntroducer() const {
     return Introducer(Bits.VarDecl.Introducer);
   }

include/swift/AST/DiagnosticsSema.def

@@ -4573,6 +4573,9 @@ ERROR(distributed_actor_remote_func_implemented_manually,none,
 ERROR(nonisolated_distributed_actor_storage,none,
       "'nonisolated' can not be applied to distributed actor stored properties",
       ())
+ERROR(nonisolated_storage_value_type,none,
+      "'nonisolated' is redundant on %0's stored properties",
+      (DescriptiveDeclKind))
 ERROR(distributed_actor_func_nonisolated, none,
       "cannot declare method %0 as both 'nonisolated' and 'distributed'",
       (DeclName))
@@ -4773,6 +4776,9 @@ ERROR(global_actor_on_actor_class,none,
       "actor %0 cannot have a global actor", (Identifier))
 ERROR(global_actor_on_local_variable,none,
       "local variable %0 cannot have a global actor", (DeclName))
+ERROR(global_actor_on_storage_of_value_type,none,
+      "stored property %0 within %1 cannot have a global actor",
+      (DeclName, DescriptiveDeclKind))
 ERROR(global_actor_non_unsafe_init,none,
       "global actor attribute %0 argument can only be '(unsafe)'", (Type))
 ERROR(global_actor_non_final_class,none,

lib/AST/Decl.cpp

@@ -6304,6 +6304,16 @@ bool VarDecl::isAsyncLet() const {
   return getAttrs().hasAttribute<AsyncAttr>();
 }
 
+bool VarDecl::isOrdinaryStoredProperty() const {
+  // we assume if it hasAttachedPropertyWrapper, it has no storage.
+  //
+  // also, we don't expect someone to call this on a local property, so for
+  // efficiency we don't check if it's not async-let. feel free to promote
+  // the assert into a full-fledged part of the condition if needed.
+  assert(!isAsyncLet());
+  return hasStorage() && !hasObservers();
+}
+
 void ParamDecl::setSpecifier(Specifier specifier) {
   // FIXME: Revisit this; in particular shouldn't __owned parameters be
   // ::Let also?

lib/Sema/TypeCheckAttr.cpp

@@ -5605,29 +5605,41 @@ void AttributeChecker::visitNonisolatedAttr(NonisolatedAttr *attr) {
   if (auto var = dyn_cast<VarDecl>(D)) {
     // stored properties have limitations as to when they can be nonisolated.
     if (var->hasStorage()) {
-      auto nominal = dyn_cast<NominalTypeDecl>(dc);
-
-      // 'nonisolated' can not be applied to stored properties inside
-      // distributed actors. Attempts of nonisolated access would be
-      // cross-actor, and that means they might be accessing on a remote actor,
-      // in which case the stored property storage does not exist.
-      //
-      // The synthesized "id" and "actorSystem" are the only exceptions,
-      // because the implementation mirrors them.
-      if (nominal && nominal->isDistributedActor() &&
-          !(var->isImplicit() &&
-            (var->getName() == Ctx.Id_id ||
-             var->getName() == Ctx.Id_actorSystem))) {
-        diagnoseAndRemoveAttr(attr,
-                              diag::nonisolated_distributed_actor_storage);
-        return;
-      }
 
       // 'nonisolated' can not be applied to mutable stored properties.
       if (var->supportsMutation()) {
         diagnoseAndRemoveAttr(attr, diag::nonisolated_mutable_storage);
         return;
       }
+
+      if (auto nominal = dyn_cast<NominalTypeDecl>(dc)) {
+        // 'nonisolated' can not be applied to stored properties inside
+        // distributed actors. Attempts of nonisolated access would be
+        // cross-actor, which means they might be accessing on a remote actor,
+        // in which case the stored property storage does not exist.
+        //
+        // The synthesized "id" and "actorSystem" are the only exceptions,
+        // because the implementation mirrors them.
+        if (nominal->isDistributedActor() &&
+            !(var->isImplicit() &&
+              (var->getName() == Ctx.Id_id ||
+               var->getName() == Ctx.Id_actorSystem))) {
+          diagnoseAndRemoveAttr(attr,
+                                diag::nonisolated_distributed_actor_storage);
+          return;
+        }
+
+        // 'nonisolated' is redundant for the stored properties of a struct.
+        if (isa<StructDecl>(nominal) &&
+            !var->isStatic() &&
+            var->isOrdinaryStoredProperty() &&
+            !isWrappedValueOfPropWrapper(var)) {
+          diagnoseAndRemoveAttr(attr, diag::nonisolated_storage_value_type,
+                                nominal->getDescriptiveKind())
+            .warnUntilSwiftVersion(6);
+          return;
+        }
+      }
     }
 
     // nonisolated can not be applied to local properties.

lib/Sema/TypeCheckConcurrency.cpp

@@ -341,15 +341,37 @@ GlobalActorAttributeRequest::evaluate(
   } else if (auto storage = dyn_cast<AbstractStorageDecl>(decl)) {
     // Subscripts and properties are fine...
     if (auto var = dyn_cast<VarDecl>(storage)) {
+
+      // ... but not if it's an async-context top-level global
       if (var->isTopLevelGlobal() && var->getDeclContext()->isAsyncContext()) {
         var->diagnose(diag::global_actor_top_level_var)
             .highlight(globalActorAttr->getRangeWithAt());
         return None;
-      } else if (var->getDeclContext()->isLocalContext()) {
+      }
+      
+      // ... and not if it's local property
+      if (var->getDeclContext()->isLocalContext()) {
         var->diagnose(diag::global_actor_on_local_variable, var->getName())
             .highlight(globalActorAttr->getRangeWithAt());
         return None;
       }
+
+      // ... and not if it's the instance storage of a struct
+      if (!var->isStatic() && var->isOrdinaryStoredProperty()) {
+        if (auto *nominal = var->getDeclContext()->getSelfNominalTypeDecl()) {
+          if (isa<StructDecl>(nominal) && !isWrappedValueOfPropWrapper(var)) {
+
+            var->diagnose(diag::global_actor_on_storage_of_value_type,
+                          var->getName(), nominal->getDescriptiveKind())
+              .highlight(globalActorAttr->getRangeWithAt())
+              .warnUntilSwiftVersion(6);
+
+            // In Swift 6, once the diag above is an error, it is disallowed.
+            if (var->getASTContext().isSwiftVersionAtLeast(6))
+              return None;
+          }
+        }
+      }
     }
   } else if (isa<ExtensionDecl>(decl)) {
     // Extensions are okay.
@@ -3604,6 +3626,16 @@ ActorIsolation ActorIsolationRequest::evaluate(
 
     case ActorIsolation::GlobalActorUnsafe:
     case ActorIsolation::GlobalActor: {
+      // Stored properties of a struct don't need global-actor isolation.
+      if (ctx.isSwiftVersionAtLeast(6))
+        if (auto *var = dyn_cast<VarDecl>(value))
+          if (!var->isStatic() && var->isOrdinaryStoredProperty())
+            if (auto *varDC = var->getDeclContext())
+              if (auto *nominal = varDC->getSelfNominalTypeDecl())
+                if (isa<StructDecl>(nominal) &&
+                    !isWrappedValueOfPropWrapper(var))
+                  return ActorIsolation::forUnspecified();
+
       auto typeExpr = TypeExpr::createImplicit(inferred.getGlobalActor(), ctx);
       auto attr = CustomAttr::create(
           ctx, SourceLoc(), typeExpr, /*implicit=*/true);

lib/Sema/TypeCheckPropertyWrapper.cpp

@@ -799,3 +799,14 @@ Expr *swift::buildPropertyWrapperInitCall(
 
   return initializer;
 }
+
+bool swift::isWrappedValueOfPropWrapper(VarDecl *var) {
+  if (!var->isStatic())
+    if (auto *dc = var->getDeclContext())
+      if (auto *nominal = dc->getSelfNominalTypeDecl())
+        if (nominal->getAttrs().hasAttribute<PropertyWrapperAttr>())
+          if (var->getName() == var->getASTContext().Id_wrappedValue)
+            return true;
+
+  return false;
+}

lib/Sema/TypeChecker.h

@@ -1276,6 +1276,10 @@ Expr *buildPropertyWrapperInitCall(
     PropertyWrapperInitKind initKind,
     llvm::function_ref<void(ApplyExpr *)> callback = [](ApplyExpr *) {});
 
+/// Check if this var is the \c wrappedValue property belonging to
+/// a property wrapper type declaration.
+bool isWrappedValueOfPropWrapper(VarDecl *var);
+
 /// Whether an overriding declaration requires the 'override' keyword.
 enum class OverrideRequiresKeyword {
   /// The keyword is never required.

test/Concurrency/Inputs/dynamically_replaceable.swift

@@ -0,0 +1,15 @@
+
+// dynamically_replaceable
+@MainActor
+public dynamic func dynamicOnMainActor() { }
+
+// property wrapper + main actor
+@propertyWrapper
+public struct IntWrapper {
+
+  public init(wrappedValue: Int) {
+    self.wrappedValue = wrappedValue
+  }
+
+  @MainActor public var wrappedValue: Int
+}

test/Concurrency/Inputs/other_global_actor_inference.swift

@@ -46,7 +46,7 @@ actor MySuperActor {
   }
 }
 
-class Point { // expected-note{{class 'Point' does not conform to the 'Sendable' protocol}}
+class Point { // expected-note 5{{class 'Point' does not conform to the 'Sendable' protocol}}
   var x : Int = 0
   var y : Int = 0
 }
@@ -109,6 +109,66 @@ func checkAsyncPropertyAccess() async {
   _ = act.point  // expected-warning{{non-sendable type 'Point' in asynchronous access to actor-isolated property 'point' cannot cross actor boundary}}
 }
 
+/// ------------------------------------------------------------------
+/// -- Value types do not need isolation on their stored properties --
+protocol MainCounter {
+  @MainActor var counter: Int { get set }
+  @MainActor var ticker: Int { get set }
+}
+
+struct InferredFromConformance: MainCounter {
+  var counter = 0
+  var ticker: Int {
+    get { 1 }
+    set {}
+  }
+}
+
+@MainActor
+struct InferredFromContext {
+  var point = Point()
+  var polygon: [Point] {
+    get { [] }
+  }
+
+  nonisolated var status: Bool = true // expected-error {{'nonisolated' can not be applied to stored properties}}{{3-15=}}
+
+  nonisolated let flag: Bool = false // expected-warning {{'nonisolated' is redundant on struct's stored properties; this is an error in Swift 6}}{{3-15=}}
+
+  subscript(_ i: Int) -> Int { return i }
+
+  static var stuff: [Int] = []
+}
+
+func checkIsolationValueType(_ formance: InferredFromConformance,
+                             _ ext: InferredFromContext,
+                             _ anno: NoGlobalActorValueType) async {
+  // these still do need an await in Swift 5
+  _ = await ext.point // expected-warning {{non-sendable type 'Point' in implicitly asynchronous access to main actor-isolated property 'point' cannot cross actor boundary}}
+  _ = await formance.counter
+  _ = await anno.point // expected-warning {{non-sendable type 'Point' in implicitly asynchronous access to global actor 'SomeGlobalActor'-isolated property 'point' cannot cross actor boundary}}
+  _ = anno.counter
+
+  // these will always need an await
+  _ = await (formance as MainCounter).counter
+  _ = await ext[1]
+  _ = await formance.ticker
+  _ = await ext.polygon // expected-warning {{non-sendable type '[Point]' in implicitly asynchronous access to main actor-isolated property 'polygon' cannot cross actor boundary}}
+  _ = await InferredFromContext.stuff
+  _ = await NoGlobalActorValueType.polygon // expected-warning {{non-sendable type '[Point]' in implicitly asynchronous access to main actor-isolated static property 'polygon' cannot cross actor boundary}}
+}
+
+// check for instance members that do not need global-actor protection
+struct NoGlobalActorValueType {
+  @SomeGlobalActor var point: Point // expected-warning {{stored property 'point' within struct cannot have a global actor; this is an error in Swift 6}}
+
+  @MainActor let counter: Int // expected-warning {{stored property 'counter' within struct cannot have a global actor; this is an error in Swift 6}}
+
+  @MainActor static var polygon: [Point] = []
+}
+
+/// -----------------------------------------------------------------
+
 @available(SwiftStdlib 5.1, *)
 extension MyActor {
   nonisolated var actorIndependentVar: Int {

test/Concurrency/actor_isolation.swift

@@ -0,0 +1,78 @@
+// RUN: %target-typecheck-verify-swift -disable-availability-checking -warn-concurrency -swift-version 6
+// REQUIRES: concurrency
+
+final class ImmutablePoint: Sendable {
+  let x : Int = 0
+  let y : Int = 0
+}
+
+actor SomeActor { }
+
+@globalActor
+struct SomeGlobalActor {
+  static let shared = SomeActor()
+}
+
+/// ------------------------------------------------------------------
+/// -- Value types do not need isolation on their stored properties --
+protocol MainCounter {
+  @MainActor var counter: Int { get set }
+  @MainActor var ticker: Int { get set }
+}
+
+struct InferredFromConformance: MainCounter {
+  var counter = 0
+  var ticker: Int {
+    get { 1 }
+    set {}
+  }
+}
+
+@MainActor
+struct InferredFromContext {
+  var point = ImmutablePoint()
+  var polygon: [ImmutablePoint] {
+    get { [] }
+  }
+
+  nonisolated let flag: Bool = false // expected-error {{'nonisolated' is redundant on struct's stored properties}}{{3-15=}}
+
+  subscript(_ i: Int) -> Int { return i }
+
+  static var stuff: [Int] = []
+}
+
+func checkIsolationValueType(_ formance: InferredFromConformance,
+                             _ ext: InferredFromContext,
+                             _ anno: NoGlobalActorValueType) async {
+  // these do not need an await, since it's a value type
+  _ = ext.point
+  _ = formance.counter
+  _ = anno.point
+  _ = anno.counter
+
+  // make sure it's just a warning if someone was awaiting on it previously
+  _ = await ext.point // expected-warning {{no 'async' operations occur within 'await' expression}}
+  _ = await formance.counter  // expected-warning {{no 'async' operations occur within 'await' expression}}
+  _ = await anno.point  // expected-warning {{no 'async' operations occur within 'await' expression}}
+  _ = await anno.counter  // expected-warning {{no 'async' operations occur within 'await' expression}}
+
+  // these do need await, regardless of reference or value type
+  _ = await (formance as MainCounter).counter
+  _ = await ext[1]
+  _ = await formance.ticker
+  _ = await ext.polygon
+  _ = await InferredFromContext.stuff
+  _ = await NoGlobalActorValueType.polygon
+}
+
+// check for instance members that do not need global-actor protection
+struct NoGlobalActorValueType {
+  @SomeGlobalActor var point: ImmutablePoint // expected-error {{stored property 'point' within struct cannot have a global actor}}
+
+  @MainActor let counter: Int // expected-error {{stored property 'counter' within struct cannot have a global actor}}
+
+  @MainActor static var polygon: [ImmutablePoint] = []
+}
+
+/// -----------------------------------------------------------------