Provide more Fix-It guidance for concurrency-unsafe global variables (SE-0412)

DougGregor · DougGregor · commit 18008f21551e · 2024-05-20T22:09:34.000-07:00
When diagnosing a concurrency-unsafe global or static variable, provide Fix-Its with specific guidance and advice. This is intended to aid the workflow for folks enabling strict concurrency checking or Swift 6. There are up to three Fix-Its attached to a diagnostic about concurrency-unsafe global/static variables: * convert 'global' to a 'let' constant to make the shared state immutable, which replaces `var` with `let` * restrict 'global' to the main actor if it will only be accessed from the main thread, which adds `@MainActor` * unsafely mark %0 as concurrency-safe if all accesses are protected by an external synchronization mechanism, which adds `nonisolated(unsafe)` I fretted over two things before deciding on this path: 1. For the second note, the reality is that any global actor will suffice, but `@MainActor` is orders of magnitude more common than any other global actor, so "common case convenience" wins over "precise but less useful. 2. For the third note, `nonisolated(unsafe)` should only be used sparingly, and surfacing it via Fix-It could cause overuse. However, developers need to know about it, and this is how we do that. It comes last in the list of notes (after the better options) and says "unsafe" in not one but two places. (cherry picked from commit d4ce618)
diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def
@@ -5571,16 +5571,17 @@ ERROR(shared_mutable_state_access,none,
 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 *))
 ERROR(shared_immutable_state_decl,none,
       "%kind1 is not concurrency-safe because non-'Sendable' type %0 may have "
       "shared mutable state",
       (Type, const ValueDecl *))
-NOTE(shared_immutable_state_decl_note,none,
-     "isolate %0 to a global actor, or conform %1 to 'Sendable'",
-     (const ValueDecl *, Type))
+NOTE(shared_state_make_immutable,none,
+     "convert %0 to a 'let' constant to make the shared state immutable",
+     (const ValueDecl *))
+NOTE(shared_state_main_actor_node,none,
+     "restrict %0 to the main actor if it will only be accessed from the main thread", (const ValueDecl *))
+NOTE(shared_state_nonisolated_unsafe,none,
+     "unsafely mark %0 as concurrency-safe if all accesses are protected by an external synchronization mechanism", (const ValueDecl *))
 ERROR(actor_isolated_witness,none,
      "%select{|distributed }0%1 %kind2 cannot be used to satisfy %3 protocol "
      "requirement",
diff --git a/lib/Sema/MiscDiagnostics.cpp b/lib/Sema/MiscDiagnostics.cpp
@@ -3374,6 +3374,28 @@ class ReturnTypePlaceholderReplacer : public ASTWalker {
 
 } // end anonymous namespace
 
+SourceLoc swift::getFixItLocForVarToLet(VarDecl *var) {
+  // Try to find the location of the 'var' so we can produce a fixit.  If
+  // this is a simple PatternBinding, use its location.
+  if (auto *PBD = var->getParentPatternBinding()) {
+    if (PBD->getSingleVar() == var)
+      return PBD->getLoc();
+  } else if (auto *pattern = var->getParentPattern()) {
+    BindingPattern *foundVP = nullptr;
+    pattern->forEachNode([&](Pattern *P) {
+      if (auto *VP = dyn_cast<BindingPattern>(P))
+        if (VP->getSingleVar() == var)
+          foundVP = VP;
+    });
+
+    if (foundVP && foundVP->getIntroducer() != VarDecl::Introducer::Let) {
+      return foundVP->getLoc();
+    }
+  }
+
+  return SourceLoc();
+}
+
 // After we have scanned the entire region, diagnose variables that could be
 // declared with a narrower usage kind.
 VarDeclUsageChecker::~VarDeclUsageChecker() {
@@ -3593,25 +3615,7 @@ VarDeclUsageChecker::~VarDeclUsageChecker() {
         // Don't warn if we have something like "let (x,y) = ..." and 'y' was
         // never mutated, but 'x' was.
         !isVarDeclPartOfPBDThatHadSomeMutation(var)) {
-      SourceLoc FixItLoc;
-
-      // Try to find the location of the 'var' so we can produce a fixit.  If
-      // this is a simple PatternBinding, use its location.
-      if (auto *PBD = var->getParentPatternBinding()) {
-        if (PBD->getSingleVar() == var)
-          FixItLoc = PBD->getLoc();
-      } else if (auto *pattern = var->getParentPattern()) {
-        BindingPattern *foundVP = nullptr;
-        pattern->forEachNode([&](Pattern *P) {
-          if (auto *VP = dyn_cast<BindingPattern>(P))
-            if (VP->getSingleVar() == var)
-              foundVP = VP;
-        });
-
-        if (foundVP && foundVP->getIntroducer() != VarDecl::Introducer::Let) {
-          FixItLoc = foundVP->getLoc();
-        }
-      }
+      SourceLoc FixItLoc = getFixItLocForVarToLet(var);
 
       // If this is a parameter explicitly marked 'var', remove it.
       if (FixItLoc.isInvalid()) {
diff --git a/lib/Sema/MiscDiagnostics.h b/lib/Sema/MiscDiagnostics.h
@@ -70,6 +70,9 @@ namespace swift {
                    AccessLevel desiredAccess, bool isForSetter = false,
                    bool shouldUseDefaultAccess = false);
 
+  /// Compute the location of the 'var' keyword for a 'var'-to-'let' Fix-It.
+  SourceLoc getFixItLocForVarToLet(VarDecl *var);
+
   /// Describes the context of a parameter, for use in diagnosing argument
   /// label problems.
   enum class ParameterContext : unsigned {
diff --git a/lib/Sema/TypeCheckConcurrency.cpp b/lib/Sema/TypeCheckConcurrency.cpp
@@ -13,6 +13,7 @@
 // This file implements type checking support for Swift's concurrency model.
 //
 //===----------------------------------------------------------------------===//
+#include "MiscDiagnostics.h"
 #include "TypeCheckConcurrency.h"
 #include "TypeCheckDistributed.h"
 #include "TypeCheckInvertible.h"
@@ -5028,23 +5029,41 @@ ActorIsolation ActorIsolationRequest::evaluate(
         if (auto *originalVar = var->getOriginalWrappedProperty()) {
           diagVar = originalVar;
         }
+
+        bool diagnosed = false;
         if (var->isLet()) {
           auto type = var->getInterfaceType();
-          bool diagnosed = diagnoseIfAnyNonSendableTypes(
+          diagnosed = diagnoseIfAnyNonSendableTypes(
               type, SendableCheckContext(var->getDeclContext()),
               /*inDerivedConformance=*/Type(), /*typeLoc=*/SourceLoc(),
               /*diagnoseLoc=*/var->getLoc(),
               diag::shared_immutable_state_decl, diagVar);
-
-          // If we diagnosed this 'let' as non-Sendable, tack on a note
-          // to suggest a course of action.
-          if (diagnosed)
-            diagVar->diagnose(diag::shared_immutable_state_decl_note,
-                              diagVar, type);
         } else {
           diagVar->diagnose(diag::shared_mutable_state_decl, diagVar)
               .warnUntilSwiftVersion(6);
-          diagVar->diagnose(diag::shared_mutable_state_decl_note, diagVar);
+          diagnosed = true;
+        }
+
+        // If we diagnosed this global, tack on notes to suggest potential
+        // courses of action.
+        if (diagnosed) {
+          if (!var->isLet()) {
+            auto diag = diagVar->diagnose(diag::shared_state_make_immutable,
+                                          diagVar);
+            SourceLoc fixItLoc = getFixItLocForVarToLet(diagVar);
+            if (fixItLoc.isValid()) {
+              diag.fixItReplace(fixItLoc, "let");
+            }
+          }
+
+          diagVar->diagnose(diag::shared_state_main_actor_node,
+                            diagVar)
+              .fixItInsert(diagVar->getAttributeInsertionLoc(false),
+                           "@MainActor ");
+          diagVar->diagnose(diag::shared_state_nonisolated_unsafe,
+                            diagVar)
+              .fixItInsert(diagVar->getAttributeInsertionLoc(true),
+                           "nonisolated(unsafe) ");
         }
       }
     }
diff --git a/test/Concurrency/Inputs/sendable_cycle_other.swift b/test/Concurrency/Inputs/sendable_cycle_other.swift
@@ -1,4 +1,5 @@
 struct Foo {
   static let member = Bar() // expected-complete-warning {{static property 'member' is not concurrency-safe because non-'Sendable' type 'Bar' may have shared mutable state; this is an error in the Swift 6 language mode}}
-  // expected-complete-note@-1 {{isolate 'member' to a global actor, or conform 'Bar' to 'Sendable'}}
+  // expected-complete-note@-1 {{restrict 'member' to the main actor if it will only be accessed from the main thread}}
+  // expected-complete-note@-2{{unsafely mark 'member' as concurrency-safe if all accesses are protected by an external synchronization mechanism}}
 }
diff --git a/test/Concurrency/actor_isolation.swift b/test/Concurrency/actor_isolation.swift
@@ -12,8 +12,10 @@ import OtherActors // expected-warning{{add '@preconcurrency' to suppress 'Senda
 
 let immutableGlobal: String = "hello"
 
-// expected-warning@+2 {{var 'mutableGlobal' is not concurrency-safe because it is non-isolated global shared mutable state; this is an error in the Swift 6 language mode}}
-// expected-note@+1 {{isolate 'mutableGlobal' to a global actor, or convert it to a 'let' constant and conform it to 'Sendable'}}
+// expected-warning@+4 {{var 'mutableGlobal' is not concurrency-safe because it is non-isolated global shared mutable state; this is an error in the Swift 6 language mode}}
+// expected-note@+3 {{convert 'mutableGlobal' to a 'let' constant to make the shared state immutable}}
+// expected-note@+2 {{restrict 'mutableGlobal' to the main actor if it will only be accessed from the main thread}}
+// expected-note@+1 {{unsafely mark 'mutableGlobal' as concurrency-safe if all accesses are protected by an external synchronization mechanism}}
 var mutableGlobal: String = "can't touch this" // expected-note 5{{var declared here}}
 
 @available(SwiftStdlib 5.1, *)
diff --git a/test/Concurrency/concurrency_warnings.swift b/test/Concurrency/concurrency_warnings.swift
@@ -9,12 +9,15 @@ class GlobalCounter { // expected-note{{class 'GlobalCounter' does not conform t
 }
 
 let rs = GlobalCounter() // expected-warning {{let 'rs' is not concurrency-safe because non-'Sendable' type 'GlobalCounter' may have shared mutable state; this is an error in the Swift 6 language mode}}
-// expected-note@-1 {{isolate 'rs' to a global actor, or conform 'GlobalCounter' to 'Sendable'}}
+// expected-note@-1 {{restrict 'rs' to the main actor if it will only be accessed from the main thread}}
+// expected-note@-2 {{unsafely mark 'rs' as concurrency-safe if all accesses are protected by an external synchronization mechanism}}
+
 
 var globalInt = 17 // expected-warning {{var 'globalInt' is not concurrency-safe because it is non-isolated global shared mutable state; this is an error in the Swift 6 language mode}}
-// expected-note@-1 {{isolate 'globalInt' to a global actor, or convert it to a 'let' constant and conform it to 'Sendable'}}
+// expected-note@-1 {{restrict 'globalInt' to the main actor if it will only be accessed from the main thread}}
 // expected-note@-2 2{{var declared here}}
-
+// expected-note@-3 {{unsafely mark 'globalInt' as concurrency-safe if all accesses are protected by an external synchronization mechanism}}
+// expected-note@-4 {{convert 'globalInt' to a 'let' constant to make the shared state immutable}}
 
 class MyError: Error { // expected-warning{{non-final class 'MyError' cannot conform to 'Sendable'; use '@unchecked Sendable'}}
   var storage = 0 // expected-warning{{stored property 'storage' of 'Sendable'-conforming class 'MyError' is mutable}}
diff --git a/test/Concurrency/concurrent_value_checking.swift b/test/Concurrency/concurrent_value_checking.swift
@@ -274,7 +274,9 @@ typealias BadGenericCF<T> = @Sendable () -> T?
 typealias GoodGenericCF<T: Sendable> = @Sendable () -> T? // okay
 
 var concurrentFuncVar: (@Sendable (NotConcurrent) -> Void)? = nil // expected-warning{{var 'concurrentFuncVar' is not concurrency-safe because it is non-isolated global shared mutable state; this is an error in the Swift 6 language mode}}
-// expected-note@-1 {{isolate 'concurrentFuncVar' to a global actor, or convert it to a 'let' constant and conform it to 'Sendable'}}
+// expected-note@-1 {{restrict 'concurrentFuncVar' to the main actor if it will only be accessed from the main thread}}
+// expected-note@-2 {{unsafely mark 'concurrentFuncVar' as concurrency-safe if all accesses are protected by an external synchronization mechanism}}
+// expected-note@-3 {{convert 'concurrentFuncVar' to a 'let' constant to make the shared state immutable}}
 
 // ----------------------------------------------------------------------
 // Sendable restriction on @Sendable closures.
diff --git a/test/Concurrency/flow_isolation.swift b/test/Concurrency/flow_isolation.swift
@@ -520,8 +520,10 @@ struct CardboardBox<T> {
 
 @available(SwiftStdlib 5.1, *)
 var globalVar: EscapeArtist? // expected-warning {{var 'globalVar' is not concurrency-safe because it is non-isolated global shared mutable state; this is an error in the Swift 6 language mode}}
-// expected-note@-1 {{isolate 'globalVar' to a global actor, or convert it to a 'let' constant and conform it to 'Sendable'}}
+// expected-note@-1 {{restrict 'globalVar' to the main actor if it will only be accessed from the main thread}}
 // expected-note@-2 2 {{var declared here}}
+// expected-note@-3 {{unsafely mark 'globalVar' as concurrency-safe if all accesses are protected by an external synchronization mechanism}}
+// expected-note@-4 {{convert 'globalVar' to a 'let' constant to make the shared state immutable}}
 
 @available(SwiftStdlib 5.1, *)
 actor EscapeArtist {
diff --git a/test/Concurrency/freestanding_top_level.swift b/test/Concurrency/freestanding_top_level.swift
@@ -1,9 +1,11 @@
 // RUN: %target-swift-frontend -concurrency-model=task-to-thread -typecheck  -verify %s
 // RUN: %target-swift-frontend -concurrency-model=task-to-thread -typecheck  -verify -verify-additional-prefix complete- -strict-concurrency=complete %s
 
-// expected-complete-warning@+3 {{var 'global' is not concurrency-safe because it is non-isolated global shared mutable state; this is an error in the Swift 6 language mode}}
-// expected-complete-note@+2 {{isolate 'global' to a global actor, or convert it to a 'let' constant and conform it to 'Sendable'}}
-// expected-complete-note@+1 {{var declared here}}
+// expected-complete-warning@+5 {{var 'global' is not concurrency-safe because it is non-isolated global shared mutable state; this is an error in the Swift 6 language mode}}
+// expected-complete-note@+4 {{restrict 'global' to the main actor if it will only be accessed from the main thread}}{{1-1=@MainActor }}
+// expected-complete-note@+3 {{var declared here}}
+// expected-complete-note@+2 {{unsafely mark 'global' as concurrency-safe if all accesses are protected by an external synchronization mechanism}}{{1-1=nonisolated(unsafe) }}
+// expected-complete-note@+1 {{convert 'global' to a 'let' constant to make the shared state immutable}}{{1-4=let}}
 var global = 10
 
 // expected-complete-warning@+1 {{reference to var 'global' is not concurrency-safe because it involves shared mutable state; this is an error in the Swift 6 language mode}}
diff --git a/test/Concurrency/global_variables.swift b/test/Concurrency/global_variables.swift
@@ -15,7 +15,9 @@ actor 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'}}
+// expected-note@-1{{restrict 'mutableNonisolatedGlobal' to the main actor if it will only be accessed from the main thread}}{{1-1=@MainActor }}
+// expected-note@-2{{unsafely mark 'mutableNonisolatedGlobal' as concurrency-safe if all accesses are protected by an external synchronization mechanism}}{{1-1=nonisolated(unsafe) }}
+// expected-note@-3{{convert 'mutableNonisolatedGlobal' to a 'let' constant to make the shared state immutable}}{{1-4=let}}
 
 let immutableGlobal = 1
 
@@ -46,20 +48,26 @@ actor TestActor {
 struct TestStatics {
   static let immutableExplicitSendable = TestSendable()
   static let immutableNonsendable = TestNonsendable() // expected-error{{static property 'immutableNonsendable' is not concurrency-safe because non-'Sendable' type 'TestNonsendable' may have shared mutable state}}
-  // expected-note@-1 {{isolate 'immutableNonsendable' to a global actor, or conform 'TestNonsendable' to 'Sendable'}}
+  // expected-note@-1 {{restrict 'immutableNonsendable' to the main actor if it will only be accessed from the main thread}}
+  // expected-note@-2 {{unsafely mark 'immutableNonsendable' as concurrency-safe if all accesses are protected by an external synchronization mechanism}}
   static nonisolated(unsafe) let immutableNonisolatedUnsafe = TestNonsendable()
   static nonisolated let immutableNonisolated = TestNonsendable() // expected-error{{static property 'immutableNonisolated' is not concurrency-safe because non-'Sendable' type 'TestNonsendable' may have shared mutable state}}
-  // expected-note@-1 {{isolate 'immutableNonisolated' to a global actor, or conform 'TestNonsendable' to 'Sendable'}}
+  // expected-note@-1 {{restrict 'immutableNonisolated' to the main actor if it will only be accessed from the main thread}}
   // expected-error@-2 {{'nonisolated' can not be applied to variable with non-'Sendable' type 'TestNonsendable'}}
+  // expected-note@-3{{unsafely mark 'immutableNonisolated' as concurrency-safe if all accesses are protected by an external synchronization mechanism}}
   static nonisolated(unsafe) let immutableNonisolatedUnsafeSendable = TestSendable()
   // expected-warning@-1 {{'nonisolated(unsafe)' is unnecessary for a constant with 'Sendable' type 'TestSendable', consider removing it}} {{10-30=}}
   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@-1{{convert 'mutable' to a 'let' constant to make the shared state immutable}}
   // expected-note@-2{{static property declared here}}
+  // expected-note@-3{{unsafely mark 'mutable' as concurrency-safe if all accesses are protected by an external synchronization mechanism}}
+  // expected-note@-4{{restrict 'mutable' to the main actor if it will only be accessed from the main thread}}
   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'}}
+  // expected-note@-1{{convert 'wrapped' to a 'let' constant to make the shared state immutable}}{{23-26=let}}
+  // expected-note@-2{{unsafely mark 'wrapped' as concurrency-safe if all accesses are protected by an external synchronization mechanism}}{{16-16=nonisolated(unsafe) }}
+  // expected-note@-3{{restrict 'wrapped' to the main actor if it will only be accessed from the main thread}}{{3-3=@MainActor }}
 }
 
 public actor TestPublicActor {
diff --git a/test/Concurrency/predates_concurrency_import.swift b/test/Concurrency/predates_concurrency_import.swift
@@ -44,7 +44,8 @@ func test(
 let nonStrictGlobal = NonStrictClass() // no warning
 
 let strictGlobal = StrictStruct() // expected-warning{{let 'strictGlobal' is not concurrency-safe because non-'Sendable' type 'StrictStruct' may have shared mutable state}}
-// expected-note@-1{{isolate 'strictGlobal' to a global actor, or conform 'StrictStruct' to 'Sendable'}}
+// expected-note@-1{{restrict 'strictGlobal' to the main actor if it will only be accessed from the main thread}}
+// expected-note@-2{{unsafely mark 'strictGlobal' as concurrency-safe if all accesses are protected by an external synchronization mechanism}}
 
 extension NonStrictClass {
   @Sendable func f() { }
@@ -61,7 +62,8 @@ struct HasStatics {
   nonisolated static let ss: StrictStruct = StrictStruct()
   // expected-warning@-1{{'nonisolated' can not be applied to variable with non-'Sendable' type 'StrictStruct'}}
   // expected-warning@-2{{static property 'ss' is not concurrency-safe because non-'Sendable' type 'StrictStruct' may have shared mutable state}}
-  // expected-note@-3{{isolate 'ss' to a global actor, or conform 'StrictStruct' to 'Sendable'}}
+  // expected-note@-3{{restrict 'ss' to the main actor if it will only be accessed from the main thread}}
+  // expected-note@-4{{unsafely mark 'ss' as concurrency-safe if all accesses are protected by an external synchronization mechanism}}
 }
 
 extension NonStrictClass2: @retroactive MySendableProto { }
diff --git a/test/Concurrency/predates_concurrency_import_swift6.swift b/test/Concurrency/predates_concurrency_import_swift6.swift
@@ -18,7 +18,8 @@ func test(ss: StrictStruct, ns: NonStrictClass) {
 
 let nonStrictGlobal = NonStrictClass()
 let strictGlobal = StrictStruct() // expected-warning{{let 'strictGlobal' is not concurrency-safe because non-'Sendable' type 'StrictStruct' may have shared mutable state}}
-// expected-note@-1{{isolate 'strictGlobal' to a global actor, or conform 'StrictStruct' to 'Sendable'}}
+// expected-note@-1{{restrict 'strictGlobal' to the main actor if it will only be accessed from the main thread}}
+// expected-note@-2{{unsafely mark 'strictGlobal' as concurrency-safe if all accesses are protected by an external synchronization mechanism}}
 
 extension NonStrictClass {
   @Sendable func f() { }

Original file line number	Diff line number	Diff line change
`@@ -1,4 +1,5 @@`
`1`	`1`	`struct Foo {`
`2`	`2`	`static let member = Bar() // expected-complete-warning {{static property 'member' is not concurrency-safe because non-'Sendable' type 'Bar' may have shared mutable state; this is an error in the Swift 6 language mode}}`
`3`		`- // expected-complete-note@-1 {{isolate 'member' to a global actor, or conform 'Bar' to 'Sendable'}}`
	`3`	`+ // expected-complete-note@-1 {{restrict 'member' to the main actor if it will only be accessed from the main thread}}`
	`4`	`+ // expected-complete-note@-2{{unsafely mark 'member' as concurrency-safe if all accesses are protected by an external synchronization mechanism}}`
`4`	`5`	`}`