Sema: Ban global actor on getters in a future lang mode, and more

include/swift/AST/DiagnosticsSema.def

@@ -6016,6 +6016,9 @@ ERROR(multiple_global_actors,none,
       (Identifier, Identifier))
 ERROR(global_actor_disallowed,none,
       "%kindonly0 cannot have a global actor", (const Decl *))
+NOTE(global_actor_attr_is_ignored,none,
+     "global actor attribute is ignored", ())
+
 ERROR(global_actor_on_actor_class,none,
       "actor %0 cannot have a global actor", (Identifier))
 ERROR(global_actor_on_local_variable,none,

lib/Sema/TypeCheckConcurrency.cpp

@@ -348,6 +348,10 @@ swift::checkGlobalActorAttributes(SourceLoc loc, DeclContext *dc,
   return std::make_pair(globalActorAttr, globalActorNominal);
 }
 
+static bool shouldIgnoreGlobalActorOnAccessor(const ASTContext &ctx) {
+  return ctx.isSwiftVersionAtLeast(6);
+}
+
 std::optional<std::pair<CustomAttr *, NominalTypeDecl *>>
 GlobalActorAttributeRequest::evaluate(
     Evaluator &evaluator,
@@ -402,27 +406,10 @@ GlobalActorAttributeRequest::evaluate(
   if (decl->getDeclContext()->getParentSourceFile() == nullptr)
     return result;
 
-  auto isStoredInstancePropertyOfStruct = [](VarDecl *var) {
-    if (var->isStatic() || !var->isOrdinaryStoredProperty())
-      return false;
-
-    auto *nominal = var->getDeclContext()->getSelfNominalTypeDecl();
-    return isa_and_nonnull<StructDecl>(nominal) &&
-           !isWrappedValueOfPropWrapper(var);
-  };
+  auto *const globalActorAttr = result->first;
 
-  auto globalActorAttr = result->first;
-  if (auto nominal = dyn_cast<NominalTypeDecl>(decl)) {
-    // Nominal types are okay...
-    if (auto classDecl = dyn_cast<ClassDecl>(nominal)){
-      if (classDecl->isActor()) {
-        // ... except for actors.
-        nominal->diagnose(diag::global_actor_on_actor_class, nominal->getName())
-            .highlight(globalActorAttr->getRangeWithAt());
-        return std::nullopt;
-      }
-    }
-  } else if (auto storage = dyn_cast<AbstractStorageDecl>(decl)) {
+  const auto isGlobalActorAnErrorOnStorage = [&](AbstractStorageDecl *storage,
+                                                 bool diagnose) {
     // Subscripts and properties are fine...
     if (auto var = dyn_cast<VarDecl>(storage)) {
 
@@ -431,65 +418,79 @@ GlobalActorAttributeRequest::evaluate(
           (var->getDeclContext()->isAsyncContext() ||
            var->getASTContext().LangOpts.StrictConcurrencyLevel >=
              StrictConcurrency::Complete)) {
-        var->diagnose(diag::global_actor_top_level_var)
-            .highlight(globalActorAttr->getRangeWithAt());
-        return std::nullopt;
+        if (diagnose) {
+          var->diagnose(diag::global_actor_top_level_var)
+              .highlight(globalActorAttr->getRangeWithAt());
+        }
+        return true;
       }
 
       // ... and not if it's local property
       if (var->getDeclContext()->isLocalContext()) {
-        var->diagnose(diag::global_actor_on_local_variable, var->getName())
+        if (diagnose) {
+          var->diagnose(diag::global_actor_on_local_variable, var->getName())
+              .highlight(globalActorAttr->getRangeWithAt());
+        }
+        return true;
+      }
+    }
+
+    return false;
+  };
+
+  if (auto nominal = dyn_cast<NominalTypeDecl>(decl)) {
+    // Nominal types are okay...
+    if (auto classDecl = dyn_cast<ClassDecl>(nominal)) {
+      if (classDecl->isActor()) {
+        // ... except for actors.
+        nominal->diagnose(diag::global_actor_on_actor_class, nominal->getName())
             .highlight(globalActorAttr->getRangeWithAt());
         return std::nullopt;
       }
     }
-  } else if (isa<ExtensionDecl>(decl)) {
-    // Extensions are okay.
-  } else if (isa<ConstructorDecl>(decl) || isa<FuncDecl>(decl) ||
-             isa<DestructorDecl>(decl)) {
-    // None of the accessors/addressors besides a getter are allowed
-    // to have a global actor attribute.
-    if (auto *accessor = dyn_cast<AccessorDecl>(decl)) {
-      if (!accessor->isGetter()) {
-        decl->diagnose(diag::global_actor_disallowed, decl)
-            .warnUntilSwiftVersion(6)
-            .fixItRemove(globalActorAttr->getRangeWithAt());
-
-        auto &ctx = decl->getASTContext();
-        auto *storage = accessor->getStorage();
-        // Let's suggest to move the attribute to the storage if
-        // this is an accessor/addressor of a property of subscript.
-        if (storage->getDeclContext()->isTypeContext()) {
-          auto canMoveAttr = [&]() {
-            // If enclosing declaration has a global actor,
-            // skip the suggestion.
-            if (storage->getGlobalActorAttr())
-              return false;
+  } else if (auto storage = dyn_cast<AbstractStorageDecl>(decl)) {
+    if (isGlobalActorAnErrorOnStorage(storage, /*diagnose=*/true)) {
+      return std::nullopt;
+    }
+  } else if (auto *accessor = dyn_cast<AccessorDecl>(decl)) {
+    auto &ctx = accessor->getASTContext();
+    const auto attrRange = globalActorAttr->getRangeWithAt();
+    const unsigned langModeForError = accessor->isGetter() ? 7 : 6;
+    const bool isError = ctx.isSwiftVersionAtLeast(langModeForError);
 
-            // Global actor attribute cannot be applied to
-            // an instance stored property of a struct.
-            if (auto *var = dyn_cast<VarDecl>(storage)) {
-              return !isStoredInstancePropertyOfStruct(var);
-            }
+    // Complain about a global actor attribute on an accessor.
+    accessor->diagnose(diag::global_actor_disallowed, decl)
+        .highlight(attrRange)
+        .warnUntilSwiftVersion(langModeForError);
 
-            return true;
-          };
+    if (!isError && shouldIgnoreGlobalActorOnAccessor(ctx)) {
+      ctx.Diags.diagnose(attrRange.Start, diag::global_actor_attr_is_ignored)
+          .highlight(attrRange);
+    }
 
-          if (canMoveAttr()) {
-            decl->diagnose(diag::move_global_actor_attr_to_storage_decl,
-                           storage)
-                .fixItInsert(
-                    storage->getAttributeInsertionLoc(/*forModifier=*/false),
-                    llvm::Twine("@", result->second->getNameStr()).str());
-          }
-        }
+    auto *storage = accessor->getStorage();
 
-        // In Swift 6, once the diag above is an error, it is disallowed.
-        if (ctx.isSwiftVersionAtLeast(6))
-          return std::nullopt;
+    // Suggest to move the attribute to the storage if it is legal and the
+    // storage does not specify some kind of explicit isolation.
+    if (!isGlobalActorAnErrorOnStorage(storage, /*diagnose=*/false)) {
+      auto storageIsolation = getInferredActorIsolation(storage);
+      if (storageIsolation.source.kind != IsolationSource::Explicit) {
+        accessor
+            ->diagnose(diag::move_global_actor_attr_to_storage_decl, storage)
+            .highlight(attrRange)
+            .fixItRemove(attrRange)
+            .fixItInsertAttribute(storage->getAttributeInsertionLoc(
+                                      globalActorAttr->isDeclModifier()),
+                                  globalActorAttr);
       }
     }
-    // Functions are okay.
+
+    // Fail if we emitted an error.
+    if (isError)
+      return std::nullopt;
+
+  } else if (isa<ExtensionDecl>(decl) || isa<AbstractFunctionDecl>(decl)) {
+    // Other functions and extensions are okay.
   } else {
     // Everything else is disallowed.
     decl->diagnose(diag::global_actor_disallowed, decl);
@@ -5467,9 +5468,11 @@ getActorIsolationForMainFuncDecl(FuncDecl *fnDecl) {
 /// Check rules related to global actor attributes on a class declaration.
 ///
 /// \returns true if an error occurred.
-static bool checkClassGlobalActorIsolation(
-    ClassDecl *classDecl, ActorIsolation isolation) {
-  assert(isolation.isGlobalActor());
+static bool checkClassIsolation(ClassDecl *classDecl,
+                                ActorIsolation isolation) {
+  if (!isolation.isGlobalActor()) {
+    return false;
+  }
 
   // A class can only be annotated with a global actor if it has no
   // superclass, the superclass is annotated with the same global actor, or
@@ -5909,8 +5912,7 @@ static InferredActorIsolation computeActorIsolation(Evaluator &evaluator,
       if (isolationFromAttr && isolationFromAttr->isGlobalActor()) {
         if (!areTypesEqual(isolationFromAttr->getGlobalActor(),
                            mainIsolation->getGlobalActor())) {
-          fd->getASTContext().Diags.diagnose(
-              fd->getLoc(), diag::main_function_must_be_mainActor);
+          fd->diagnose(diag::main_function_must_be_mainActor);
         }
       }
       return {
@@ -5920,17 +5922,29 @@ static InferredActorIsolation computeActorIsolation(Evaluator &evaluator,
     }
   }
 
-  // If this declaration has one of the actor isolation attributes, report
-  // that.
+  // If this declaration has one of the actor isolation attributes, consider
+  // reporting that.
   if (isolationFromAttr) {
-    // Classes with global actors have additional rules regarding inheritance.
-    if (isolationFromAttr->isGlobalActor()) {
-      if (auto classDecl = dyn_cast<ClassDecl>(value))
-        checkClassGlobalActorIsolation(classDecl, *isolationFromAttr);
+    if (isa<AccessorDecl>(value) && shouldIgnoreGlobalActorOnAccessor(ctx)) {
+      // Global actor attributes on accessors are deprecated, whereas other
+      // isolation attributes are unconditionally unapplicable to accessors.
+      //
+      // In Swift 6 mode and onward, disregard the global actor attribute and
+      // proceed to infer the isolation from the storage declaration.
+    } else {
+      // Classes with global actors have additional rules regarding inheritance.
+      if (auto classDecl = dyn_cast<ClassDecl>(value)) {
+        checkClassIsolation(classDecl, *isolationFromAttr);
+      }
+
+      return {*isolationFromAttr,
+              IsolationSource(/*source*/ nullptr, IsolationSource::Explicit)};
     }
+  }
 
-    return {*isolationFromAttr,
-            IsolationSource(/*source*/ nullptr, IsolationSource::Explicit)};
+  // For an accessor, use the actor isolation of its storage declaration.
+  if (auto *accessor = dyn_cast<AccessorDecl>(value)) {
+    return getInferredActorIsolation(accessor->getStorage());
   }
 
   InferredActorIsolation defaultIsolation;
@@ -6046,12 +6060,6 @@ static InferredActorIsolation computeActorIsolation(Evaluator &evaluator,
     }
   }
 
-  // If this is an accessor, use the actor isolation of its storage
-  // declaration.
-  if (auto accessor = dyn_cast<AccessorDecl>(value)) {
-    return getInferredActorIsolation(accessor->getStorage());
-  }
-
   if (auto var = dyn_cast<VarDecl>(value)) {
     auto &ctx = var->getASTContext();
     if (!ctx.LangOpts.isConcurrencyModelTaskToThread() &&
@@ -6174,10 +6182,8 @@ static InferredActorIsolation computeActorIsolation(Evaluator &evaluator,
     // attributes, use that.
     if (auto ext = dyn_cast<ExtensionDecl>(value->getDeclContext())) {
       if (auto isolationFromAttr = getIsolationFromAttributes(ext)) {
-        return {
-          inferredIsolation(*isolationFromAttr, onlyGlobal),
-          IsolationSource(ext, IsolationSource::Explicit)
-        };
+        return {inferredIsolation(*isolationFromAttr, onlyGlobal),
+                IsolationSource(ext, IsolationSource::LexicalContext)};
       }
     }
 
@@ -6197,8 +6203,15 @@ static InferredActorIsolation computeActorIsolation(Evaluator &evaluator,
           isolation = ActorIsolation::forCallerIsolationInheriting();
         }
 
-        return {inferredIsolation(isolation, onlyGlobal),
-                selfTypeIsolation.source};
+        // If the type context has explicit isolation, this one is inferred from
+        // context.
+        auto isolationSource = selfTypeIsolation.source;
+        if (isolationSource.kind == IsolationSource::Explicit) {
+          isolationSource =
+              IsolationSource(selfTypeDecl, IsolationSource::LexicalContext);
+        }
+
+        return {inferredIsolation(isolation, onlyGlobal), isolationSource};
       }
     }
   }