Don’t recommend @preconcurrency on audited imports

If a type is explicitly non-Sendable, we don’t want to recommend softening errors with `@preconcurrency import` (even thought that would still work), because these probably represent *real* errors that you ought to fix. And at the extreme end of this, if a module is built with `-warn-concurrency` or `-swift-version 6`, *all* of its types should be treated as though they have explicit sendability, since we assume the entire module has been audited.

(Eventually, we plan to do this at module build time by actually serializing out unavailable conformances rather than by looking at the settings used to build the module, but we aren’t there quite yet.)

Implement this and test it by checking that the preconcurrency sendable tests never recommend adding `@preconcurrency` to an import of `StrictModule`, even when we do the same illegal things with it that cause that remark to be printed for `NonStrictModule`.

Author: beccadax

lib/Sema/TypeCheckConcurrency.cpp

@@ -770,10 +770,17 @@ DiagnosticBehavior SendableCheckContext::defaultDiagnosticBehavior() const {
   return defaultSendableDiagnosticBehavior(fromDC->getASTContext().LangOpts);
 }
 
-/// Determine whether the given nominal type that is within the current module
-/// has an explicit Sendable.
-static bool hasExplicitSendableConformance(NominalTypeDecl *nominal) {
+/// Determine whether the given nominal type has an explicit Sendable
+/// conformance (regardless of its availability).
+static bool hasExplicitSendableConformance(NominalTypeDecl *nominal,
+                                           bool applyModuleDefault = true) {
   ASTContext &ctx = nominal->getASTContext();
+  auto nominalModule = nominal->getParentModule();
+
+  // In a concurrency-checked module, a missing conformance is equivalent to
+  // an explicitly unavailable one. If we want to apply this rule, do so now.
+  if (applyModuleDefault && nominalModule->isConcurrencyChecked())
+    return true;
 
   // Look for any conformance to `Sendable`.
   auto proto = ctx.getProtocol(KnownProtocolKind::Sendable);
@@ -782,7 +789,7 @@ static bool hasExplicitSendableConformance(NominalTypeDecl *nominal) {
 
   // Look for a conformance. If it's present and not (directly) missing,
   // we're done.
-  auto conformance = nominal->getParentModule()->lookupConformance(
+  auto conformance = nominalModule->lookupConformance(
       nominal->getDeclaredInterfaceType(), proto, /*allowMissing=*/true);
   return conformance &&
       !(isa<BuiltinProtocolConformance>(conformance.getConcrete()) &&
@@ -826,18 +833,13 @@ static Optional<AttributedImport<ImportedModule>> findImportFor(
 /// nominal type.
 DiagnosticBehavior SendableCheckContext::diagnosticBehavior(
     NominalTypeDecl *nominal) const {
-  // Determine whether the type was explicitly non-Sendable.
-  auto nominalModule = nominal->getParentModule();
-  bool isExplicitlyNonSendable = nominalModule->isConcurrencyChecked() ||
-      hasExplicitSendableConformance(nominal);
-
   // Determine whether this nominal type is visible via a @preconcurrency
   // import.
   auto import = findImportFor(nominal, fromDC);
+  auto sourceFile = fromDC->getParentSourceFile();
 
   // When the type is explicitly non-Sendable...
-  auto sourceFile = fromDC->getParentSourceFile();
-  if (isExplicitlyNonSendable) {
+  if (hasExplicitSendableConformance(nominal)) {
     // @preconcurrency imports downgrade the diagnostic to a warning in Swift 6,
     if (import && import->options.contains(ImportFlags::Preconcurrency)) {
       if (sourceFile)
@@ -857,7 +859,7 @@ DiagnosticBehavior SendableCheckContext::diagnosticBehavior(
     if (sourceFile)
       sourceFile->setImportUsedPreconcurrency(*import);
 
-    return nominalModule->getASTContext().LangOpts.isSwiftVersionAtLeast(6)
+    return nominal->getASTContext().LangOpts.isSwiftVersionAtLeast(6)
         ? DiagnosticBehavior::Warning
         : DiagnosticBehavior::Ignore;
   }
@@ -917,29 +919,34 @@ static bool diagnoseSingleNonSendableType(
         diag::non_sendable_nominal, nominal->getDescriptiveKind(),
         nominal->getName());
 
-    // This type was imported from another module; try to find the
-    // corresponding import.
-    Optional<AttributedImport<swift::ImportedModule>> import;
-    SourceFile *sourceFile = fromContext.fromDC->getParentSourceFile();
-    if (sourceFile) {
-      import = findImportFor(nominal, fromContext.fromDC);
-    }
+    // When the type is explicitly Sendable *or* explicitly non-Sendable, we
+    // assume it has been audited and `@preconcurrency` is not recommended even
+    // though it would actually affect the diagnostic.
+    if (!hasExplicitSendableConformance(nominal)) {
+      // This type was imported from another module; try to find the
+      // corresponding import.
+      Optional<AttributedImport<swift::ImportedModule>> import;
+      SourceFile *sourceFile = fromContext.fromDC->getParentSourceFile();
+      if (sourceFile) {
+        import = findImportFor(nominal, fromContext.fromDC);
+      }
 
-    // If we found the import that makes this nominal type visible, remark
-    // that it can be @preconcurrency import.
-    // Only emit this remark once per source file, because it can happen a
-    // lot.
-    if (import && !import->options.contains(ImportFlags::Preconcurrency) &&
-        import->importLoc.isValid() && sourceFile &&
-        !sourceFile->hasImportUsedPreconcurrency(*import)) {
-      SourceLoc importLoc = import->importLoc;
-      ctx.Diags.diagnose(
-          importLoc, diag::add_predates_concurrency_import,
-          ctx.LangOpts.isSwiftVersionAtLeast(6),
-          nominal->getParentModule()->getName())
-        .fixItInsert(importLoc, "@preconcurrency ");
+      // If we found the import that makes this nominal type visible, remark
+      // that it can be @preconcurrency import.
+      // Only emit this remark once per source file, because it can happen a
+      // lot.
+      if (import && !import->options.contains(ImportFlags::Preconcurrency) &&
+          import->importLoc.isValid() && sourceFile &&
+          !sourceFile->hasImportUsedPreconcurrency(*import)) {
+        SourceLoc importLoc = import->importLoc;
+        ctx.Diags.diagnose(
+            importLoc, diag::add_predates_concurrency_import,
+            ctx.LangOpts.isSwiftVersionAtLeast(6),
+            nominal->getParentModule()->getName())
+          .fixItInsert(importLoc, "@preconcurrency ");
 
-      sourceFile->setImportUsedPreconcurrency(*import);
+        sourceFile->setImportUsedPreconcurrency(*import);
+      }
     }
   }
 
@@ -1156,7 +1163,7 @@ void swift::diagnoseMissingExplicitSendable(NominalTypeDecl *nominal) {
     return;
 
   // If the conformance is explicitly stated, do nothing.
-  if (hasExplicitSendableConformance(nominal))
+  if (hasExplicitSendableConformance(nominal, /*applyModuleDefault=*/false))
     return;
 
   // Diagnose it.

test/Concurrency/Inputs/StrictModule.swift

@@ -1 +1,3 @@
 public struct StrictStruct: Hashable { }
+
+open class StrictClass { }

test/Concurrency/sendable_preconcurrency.swift

@@ -5,7 +5,7 @@
 
 // REQUIRES: concurrency
 
-import StrictModule
+import StrictModule // no remark: we never recommend @preconcurrency due to an explicitly non-Sendable (via -warn-concurrency) type
 @preconcurrency import NonStrictModule
 
 actor A {
@@ -27,12 +27,14 @@ struct MyType3 {
   var nsc: NonStrictClass
 }
 
-func testA(ns: NS, mt: MyType, mt2: MyType2, mt3: MyType3) async {
+func testA(ns: NS, mt: MyType, mt2: MyType2, mt3: MyType3, sc: StrictClass, nsc: NonStrictClass) async {
   Task {
     print(ns) // expected-warning{{capture of 'ns' with non-sendable type 'NS' in a `@Sendable` closure}}
     print(mt) // no warning: MyType is Sendable because we suppressed NonStrictClass's warning
     print(mt2)
     print(mt3)
+    print(sc) // expected-warning {{capture of 'sc' with non-sendable type 'StrictClass' in a `@Sendable` closure}}
+    print(nsc)
   }
 }
 

test/Concurrency/sendable_without_preconcurrency.swift

@@ -5,7 +5,7 @@
 
 // REQUIRES: concurrency
 
-import StrictModule
+import StrictModule // no remark: we never recommend @preconcurrency due to an explicitly non-Sendable (via -warn-concurrency) type
 import NonStrictModule
 
 actor A {
@@ -23,11 +23,12 @@ struct MyType2 { // expected-note{{consider making struct 'MyType2' conform to t
   var ns: NS
 }
 
-func testA(ns: NS, mt: MyType, mt2: MyType2) async {
+func testA(ns: NS, mt: MyType, mt2: MyType2, sc: StrictClass, nsc: NonStrictClass) async {
   Task {
     print(ns) // expected-warning{{capture of 'ns' with non-sendable type 'NS' in a `@Sendable` closure}}
     print(mt) // no warning: MyType is Sendable because we suppressed NonStrictClass's warning
     print(mt2) // expected-warning{{capture of 'mt2' with non-sendable type 'MyType2' in a `@Sendable` closure}}
+    print(sc) // expected-warning{{capture of 'sc' with non-sendable type 'StrictClass' in a `@Sendable` closure}}
   }
 }
 

test/Concurrency/sendable_without_preconcurrency_2.swift

@@ -5,7 +5,7 @@
 
 // REQUIRES: concurrency
 
-import StrictModule
+import StrictModule // no remark: we never recommend @preconcurrency due to an explicitly non-Sendable (via -warn-concurrency) type
 import NonStrictModule // expected-remark{{add '@preconcurrency' to suppress 'Sendable'-related warnings from module 'NonStrictModule'}}
 
 actor A {
@@ -23,11 +23,13 @@ struct MyType2: Sendable {
   var ns: NS // expected-warning{{stored property 'ns' of 'Sendable'-conforming struct 'MyType2' has non-sendable type 'NS'}}
 }
 
-func testA(ns: NS, mt: MyType, mt2: MyType2) async {
+func testA(ns: NS, mt: MyType, mt2: MyType2, sc: StrictClass, nsc: NonStrictClass) async {
   Task {
     print(ns) // expected-warning{{capture of 'ns' with non-sendable type 'NS' in a `@Sendable` closure}}
     print(mt) // no warning: MyType is Sendable because we suppressed NonStrictClass's warning
     print(mt2)
+    print(sc) // expected-warning {{capture of 'sc' with non-sendable type 'StrictClass' in a `@Sendable` closure}}
+    print(nsc) // expected-warning {{capture of 'nsc' with non-sendable type 'NonStrictClass' in a `@Sendable` closure}}
   }
 }