Suggest @_predatesConcurrency import to suppress diagnostics.

DougGregor · DougGregor · commit 23cc638ccc82 · 2021-12-21T11:24:56.000-08:00
When diagnosing a `Sendable` violation, suggest the use of
`@_predatesConcurrency` on the corresponding import to suppress those
diagnostics.
diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def
@@ -1952,6 +1952,10 @@ NOTE(non_sendable_nominal,none,
 NOTE(add_nominal_sendable_conformance,none,
      "consider making %0 %1 conform to the 'Sendable' protocol",
      (DescriptiveDeclKind, DeclName))
+NOTE(add_predates_concurrency_import,none,
+     "add '@_predatesConcurrency' to %select{suppress|treat}0 "
+     "'Sendable'-related %select{warnings|errors}0 from module %1"
+     "%select{| as warnings}0", (bool, Identifier))
 WARNING(public_decl_needs_sendable,none,
         "public %0 %1 does not specify whether it is 'Sendable' or not",
         (DescriptiveDeclKind, DeclName))
diff --git a/include/swift/AST/Import.h b/include/swift/AST/Import.h
@@ -537,6 +537,9 @@ struct AttributedImport {
   /// Information about the module and access path being imported.
   ModuleInfo module;
 
+  /// The location of the 'import' keyword, for an explicit import.
+  SourceLoc importLoc;
+
   /// Flags indicating which attributes of this import are present.
   ImportOptions options;
 
@@ -551,19 +554,20 @@ struct AttributedImport {
   /// is the source range covering the annotation.
   SourceRange predatesConcurrencyRange;
 
-  AttributedImport(ModuleInfo module, ImportOptions options = ImportOptions(),
+  AttributedImport(ModuleInfo module, SourceLoc importLoc = SourceLoc(),
+                   ImportOptions options = ImportOptions(),
                    StringRef filename = {}, ArrayRef<Identifier> spiGroups = {})
-      : module(module), options(options), sourceFileArg(filename),
-        spiGroups(spiGroups) {
+      : module(module), importLoc(importLoc), options(options),
+        sourceFileArg(filename), spiGroups(spiGroups) {
     assert(!(options.contains(ImportFlags::Exported) &&
              options.contains(ImportFlags::ImplementationOnly)) ||
            options.contains(ImportFlags::Reserved));
   }
 
   template<class OtherModuleInfo>
   AttributedImport(ModuleInfo module, AttributedImport<OtherModuleInfo> other)
-    : AttributedImport(module, other.options, other.sourceFileArg,
-                       other.spiGroups) { }
+    : AttributedImport(module, other.importLoc, other.options,
+                       other.sourceFileArg, other.spiGroups) { }
 
   friend bool operator==(const AttributedImport<ModuleInfo> &lhs,
                          const AttributedImport<ModuleInfo> &rhs) {
@@ -713,17 +717,20 @@ struct DenseMapInfo<swift::AttributedImport<ModuleInfo>> {
   using ModuleInfoDMI = DenseMapInfo<ModuleInfo>;
   using ImportOptionsDMI = DenseMapInfo<swift::ImportOptions>;
   using StringRefDMI = DenseMapInfo<StringRef>;
+  using SourceLocDMI = DenseMapInfo<swift::SourceLoc>;
   // We can't include spiGroups in the hash because ArrayRef<Identifier> is not
   // DenseMapInfo-able, but we do check that the spiGroups match in isEqual().
 
   static inline AttributedImport getEmptyKey() {
     return AttributedImport(ModuleInfoDMI::getEmptyKey(),
+                            SourceLocDMI::getEmptyKey(),
                             ImportOptionsDMI::getEmptyKey(),
                             StringRefDMI::getEmptyKey(),
                             {});
   }
   static inline AttributedImport getTombstoneKey() {
     return AttributedImport(ModuleInfoDMI::getTombstoneKey(),
+                            SourceLocDMI::getEmptyKey(),
                             ImportOptionsDMI::getTombstoneKey(),
                             StringRefDMI::getTombstoneKey(),
                             {});
@@ -732,12 +739,15 @@ struct DenseMapInfo<swift::AttributedImport<ModuleInfo>> {
     return detail::combineHashValue(
         ModuleInfoDMI::getHashValue(import.module),
         detail::combineHashValue(
-            ImportOptionsDMI::getHashValue(import.options),
-            StringRefDMI::getHashValue(import.sourceFileArg)));
+            SourceLocDMI::getHashValue(import.importLoc),
+            detail::combineHashValue(
+              ImportOptionsDMI::getHashValue(import.options),
+              StringRefDMI::getHashValue(import.sourceFileArg))));
   }
   static bool isEqual(const AttributedImport &a,
                       const AttributedImport &b) {
     return ModuleInfoDMI::isEqual(a.module, b.module) &&
+           SourceLocDMI::isEqual(a.importLoc, b.importLoc),
            ImportOptionsDMI::isEqual(a.options, b.options) &&
            StringRefDMI::isEqual(a.sourceFileArg, b.sourceFileArg) &&
            a.spiGroups == b.spiGroups;
diff --git a/lib/Frontend/Frontend.cpp b/lib/Frontend/Frontend.cpp
@@ -881,7 +881,8 @@ ImplicitImportInfo CompilerInstance::getImplicitImportInfo() const {
     ImportPath::Builder importPath(Context->getIdentifier(moduleStr));
     UnloadedImportedModule import(importPath.copyTo(*Context),
                                   /*isScoped=*/false);
-    imports.AdditionalUnloadedImports.emplace_back(import, options);
+    imports.AdditionalUnloadedImports.emplace_back(
+        import, SourceLoc(), options);
   };
 
   for (auto &moduleStrAndTestable : frontendOpts.getImplicitImportModuleNames()) {
diff --git a/lib/Sema/ImportResolution.cpp b/lib/Sema/ImportResolution.cpp
@@ -56,9 +56,6 @@ struct UnboundImport {
   /// determine the behavior expected for this import.
   AttributedImport<UnloadedImportedModule> import;
 
-  /// The source location to use when diagnosing errors for this import.
-  SourceLoc importLoc;
-
   /// If this UnboundImport directly represents an ImportDecl, contains the
   /// ImportDecl it represents. This should only be used for diagnostics and
   /// for updating the AST; if you want to read information about the import,
@@ -480,7 +477,8 @@ ModuleImplicitImportsRequest::evaluate(Evaluator &evaluator,
       !clangImporter->importBridgingHeader(bridgingHeaderPath, module)) {
     auto *headerModule = clangImporter->getImportedHeaderModule();
     assert(headerModule && "Didn't load bridging header?");
-    imports.emplace_back(ImportedModule(headerModule), ImportFlags::Exported);
+    imports.emplace_back(
+        ImportedModule(headerModule), SourceLoc(), ImportFlags::Exported);
   }
 
   // Implicitly import the underlying Clang half of this module if needed.
@@ -490,7 +488,7 @@ ModuleImplicitImportsRequest::evaluate(Evaluator &evaluator,
     ImportPath::Builder importPath(module->getName());
     unloadedImports.emplace_back(UnloadedImportedModule(importPath.copyTo(ctx),
                                                         /*isScoped=*/false),
-                                 ImportFlags::Exported);
+                                 SourceLoc(), ImportFlags::Exported);
   }
 
   return { ctx.AllocateCopy(imports), ctx.AllocateCopy(unloadedImports) };
@@ -513,7 +511,7 @@ void ImportResolver::addImplicitImports() {
 }
 
 UnboundImport::UnboundImport(AttributedImport<UnloadedImportedModule> implicit)
-  : import(implicit), importLoc(),
+  : import(implicit),
     importOrUnderlyingModuleDecl(static_cast<ImportDecl *>(nullptr)) {}
 
 //===----------------------------------------------------------------------===//
@@ -523,8 +521,8 @@ UnboundImport::UnboundImport(AttributedImport<UnloadedImportedModule> implicit)
 /// Create an UnboundImport for a user-written import declaration.
 UnboundImport::UnboundImport(ImportDecl *ID)
   : import(UnloadedImportedModule(ID->getImportPath(), ID->getImportKind()),
-           {}),
-    importLoc(ID->getLoc()), importOrUnderlyingModuleDecl(ID)
+           ID->getStartLoc(), {}),
+    importOrUnderlyingModuleDecl(ID)
 {
   if (ID->isExported())
     import.options |= ImportFlags::Exported;
@@ -571,10 +569,11 @@ bool UnboundImport::checkNotTautological(const SourceFile &SF) {
 
   StringRef filename = llvm::sys::path::filename(SF.getFilename());
   if (filename.empty())
-    ctx.Diags.diagnose(importLoc, diag::sema_import_current_module,
+    ctx.Diags.diagnose(import.importLoc, diag::sema_import_current_module,
                        modulePath.front().Item);
   else
-    ctx.Diags.diagnose(importLoc, diag::sema_import_current_module_with_file,
+    ctx.Diags.diagnose(import.importLoc,
+                       diag::sema_import_current_module_with_file,
                        filename, modulePath.front().Item);
 
   return false;
@@ -584,7 +583,7 @@ bool UnboundImport::checkModuleLoaded(ModuleDecl *M, SourceFile &SF) {
   if (M)
     return true;
 
-  diagnoseNoSuchModule(SF.getParentModule(), importLoc,
+  diagnoseNoSuchModule(SF.getParentModule(), import.importLoc,
                        import.module.getModulePath(), /*nonfatalInREPL=*/true);
   return false;
 }
@@ -963,9 +962,9 @@ UnboundImport::UnboundImport(
     ASTContext &ctx, const UnboundImport &base, Identifier overlayName,
     const AttributedImport<ImportedModule> &declaringImport,
     const AttributedImport<ImportedModule> &bystandingImport)
-    : import(makeUnimportedCrossImportOverlay(ctx, overlayName, base,
-                                              declaringImport), {}),
-      importLoc(base.importLoc),
+    : import(makeUnimportedCrossImportOverlay(
+                 ctx, overlayName, base, declaringImport),
+                 base.import.importLoc, {}),
       importOrUnderlyingModuleDecl(declaringImport.module.importedModule)
 {
   // A cross-import is never private or testable, and never comes from a private
@@ -1099,15 +1098,16 @@ void ImportResolver::findCrossImports(
   // Find modules we need to import.
   SmallVector<Identifier, 4> names;
   declaringImport.module.importedModule->findDeclaredCrossImportOverlays(
-      bystandingImport.module.importedModule->getName(), names, I.importLoc);
+      bystandingImport.module.importedModule->getName(), names,
+      I.import.importLoc);
 
   // If we're diagnosing cases where we cross-import in both directions, get the
   // inverse list. Otherwise, leave the list empty.
   SmallVector<Identifier, 4> oppositeNames;
   if (shouldDiagnoseRedundantCrossImports)
     bystandingImport.module.importedModule->findDeclaredCrossImportOverlays(
         declaringImport.module.importedModule->getName(), oppositeNames,
-        I.importLoc);
+        I.import.importLoc);
 
   if (ctx.Stats && !names.empty())
     ++ctx.Stats->getFrontendCounters().NumCrossImportsFound;
@@ -1124,13 +1124,14 @@ void ImportResolver::findCrossImports(
         declaringImport, bystandingImport);
 
     if (llvm::is_contained(oppositeNames, name))
-      ctx.Diags.diagnose(I.importLoc, diag::cross_imported_by_both_modules,
+      ctx.Diags.diagnose(I.import.importLoc,
+                         diag::cross_imported_by_both_modules,
                          declaringImport.module.importedModule->getName(),
                          bystandingImport.module.importedModule->getName(),
                          name);
 
     if (ctx.LangOpts.EnableCrossImportRemarks)
-      ctx.Diags.diagnose(I.importLoc, diag::cross_import_added,
+      ctx.Diags.diagnose(I.import.importLoc, diag::cross_import_added,
                          declaringImport.module.importedModule->getName(),
                          bystandingImport.module.importedModule->getName(),
                          name);
diff --git a/lib/Sema/TypeCheckConcurrency.cpp b/lib/Sema/TypeCheckConcurrency.cpp
@@ -756,16 +756,37 @@ static bool diagnoseSingleNonSendableType(
     // 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 &&
-             (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 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());
+
+    // If we can find an import in this context that makes this nominal
+    // type visible, note that it can be `@_predatesConcurrency` import.
+    auto import = findImportFor(nominal, fromContext.fromDC);
+    if (import && !import->options.contains(ImportFlags::PredatesConcurrency) &&
+        import->importLoc.isValid()) {
+      SourceLoc importLoc = import->importLoc;
+      ctx.Diags.diagnose(
+          importLoc, diag::add_predates_concurrency_import,
+          ctx.LangOpts.isSwiftVersionAtLeast(6),
+          nominal->getParentModule()->getName())
+        .fixItInsert(importLoc, "@_predatesConcurrency ");
+    }
   }
 
   return behavior == DiagnosticBehavior::Unspecified && !wasSuppressed;
diff --git a/test/Concurrency/actor_isolation.swift b/test/Concurrency/actor_isolation.swift
@@ -3,7 +3,7 @@
 // RUN: %target-typecheck-verify-swift -I %t  -disable-availability-checking -warn-concurrency
 // REQUIRES: concurrency
 
-import OtherActors
+import OtherActors // expected-note 2{{add '@_predatesConcurrency' to suppress 'Sendable'-related warnings from module 'OtherActors'}}{{1-1=@_predatesConcurrency }}
 
 let immutableGlobal: String = "hello"
 var mutableGlobal: String = "can't touch this" // expected-note 5{{var declared here}}
