Merge pull request #67974 from sophiapoirier/global-static-data-race-safety

enforce under strict concurrency that globals and statics be either…

Author: sophiapoirier

include/swift/AST/DiagnosticsSema.def

@@ -5248,6 +5248,15 @@ NOTE(actor_mutable_state,none,
 WARNING(shared_mutable_state_access,none,
         "reference to %kind0 is not concurrency-safe because it involves "
         "shared mutable state", (const ValueDecl *))
+WARNING(shared_mutable_state_decl,none,
+        "%kind0 is not concurrency-safe because it is non-isolated global "
+        "shared mutable state", (const ValueDecl *))
+NOTE(shared_mutable_state_decl_note,none,
+     "isolate %0 to a global actor, or convert it to a 'let' constant and "
+     "conform it to 'Sendable'", (const ValueDecl *))
+WARNING(shared_immutable_state_decl,none,
+        "%kind0 is not concurrency-safe because it is not either conforming to "
+        "'Sendable' or isolated to a global actor", (const ValueDecl *))
 ERROR(actor_isolated_witness,none,
      "%select{|distributed }0%1 %kind2 cannot be used to satisfy %3 protocol "
      "requirement",

include/swift/Basic/Features.def

@@ -219,6 +219,9 @@ EXPERIMENTAL_FEATURE(StrictConcurrency, true)
 /// Defer Sendable checking to SIL diagnostic phase.
 EXPERIMENTAL_FEATURE(SendNonSendable, false)
 
+/// Within strict concurrency, narrow global variable isolation requirements.
+EXPERIMENTAL_FEATURE(GlobalConcurrency, false)
+
 /// Enable extended callbacks (with additional parameters) to be used when the
 /// "playground transform" is enabled.
 EXPERIMENTAL_FEATURE(PlaygroundExtendedCallbacks, true)

lib/AST/ASTPrinter.cpp

@@ -3489,6 +3489,8 @@ static bool usesFeatureSendNonSendable(Decl *decl) {
   return false;
 }
 
+static bool usesFeatureGlobalConcurrency(Decl *decl) { return false; }
+
 static bool usesFeaturePlaygroundExtendedCallbacks(Decl *decl) {
   return false;
 }

lib/Frontend/CompilerInvocation.cpp

@@ -954,14 +954,7 @@ static bool ParseLangArgs(LangOptions &Opts, ArgList &Args,
   if (Opts.isSwiftVersionAtLeast(6)) {
     Opts.StrictConcurrencyLevel = StrictConcurrency::Complete;
   } else if (const Arg *A = Args.getLastArg(OPT_strict_concurrency)) {
-    auto value =
-        llvm::StringSwitch<llvm::Optional<StrictConcurrency>>(A->getValue())
-            .Case("minimal", StrictConcurrency::Minimal)
-            .Case("targeted", StrictConcurrency::Targeted)
-            .Case("complete", StrictConcurrency::Complete)
-            .Default(llvm::None);
-
-    if (value)
+    if (auto value = parseStrictConcurrency(A->getValue()))
       Opts.StrictConcurrencyLevel = *value;
     else
       Diags.diagnose(SourceLoc(), diag::error_invalid_arg_value,

lib/Sema/TypeCheckConcurrency.cpp

@@ -3184,10 +3184,7 @@ namespace {
           isolatedActor, llvm::None, llvm::None, getClosureActorIsolation);
       switch (result) {
       case ActorReferenceResult::SameConcurrencyDomain:
-        if (diagnoseReferenceToUnsafeGlobal(decl, loc))
-          return true;
-
-        return false;
+        return diagnoseReferenceToUnsafeGlobal(decl, loc);
 
       case ActorReferenceResult::ExitsActorToNonisolated:
         if (diagnoseReferenceToUnsafeGlobal(decl, loc))
@@ -4112,6 +4109,33 @@ ActorIsolation ActorIsolationRequest::evaluate(
       return ActorIsolation::forActorInstanceParameter(actor, *paramIdx);
   }
 
+  // Diagnose global state that is not either immutable plus Sendable or
+  // isolated to a global actor.
+  auto checkGlobalIsolation = [var = dyn_cast<VarDecl>(value)](
+                                  ActorIsolation isolation) {
+    if (var && var->getLoc() &&
+        var->getASTContext().LangOpts.hasFeature(Feature::GlobalConcurrency) &&
+        !isolation.isGlobalActor()) {
+      const bool isGlobalState =
+          var->isStatic() || var->getDeclContext()->isModuleScopeContext() ||
+          (var->getDeclContext()->isTypeContext() && !var->isInstanceMember());
+      auto *classDecl = var->getDeclContext()->getSelfClassDecl();
+      const bool isActorType = classDecl && classDecl->isAnyActor();
+      if (isGlobalState && !isActorType) {
+        if (var->isLet()) {
+          if (!isSendableType(var->getModuleContext(),
+                              var->getInterfaceType())) {
+            var->diagnose(diag::shared_immutable_state_decl, var);
+          }
+        } else {
+          var->diagnose(diag::shared_mutable_state_decl, var);
+          var->diagnose(diag::shared_mutable_state_decl_note, var);
+        }
+      }
+    }
+    return isolation;
+  };
+
   auto isolationFromAttr = getIsolationFromAttributes(value);
   if (FuncDecl *fd = dyn_cast<FuncDecl>(value)) {
     // Main.main() and Main.$main are implicitly MainActor-protected.
@@ -4138,7 +4162,7 @@ ActorIsolation ActorIsolationRequest::evaluate(
         checkClassGlobalActorIsolation(classDecl, *isolationFromAttr);
     }
 
-    return *isolationFromAttr;
+    return checkGlobalIsolation(*isolationFromAttr);
   }
 
   // Determine the default isolation for this declaration, which may still be
@@ -4169,78 +4193,82 @@ ActorIsolation ActorIsolationRequest::evaluate(
   }
 
   // Function used when returning an inferred isolation.
-  auto inferredIsolation = [&](
-      ActorIsolation inferred, bool onlyGlobal = false) {
-    // check if the inferred isolation is valid in the context of
-    // its overridden isolation.
-    if (overriddenValue) {
-      // if the inferred isolation is not valid, then carry-over the overridden
-      // declaration's isolation as this decl's inferred isolation.
-      switch (validOverrideIsolation(
-                  value, inferred, overriddenValue, *overriddenIso)) {
-      case OverrideIsolationResult::Allowed:
-      case OverrideIsolationResult::Sendable:
-        break;
-
-      case OverrideIsolationResult::Disallowed:
-        inferred = *overriddenIso;
-        break;
-      }
-    }
+  auto inferredIsolation = [&](ActorIsolation inferred,
+                               bool onlyGlobal = false) {
+    // Invoke the body within checkGlobalIsolation to check the result.
+    return checkGlobalIsolation([&] {
+      // check if the inferred isolation is valid in the context of
+      // its overridden isolation.
+      if (overriddenValue) {
+        // if the inferred isolation is not valid, then carry-over the
+        // overridden declaration's isolation as this decl's inferred isolation.
+        switch (validOverrideIsolation(value, inferred, overriddenValue,
+                                       *overriddenIso)) {
+        case OverrideIsolationResult::Allowed:
+        case OverrideIsolationResult::Sendable:
+          break;
 
-    // Add an implicit attribute to capture the actor isolation that was
-    // inferred, so that (e.g.) it will be printed and serialized.
-    ASTContext &ctx = value->getASTContext();
-    switch (inferred) {
-    case ActorIsolation::Independent:
-      // Stored properties cannot be non-isolated, so don't infer it.
-      if (auto var = dyn_cast<VarDecl>(value)) {
-        if (!var->isStatic() && var->hasStorage())
-          return ActorIsolation::forUnspecified()
-                    .withPreconcurrency(inferred.preconcurrency());
+        case OverrideIsolationResult::Disallowed:
+          inferred = *overriddenIso;
+          break;
+        }
       }
 
+      // Add an implicit attribute to capture the actor isolation that was
+      // inferred, so that (e.g.) it will be printed and serialized.
+      ASTContext &ctx = value->getASTContext();
+      switch (inferred) {
+      case ActorIsolation::Independent:
+        // Stored properties cannot be non-isolated, so don't infer it.
+        if (auto var = dyn_cast<VarDecl>(value)) {
+          if (!var->isStatic() && var->hasStorage())
+            return ActorIsolation::forUnspecified().withPreconcurrency(
+                inferred.preconcurrency());
+        }
 
-      if (onlyGlobal)
-        return ActorIsolation::forUnspecified()
-                  .withPreconcurrency(inferred.preconcurrency());
+        if (onlyGlobal) {
+          return ActorIsolation::forUnspecified().withPreconcurrency(
+              inferred.preconcurrency());
+        }
 
-      value->getAttrs().add(new (ctx) NonisolatedAttr(/*IsImplicit=*/true));
-      break;
+        value->getAttrs().add(new (ctx) NonisolatedAttr(/*IsImplicit=*/true));
+        break;
 
-    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()
-                              .withPreconcurrency(inferred.preconcurrency());
-
-      auto typeExpr = TypeExpr::createImplicit(inferred.getGlobalActor(), ctx);
-      auto attr = CustomAttr::create(
-          ctx, SourceLoc(), typeExpr, /*implicit=*/true);
-      if (inferred == ActorIsolation::GlobalActorUnsafe)
-        attr->setArgIsUnsafe(true);
-      value->getAttrs().add(attr);
-      break;
-    }
+      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().withPreconcurrency(
+                        inferred.preconcurrency());
+
+        auto typeExpr =
+            TypeExpr::createImplicit(inferred.getGlobalActor(), ctx);
+        auto attr =
+            CustomAttr::create(ctx, SourceLoc(), typeExpr, /*implicit=*/true);
+        if (inferred == ActorIsolation::GlobalActorUnsafe)
+          attr->setArgIsUnsafe(true);
+        value->getAttrs().add(attr);
+        break;
+      }
 
-    case ActorIsolation::ActorInstance:
-    case ActorIsolation::Unspecified:
-      if (onlyGlobal)
-        return ActorIsolation::forUnspecified()
-                    .withPreconcurrency(inferred.preconcurrency());
+      case ActorIsolation::ActorInstance:
+      case ActorIsolation::Unspecified:
+        if (onlyGlobal)
+          return ActorIsolation::forUnspecified().withPreconcurrency(
+              inferred.preconcurrency());
 
-      // Nothing to do.
-      break;
-    }
+        // Nothing to do.
+        break;
+      }
 
-    return inferred;
+      return inferred;
+    }());
   };
 
   // If this is a local function, inherit the actor isolation from its
@@ -4371,7 +4399,7 @@ ActorIsolation ActorIsolationRequest::evaluate(
   }
 
   // Default isolation for this member.
-  return defaultIsolation;
+  return checkGlobalIsolation(defaultIsolation);
 }
 
 bool HasIsolatedSelfRequest::evaluate(

test/Concurrency/experimental_feature_strictconcurrency.swift

@@ -1,5 +1,5 @@
-// RUN: %target-typecheck-verify-swift -enable-experimental-feature StrictConcurrency
-// RUN: %target-typecheck-verify-swift -enable-experimental-feature StrictConcurrency=complete
+// RUN: %target-typecheck-verify-swift -disable-availability-checking -parse-as-library -enable-experimental-feature StrictConcurrency -enable-experimental-feature GlobalConcurrency
+// RUN: %target-typecheck-verify-swift -disable-availability-checking -parse-as-library -enable-experimental-feature StrictConcurrency=complete -enable-experimental-feature GlobalConcurrency
 // REQUIRES: concurrency
 
 class C1 { } // expected-note{{class 'C1' does not conform to the 'Sendable' protocol}}
@@ -20,3 +20,40 @@ struct Test2: TestProtocol { // expected-warning{{conformance of 'C2' to 'Sendab
   // expected-note@-1{{in associated type 'Self.Value' (inferred as 'C2')}}
   typealias Value = C2
 }
+
+@globalActor
+actor TestGlobalActor {
+  static var shared = TestGlobalActor()
+}
+
+@TestGlobalActor
+var mutableIsolatedGlobal = 1
+
+var mutableNonisolatedGlobal = 1 // expected-warning{{var 'mutableNonisolatedGlobal' is not concurrency-safe because it is non-isolated global shared mutable state}}
+// expected-note@-1{{isolate 'mutableNonisolatedGlobal' to a global actor, or convert it to a 'let' constant and conform it to 'Sendable'}}
+
+let immutableGlobal = 1
+
+final class TestSendable: Sendable {
+  init() {}
+}
+
+final class TestNonsendable {
+  init() {}
+}
+
+struct A {
+  static let immutableExplicitSendable = TestSendable()
+  static let immutableNonsendableGlobal = TestNonsendable() // expected-warning{{static property 'immutableNonsendableGlobal' is not concurrency-safe because it is not either conforming to 'Sendable' or isolated to a global actor}}
+  static let immutableInferredSendable = 0
+  static var mutable = 0 // expected-warning{{static property 'mutable' is not concurrency-safe because it is non-isolated global shared mutable state}}
+  // expected-note@-1{{isolate 'mutable' to a global actor, or convert it to a 'let' constant and conform it to 'Sendable'}}
+  // expected-note@-2{{static property declared here}}
+}
+
+@TestGlobalActor
+func f() {
+  print(A.immutableExplicitSendable)
+  print(A.immutableInferredSendable)
+  print(A.mutable) // expected-warning{{reference to static property 'mutable' is not concurrency-safe because it involves shared mutable state}}
+}