Diagnose @preconcurrency imports as a strict safety issue

@preconcurrency imports disable Sendable checking, which can lead to
data races that undermine memory safety. Diagnose such imports, and
require `@safe(unchecked)` to suppress the diagnostic.

Author: DougGregor

include/swift/AST/DiagnosticsSema.def

@@ -8119,6 +8119,10 @@ GROUPED_WARNING(reference_to_unsafe_decl,Unsafe,none,
 GROUPED_WARNING(reference_to_unsafe_typed_decl,Unsafe,none,
       "%select{reference|call}0 to %kindbase1 involves unsafe type %2",
       (bool, const ValueDecl *, Type))
+GROUPED_WARNING(preconcurrency_import_unsafe,Unsafe,none,
+      "@preconcurrency import is not memory-safe because it can silently "
+      "introduce data races; use '@safe(unchecked)' to assert that the "
+      "code is memory-safe", ())
 NOTE(unsafe_decl_here,none,
      "unsafe %kindbase0 declared here", (const ValueDecl *))
 NOTE(encapsulate_unsafe_in_enclosing_context,none,

include/swift/AST/UnsafeUse.h

@@ -45,6 +45,8 @@ class UnsafeUse {
     ReferenceToUnsafe,
     /// A call to an unsafe declaration.
     CallToUnsafe,
+    /// A @preconcurrency import.
+    PreconcurrencyImport,
   };
 
 private:
@@ -77,6 +79,8 @@ class UnsafeUse {
       TypeBase *type;
       const void *location;
     } entity;
+
+    const ImportDecl *importDecl;
   } storage;
 
   UnsafeUse(Kind kind) : kind(kind) { }
@@ -173,6 +177,12 @@ class UnsafeUse {
                         decl, type, location);
   }
 
+  static UnsafeUse forPreconcurrencyImport(const ImportDecl *importDecl) {
+    UnsafeUse result(PreconcurrencyImport);
+    result.storage.importDecl = importDecl;
+    return result;
+  }
+
   Kind getKind() const { return kind; }
 
   /// The location at which this use will be diagnosed.
@@ -198,6 +208,9 @@ class UnsafeUse {
     case CallToUnsafe:
       return SourceLoc(
           llvm::SMLoc::getFromPointer((const char *)storage.entity.location));
+
+    case PreconcurrencyImport:
+      return storage.importDecl->getLoc();
     }
   }
 
@@ -219,6 +232,9 @@ class UnsafeUse {
 
     case UnsafeConformance:
       return nullptr;
+
+    case PreconcurrencyImport:
+      return storage.importDecl;
     }
   }
 
@@ -246,6 +262,9 @@ class UnsafeUse {
 
     case UnsafeConformance:
       return storage.conformance.declContext;
+
+    case PreconcurrencyImport:
+      return storage.importDecl->getDeclContext();
     }
   }
 
@@ -264,6 +283,7 @@ class UnsafeUse {
     case ReferenceToUnsafe:
     case CallToUnsafe:
     case UnsafeConformance:
+    case PreconcurrencyImport:
       return nullptr;
     }
   }
@@ -273,6 +293,7 @@ class UnsafeUse {
     switch (getKind()) {
     case Override:
     case Witness:
+    case PreconcurrencyImport:
       return nullptr;
 
     case UnsafeConformance:
@@ -307,6 +328,7 @@ class UnsafeUse {
     case NonisolatedUnsafe:
     case ReferenceToUnsafe:
     case CallToUnsafe:
+    case PreconcurrencyImport:
       return ProtocolConformanceRef::forInvalid();
     }
   }

lib/Sema/TypeCheckDeclPrimary.cpp

@@ -54,6 +54,7 @@
 #include "swift/AST/TypeCheckRequests.h"
 #include "swift/AST/TypeDifferenceVisitor.h"
 #include "swift/AST/TypeWalker.h"
+#include "swift/AST/UnsafeUse.h"
 #include "swift/Basic/Assertions.h"
 #include "swift/Basic/Defer.h"
 #include "swift/Basic/Statistic.h"
@@ -2462,6 +2463,15 @@ class DeclChecker : public DeclVisitor<DeclChecker> {
           inFlight.limitBehavior(DiagnosticBehavior::Warning);
       }
     }
+
+    // Preconcurrency imports aren't strictly memory-safe when we have strict
+    // concurrency checking enabled.
+    if (ID->preconcurrency() &&
+        Ctx.LangOpts.StrictConcurrencyLevel == StrictConcurrency::Complete &&
+        Ctx.LangOpts.hasFeature(Feature::WarnUnsafe) && !
+        ID->getAttrs().hasAttribute<SafeAttr>()) {
+      diagnoseUnsafeUse(UnsafeUse::forPreconcurrencyImport(ID));
+    }
   }
 
   void visitOperatorDecl(OperatorDecl *OD) {

lib/Sema/TypeCheckUnsafe.cpp

@@ -39,6 +39,7 @@ static bool isUnsafeUseInDefinition(const UnsafeUse &use) {
   case UnsafeUse::Override:
   case UnsafeUse::Witness:
   case UnsafeUse::TypeWitness:
+    case UnsafeUse::PreconcurrencyImport:
     // Never part of the definition. These are always part of the interface.
     return false;
 
@@ -88,48 +89,6 @@ static void suggestUnsafeMarkerOnConformance(
   ).fixItInsert(decl->getAttributeInsertionLoc(false), "@unsafe ");
 }
 
-/// Enumerate all of the unsafe conformances within this conformance,
-/// returning `true` and aborting the search if any callback returns `true`.
-static bool forEachUnsafeConformance(
-    ProtocolConformanceRef conformance,
-    llvm::function_ref<bool(ProtocolConformance *conformance)> fn) {
-  if (conformance.isInvalid() || conformance.isAbstract())
-    return false;
-
-  if (conformance.isPack()) {
-    for (auto packedConformance :
-             conformance.getPack()->getPatternConformances()) {
-      if (forEachUnsafeConformance(packedConformance, fn))
-        return true;
-    }
-
-    return false;
-  }
-
-  // Is this an unsafe conformance?
-  ProtocolConformance *concreteConf = conformance.getConcrete();
-  RootProtocolConformance *rootConf = concreteConf->getRootConformance();
-  if (auto normalConf = dyn_cast<NormalProtocolConformance>(rootConf)) {
-    // @unchecked Sendable conformances are considered unsafe when complete
-    // checking is enabled.
-    if (normalConf->isUnchecked() &&
-        normalConf->getProtocol()->isSpecificProtocol(KnownProtocolKind::Sendable) &&
-        normalConf->getProtocol()->getASTContext()
-          .LangOpts.StrictConcurrencyLevel == StrictConcurrency::Complete)
-      if (fn(concreteConf))
-        return true;
-  }
-
-  // Check conformances that are part of this conformance.
-  auto subMap = concreteConf->getSubstitutionMap();
-  for (auto conformance : subMap.getConformances()) {
-    if (forEachUnsafeConformance(conformance, fn))
-      return true;
-  }
-
-  return false;
-}
-
 /// Retrieve the extra information
 static SourceFileExtras *getSourceFileExtrasFor(const Decl *decl) {
   auto dc = decl->getDeclContext();
@@ -280,6 +239,14 @@ void swift::diagnoseUnsafeUse(const UnsafeUse &use, bool asNote) {
 
     return;
   }
+
+  case UnsafeUse::PreconcurrencyImport: {
+    auto importDecl = cast<ImportDecl>(use.getDecl());
+    importDecl->diagnose(diag::preconcurrency_import_unsafe)
+      .fixItInsert(importDecl->getAttributeInsertionLoc(false),
+                   "@safe(unchecked) ");
+    return;
+  }
   }
 }
 

test/Unsafe/unsafe_concurrency.swift

@@ -1,9 +1,14 @@
-// RUN: %target-typecheck-verify-swift -enable-experimental-feature WarnUnsafe -enable-experimental-feature StrictConcurrency
+// RUN: %empty-directory(%t)
+// RUN: %target-swift-frontend -emit-module-path %t/unsafe_swift_decls.swiftmodule %S/Inputs/unsafe_swift_decls.swift -enable-experimental-feature AllowUnsafeAttribute
+
+// RUN: %target-typecheck-verify-swift -enable-experimental-feature WarnUnsafe -enable-experimental-feature StrictConcurrency -I %t
 
 // REQUIRES: concurrency
 // REQUIRES: swift_feature_StrictConcurrency
 // REQUIRES: swift_feature_WarnUnsafe
 
+@preconcurrency import unsafe_swift_decls // expected-warning{{@preconcurrency import is not memory-safe because it can silently introduce data races; use '@safe(unchecked)' to assert that the code is memory-safe}}{{1-1=@safe(unchecked) }}
+
 class C: @unchecked Sendable {
   var counter: Int = 0
 }