[NFC] Helper for @preconcurrency-sensitive errors

beccadax · beccadax · commit c5fde064ba0e · 2022-04-08T13:07:56.000-07:00
`diagnoseAsNonSendableBasedOn()` extracts just the “make a diagnostic sensitive to @preconcurrency import of this type” logic from `diagnoseNonSendableTypes()`. We need it because conformance and override checking should be sensitive to whether the conformed-to protocol/base class was imported with @preconcurrency or not, although this change is a pure refactoring.
diff --git a/lib/Sema/TypeCheckConcurrency.cpp b/lib/Sema/TypeCheckConcurrency.cpp
@@ -757,82 +757,107 @@ DiagnosticBehavior SendableCheckContext::diagnosticBehavior(
   return defaultBehavior;
 }
 
-/// Produce a diagnostic for a single instance of a non-Sendable type where
-/// a Sendable type is required.
-static bool diagnoseSingleNonSendableType(
-    Type type, SendableCheckContext fromContext, SourceLoc loc,
-    llvm::function_ref<bool(Type, DiagnosticBehavior)> diagnose) {
-
+bool swift::diagnoseSendabilityErrorBasedOn(
+    NominalTypeDecl *nominal, SendableCheckContext fromContext,
+    llvm::function_ref<bool(DiagnosticBehavior)> diagnose) {
   auto behavior = DiagnosticBehavior::Unspecified;
 
-  auto module = fromContext.fromDC->getParentModule();
-  ASTContext &ctx = module->getASTContext();
-  auto nominal = type->getAnyNominal();
   if (nominal) {
     behavior = fromContext.diagnosticBehavior(nominal);
   } else {
     behavior = fromContext.defaultDiagnosticBehavior();
   }
 
-  bool wasSuppressed = diagnose(type, behavior);
-
-  if (behavior == DiagnosticBehavior::Ignore || wasSuppressed) {
-    // Don't emit any other diagnostics.
-  } else if (type->is<FunctionType>()) {
-    ctx.Diags.diagnose(loc, diag::nonsendable_function_type);
-  } else if (nominal && nominal->getParentModule() == module) {
-    // If the nominal type is in the current module, suggest adding
-    // `Sendable` if it might make sense. Otherwise, just complain.
-    if (isa<StructDecl>(nominal) || isa<EnumDecl>(nominal)) {
-      auto note = nominal->diagnose(
-          diag::add_nominal_sendable_conformance,
-          nominal->getDescriptiveKind(), nominal->getName());
-      addSendableFixIt(nominal, note, /*unchecked=*/false);
-    } else {
-      nominal->diagnose(
-          diag::non_sendable_nominal, nominal->getDescriptiveKind(),
-          nominal->getName());
+  bool wasSuppressed = diagnose(behavior);
+
+  bool emittedDiagnostics =
+      behavior != DiagnosticBehavior::Ignore && !wasSuppressed;
+
+  // 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.
+  bool nominalIsImportedAndHasImplicitSendability =
+      nominal &&
+      nominal->getParentModule() != fromContext.fromDC->getParentModule() &&
+      !hasExplicitSendableConformance(nominal);
+
+  if (emittedDiagnostics && nominalIsImportedAndHasImplicitSendability) {
+    // 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);
     }
-  } else if (nominal) {
-    // Note which nominal type does not conform to `Sendable`.
-    nominal->diagnose(
-        diag::non_sendable_nominal, nominal->getDescriptiveKind(),
-        nominal->getName());
 
-    // 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;
+      ASTContext &ctx = nominal->getASTContext();
 
-      // 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 ");
+      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);
     }
   }
 
   return behavior == DiagnosticBehavior::Unspecified && !wasSuppressed;
 }
 
+/// Produce a diagnostic for a single instance of a non-Sendable type where
+/// a Sendable type is required.
+static bool diagnoseSingleNonSendableType(
+    Type type, SendableCheckContext fromContext, SourceLoc loc,
+    llvm::function_ref<bool(Type, DiagnosticBehavior)> diagnose) {
+
+  auto module = fromContext.fromDC->getParentModule();
+  auto nominal = type->getAnyNominal();
+
+  return diagnoseSendabilityErrorBasedOn(nominal, fromContext,
+                                         [&](DiagnosticBehavior behavior) {
+    bool wasSuppressed = diagnose(type, behavior);
+
+    // Don't emit the following notes if we didn't have any diagnostics to
+    // attach them to.
+    if (wasSuppressed || behavior == DiagnosticBehavior::Ignore)
+      return true;
+
+    if (type->is<FunctionType>()) {
+      module->getASTContext().Diags
+          .diagnose(loc, diag::nonsendable_function_type);
+    } else if (nominal && nominal->getParentModule() == module) {
+      // If the nominal type is in the current module, suggest adding
+      // `Sendable` if it might make sense. Otherwise, just complain.
+      if (isa<StructDecl>(nominal) || isa<EnumDecl>(nominal)) {
+        auto note = nominal->diagnose(
+            diag::add_nominal_sendable_conformance,
+            nominal->getDescriptiveKind(), nominal->getName());
+        addSendableFixIt(nominal, note, /*unchecked=*/false);
+      } else {
+        nominal->diagnose(
+            diag::non_sendable_nominal, nominal->getDescriptiveKind(),
+            nominal->getName());
+      }
+    } else if (nominal) {
+      // Note which nominal type does not conform to `Sendable`.
+      nominal->diagnose(
+          diag::non_sendable_nominal, nominal->getDescriptiveKind(),
+          nominal->getName());
+    }
+
+    return false;
+  });
+}
+
 bool swift::diagnoseNonSendableTypes(
     Type type, SendableCheckContext fromContext, SourceLoc loc,
     llvm::function_ref<bool(Type, DiagnosticBehavior)> diagnose) {
diff --git a/lib/Sema/TypeCheckConcurrency.h b/lib/Sema/TypeCheckConcurrency.h
@@ -353,9 +353,10 @@ struct SendableCheckContext {
 ///
 /// \param diagnose Emit a diagnostic indicating that the current type
 /// is non-Sendable, with the suggested behavior limitation. Returns \c true
-/// if an error was produced.
+/// if it did not emit any diagnostics.
 ///
-/// \returns \c true if any diagnostics were produced, \c false otherwise.
+/// \returns \c true if any errors were produced, \c false if no diagnostics or
+/// only warnings and notes were produced.
 bool diagnoseNonSendableTypes(
     Type type, SendableCheckContext fromContext, SourceLoc loc,
     llvm::function_ref<bool(Type, DiagnosticBehavior)> diagnose);
@@ -370,7 +371,8 @@ namespace detail {
 /// Diagnose any non-Sendable types that occur within the given type, using
 /// the given diagnostic.
 ///
-/// \returns \c true if any errors were produced, \c false otherwise.
+/// \returns \c true if any errors were produced, \c false if no diagnostics or
+/// only warnings and notes were produced.
 template<typename ...DiagArgs>
 bool diagnoseNonSendableTypes(
     Type type, SendableCheckContext fromContext, SourceLoc loc,
@@ -389,6 +391,26 @@ bool diagnoseNonSendableTypes(
   });
 }
 
+/// Diagnose this sendability error with behavior based on the import of
+/// \p nominal . For instance, depending on how \p nominal is imported into
+/// \p fromContext , the diagnostic behavior limitation may be lower or the
+/// compiler might emit a fix-it adding \c \@preconcurrency to the \c import .
+///
+/// \param nominal The declaration whose import we should check, or \c nullptr
+/// to get behavior for a non-nominal type.
+///
+/// \param fromContext The context where the error will be emitted.
+///
+/// \param diagnose Emit a diagnostic indicating a sendability problem,
+/// with the suggested behavior limitation. Returns \c true
+/// if it did \em not emit any diagnostics.
+///
+/// \returns \c true if any errors were produced, \c false if no diagnostics or
+/// only warnings and notes were produced.
+bool diagnoseSendabilityErrorBasedOn(
+    NominalTypeDecl *nominal, SendableCheckContext fromContext,
+    llvm::function_ref<bool(DiagnosticBehavior)> diagnose);
+
 /// Given a set of custom attributes, pick out the global actor attributes
 /// and perform any necessary resolution and diagnostics, returning the
 /// global actor attribute and type it refers to (or \c None).
