Merge pull request #72078 from simanerush/redundant-nonisolated-unsafe

[Concurrency] Diagnose a redundant `nonisolated(unsafe)`

Author: hborla

include/swift/AST/DiagnosticsSema.def

@@ -5696,6 +5696,18 @@ NOTE(nonisolated_mutable_storage_note,none,
      "convert %0 to a 'let' constant or consider declaring it "
      "'nonisolated(unsafe)' if manually managing concurrency safety",
      (const VarDecl *))
+WARNING(nonisolated_unsafe_sendable_constant,none,
+     "'nonisolated(unsafe)' is unnecessary for a constant with 'Sendable' type "
+     "%0, consider removing it",
+     (Type))
+WARNING(unsafe_sendable_actor_constant,none,
+     "'(unsafe)' is unnecessary for a constant public actor property with "
+     "'Sendable' type %0, consider removing it",
+     (Type))
+WARNING(nonisolated_unsafe_sendable_actor_constant,none,
+     "'nonisolated(unsafe)' is unnecessary for a constant actor-isolated "
+     "property with 'Sendable' type %0, consider removing it",
+     (Type))
 ERROR(nonisolated_non_sendable,none,
       "'nonisolated' can not be applied to variable with non-'Sendable' "
       "type %0",

lib/Sema/TypeCheckAttr.cpp

@@ -6921,6 +6921,40 @@ void AttributeChecker::visitNonisolatedAttr(NonisolatedAttr *attr) {
           return;
         }
       }
+
+      // 'nonisolated(unsafe)' is redundant for 'Sendable' immutables.
+      if (attr->isUnsafe() &&
+          type->isSendableType() &&
+          var->isLet()) {
+
+        // '(unsafe)' is redundant for a public actor-isolated 'Sendable'
+        // immutable.
+        auto nominal = dyn_cast<NominalTypeDecl>(dc);
+        if (nominal && nominal->isActor()) {
+          auto access = nominal->getFormalAccessScope(
+              /*useDC=*/nullptr,
+              /*treatUsableFromInlineAsPublic=*/true);
+          if (access.isPublic()) {
+            // Get the location where '(unsafe)' starts.
+            SourceLoc unsafeStart = Lexer::getLocForEndOfToken(
+                Ctx.SourceMgr, attr->getRange().Start);
+            diagnose(unsafeStart, diag::unsafe_sendable_actor_constant, type)
+                .fixItRemoveChars(unsafeStart,
+                                  attr->getRange().End.getAdvancedLoc(1));
+          } else {
+            // This actor is not public, so suggest to remove
+            // 'nonisolated(unsafe)'.
+            diagnose(attr->getLocation(),
+                     diag::nonisolated_unsafe_sendable_actor_constant, type)
+                .fixItRemove(attr->getRange());
+          }
+
+        } else {
+          diagnose(attr->getLocation(),
+                   diag::nonisolated_unsafe_sendable_constant, type)
+              .fixItRemove(attr->getRange());
+        }
+      }
     }
 
     // Using 'nonisolated' with wrapped properties is unsupported, because

test/Concurrency/global_variables.swift

@@ -37,6 +37,12 @@ public struct TestWrapper {
   }
 }
 
+// https://github.com/apple/swift/issues/71546
+actor TestActor {
+  nonisolated(unsafe) let immutableActorIsolated = TestSendable()
+  // expected-warning@-1 {{'nonisolated(unsafe)' is unnecessary for a constant actor-isolated property with 'Sendable' type 'TestSendable', consider removing it}} {{3-23=}}
+}
+
 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}}
@@ -45,6 +51,8 @@ struct TestStatics {
   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-error@-2 {{'nonisolated' can not be applied to variable with non-'Sendable' type 'TestNonsendable'}}
+  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'}}
@@ -54,6 +62,11 @@ struct TestStatics {
   // expected-note@-1{{isolate 'wrapped' to a global actor, or convert it to a 'let' constant and conform it to 'Sendable'}}
 }
 
+public actor TestPublicActor {
+  nonisolated(unsafe) let immutableNonisolatedUnsafeSendable = TestSendable()
+  // expected-warning@-1 {{'(unsafe)' is unnecessary for a constant public actor property with 'Sendable' type 'TestSendable', consider removing it}} {{14-22=}}
+}
+
 @TestGlobalActor
 func f() {
   print(TestStatics.immutableExplicitSendable)
@@ -63,6 +76,9 @@ func f() {
 }
 
 func testLocalNonisolatedUnsafe() async {
+  nonisolated(unsafe) let immutable = 1
+  // expected-warning@-1{{'nonisolated(unsafe)' is unnecessary for a constant with 'Sendable' type 'Int', consider removing it}} {{3-23=}}
+  // expected-warning@-2{{initialization of immutable value 'immutable' was never used; consider replacing with assignment to '_' or removing it}}
   nonisolated(unsafe) var value = 1
   let task = Task {
     value = 2