Sema: Diagnose inconsistent use of @_weakLinked on import declarations.

Author: tshortli

Diff for: include/swift/AST/DiagnosticsSema.def

@@ -2978,6 +2978,13 @@ ERROR(implementation_only_override_import_without_attr,none,
       "override of %0 imported as implementation-only must be declared "
       "'@_implementationOnly'", (DescriptiveDeclKind))
 
+ERROR(import_attr_conflict,none,
+      "%0 inconsistently imported with %1",
+      (Identifier, DeclAttribute))
+NOTE(import_attr_conflict_here,none,
+     "imported with %0 here",
+     (DeclAttribute))
+
 // Derived conformances
 ERROR(cannot_synthesize_init_in_extension_of_nonfinal,none,
       "implementation of %0 for non-final class cannot be automatically "

Diff for: include/swift/AST/SourceFile.h

@@ -176,7 +176,7 @@ class SourceFile final : public FileUnit {
 
   friend class HasImportsMatchingFlagRequest;
 
-  /// Indicates which import options have a valid caches. Storage for
+  /// Indicates which import options have valid caches. Storage for
   /// \c HasImportsMatchingFlagRequest.
   ImportOptions validCachedImportOptions;
 
@@ -352,9 +352,9 @@ class SourceFile final : public FileUnit {
   hasTestableOrPrivateImport(AccessLevel accessLevel, const ValueDecl *ofDecl,
                              ImportQueryKind kind = TestableAndPrivate) const;
 
-  /// Does this source file have any implementation-only imports?
+  /// Does this source file have any imports with \c flag?
   /// If not, we can fast-path module checks.
-  bool hasImplementationOnlyImports() const;
+  bool hasImportsWithFlag(ImportFlags flag) const;
 
   /// Get the most permissive restriction applied to the imports of \p module.
   RestrictedImportKind getRestrictedImportKind(const ModuleDecl *module) const;

Diff for: include/swift/AST/TypeCheckRequests.h

@@ -3263,15 +3263,15 @@ class ResolveTypeRequest
 void simple_display(llvm::raw_ostream &out, const TypeResolution *resolution);
 SourceLoc extractNearestSourceLoc(const TypeRepr *repr);
 
-/// Checks to see if any of the imports in a module use `@_implementationOnly`
+/// Checks to see if any of the imports in a module use \c @_implementationOnly
 /// in one file and not in another.
 ///
 /// Like redeclaration checking, but for imports.
 ///
 /// This is a request purely to ensure that we don't need to perform the same
 /// checking for each file we resolve imports for.
 /// FIXME: Once import resolution operates at module-level, this checking can
-/// integrated into it.
+/// be integrated into it.
 class CheckInconsistentImplementationOnlyImportsRequest
     : public SimpleRequest<CheckInconsistentImplementationOnlyImportsRequest,
                            evaluator::SideEffect(ModuleDecl *),
@@ -3289,6 +3289,32 @@ class CheckInconsistentImplementationOnlyImportsRequest
   bool isCached() const { return true; }
 };
 
+/// Checks to see if any of the imports in a module use \c @_weakLinked
+/// in one file and not in another.
+///
+/// Like redeclaration checking, but for imports.
+///
+/// This is a request purely to ensure that we don't need to perform the same
+/// checking for each file we resolve imports for.
+/// FIXME: Once import resolution operates at module-level, this checking can
+/// be integrated into it.
+class CheckInconsistentWeakLinkedImportsRequest
+    : public SimpleRequest<CheckInconsistentWeakLinkedImportsRequest,
+                           evaluator::SideEffect(ModuleDecl *),
+                           RequestFlags::Cached> {
+public:
+  using SimpleRequest::SimpleRequest;
+
+private:
+  friend SimpleRequest;
+
+  evaluator::SideEffect evaluate(Evaluator &evaluator, ModuleDecl *mod) const;
+
+public:
+  // Cached.
+  bool isCached() const { return true; }
+};
+
 /// Retrieves the primary source files in the main module.
 // FIXME: This isn't really a type-checking request, if we ever split off a
 // zone for more basic AST requests, this should be moved there.

Diff for: include/swift/AST/TypeCheckerTypeIDZone.def

@@ -33,6 +33,8 @@ SWIFT_REQUEST(TypeChecker, CallerSideDefaultArgExprRequest,
               Expr *(DefaultArgumentExpr *), SeparatelyCached, NoLocationInfo)
 SWIFT_REQUEST(TypeChecker, CheckInconsistentImplementationOnlyImportsRequest,
               evaluator::SideEffect(ModuleDecl *), Cached, NoLocationInfo)
+SWIFT_REQUEST(TypeChecker, CheckInconsistentWeakLinkedImportsRequest,
+              evaluator::SideEffect(ModuleDecl *), Cached, NoLocationInfo)
 SWIFT_REQUEST(TypeChecker, CheckRedeclarationRequest,
               evaluator::SideEffect(ValueDecl *),
               Uncached, NoLocationInfo)

Diff for: lib/AST/Module.cpp

@@ -2442,13 +2442,11 @@ void swift::simple_display(llvm::raw_ostream &out, ImportOptions options) {
   out << " }";
 }
 
-bool SourceFile::hasImplementationOnlyImports() const {
+bool SourceFile::hasImportsWithFlag(ImportFlags flag) const {
   auto &ctx = getASTContext();
   auto *mutableThis = const_cast<SourceFile *>(this);
-  return evaluateOrDefault(ctx.evaluator,
-                           HasImportsMatchingFlagRequest{
-                               mutableThis, ImportFlags::ImplementationOnly},
-                           false);
+  return evaluateOrDefault(
+      ctx.evaluator, HasImportsMatchingFlagRequest{mutableThis, flag}, false);
 }
 
 bool SourceFile::hasTestableOrPrivateImport(

Diff for: lib/Sema/ImportResolution.cpp

@@ -693,6 +693,16 @@ void UnboundImport::diagnoseInvalidAttr(DeclAttrKind attrKind,
   attr->setInvalid();
 }
 
+/// Returns true if any file in the module contains an import with \c flag.
+static bool moduleHasAnyImportsMatchingFlag(ModuleDecl *mod, ImportFlags flag) {
+  for (const FileUnit *F : mod->getFiles()) {
+    auto *SF = dyn_cast<SourceFile>(F);
+    if (SF && SF->hasImportsWithFlag(flag))
+      return true;
+  }
+  return false;
+}
+
 /// Finds all import declarations for a single module that inconsistently match
 /// \c predicate and passes each pair of inconsistent imports to \c diagnose.
 template <typename Pred, typename Diag>
@@ -747,12 +757,7 @@ static void findInconsistentImports(ModuleDecl *mod, Pred predicate,
 evaluator::SideEffect
 CheckInconsistentImplementationOnlyImportsRequest::evaluate(
     Evaluator &evaluator, ModuleDecl *mod) const {
-  bool hasAnyImplementationOnlyImports =
-      llvm::any_of(mod->getFiles(), [](const FileUnit *F) -> bool {
-        auto *SF = dyn_cast<SourceFile>(F);
-        return SF && SF->hasImplementationOnlyImports();
-      });
-  if (!hasAnyImplementationOnlyImports)
+  if (!moduleHasAnyImportsMatchingFlag(mod, ImportFlags::ImplementationOnly))
     return {};
 
   auto diagnose = [mod](const ImportDecl *normalImport,
@@ -783,6 +788,31 @@ CheckInconsistentImplementationOnlyImportsRequest::evaluate(
   return {};
 }
 
+evaluator::SideEffect
+CheckInconsistentWeakLinkedImportsRequest::evaluate(Evaluator &evaluator,
+                                                    ModuleDecl *mod) const {
+  if (!moduleHasAnyImportsMatchingFlag(mod, ImportFlags::WeakLinked))
+    return {};
+
+  auto diagnose = [mod](const ImportDecl *otherImport,
+                        const ImportDecl *weakLinkedImport) {
+    auto attr = weakLinkedImport->getAttrs().getAttribute<WeakLinkedAttr>();
+    auto &diags = mod->getDiags();
+    diags
+        .diagnose(otherImport, diag::import_attr_conflict,
+                  otherImport->getModule()->getName(), attr)
+        .fixItInsert(otherImport->getStartLoc(), "@_weakLinked ");
+    diags.diagnose(weakLinkedImport, diag::import_attr_conflict_here, attr);
+  };
+
+  auto predicate = [](ImportDecl *decl) {
+    return decl->getAttrs().hasAttribute<WeakLinkedAttr>();
+  };
+
+  findInconsistentImports(mod, predicate, diagnose);
+  return {};
+}
+
 //===----------------------------------------------------------------------===//
 // MARK: Scoped imports
 //===----------------------------------------------------------------------===//

Diff for: lib/Sema/TypeCheckAccess.cpp

@@ -1516,7 +1516,7 @@ swift::getDisallowedOriginKind(const Decl *decl,
     // just in this case.
     if (howImported == RestrictedImportKind::Implicit &&
         !SF->getASTContext().isSwiftVersionAtLeast(6) &&
-        !SF->hasImplementationOnlyImports()) {
+        !SF->hasImportsWithFlag(ImportFlags::ImplementationOnly)) {
       downgradeToWarning = DowngradeToWarning::Yes;
     }
 

Diff for: lib/Sema/TypeChecker.cpp

@@ -329,12 +329,16 @@ TypeCheckSourceFileRequest::evaluate(Evaluator &eval, SourceFile *SF) const {
 
   diagnoseUnnecessaryPreconcurrencyImports(*SF);
 
-  // Check to see if there's any inconsistent @_implementationOnly imports.
+  // Check to see if there are any inconsistent imports.
   evaluateOrDefault(
       Ctx.evaluator,
       CheckInconsistentImplementationOnlyImportsRequest{SF->getParentModule()},
       {});
 
+  evaluateOrDefault(
+      Ctx.evaluator,
+      CheckInconsistentWeakLinkedImportsRequest{SF->getParentModule()}, {});
+
   // Perform various AST transforms we've been asked to perform.
   if (!Ctx.hadError() && Ctx.LangOpts.DebuggerTestingTransform)
     performDebuggerTestingTransform(*SF);

Diff for: test/decl/import/inconsistent-weaklinked-import.swift

@@ -0,0 +1,42 @@
+// RUN: %empty-directory(%t)
+// RUN: split-file %s %t
+
+// Check that the diagnostics are produced regardless of what primary file we're using.
+// RUN: %target-swift-frontend -typecheck -primary-file %t/1.swift %t/2.swift %t/3.swift -I %S/Inputs/inconsistent-implementation-only/ -verify
+// RUN: %target-swift-frontend -typecheck %t/1.swift -primary-file %t/2.swift %t/3.swift -I %S/Inputs/inconsistent-implementation-only/ -verify
+// RUN: %target-swift-frontend -typecheck %t/1.swift %t/2.swift -primary-file %t/3.swift -I %S/Inputs/inconsistent-implementation-only/ -verify
+// RUN: %target-swift-frontend -typecheck -primary-file %t/1.swift -primary-file %t/2.swift -primary-file %t/3.swift -I %S/Inputs/inconsistent-implementation-only/ -verify
+// RUN: %target-swift-frontend -typecheck %t/1.swift %t/2.swift %t/3.swift -I %S/Inputs/inconsistent-implementation-only/ -verify
+
+// UNSUPPORTED: OS=windows-msvc
+
+//--- 1.swift
+
+@_weakLinked import NotSoSecret // expected-note {{imported with @_weakLinked here}}
+import NotSoSecret2 // expected-error {{'NotSoSecret2' inconsistently imported with @_weakLinked}} {{1-1=@_weakLinked }}
+import NotSoSecret3 // expected-error {{'NotSoSecret3' inconsistently imported with @_weakLinked}} {{1-1=@_weakLinked }}
+
+@_weakLinked import ActuallySecret // no-error
+import ActuallyOkay // no-error
+
+@_exported import Contradictory // expected-error {{'Contradictory' inconsistently imported with @_weakLinked}} {{12-12=@_weakLinked }}
+
+//--- 2.swift
+
+import NotSoSecret // expected-error {{'NotSoSecret' inconsistently imported with @_weakLinked}}
+@_weakLinked import NotSoSecret2 // expected-note 2 {{imported with @_weakLinked here}}
+import NotSoSecret3 // expected-error {{'NotSoSecret3' inconsistently imported with @_weakLinked}}
+
+@_weakLinked import ActuallySecret // no-error
+import ActuallyOkay // no-error
+
+@_weakLinked import Contradictory // expected-note {{imported with @_weakLinked here}}
+
+//--- 3.swift
+
+@_weakLinked import NotSoSecret
+import NotSoSecret2 // expected-error {{'NotSoSecret2' inconsistently imported with @_weakLinked}}
+@_weakLinked import NotSoSecret3 // expected-note 2 {{imported with @_weakLinked here}}
+
+@_weakLinked import ActuallySecret // no-error
+import ActuallyOkay // no-error