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

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.

Author: DougGregor

include/swift/AST/DiagnosticsSema.def

@@ -5610,16 +5610,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",

lib/Sema/MiscDiagnostics.cpp

@@ -3779,6 +3779,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() {
@@ -3998,25 +4020,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()) {

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 {

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"
@@ -5019,23 +5020,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) ");
         }
       }
     }

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}}
 }

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, *)

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}}

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.

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 {

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}}