Merge pull request #73720 from DougGregor/preconcurrency-fixes-6.0

[6.0] Respect `@preconcurrency` everywhere we diagnose `Sendable` issues

Author: DougGregor

include/swift/AST/DiagnosticsSIL.def

@@ -692,9 +692,6 @@ ERROR(isolated_after_nonisolated, none,
 NOTE(nonisolated_blame, none, "after %1%2 %3, "
      "only non-isolated properties of 'self' can be accessed from "
      "%select{this init|a deinit}0", (bool, StringRef, StringRef, DeclName))
-ERROR(non_sendable_from_deinit,none,
-        "cannot access %1 %2 with a non-sendable type %0 from non-isolated deinit",
-        (Type, DescriptiveDeclKind, DeclName))
 ERROR(isolated_property_mutation_in_nonisolated_context,none,
       "actor-isolated %kind0 can not be %select{referenced|mutated}1 "
       "from a non-isolated context",

include/swift/AST/DiagnosticsSema.def

@@ -5481,8 +5481,8 @@ ERROR(sendable_isolated_sync_function,none,
       "%0 synchronous %kind1 cannot be marked as '@Sendable'",
       (ActorIsolation, const ValueDecl *))
 ERROR(nonsendable_instance_method,none,
-      "instance methods of non-Sendable types cannot be marked as '@Sendable'",
-      ())
+      "instance method of non-Sendable type %0 cannot be marked as '@Sendable'",
+      (Type))
 ERROR(concurrent_access_of_local_capture,none,
       "%select{mutation of|reference to}0 captured %kind1 in "
       "concurrently-executing code",
@@ -5575,9 +5575,9 @@ 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,
-      "%kind0 is not concurrency-safe because non-'Sendable' type %1 may have "
+      "%kind1 is not concurrency-safe because non-'Sendable' type %0 may have "
       "shared mutable state",
-      (const ValueDecl *, Type))
+      (Type, const ValueDecl *))
 NOTE(shared_immutable_state_decl_note,none,
      "isolate %0 to a global actor, or conform %1 to 'Sendable'",
      (const ValueDecl *, Type))
@@ -5757,6 +5757,10 @@ ERROR(nonisolated_wrapped_property,none,
       "'nonisolated' is not supported on properties with property wrappers",
       ())
 
+ERROR(non_sendable_from_deinit,none,
+        "cannot access %1 %2 with a non-sendable type %0 from non-isolated deinit",
+        (Type, DescriptiveDeclKind, DeclName))
+
 ERROR(actor_instance_property_wrapper,none,
       "%0 property in property wrapper type %1 cannot be isolated to "
       "the actor instance; consider 'nonisolated'",

include/swift/Sema/Concurrency.h

@@ -22,8 +22,10 @@
 
 namespace swift {
 
+class DeclContext;
 class SourceFile;
 class NominalTypeDecl;
+class VarDecl;
 
 /// If any of the imports in this source file was @preconcurrency but there were
 /// no diagnostics downgraded or suppressed due to that @preconcurrency, suggest
@@ -35,6 +37,13 @@ void diagnoseUnnecessaryPreconcurrencyImports(SourceFile &sf);
 bool hasExplicitSendableConformance(NominalTypeDecl *nominal,
                                     bool applyModuleDefault = true);
 
+/// Diagnose the use of an instance property of non-sendable type from an
+/// nonisolated deinitializer within an actor-isolated type.
+///
+/// \returns true iff a diagnostic was emitted for this reference.
+bool diagnoseNonSendableFromDeinit(
+    SourceLoc refLoc, VarDecl *var, DeclContext *dc);
+
 } // namespace swift
 
 #endif

lib/SILOptimizer/Mandatory/FlowIsolation.cpp

@@ -16,6 +16,7 @@
 #include "swift/AST/Expr.h"
 #include "swift/AST/ActorIsolation.h"
 #include "swift/AST/DiagnosticsSIL.h"
+#include "swift/Sema/Concurrency.h"
 #include "swift/SIL/ApplySite.h"
 #include "swift/SIL/BitDataflow.h"
 #include "swift/SIL/BasicBlockBits.h"
@@ -528,31 +529,17 @@ static bool onlyDeinitAccess(RefElementAddrInst *inst) {
 /// is happening in a deinit that uses flow-isolation.
 /// \returns true iff a diagnostic was emitted for this reference.
 static bool diagnoseNonSendableFromDeinit(RefElementAddrInst *inst) {
-  VarDecl *var = inst->getField();
-  Type ty = var->getTypeInContext();
-  DeclContext *dc = inst->getFunction()->getDeclContext();
+  auto dc = inst->getFunction()->getDeclContext();
 
-// FIXME: we should emit diagnostics in other modes using:
-//
-//  if (!shouldDiagnoseExistingDataRaces(dc))
-//    return false;
-//
-// but until we decide how we want to handle isolated state from deinits,
-// we're going to limit the noise to complete mode for now.
+  // For historical reasons, only diagnose this issue in strict mode.
   if (dc->getASTContext().LangOpts.StrictConcurrencyLevel
-      != StrictConcurrency::Complete)
-      return false;
-
-  if (ty->isSendableType())
+        != StrictConcurrency::Complete)
     return false;
 
-  auto &diag = var->getASTContext().Diags;
-
-  diag.diagnose(inst->getLoc().getSourceLoc(), diag::non_sendable_from_deinit,
-                ty, var->getDescriptiveKind(), var->getName())
-      .warnUntilSwiftVersion(6);
-
-  return true;
+  return swift::diagnoseNonSendableFromDeinit(
+      inst->getLoc().getSourceLoc(),
+      inst->getField(),
+      dc);
 }
 
 class OperandWorklist {

lib/Sema/TypeCheckAttr.cpp

@@ -6918,10 +6918,13 @@ void AttributeChecker::visitSendableAttr(SendableAttr *attr) {
   // Prevent Sendable Attr from being added to methods of non-sendable types
   if (auto *funcDecl = dyn_cast<AbstractFunctionDecl>(D)) {
     if (auto selfDecl = funcDecl->getImplicitSelfDecl()) {
-      if (!selfDecl->getTypeInContext()->isSendableType()) {
-        diagnose(attr->getLocation(), diag::nonsendable_instance_method)
-        .warnUntilSwiftVersion(6);
-      }
+      diagnoseIfAnyNonSendableTypes(
+          selfDecl->getTypeInContext(),
+          SendableCheckContext(funcDecl),
+          Type(),
+          SourceLoc(),
+          attr->getLocation(),
+          diag::nonsendable_instance_method);
     }
   }
 }
@@ -6945,13 +6948,16 @@ void AttributeChecker::visitNonisolatedAttr(NonisolatedAttr *attr) {
 
       // 'nonisolated' without '(unsafe)' is not allowed on non-Sendable variables.
       auto type = var->getTypeInContext();
-      if (!attr->isUnsafe() && !type->hasError() &&
-          !type->isSendableType()) {
-        Ctx.Diags.diagnose(attr->getLocation(),
-                           diag::nonisolated_non_sendable,
-                           type)
-          .warnUntilSwiftVersion(6);
-        return;
+      if (!attr->isUnsafe() && !type->hasError()) {
+        bool diagnosed = diagnoseIfAnyNonSendableTypes(
+            type,
+            SendableCheckContext(dc),
+            Type(),
+            SourceLoc(),
+            attr->getLocation(),
+            diag::nonisolated_non_sendable);
+        if (diagnosed)
+          return;
       }
 
       if (auto nominal = dyn_cast<NominalTypeDecl>(dc)) {

lib/Sema/TypeCheckConcurrency.cpp

@@ -511,7 +511,6 @@ static bool varIsSafeAcrossActors(const ModuleDecl *fromModule,
     return true;
 
   if (!var->isLet()) {
-    ASTContext &ctx = var->getASTContext();
     // A mutable storage of a value type accessed from within the module is
     // okay.
     if (dyn_cast_or_null<StructDecl>(var->getDeclContext()->getAsDecl()) &&
@@ -841,16 +840,25 @@ SendableCheckContext::preconcurrencyBehavior(Decl *decl) const {
     return std::nullopt;
 
   if (auto *nominal = dyn_cast<NominalTypeDecl>(decl)) {
-    // Determine whether this nominal type is visible via a @preconcurrency
-    // import.
-    auto import = nominal->findImport(fromDC);
-    auto sourceFile = fromDC->getParentSourceFile();
+    ModuleDecl *importedModule = nullptr;
+    if (nominal->getAttrs().hasAttribute<PreconcurrencyAttr>()) {
+      // If the declaration itself has the @preconcurrency attribute,
+      // respect it.
+      importedModule = nominal->getParentModule();
+    } else {
+      // Determine whether this nominal type is visible via a @preconcurrency
+      // import.
+      auto import = nominal->findImport(fromDC);
+      auto sourceFile = fromDC->getParentSourceFile();
 
-    if (!import || !import->options.contains(ImportFlags::Preconcurrency))
-      return std::nullopt;
+      if (!import || !import->options.contains(ImportFlags::Preconcurrency))
+        return std::nullopt;
 
-    if (sourceFile)
-      sourceFile->setImportUsedPreconcurrency(*import);
+      if (sourceFile)
+        sourceFile->setImportUsedPreconcurrency(*import);
+
+      importedModule = import->module.importedModule;
+    }
 
     // When the type is explicitly non-Sendable, @preconcurrency imports
     // downgrade the diagnostic to a warning in Swift 6.
@@ -859,7 +867,7 @@ SendableCheckContext::preconcurrencyBehavior(Decl *decl) const {
 
     // When the type is implicitly non-Sendable, `@preconcurrency` suppresses
     // diagnostics until the imported module enables Swift 6.
-    return import->module.importedModule->isConcurrencyChecked()
+    return importedModule->isConcurrencyChecked()
         ? DiagnosticBehavior::Warning
         : DiagnosticBehavior::Ignore;
   }
@@ -5011,13 +5019,17 @@ ActorIsolation ActorIsolationRequest::evaluate(
         }
         if (var->isLet()) {
           auto type = var->getInterfaceType();
-          if (!type->isSendableType()) {
-            diagVar->diagnose(diag::shared_immutable_state_decl,
-                              diagVar, type)
-                .warnUntilSwiftVersion(6);
+          bool 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);
@@ -5705,7 +5717,7 @@ static bool checkSendableInstanceStorage(
           /*inDerivedConformance*/Type(), element->getLoc(),
           [&](Type type, DiagnosticBehavior behavior) {
             auto preconcurrency =
-                context.preconcurrencyBehavior(elementType->getAnyNominal());
+                context.preconcurrencyBehavior(type->getAnyNominal());
             if (isImplicitSendableCheck(check)) {
               // If this is for an externally-visible conformance, fail.
               if (check == SendableCheck::ImplicitForExternallyVisible) {
@@ -5716,7 +5728,7 @@ static bool checkSendableInstanceStorage(
               // If we are to ignore this diagnostic, just continue.
               if (behavior == DiagnosticBehavior::Ignore ||
                   preconcurrency == DiagnosticBehavior::Ignore)
-                return false;
+                return true;
 
               invalid = true;
               return true;
@@ -6827,3 +6839,14 @@ ActorReferenceResult ActorReferenceResult::forReference(
 
   return forEntersActor(declIsolation, options);
 }
+
+bool swift::diagnoseNonSendableFromDeinit(
+    SourceLoc refLoc, VarDecl *var, DeclContext *dc) {
+  return diagnoseIfAnyNonSendableTypes(var->getTypeInContext(),
+                                SendableCheckContext(dc),
+                                Type(),
+                                SourceLoc(),
+                                refLoc,
+                                diag::non_sendable_from_deinit,
+                                var->getDescriptiveKind(), var->getName());
+}

lib/Sema/TypeCheckConcurrency.h

@@ -436,20 +436,64 @@ bool diagnoseNonSendableTypes(
     Diag<Type, DiagArgs...> diag,
     typename detail::Identity<DiagArgs>::type ...diagArgs) {
 
-    ASTContext &ctx = fromContext.fromDC->getASTContext();
-    return diagnoseNonSendableTypes(
-        type, fromContext, derivedConformance, typeLoc,
-        [&](Type specificType, DiagnosticBehavior behavior) {
-          auto preconcurrency =
-              fromContext.preconcurrencyBehavior(type->getAnyNominal());
+  ASTContext &ctx = fromContext.fromDC->getASTContext();
+  return diagnoseNonSendableTypes(
+      type, fromContext, derivedConformance, typeLoc,
+      [&](Type specificType, DiagnosticBehavior behavior) {
+        auto preconcurrency =
+          fromContext.preconcurrencyBehavior(specificType->getAnyNominal());
+
+        ctx.Diags.diagnose(diagnoseLoc, diag, type, diagArgs...)
+            .limitBehaviorUntilSwiftVersion(behavior, 6)
+            .limitBehaviorIf(preconcurrency);
+
+        return (behavior == DiagnosticBehavior::Ignore ||
+                preconcurrency == DiagnosticBehavior::Ignore);
+      });
+}
+
+/// Emit a diagnostic if there are any non-Sendable types for which
+/// the Sendable diagnostic wasn't suppressed. This diagnostic will
+/// only be emitted once, but there might be additional notes for the
+/// various occurrences of Sendable types.
+///
+/// \param typeLoc is the source location of the type being diagnosed
+///
+/// \param diagnoseLoc is the source location at which the main diagnostic should
+/// be reported, which can differ from typeLoc
+///
+/// \returns \c true if any diagnostics.
+template<typename ...DiagArgs>
+bool diagnoseIfAnyNonSendableTypes(
+    Type type, SendableCheckContext fromContext,
+    Type derivedConformance,
+    SourceLoc typeLoc, SourceLoc diagnoseLoc,
+    Diag<Type, DiagArgs...> diag,
+    typename detail::Identity<DiagArgs>::type ...diagArgs) {
+
+  ASTContext &ctx = fromContext.fromDC->getASTContext();
+  bool diagnosed = false;
+  diagnoseNonSendableTypes(
+      type, fromContext, derivedConformance, typeLoc,
+      [&](Type specificType, DiagnosticBehavior behavior) {
+        auto preconcurrency =
+          fromContext.preconcurrencyBehavior(specificType->getAnyNominal());
 
+        if (behavior == DiagnosticBehavior::Ignore ||
+            preconcurrency == DiagnosticBehavior::Ignore)
+          return true;
+
+        if (!diagnosed) {
           ctx.Diags.diagnose(diagnoseLoc, diag, type, diagArgs...)
               .limitBehaviorUntilSwiftVersion(behavior, 6)
               .limitBehaviorIf(preconcurrency);
+          diagnosed = true;
+        }
+
+        return false;
+      });
 
-          return (behavior == DiagnosticBehavior::Ignore ||
-                  preconcurrency == DiagnosticBehavior::Ignore);
-        });
+  return diagnosed;
 }
 
 /// Diagnose any non-Sendable types that occur within the given type, using

test/Concurrency/Inputs/NonStrictModule.swift

@@ -1,6 +1,7 @@
 public struct NonStrictStruct { }
 
 open class NonStrictClass {
+  public init() {}
   open func send(_ body: @Sendable () -> Void) {}
   open func dontSend(_ body: () -> Void) {}
 }

test/Concurrency/Inputs/StrictModule.swift

@@ -1,4 +1,6 @@
-public struct StrictStruct: Hashable { }
+public struct StrictStruct: Hashable {
+  public init() {}
+}
 
 open class StrictClass {
   open func send(_ body: @Sendable () -> Void) {}

test/Concurrency/concurrency_warnings.swift

@@ -4,7 +4,7 @@
 // REQUIRES: concurrency
 // REQUIRES: asserts
 
-class GlobalCounter {
+class GlobalCounter { // expected-note{{class 'GlobalCounter' does not conform to the 'Sendable' protocol}}
   var counter: Int = 0
 }
 

test/Concurrency/concurrent_value_checking.swift

@@ -215,7 +215,7 @@ func testUnsafeSendableInAsync() async {
 // ----------------------------------------------------------------------
 // Sendable restriction on key paths.
 // ----------------------------------------------------------------------
-class NC: Hashable { // expected-note 2{{class 'NC' does not conform to the 'Sendable' protocol}}
+class NC: Hashable { // expected-note 3{{class 'NC' does not conform to the 'Sendable' protocol}}
   func hash(into: inout Hasher) { }
   static func==(_: NC, _: NC) -> Bool { true }
 }

test/Concurrency/flow_isolation.swift

@@ -15,7 +15,7 @@ func takeNonSendable(_ ns: NonSendableType) {}
 @available(SwiftStdlib 5.1, *)
 func takeSendable(_ s: SendableType) {}
 
-class NonSendableType {
+class NonSendableType { // expected-note *{{class 'NonSendableType' does not conform to the 'Sendable' protocol}}
   var x: Int = 0
   func f() {}
 }

test/Concurrency/global_variables.swift

@@ -23,7 +23,7 @@ final class TestSendable: Sendable {
   init() {}
 }
 
-final class TestNonsendable {
+final class TestNonsendable { // expected-note 3{{class 'TestNonsendable' does not conform to the 'Sendable' protocol}}
   init() {}
 }