Merge pull request #23858 from jrose-apple/5.1-implementation-only-diagnostics

[5.1] Implementation-only import checking

Author: jrose-apple

include/swift/AST/AccessScopeChecker.h

@@ -23,47 +23,79 @@
 
 namespace swift {
 
-class AbstractStorageDecl;
-class ExtensionDecl;
+class BoundGenericType;
+class ComponentIdentTypeRepr;
+class NominalType;
 class SourceFile;
-class ValueDecl;
+class TypeAliasType;
 
 class AccessScopeChecker {
   const SourceFile *File;
   bool TreatUsableFromInlineAsPublic;
 
-protected:
-  ASTContext &Context;
   Optional<AccessScope> Scope = AccessScope::getPublic();
 
   AccessScopeChecker(const DeclContext *useDC,
                      bool treatUsableFromInlineAsPublic);
-  bool visitDecl(ValueDecl *VD);
-};
-
-class TypeReprAccessScopeChecker : private ASTWalker, AccessScopeChecker {
-  TypeReprAccessScopeChecker(const DeclContext *useDC,
-                             bool treatUsableFromInlineAsPublic);
-
-  bool walkToTypeReprPre(TypeRepr *TR) override;
-  bool walkToTypeReprPost(TypeRepr *TR) override;
+  bool visitDecl(const ValueDecl *VD);
 
 public:
   static Optional<AccessScope>
   getAccessScope(TypeRepr *TR, const DeclContext *useDC,
                  bool treatUsableFromInlineAsPublic = false);
+  static Optional<AccessScope>
+  getAccessScope(Type T, const DeclContext *useDC,
+                 bool treatUsableFromInlineAsPublic = false);
 };
 
-class TypeAccessScopeChecker : private TypeWalker, AccessScopeChecker {
-  TypeAccessScopeChecker(const DeclContext *useDC,
-                         bool treatUsableFromInlineAsPublic);
+/// Walks a Type to find all NominalTypes, BoundGenericTypes, and
+/// TypeAliasTypes.
+class TypeDeclFinder : public TypeWalker {
+  Action walkToTypePre(Type T) override;
+
+public:
+  virtual Action visitNominalType(const NominalType *ty) {
+    return Action::Continue;
+  }
+  virtual Action visitBoundGenericType(const BoundGenericType *ty) {
+    return Action::Continue;
+  }
+  virtual Action visitTypeAliasType(const TypeAliasType *ty) {
+    return Action::Continue;
+  }
+};
 
-  Action walkToTypePre(Type T);
+/// A TypeDeclFinder for use cases where all types should be treated
+/// equivalently and where generic arguments can be walked to separately from
+/// the generic type.
+class SimpleTypeDeclFinder : public TypeDeclFinder {
+  /// The function to call when a ComponentIdentTypeRepr is seen.
+  llvm::function_ref<Action(const TypeDecl *)> Callback;
+
+  Action visitNominalType(const NominalType *ty) override;
+  Action visitBoundGenericType(const BoundGenericType *ty) override;
+  Action visitTypeAliasType(const TypeAliasType *ty) override;
 
 public:
-  static Optional<AccessScope>
-  getAccessScope(Type T, const DeclContext *useDC,
-                 bool treatUsableFromInlineAsPublic = false);
+  explicit SimpleTypeDeclFinder(
+      llvm::function_ref<Action(const TypeDecl *)> callback)
+    : Callback(callback) {}
+};
+
+/// Walks a TypeRepr to find all ComponentIdentTypeReprs with bound TypeDecls.
+///
+/// Subclasses can either override #visitTypeDecl if they only care about
+/// types on their own, or #visitComponentIdentTypeRepr if they want to keep
+/// the TypeRepr around.
+class TypeReprIdentFinder : public ASTWalker {
+  /// The function to call when a ComponentIdentTypeRepr is seen.
+  llvm::function_ref<bool(const ComponentIdentTypeRepr *)> Callback;
+
+  bool walkToTypeReprPost(TypeRepr *TR) override;
+public:
+  explicit TypeReprIdentFinder(
+      llvm::function_ref<bool(const ComponentIdentTypeRepr *)> callback)
+    : Callback(callback) {}
 };
 
 }

include/swift/AST/DiagnosticsSema.def

@@ -2397,6 +2397,11 @@ NOTE(enum_raw_value_incrementing_from_zero,none,
 NOTE(construct_raw_representable_from_unwrapped_value,none,
      "construct %0 from unwrapped %1 value", (Type, Type))
 
+ERROR(decl_from_implementation_only_module,none,
+      "cannot use %0 here; %1 has been imported as "
+      "'@_implementationOnly'",
+      (DeclName, Identifier))
+
 // Derived conformances
 ERROR(cannot_synthesize_init_in_extension_of_nonfinal,none,
       "implementation of %0 for non-final class cannot be automatically "
@@ -4104,6 +4109,11 @@ WARNING(resilience_decl_unavailable_warn,
         "should not be referenced from " FRAGILE_FUNC_KIND "3",
         (DescriptiveDeclKind, DeclName, AccessLevel, unsigned, bool))
 
+ERROR(inlinable_decl_ref_implementation_only,
+      none, "%0 %1 cannot be used in an inlinable "
+      "function because its module was imported implementation-only",
+      (DescriptiveDeclKind, DeclName))
+
 #undef FRAGILE_FUNC_KIND
 
 NOTE(resilience_decl_declared_here_public,

include/swift/AST/Module.h

@@ -959,6 +959,10 @@ class SourceFile final : public FileUnit {
   /// May be -1, to indicate no association with a buffer.
   int BufferID;
 
+  /// Does this source file have any implementation-only imports?
+  /// If not, we can fast-path module checks.
+  bool HasImplementationOnlyImports = false;
+
   /// The list of protocol conformances that were "used" within this
   /// source file.
   llvm::SetVector<NormalProtocolConformance *> UsedConformances;
@@ -1047,6 +1051,12 @@ class SourceFile final : public FileUnit {
   hasTestableOrPrivateImport(AccessLevel accessLevel, const ValueDecl *ofDecl,
                              ImportQueryKind kind = TestableAndPrivate) const;
 
+  bool hasImplementationOnlyImports() const {
+    return HasImplementationOnlyImports;
+  }
+
+  bool isImportedImplementationOnly(const ModuleDecl *module) const;
+
   void clearLookupCache();
 
   void cacheVisibleDecls(SmallVectorImpl<ValueDecl *> &&globals) const;

lib/AST/AccessScopeChecker.cpp

@@ -24,68 +24,76 @@ using namespace swift;
 AccessScopeChecker::AccessScopeChecker(const DeclContext *useDC,
                                        bool treatUsableFromInlineAsPublic)
   : File(useDC->getParentSourceFile()),
-    TreatUsableFromInlineAsPublic(treatUsableFromInlineAsPublic),
-    Context(File->getASTContext()) {}
+    TreatUsableFromInlineAsPublic(treatUsableFromInlineAsPublic) {}
 
 bool
-AccessScopeChecker::visitDecl(ValueDecl *VD) {
-  if (!VD || isa<GenericTypeParamDecl>(VD))
+AccessScopeChecker::visitDecl(const ValueDecl *VD) {
+  if (isa<GenericTypeParamDecl>(VD))
     return true;
 
   auto AS = VD->getFormalAccessScope(File, TreatUsableFromInlineAsPublic);
   Scope = Scope->intersectWith(AS);
   return Scope.hasValue();
 }
 
-TypeReprAccessScopeChecker::TypeReprAccessScopeChecker(const DeclContext *useDC,
-                                                       bool treatUsableFromInlineAsPublic)
-  : AccessScopeChecker(useDC, treatUsableFromInlineAsPublic) {
+bool TypeReprIdentFinder::walkToTypeReprPost(TypeRepr *TR) {
+  auto CITR = dyn_cast<ComponentIdentTypeRepr>(TR);
+  if (!CITR || !CITR->getBoundDecl())
+    return true;
+  return Callback(CITR);
 }
 
-bool
-TypeReprAccessScopeChecker::walkToTypeReprPre(TypeRepr *TR) {
-  if (auto CITR = dyn_cast<ComponentIdentTypeRepr>(TR))
-    return visitDecl(CITR->getBoundDecl());
-  return true;
+Optional<AccessScope>
+AccessScopeChecker::getAccessScope(TypeRepr *TR, const DeclContext *useDC,
+                                   bool treatUsableFromInlineAsPublic) {
+  AccessScopeChecker checker(useDC, treatUsableFromInlineAsPublic);
+  TR->walk(TypeReprIdentFinder([&](const ComponentIdentTypeRepr *typeRepr) {
+    return checker.visitDecl(typeRepr->getBoundDecl());
+  }));
+  return checker.Scope;
 }
 
-bool
-TypeReprAccessScopeChecker::walkToTypeReprPost(TypeRepr *TR) {
-  return Scope.hasValue();
-}
+TypeWalker::Action TypeDeclFinder::walkToTypePre(Type T) {
+  if (auto *TAT = dyn_cast<TypeAliasType>(T.getPointer()))
+    return visitTypeAliasType(TAT);
 
-Optional<AccessScope>
-TypeReprAccessScopeChecker::getAccessScope(TypeRepr *TR, const DeclContext *useDC,
-                                           bool treatUsableFromInlineAsPublic) {
-  TypeReprAccessScopeChecker checker(useDC, treatUsableFromInlineAsPublic);
-  TR->walk(checker);
-  return checker.Scope;
+  // FIXME: We're looking through sugar here so that we visit, e.g.,
+  // Swift.Array when we see `[Int]`. But that means we do redundant work when
+  // we see sugar that's purely structural, like `(Int)`. Fortunately, paren
+  // types are the only such purely structural sugar at the time this comment
+  // was written, and they're not so common in the first place.
+  if (auto *BGT = T->getAs<BoundGenericType>())
+    return visitBoundGenericType(BGT);
+  if (auto *NT = T->getAs<NominalType>())
+    return visitNominalType(NT);
+
+  return Action::Continue;
 }
 
-TypeAccessScopeChecker::TypeAccessScopeChecker(const DeclContext *useDC,
-                                               bool treatUsableFromInlineAsPublic)
-  : AccessScopeChecker(useDC, treatUsableFromInlineAsPublic) {}
+TypeWalker::Action
+SimpleTypeDeclFinder::visitNominalType(const NominalType *ty) {
+  return Callback(ty->getDecl());
+}
 
 TypeWalker::Action
-TypeAccessScopeChecker::walkToTypePre(Type T) {
-  ValueDecl *VD;
-  if (auto *BNAD = dyn_cast<TypeAliasType>(T.getPointer()))
-    VD = BNAD->getDecl();
-  else if (auto *NTD = T->getAnyNominal())
-    VD = NTD;
-  else
-    VD = nullptr;
-
-  if (!visitDecl(VD))
-    return Action::Stop;
+SimpleTypeDeclFinder::visitBoundGenericType(const BoundGenericType *ty) {
+  return Callback(ty->getDecl());
+}
 
-  return Action::Continue;
+TypeWalker::Action
+SimpleTypeDeclFinder::visitTypeAliasType(const TypeAliasType *ty) {
+  return Callback(ty->getDecl());
 }
 
+
 Optional<AccessScope>
-TypeAccessScopeChecker::getAccessScope(Type T, const DeclContext *useDC,
-                                       bool treatUsableFromInlineAsPublic) {
-  TypeAccessScopeChecker checker(useDC, treatUsableFromInlineAsPublic);
-  T.walk(checker);
+AccessScopeChecker::getAccessScope(Type T, const DeclContext *useDC,
+                                   bool treatUsableFromInlineAsPublic) {
+  AccessScopeChecker checker(useDC, treatUsableFromInlineAsPublic);
+  T.walk(SimpleTypeDeclFinder([&](const ValueDecl *VD) {
+    if (checker.visitDecl(VD))
+      return TypeWalker::Action::Continue;
+    return TypeWalker::Action::Stop;
+  }));
   return checker.Scope;
 }

lib/AST/Module.cpp

@@ -1421,6 +1421,14 @@ void SourceFile::addImports(ArrayRef<ImportedModuleDesc> IM) {
   assert(iter == newBuf.end());
 
   Imports = newBuf;
+
+  // Update the HasImplementationOnlyImports flag.
+  if (!HasImplementationOnlyImports) {
+    for (auto &desc : IM) {
+      if (desc.importOptions.contains(ImportFlags::ImplementationOnly))
+        HasImplementationOnlyImports = true;
+    }
+  }
 }
 
 bool SourceFile::hasTestableOrPrivateImport(
@@ -1485,6 +1493,43 @@ bool SourceFile::hasTestableOrPrivateImport(
                      });
 }
 
+bool SourceFile::isImportedImplementationOnly(const ModuleDecl *module) const {
+  // Implementation-only imports are (currently) always source-file-specific,
+  // so if we don't have any, we know the search is complete.
+  if (!hasImplementationOnlyImports())
+    return false;
+
+  auto isImportedBy = [](const ModuleDecl *dest, const ModuleDecl *src) {
+    // Fast path.
+    if (dest == src) return true;
+
+    // Walk the transitive imports, respecting visibility.
+    // This return true if the search *didn't* short-circuit, and it short
+    // circuits if we found `dest`, so we need to invert the sense before
+    // returning.
+    return !const_cast<ModuleDecl*>(src)
+              ->forAllVisibleModules({}, [dest](ModuleDecl::ImportedModule im) {
+      // Continue searching as long as we haven't found `dest` yet.
+      return im.second != dest;
+    });
+  };
+
+  // Look at the imports of this source file.
+  for (auto &desc : Imports) {
+    // Ignore implementation-only imports.
+    if (desc.importOptions.contains(ImportFlags::ImplementationOnly))
+      continue;
+
+    // If the module is imported this way, it's not imported
+    // implementation-only.
+    if (isImportedBy(module, desc.module.second))
+      return false;
+  }
+
+  // Now check this file's enclosing module in case there are re-exports.
+  return !isImportedBy(module, getParentModule());
+}
+
 void SourceFile::clearLookupCache() {
   if (!Cache)
     return;

lib/Sema/ResilienceDiagnostics.cpp

@@ -102,6 +102,8 @@ bool TypeChecker::diagnoseInlinableDeclRef(SourceLoc loc,
                                            const DeclContext *DC,
                                            FragileFunctionKind Kind,
                                            bool TreatUsableFromInlineAsPublic) {
+  // Do some important fast-path checks that apply to all cases.
+
   // Local declarations are OK.
   if (D->getDeclContext()->isLocalContext())
     return false;
@@ -110,6 +112,23 @@ bool TypeChecker::diagnoseInlinableDeclRef(SourceLoc loc,
   if (isa<AbstractTypeParamDecl>(D))
     return false;
 
+  // Check whether the declaration is accessible.
+  if (diagnoseInlinableDeclRefAccess(loc, D, DC, Kind,
+                                     TreatUsableFromInlineAsPublic))
+    return true;
+
+  // Check whether the declaration comes from a publically-imported module.
+  if (diagnoseDeclRefExportability(loc, D, DC))
+    return true;
+
+  return false;
+}
+
+bool TypeChecker::diagnoseInlinableDeclRefAccess(SourceLoc loc,
+                                           const ValueDecl *D,
+                                           const DeclContext *DC,
+                                           FragileFunctionKind Kind,
+                                           bool TreatUsableFromInlineAsPublic) {
   // Public declarations are OK.
   if (D->getFormalAccessScope(/*useDC=*/nullptr,
                               TreatUsableFromInlineAsPublic).isPublic())
@@ -178,3 +197,38 @@ bool TypeChecker::diagnoseInlinableDeclRef(SourceLoc loc,
   return (downgradeToWarning == DowngradeToWarning::No);
 }
 
+bool TypeChecker::diagnoseDeclRefExportability(SourceLoc loc,
+                                               const ValueDecl *D,
+                                               const DeclContext *DC) {
+  // We're only interested in diagnosing uses from source files.
+  auto userSF = DC->getParentSourceFile();
+  if (!userSF)
+    return false;
+
+  // If the source file doesn't have any implementation-only imports,
+  // we can fast-path this.  In the current language design, we never
+  // need to consider the possibility of implementation-only imports
+  // from other source files in the module (or indirectly in other modules).
+  // TODO: maybe check whether D is from a bridging header?
+  if (!userSF->hasImplementationOnlyImports())
+    return false;
+
+  auto userModule = userSF->getParentModule();
+  auto definingModule = D->getModuleContext();
+
+  // Nothing to diagnose in the very common case of the same module.
+  if (userModule == definingModule)
+    return false;
+
+  // Nothing to diagnose in the very common case that the module is
+  // imported for use in signatures.
+  if (!userSF->isImportedImplementationOnly(definingModule))
+    return false;
+
+  // TODO: different diagnostics
+  diagnose(loc, diag::inlinable_decl_ref_implementation_only,
+           D->getDescriptiveKind(), D->getFullName());
+
+  // TODO: notes explaining why
+  return true;
+}