Diagnose references to nonisolated(unsafe) declarations in strict-concurrency code

DougGregor · DougGregor · commit 683dcfca54a8 · 2024-12-19T15:50:57.000-08:00
A nonisolated(unsafe) declaration clearly indicates that the
declaration itself is unsafe, so it doesn't need to be diagnosted.
Instead, diagnose any reference to such a declaration that occurs
when strict concurrency is enabled. Make this a collatable unsafe use.
diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def
@@ -8081,6 +8081,9 @@ NOTE(note_reference_to_unsafe_decl,none,
 NOTE(note_reference_to_unsafe_typed_decl,none,
      "%select{reference|call}0 to %kind1 involves unsafe type %2",
      (bool, const ValueDecl *, Type))
+NOTE(note_reference_to_nonisolated_unsafe,none,
+     "reference to nonisolated(unsafe) %kind0 is unsafe in concurrently-executing code",
+     (const ValueDecl *))
 
 GROUPED_WARNING(override_safe_withunsafe,Unsafe,none,
       "override of safe %0 with unsafe %0", (DescriptiveDeclKind))
@@ -8094,8 +8097,9 @@ GROUPED_WARNING(unchecked_conformance_is_unsafe,Unsafe,none,
       "@unchecked conformance involves unsafe code", ())
 GROUPED_WARNING(unowned_unsafe_is_unsafe,Unsafe,none,
       "unowned(unsafe) involves unsafe code", ())
-GROUPED_WARNING(nonisolated_unsafe_is_unsafe,Unsafe,none,
-      "nonisolated(unsafe) involves unsafe code", ())
+GROUPED_WARNING(reference_to_nonisolated_unsafe,Unsafe,none,
+      "reference to nonisolated(unsafe) %kind0 is unsafe in concurrently-executing code",
+       (const ValueDecl *))
 GROUPED_WARNING(reference_to_unsafe_decl,Unsafe,none,
       "%select{reference|call}0 to unsafe %kindbase1",
       (bool, const ValueDecl *))
diff --git a/lib/Sema/TypeCheckAttr.cpp b/lib/Sema/TypeCheckAttr.cpp
@@ -7199,15 +7199,6 @@ void AttributeChecker::visitNonisolatedAttr(NonisolatedAttr *attr) {
   // that do not have storage.
   auto dc = D->getDeclContext();
 
-  // nonisolated(unsafe) is unsafe, but only under strict concurrency.
-  if (attr->isUnsafe() &&
-      Ctx.LangOpts.hasFeature(Feature::WarnUnsafe) &&
-      Ctx.LangOpts.StrictConcurrencyLevel == StrictConcurrency::Complete &&
-      !D->allowsUnsafe()) {
-    diagnoseUnsafeUse(
-        UnsafeUse::forNonisolatedUnsafe(D, attr->getLocation(), dc));
-  }
-
   if (auto var = dyn_cast<VarDecl>(D)) {
     // stored properties have limitations as to when they can be nonisolated.
     auto type = var->getTypeInContext();
diff --git a/lib/Sema/TypeCheckAvailability.cpp b/lib/Sema/TypeCheckAvailability.cpp
@@ -4185,6 +4185,20 @@ diagnoseDeclAsyncAvailability(const ValueDecl *D, SourceRange R,
   return true;
 }
 
+/// Determine whether a reference to the given variable is treated as
+/// nonisolated(unsafe).
+static bool isReferenceToNonisolatedUnsafe(
+    ValueDecl *decl,
+    DeclContext *fromDC
+) {
+  auto isolation = getActorIsolationForReference(decl, fromDC);
+  if (!isolation.isNonisolated())
+    return false;
+
+  auto attr = decl->getAttrs().getAttribute<NonisolatedAttr>();
+  return attr && attr->isUnsafe();
+}
+
 /// Diagnose uses of unsafe declarations.
 static void
 diagnoseDeclUnsafe(const ValueDecl *D, SourceRange R,
@@ -4193,16 +4207,29 @@ diagnoseDeclUnsafe(const ValueDecl *D, SourceRange R,
   if (!ctx.LangOpts.hasFeature(Feature::WarnUnsafe))
     return;
 
-  if (!D->isUnsafe())
-    return;
-
   if (Where.getAvailability().allowsUnsafe())
     return;
 
   SourceLoc diagLoc = call ? call->getLoc() : R.Start;
-  diagnoseUnsafeUse(
-      UnsafeUse::forReferenceToUnsafe(
-        D, call != nullptr, Where.getDeclContext(), Type(), diagLoc));
+  if (D->isUnsafe()) {
+    diagnoseUnsafeUse(
+        UnsafeUse::forReferenceToUnsafe(
+          D, call != nullptr, Where.getDeclContext(), Type(), diagLoc));
+    return;
+  }
+
+  if (auto valueDecl = dyn_cast<ValueDecl>(D)) {
+    // Use of a nonisolated(unsafe) declaration is unsafe, but is only
+    // diagnosed as such under strict concurrency.
+    if (ctx.LangOpts.StrictConcurrencyLevel == StrictConcurrency::Complete &&
+        isReferenceToNonisolatedUnsafe(const_cast<ValueDecl *>(valueDecl),
+                                       Where.getDeclContext())) {
+      diagnoseUnsafeUse(
+          UnsafeUse::forNonisolatedUnsafe(
+          valueDecl, R.Start, Where.getDeclContext()));
+      return;
+    }
+  }
 }
 
 /// Diagnose uses of unavailable declarations. Returns true if a diagnostic
diff --git a/lib/Sema/TypeCheckConcurrency.cpp b/lib/Sema/TypeCheckConcurrency.cpp
@@ -1645,9 +1645,6 @@ ReferencedActor ReferencedActor::forGlobalActor(VarDecl *actor,
   return ReferencedActor(actor, isPotentiallyIsolated, kind, globalActor);
 }
 
-static ActorIsolation getActorIsolationForReference(
-    ValueDecl *decl, const DeclContext *fromDC);
-
 bool ReferencedActor::isKnownToBeLocal() const {
   switch (kind) {
   case GlobalActor:
@@ -7159,7 +7156,7 @@ bool swift::isPotentiallyIsolatedActor(
 
 /// Determine the actor isolation used when we are referencing the given
 /// declaration.
-static ActorIsolation getActorIsolationForReference(ValueDecl *decl,
+ActorIsolation swift::getActorIsolationForReference(ValueDecl *decl,
                                                     const DeclContext *fromDC) {
   auto declIsolation = getActorIsolation(decl);
 
diff --git a/lib/Sema/TypeCheckConcurrency.h b/lib/Sema/TypeCheckConcurrency.h
@@ -564,6 +564,11 @@ checkGlobalActorAttributes(SourceLoc loc, DeclContext *dc,
 /// Get the explicit global actor specified for a closure.
 Type getExplicitGlobalActor(ClosureExpr *closure);
 
+/// Determine the actor isolation used when we are referencing the given
+/// declaration.
+ActorIsolation getActorIsolationForReference(ValueDecl *decl,
+                                             const DeclContext *fromDC);
+
 /// Adjust the type of the variable for concurrency.
 Type adjustVarTypeForConcurrency(
     Type type, VarDecl *var, DeclContext *dc,
diff --git a/lib/Sema/TypeCheckUnsafe.cpp b/lib/Sema/TypeCheckUnsafe.cpp
@@ -40,12 +40,12 @@ static bool isUnsafeUseInDefinition(const UnsafeUse &use) {
   case UnsafeUse::TypeWitness:
   case UnsafeUse::UnsafeConformance:
   case UnsafeUse::UnownedUnsafe:
-  case UnsafeUse::NonisolatedUnsafe:
     // Never part of the definition. These are always part of the interface.
     return false;
 
   case UnsafeUse::ReferenceToUnsafe:
   case UnsafeUse::CallToUnsafe:
+  case UnsafeUse::NonisolatedUnsafe:
     return enclosingContextForUnsafe(use).second;
   }
 }
@@ -99,7 +99,8 @@ void swift::diagnoseUnsafeUse(const UnsafeUse &use, bool asNote) {
     // It will be diagnosed later, along with all other unsafe uses within this
     // same declaration.
     if (use.getKind() == UnsafeUse::ReferenceToUnsafe ||
-        use.getKind() == UnsafeUse::CallToUnsafe) {
+        use.getKind() == UnsafeUse::CallToUnsafe ||
+        use.getKind() == UnsafeUse::NonisolatedUnsafe) {
       auto [enclosingDecl, _] = enclosingContextForUnsafe(
           use.getLocation(), use.getDeclContext());
       if (enclosingDecl) {
@@ -172,10 +173,16 @@ void swift::diagnoseUnsafeUse(const UnsafeUse &use, bool asNote) {
   case UnsafeUse::NonisolatedUnsafe: {
     auto decl = use.getDecl();
     ASTContext &ctx = decl->getASTContext();
-    ctx.Diags.diagnose(use.getLocation(), diag::nonisolated_unsafe_is_unsafe);
-    if (auto var = dyn_cast<VarDecl>(decl)) {
-      var->diagnose(diag::make_enclosing_context_unsafe, var)
-        .fixItInsert(var->getAttributeInsertionLoc(false), "@unsafe ");
+    ctx.Diags.diagnose(
+        use.getLocation(),
+        asNote ? diag::note_reference_to_nonisolated_unsafe
+               : diag::note_reference_to_nonisolated_unsafe,
+        cast<ValueDecl>(decl));
+    if (!asNote) {
+      if (auto var = dyn_cast<VarDecl>(decl)) {
+        var->diagnose(diag::make_enclosing_context_unsafe, var)
+          .fixItInsert(var->getAttributeInsertionLoc(false), "@unsafe ");
+      }
     }
     return;
   }
diff --git a/test/Unsafe/unsafe_concurrency.swift b/test/Unsafe/unsafe_concurrency.swift
@@ -1,4 +1,4 @@
-// RUN: %target-typecheck-verify-swift -enable-experimental-feature AllowUnsafeAttribute -enable-experimental-feature WarnUnsafe -enable-experimental-feature StrictConcurrency
+// RUN: %target-typecheck-verify-swift -enable-experimental-feature WarnUnsafe -enable-experimental-feature StrictConcurrency
 
 // REQUIRES: concurrency
 // REQUIRES: swift_feature_AllowUnsafeAttribute
@@ -11,15 +11,15 @@ class C: @unchecked Sendable {
   var counter: Int = 0
 }
 
+nonisolated(unsafe) var globalCounter = 0
+
 @available(SwiftStdlib 5.1, *)
-func f() async {
-  // expected-warning@+2{{nonisolated(unsafe) involves unsafe code}}
-  // expected-note@+1{{make var 'counter' @unsafe to indicate that its use is not memory-safe}}
+func f() async { // expected-warning{{global function 'f' involves unsafe code; use '@safe(unchecked)' to assert that the code is memory-safe}}
   nonisolated(unsafe) var counter = 0
   Task.detached {
-    counter += 1
+    counter += 1 // expected-note{{reference to nonisolated(unsafe) var 'counter' is unsafe in concurrently-executing code}}
   }
-  counter += 1
-  print(counter)
+  counter += 1 // expected-note{{reference to nonisolated(unsafe) var 'counter' is unsafe in concurrently-executing code}}
+  print(counter) // expected-note{{reference to nonisolated(unsafe) var 'counter' is unsafe in concurrently-executing code}}
+  print(globalCounter) // expected-note{{reference to nonisolated(unsafe) var 'globalCounter' is unsafe in concurrently-executing code}}
 }
-
