Merge pull request #40650 from DougGregor/predates-concurrency-import

DougGregor · web-flow · commit d8a3123efdbd · 2021-12-21T21:00:26.000-08:00
diff --git a/include/swift/AST/Attr.def b/include/swift/AST/Attr.def
@@ -690,7 +690,7 @@ SIMPLE_DECL_ATTR(_noAllocation, NoAllocation,
 
 SIMPLE_DECL_ATTR(_predatesConcurrency, PredatesConcurrency,
   OnFunc | OnConstructor | OnProtocol | OnGenericType | OnVar | OnSubscript |
-  OnEnumElement | UserInaccessible |
+  OnEnumElement | OnImport | UserInaccessible |
   ABIBreakingToAdd | ABIBreakingToRemove | APIBreakingToAdd | APIBreakingToRemove,
   125)
 
diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def
@@ -1952,6 +1952,12 @@ NOTE(non_sendable_nominal,none,
 NOTE(add_nominal_sendable_conformance,none,
      "consider making %0 %1 conform to the 'Sendable' protocol",
      (DescriptiveDeclKind, DeclName))
+REMARK(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))
+REMARK(remove_predates_concurrency_import,none,
+     "'@_predatesConcurrency' attribute on module %0 is unused", (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
@@ -80,6 +80,10 @@ enum class ImportFlags {
   /// implementation detail of this file.
   SPIAccessControl = 0x10,
 
+  /// The module is imported assuming that the module itself predates
+  /// concurrency.
+  PredatesConcurrency = 0x20,
+
   /// Used for DenseMap.
   Reserved = 0x80
 };
@@ -533,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;
 
@@ -543,19 +550,27 @@ struct AttributedImport {
   /// Names of explicitly imported SPI groups.
   ArrayRef<Identifier> spiGroups;
 
-  AttributedImport(ModuleInfo module, ImportOptions options = ImportOptions(),
-                   StringRef filename = {}, ArrayRef<Identifier> spiGroups = {})
-      : module(module), options(options), sourceFileArg(filename),
-        spiGroups(spiGroups) {
+  /// When the import declaration has a `@_predatesConcurrency` annotation, this
+  /// is the source range covering the annotation.
+  SourceRange predatesConcurrencyRange;
+
+  AttributedImport(ModuleInfo module, SourceLoc importLoc = SourceLoc(),
+                   ImportOptions options = ImportOptions(),
+                   StringRef filename = {}, ArrayRef<Identifier> spiGroups = {},
+                   SourceRange predatesConcurrencyRange = {})
+      : module(module), importLoc(importLoc), options(options),
+        sourceFileArg(filename), spiGroups(spiGroups),
+        predatesConcurrencyRange(predatesConcurrencyRange) {
     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,
+                       other.predatesConcurrencyRange) { }
 
   friend bool operator==(const AttributedImport<ModuleInfo> &lhs,
                          const AttributedImport<ModuleInfo> &rhs) {
@@ -705,17 +720,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(),
                             {});
@@ -724,8 +742,8 @@ struct DenseMapInfo<swift::AttributedImport<ModuleInfo>> {
     return detail::combineHashValue(
         ModuleInfoDMI::getHashValue(import.module),
         detail::combineHashValue(
-            ImportOptionsDMI::getHashValue(import.options),
-            StringRefDMI::getHashValue(import.sourceFileArg)));
+          ImportOptionsDMI::getHashValue(import.options),
+          StringRefDMI::getHashValue(import.sourceFileArg)));
   }
   static bool isEqual(const AttributedImport &a,
                       const AttributedImport &b) {
diff --git a/include/swift/AST/SourceFile.h b/include/swift/AST/SourceFile.h
@@ -84,6 +84,10 @@ class SourceFile final : public FileUnit {
   /// This is \c None until it is filled in by the import resolution phase.
   Optional<ArrayRef<AttributedImport<ImportedModule>>> Imports;
 
+  /// Which imports have made use of @_predatesConcurrency.
+  llvm::SmallDenseSet<AttributedImport<ImportedModule>>
+      PredatesConcurrencyImportsUsed;
+
   /// A unique identifier representing this file; used to mark private decls
   /// within the file to keep them from conflicting with other files in the
   /// same module.
@@ -293,6 +297,14 @@ class SourceFile final : public FileUnit {
   /// resolution.
   void setImports(ArrayRef<AttributedImport<ImportedModule>> imports);
 
+  /// Whether the given import has used @_predatesConcurrency.
+  bool hasImportUsedPredatesConcurrency(
+      AttributedImport<ImportedModule> import) const;
+
+  /// Note that the given import has used @_predatesConcurrency/
+  void setImportUsedPredatesConcurrency(
+      AttributedImport<ImportedModule> import);
+
   enum ImportQueryKind {
     /// Return the results for testable or private imports.
     TestableAndPrivate,
diff --git a/lib/AST/Module.cpp b/lib/AST/Module.cpp
@@ -2240,6 +2240,16 @@ SourceFile::setImports(ArrayRef<AttributedImport<ImportedModule>> imports) {
   Imports = getASTContext().AllocateCopy(imports);
 }
 
+bool SourceFile::hasImportUsedPredatesConcurrency(
+    AttributedImport<ImportedModule> import) const {
+  return PredatesConcurrencyImportsUsed.count(import) != 0;
+}
+
+void SourceFile::setImportUsedPredatesConcurrency(
+    AttributedImport<ImportedModule> import) {
+  PredatesConcurrencyImportsUsed.insert(import);
+}
+
 bool HasImplementationOnlyImportsRequest::evaluate(Evaluator &evaluator,
                                                    SourceFile *SF) const {
   return llvm::any_of(SF->getImports(),
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
@@ -480,7 +480,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 +491,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) };
@@ -523,7 +524,7 @@ UnboundImport::UnboundImport(AttributedImport<UnloadedImportedModule> implicit)
 /// Create an UnboundImport for a user-written import declaration.
 UnboundImport::UnboundImport(ImportDecl *ID)
   : import(UnloadedImportedModule(ID->getImportPath(), ID->getImportKind()),
-           {}),
+           ID->getStartLoc(), {}),
     importLoc(ID->getLoc()), importOrUnderlyingModuleDecl(ID)
 {
   if (ID->isExported())
@@ -548,6 +549,11 @@ UnboundImport::UnboundImport(ImportDecl *ID)
     spiGroups.append(attrSPIs.begin(), attrSPIs.end());
   }
   import.spiGroups = ID->getASTContext().AllocateCopy(spiGroups);
+
+  if (auto attr = ID->getAttrs().getAttribute<PredatesConcurrencyAttr>()) {
+    import.options |= ImportFlags::PredatesConcurrency;
+    import.predatesConcurrencyRange = attr->getRangeWithAt();
+  }
 }
 
 bool UnboundImport::checkNotTautological(const SourceFile &SF) {
diff --git a/lib/Sema/TypeCheckConcurrency.cpp b/lib/Sema/TypeCheckConcurrency.cpp
@@ -20,6 +20,7 @@
 #include "swift/Strings.h"
 #include "swift/AST/ASTWalker.h"
 #include "swift/AST/GenericEnvironment.h"
+#include "swift/AST/ImportCache.h"
 #include "swift/AST/Initializer.h"
 #include "swift/AST/ParameterList.h"
 #include "swift/AST/ProtocolConformance.h"
@@ -674,6 +675,38 @@ static bool hasExplicitSendableConformance(NominalTypeDecl *nominal) {
           conformance.getConcrete())->isMissing());
 }
 
+/// Find the import that makes the given nominal declaration available.
+static Optional<AttributedImport<ImportedModule>> findImportFor(
+    NominalTypeDecl *nominal, const DeclContext *fromDC) {
+  // If the nominal type is from the current module, there's no import.
+  auto nominalModule = nominal->getParentModule();
+  if (nominalModule == fromDC->getParentModule())
+    return None;
+
+  auto fromSourceFile = fromDC->getParentSourceFile();
+  if (!fromSourceFile)
+    return None;
+
+  // Look to see if the owning module was directly imported.
+  for (const auto &import : fromSourceFile->getImports()) {
+    if (import.module.importedModule == nominalModule)
+      return import;
+  }
+
+  // Now look for transitive imports.
+  auto &importCache = nominal->getASTContext().getImportCache();
+  for (const auto &import : fromSourceFile->getImports()) {
+    auto &importSet = importCache.getImportSet(import.module.importedModule);
+    for (const auto &transitive : importSet.getTransitiveImports()) {
+      if (transitive.importedModule == nominalModule) {
+        return import;
+      }
+    }
+  }
+
+  return None;
+}
+
 /// Determine the diagnostic behavior for a Sendable reference to the given
 /// nominal type.
 DiagnosticBehavior SendableCheckContext::diagnosticBehavior(
@@ -686,12 +719,12 @@ DiagnosticBehavior SendableCheckContext::diagnosticBehavior(
 
   // Determine whether this nominal type is visible via a @_predatesConcurrency
   // import.
-  ImportDecl *predatesConcurrencyImport = nullptr;
+  auto import = findImportFor(nominal, fromDC);
 
   // When the type is explicitly non-Sendable...
   if (isExplicitlyNonSendable) {
     // @_predatesConcurrency imports downgrade the diagnostic to a warning.
-    if (predatesConcurrencyImport) {
+    if (import && import->options.contains(ImportFlags::PredatesConcurrency)) {
       // FIXME: Note that this @_predatesConcurrency import was "used".
       return DiagnosticBehavior::Warning;
     }
@@ -701,10 +734,13 @@ DiagnosticBehavior SendableCheckContext::diagnosticBehavior(
 
   // When the type is implicitly non-Sendable...
 
-  // @_predatesConcurrency always suppresses the diagnostic.
-  if (predatesConcurrencyImport) {
+  // @_predatesConcurrency suppresses the diagnostic in Swift 5.x, and
+  // downgrades it to a warning in Swift 6 and later.
+  if (import && import->options.contains(ImportFlags::PredatesConcurrency)) {
     // FIXME: Note that this @_predatesConcurrency import was "used".
-    return DiagnosticBehavior::Ignore;
+    return nominalModule->getASTContext().LangOpts.isSwiftVersionAtLeast(6)
+        ? DiagnosticBehavior::Warning
+        : DiagnosticBehavior::Ignore;
   }
 
   return defaultDiagnosticBehavior();
@@ -729,20 +765,61 @@ static bool diagnoseSingleNonSendableType(
 
   bool wasSuppressed = diagnose(type, behavior);
 
+  // If this type was imported from another module, try to find the
+  // corresponding import.
+  Optional<AttributedImport<swift::ImportedModule>> import;
+  SourceFile *sourceFile = fromContext.fromDC->getParentSourceFile();
+  if (nominal && nominal->getParentModule() != module) {
+    import = findImportFor(nominal, fromContext.fromDC);
+  }
+
   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 &&
-             (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 found the import that makes this nominal type visible, remark
+    // that it can be @_predatesConcurrency import.
+    // Only emit this remark once per source file, because it can happen a
+    // lot.
+    if (import && !import->options.contains(ImportFlags::PredatesConcurrency) &&
+        import->importLoc.isValid() && sourceFile &&
+        !sourceFile->hasImportUsedPredatesConcurrency(*import)) {
+      SourceLoc importLoc = import->importLoc;
+      ctx.Diags.diagnose(
+          importLoc, diag::add_predates_concurrency_import,
+          ctx.LangOpts.isSwiftVersionAtLeast(6),
+          nominal->getParentModule()->getName())
+        .fixItInsert(importLoc, "@_predatesConcurrency ");
+
+      sourceFile->setImportUsedPredatesConcurrency(*import);
+    }
+  }
+
+  // If we found an import that makes this nominal type visible, and that
+  // was a @_predatesConcurrency import, note that we have made use of the
+  // attribute.
+  if (import && import->options.contains(ImportFlags::PredatesConcurrency) &&
+      sourceFile) {
+    sourceFile->setImportUsedPredatesConcurrency(*import);
   }
 
   return behavior == DiagnosticBehavior::Unspecified && !wasSuppressed;
diff --git a/lib/Sema/TypeChecker.cpp b/lib/Sema/TypeChecker.cpp
@@ -264,6 +264,23 @@ void swift::performTypeChecking(SourceFile &SF) {
                                  TypeCheckSourceFileRequest{&SF}, {});
 }
 
+/// If any of the imports in this source file was @_predatesConcurrency but
+/// there were no diagnostics downgraded or suppressed due to that
+/// @_predatesConcurrency, suggest that the attribute be removed.
+static void diagnoseUnnecessaryPredatesConcurrencyImports(SourceFile &sf) {
+  ASTContext &ctx = sf.getASTContext();
+  for (const auto &import : sf.getImports()) {
+    if (import.options.contains(ImportFlags::PredatesConcurrency) &&
+        import.importLoc.isValid() &&
+        !sf.hasImportUsedPredatesConcurrency(import)) {
+      ctx.Diags.diagnose(
+          import.importLoc, diag::remove_predates_concurrency_import,
+          import.module.importedModule->getName())
+        .fixItRemove(import.predatesConcurrencyRange);
+    }
+  }
+}
+
 evaluator::SideEffect
 TypeCheckSourceFileRequest::evaluate(Evaluator &eval, SourceFile *SF) const {
   assert(SF && "Source file cannot be null!");
@@ -305,6 +322,8 @@ TypeCheckSourceFileRequest::evaluate(Evaluator &eval, SourceFile *SF) const {
     typeCheckDelayedFunctions(*SF);
   }
 
+  diagnoseUnnecessaryPredatesConcurrencyImports(*SF);
+
   // Check to see if there's any inconsistent @_implementationOnly imports.
   evaluateOrDefault(
       Ctx.evaluator,
diff --git a/test/ClangImporter/objc_async.swift b/test/ClangImporter/objc_async.swift
@@ -4,6 +4,7 @@
 // REQUIRES: concurrency
 import Foundation
 import ObjCConcurrency
+// expected-remark@-1{{add '@_predatesConcurrency' to suppress 'Sendable'-related warnings from module 'ObjCConcurrency'}}
 
 if #available(SwiftStdlib 5.5, *) {
 
diff --git a/test/ClangImporter/predates_concurrency_import_swift6.swift b/test/ClangImporter/predates_concurrency_import_swift6.swift
diff --git a/test/Concurrency/actor_isolation.swift b/test/Concurrency/actor_isolation.swift
diff --git a/test/Concurrency/predates_concurrency_import.swift b/test/Concurrency/predates_concurrency_import.swift
diff --git a/test/Concurrency/predates_concurrency_import_swift6.swift b/test/Concurrency/predates_concurrency_import_swift6.swift
