Merge pull request #73739 from DougGregor/global-var-concurrency-fixits

DougGregor · web-flow · commit 6eb42dff9fd5 · 2024-05-19T18:33:01.000-07:00
Provide more Fix-It guidance for concurrency-unsafe global variables (SE-0412)
diff --git a/include/swift/AST/DiagnosticsSema.def b/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",
diff --git a/lib/Sema/MiscDiagnostics.cpp b/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()) {
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"
@@ -5030,23 +5031,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`	`}`