Merge pull request #70811 from sophiapoirier/globals-preconcurrency-warning

🍒 @preconcurrency import downgrades globals concurrency errors to warnings

Author: sophiapoirier

include/swift/AST/DiagnosticEngine.h

@@ -561,6 +561,16 @@ namespace swift {
     /// emitted as a warning, but a note will still be emitted as a note.
     InFlightDiagnostic &limitBehavior(DiagnosticBehavior limit);
 
+    /// Conditionally prevent the diagnostic from behaving more severely than \p
+    /// limit. If the condition is false, no limit is imposed.
+    InFlightDiagnostic &limitBehaviorIf(bool shouldLimit,
+                                        DiagnosticBehavior limit) {
+      if (!shouldLimit) {
+        return *this;
+      }
+      return limitBehavior(limit);
+    }
+
     /// Limit the diagnostic behavior to warning until the specified version.
     ///
     /// This helps stage in fixes for stricter diagnostics as warnings

include/swift/AST/DiagnosticsSema.def

@@ -5329,18 +5329,18 @@ ERROR(distributed_actor_remote_func_must_not_be_distributed,none,
 NOTE(actor_mutable_state,none,
      "mutation of this %0 is only permitted within the actor",
      (DescriptiveDeclKind))
-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 *))
+ERROR(shared_mutable_state_access,none,
+      "reference to %kind0 is not concurrency-safe because it involves shared "
+      "mutable state", (const ValueDecl *))
+ERROR(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(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

@@ -118,6 +118,7 @@ UPCOMING_FEATURE(BareSlashRegexLiterals, 354, 6)
 UPCOMING_FEATURE(DeprecateApplicationMain, 383, 6)
 UPCOMING_FEATURE(ImportObjcForwardDeclarations, 384, 6)
 UPCOMING_FEATURE(DisableOutwardActorInference, 401, 6)
+UPCOMING_FEATURE(GlobalConcurrency, 412, 6)
 
 UPCOMING_FEATURE(ExistentialAny, 335, 7)
 
@@ -221,9 +222,6 @@ 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)
-
 /// Allow default values to require isolation at the call-site.
 EXPERIMENTAL_FEATURE(IsolatedDefaultValues, false)
 

lib/Frontend/CompilerInvocation.cpp

@@ -976,6 +976,11 @@ static bool ParseLangArgs(LangOptions &Opts, ArgList &Args,
     Opts.StrictConcurrencyLevel = StrictConcurrency::Minimal;
   }
 
+  // StrictConcurrency enables all data-race safety upcoming features.
+  if (Opts.hasFeature(Feature::StrictConcurrency)) {
+    Opts.Features.insert(Feature::GlobalConcurrency);
+  }
+
   Opts.WarnImplicitOverrides =
     Args.hasArg(OPT_warn_implicit_overrides);
 

lib/Sema/TypeCheckConcurrency.cpp

@@ -821,12 +821,12 @@ static bool hasExplicitSendableConformance(NominalTypeDecl *nominal,
           conformance.getConcrete())->isMissing());
 }
 
-/// Find the import that makes the given nominal declaration available.
+/// Find the import that makes the given declaration available.
 static llvm::Optional<AttributedImport<ImportedModule>>
-findImportFor(NominalTypeDecl *nominal, const DeclContext *fromDC) {
-  // If the nominal type is from the current module, there's no import.
-  auto nominalModule = nominal->getParentModule();
-  if (nominalModule == fromDC->getParentModule())
+findImportFor(const DeclContext *dc, const DeclContext *fromDC) {
+  // If the type is from the current module, there's no import.
+  auto module = dc->getParentModule();
+  if (module == fromDC->getParentModule())
     return llvm::None;
 
   auto fromSourceFile = fromDC->getParentSourceFile();
@@ -835,16 +835,16 @@ findImportFor(NominalTypeDecl *nominal, const DeclContext *fromDC) {
 
   // Look to see if the owning module was directly imported.
   for (const auto &import : fromSourceFile->getImports()) {
-    if (import.module.importedModule == nominalModule)
+    if (import.module.importedModule == module)
       return import;
   }
 
   // Now look for transitive imports.
-  auto &importCache = nominal->getASTContext().getImportCache();
+  auto &importCache = dc->getASTContext().getImportCache();
   for (const auto &import : fromSourceFile->getImports()) {
     auto &importSet = importCache.getImportSet(import.module.importedModule);
     for (const auto &transitive : importSet.getTransitiveImports()) {
-      if (transitive.importedModule == nominalModule) {
+      if (transitive.importedModule == module) {
         return import;
       }
     }
@@ -2800,7 +2800,8 @@ namespace {
     ///
     /// \returns true if we diagnosed the entity, \c false otherwise.
     bool diagnoseReferenceToUnsafeGlobal(ValueDecl *value, SourceLoc loc) {
-      switch (value->getASTContext().LangOpts.StrictConcurrencyLevel) {
+      auto &ctx = value->getASTContext();
+      switch (ctx.LangOpts.StrictConcurrencyLevel) {
       case StrictConcurrency::Minimal:
       case StrictConcurrency::Targeted:
         // Never diagnose.
@@ -2823,16 +2824,32 @@ namespace {
         return false;
 
       // If it's actor-isolated, it's already been dealt with.
-      if (getActorIsolation(value).isActorIsolated())
+      const auto isolation = getActorIsolation(value);
+      if (isolation.isActorIsolated())
         return false;
 
       if (auto attr = value->getAttrs().getAttribute<NonisolatedAttr>();
           attr && attr->isUnsafe()) {
         return false;
       }
 
-      ctx.Diags.diagnose(loc, diag::shared_mutable_state_access, value);
+      const auto import =
+          findImportFor(var->getDeclContext(), getDeclContext());
+      const bool isPreconcurrencyImport =
+          import && import->options.contains(ImportFlags::Preconcurrency);
+      const auto isPreconcurrencyUnspecifiedIsolation =
+          isPreconcurrencyImport && isolation.isUnspecified();
+      ctx.Diags.diagnose(loc, diag::shared_mutable_state_access, value)
+          .warnUntilSwiftVersionIf(!isPreconcurrencyUnspecifiedIsolation, 6)
+          .limitBehaviorIf(isPreconcurrencyImport, DiagnosticBehavior::Warning)
+          .limitBehaviorIf(!ctx.isSwiftVersionAtLeast(6) &&
+                               isPreconcurrencyUnspecifiedIsolation,
+                           DiagnosticBehavior::Ignore);
       value->diagnose(diag::kind_declared_here, value->getDescriptiveKind());
+      if (const auto sourceFile = getDeclContext()->getParentSourceFile();
+          sourceFile && isPreconcurrencyImport) {
+        sourceFile->setImportUsedPreconcurrency(*import);
+      }
       return true;
     }
 

test/Concurrency/Inputs/GlobalVariables.swift

@@ -0,0 +1,11 @@
+public struct Globals {
+  public static let integerConstant = 0
+  public static var integerMutable = 0
+
+  public static nonisolated(unsafe) let nonisolatedUnsafeIntegerConstant = 0
+  public static nonisolated(unsafe) var nonisolatedUnsafeIntegerMutable = 0
+
+  @MainActor public static var actorInteger = 0
+
+  public init() {}
+}

test/Concurrency/experimental_feature_strictconcurrency.swift

@@ -1,5 +1,5 @@
-// RUN: %target-swift-frontend -disable-availability-checking -parse-as-library -enable-experimental-feature StrictConcurrency -enable-experimental-feature GlobalConcurrency -emit-sil -o /dev/null -verify %s
-// RUN: %target-swift-frontend -disable-availability-checking -parse-as-library -enable-experimental-feature StrictConcurrency=complete -enable-experimental-feature GlobalConcurrency -emit-sil -o /dev/null -verify %s
+// RUN: %target-swift-frontend -disable-availability-checking -enable-experimental-feature StrictConcurrency -emit-sil -o /dev/null -verify %s
+// RUN: %target-swift-frontend -disable-availability-checking -enable-experimental-feature StrictConcurrency=complete -emit-sil -o /dev/null -verify %s
 
 // REQUIRES: concurrency
 // REQUIRES: asserts
@@ -23,67 +23,6 @@ struct Test2: TestProtocol { // expected-warning{{conformance of 'C2' to 'Sendab
   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() {}
-}
-
-nonisolated(unsafe) let immutableNonisolatedUnsafeTopLevelGlobal = TestNonsendable()
-
-@propertyWrapper
-public struct TestWrapper {
-  public init() {}
-  public var wrappedValue: Int {
-    return 0
-  }
-}
-
-struct TestStatics {
-  static let immutableExplicitSendable = TestSendable()
-  static let immutableNonsendable = TestNonsendable() // expected-warning{{static property 'immutableNonsendable' is not concurrency-safe because it is not either conforming to 'Sendable' or isolated to a global actor}}
-  static nonisolated(unsafe) let immutableNonisolatedUnsafe = TestNonsendable()
-  static nonisolated let immutableNonisolated = TestNonsendable() // expected-warning{{static property 'immutableNonisolated' 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}}
-  static var computedProperty: Int { 0 } // computed property that, though static, has no storage so is not a global
-  @TestWrapper static var wrapped: Int // expected-warning{{static property 'wrapped' is not concurrency-safe because it is non-isolated global shared mutable state}}
-  // expected-note@-1{{isolate 'wrapped' to a global actor, or convert it to a 'let' constant and conform it to 'Sendable'}}
-}
-
-@TestGlobalActor
-func f() {
-  print(TestStatics.immutableExplicitSendable)
-  print(TestStatics.immutableInferredSendable)
-  print(TestStatics.mutable) // expected-warning{{reference to static property 'mutable' is not concurrency-safe because it involves shared mutable state}}
-}
-
-func testLocalNonisolatedUnsafe() async {
-  nonisolated(unsafe) var value = 1
-  let task = Task {
-    value = 2
-    return value
-  }
-  print(await task.value)
-}
-
 @MainActor
 func iterate(stream: AsyncStream<Int>) async {
   nonisolated(unsafe) var it = stream.makeAsyncIterator()

test/Concurrency/global_variables.swift

@@ -0,0 +1,78 @@
+// RUN: %empty-directory(%t)
+// RUN: %target-swift-frontend -emit-module -emit-module-path %t/GlobalVariables.swiftmodule -module-name GlobalVariables -parse-as-library -strict-concurrency=minimal -swift-version 5 %S/Inputs/GlobalVariables.swift
+// RUN: %target-swift-frontend -disable-availability-checking -parse-as-library -swift-version 6 -I %t %s %s -emit-sil -o /dev/null -verify %s
+
+// REQUIRES: concurrency
+// REQUIRES: asserts
+
+@preconcurrency import GlobalVariables
+
+@globalActor
+actor TestGlobalActor {
+  static var shared = TestGlobalActor()
+}
+
+@TestGlobalActor
+var mutableIsolatedGlobal = 1
+
+var mutableNonisolatedGlobal = 1 // expected-error{{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() {}
+}
+
+nonisolated(unsafe) let immutableNonisolatedUnsafeTopLevelGlobal = TestNonsendable()
+
+@propertyWrapper
+public struct TestWrapper {
+  public init() {}
+  public var wrappedValue: Int {
+    return 0
+  }
+}
+
+struct TestStatics {
+  static let immutableExplicitSendable = TestSendable()
+  static let immutableNonsendable = TestNonsendable() // expected-error{{static property 'immutableNonsendable' is not concurrency-safe because it is not either conforming to 'Sendable' or isolated to a global actor}}
+  static nonisolated(unsafe) let immutableNonisolatedUnsafe = TestNonsendable()
+  static nonisolated let immutableNonisolated = TestNonsendable() // expected-error{{static property 'immutableNonisolated' 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-error{{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}}
+  static var computedProperty: Int { 0 } // computed property that, though static, has no storage so is not a global
+  @TestWrapper static var wrapped: Int // expected-error{{static property 'wrapped' is not concurrency-safe because it is non-isolated global shared mutable state}}
+  // expected-note@-1{{isolate 'wrapped' to a global actor, or convert it to a 'let' constant and conform it to 'Sendable'}}
+}
+
+@TestGlobalActor
+func f() {
+  print(TestStatics.immutableExplicitSendable)
+  print(TestStatics.immutableInferredSendable)
+  print(TestStatics.mutable) // expected-error{{reference to static property 'mutable' is not concurrency-safe because it involves shared mutable state}}
+  print(Globals.actorInteger) // expected-error{{main actor-isolated static property 'actorInteger' can not be referenced from global actor 'TestGlobalActor'}}
+}
+
+func testLocalNonisolatedUnsafe() async {
+  nonisolated(unsafe) var value = 1
+  let task = Task {
+    value = 2
+    return value
+  }
+  print(await task.value)
+}
+
+func testImportedGlobals() { // expected-note{{add '@MainActor' to make global function 'testImportedGlobals()' part of global actor 'MainActor'}}
+  let _ = Globals.integerConstant
+  let _ = Globals.integerMutable // expected-warning{{reference to static property 'integerMutable' is not concurrency-safe because it involves shared mutable state}}
+  let _ = Globals.nonisolatedUnsafeIntegerConstant
+  let _ = Globals.nonisolatedUnsafeIntegerMutable
+  let _ = Globals.actorInteger // expected-error{{main actor-isolated static property 'actorInteger' can not be referenced from a non-isolated context}}
+}

test/expr/closure/closures_swift6.swift

@@ -81,7 +81,7 @@ class C_56501 {
   }
 }
 
-public class TestImplicitSelfForWeakSelfCapture {
+public final class TestImplicitSelfForWeakSelfCapture: Sendable {
   static let staticOptional: TestImplicitSelfForWeakSelfCapture? = .init()
   func method() { }