[NFC] Clarify semantics of getImportedModules.

The lack of clarity manifested as unexpected behavior when using
getImportedModules to create the module import graph. The new behavior
makes SPI-ness and Shadowing-ness behave similarly in terms of
filtering. We also check if a filter is well-formed to avoid
accidental empty import lists.

Author: varungandhi-apple

include/swift/AST/Module.h

@@ -653,7 +653,8 @@ class ModuleDecl : public DeclContext, public TypeDecl {
     Default = 1 << 1,
     /// Include imports declared with `@_implementationOnly`.
     ImplementationOnly = 1 << 2,
-    /// Include imports of SPIs declared with `@_spi`
+    /// Include imports of SPIs declared with `@_spi`. Non-SPI imports are
+    /// included whether or not this flag is specified.
     SPIAccessControl = 1 << 3,
     /// Include imports shadowed by a cross-import overlay. Unshadowed imports
     /// are included whether or not this flag is specified.
@@ -664,8 +665,33 @@ class ModuleDecl : public DeclContext, public TypeDecl {
 
   /// Looks up which modules are imported by this module.
   ///
-  /// \p filter controls whether public, private, or any imports are included
-  /// in this list.
+  /// \p filter controls which imports are included in the list.
+  ///
+  /// There are three axes for categorizing imports:
+  /// 1. Privacy: Exported/Private/ImplementationOnly (mutually exclusive).
+  /// 2. SPI/non-SPI: An import of any privacy level may be @_spi("SPIName").
+  /// 3. Shadowed/Non-shadowed: An import of any privacy level may be shadowed
+  ///    by a cross-import overlay.
+  ///
+  /// It is also possible for SPI imports to be shadowed by a cross-import
+  /// overlay.
+  ///
+  /// If \p filter contains multiple privacy levels, modules at all the privacy
+  /// levels are included.
+  ///
+  /// If \p filter contains \c ImportFilterKind::SPIAccessControl, then both
+  /// SPI and non-SPI imports are included. Otherwise, only non-SPI imports are
+  /// included.
+  ///
+  /// If \p filter contains \c ImportFilterKind::ShadowedByCrossImportOverlay,
+  /// both shadowed and non-shadowed imports are included. Otherwise, only
+  /// non-shadowed imports are included.
+  ///
+  /// Clang modules have some additional complexities; see the implementation of
+  /// \c ClangModuleUnit::getImportedModules for details.
+  ///
+  /// \pre \p filter must contain at least one privacy level, i.e. one of
+  ///      \c Exported or \c Private or \c ImplementationOnly.
   void getImportedModules(SmallVectorImpl<ImportedModule> &imports,
                           ImportFilter filter = ImportFilterKind::Exported) const;
 

include/swift/Basic/OptionSet.h

@@ -19,6 +19,7 @@
 
 #include "llvm/ADT/None.h"
 
+#include <cassert>
 #include <type_traits>
 #include <cstdint>
 #include <initializer_list>
@@ -98,6 +99,14 @@ class OptionSet {
     return Storage == set.Storage;
   }
 
+  /// Check if this option set contains any options from \p set.
+  ///
+  /// \pre \p set must be non-empty.
+  bool containsAny(OptionSet set) const {
+    assert((bool)set && "argument must be non-empty");
+    return (bool)((*this) & set);
+  }
+
   // '==' and '!=' are deliberately not defined because they provide a pitfall
   // where someone might use '==' but really want 'contains'. If you actually
   // want '==' behavior, use 'containsOnly'.

lib/AST/ImportCache.cpp

@@ -175,6 +175,7 @@ ImportSet &ImportCache::getImportSet(const DeclContext *dc) {
       ModuleDecl::ImportedModule{ImportPath::Access(), mod});
 
   if (file) {
+    // Should include both SPI & non-SPI.
     file->getImportedModules(imports,
                              {ModuleDecl::ImportFilterKind::Default,
                               ModuleDecl::ImportFilterKind::ImplementationOnly,
@@ -260,6 +261,7 @@ ImportCache::getAllAccessPathsNotShadowedBy(const ModuleDecl *mod,
       ModuleDecl::ImportedModule{ImportPath::Access(), currentMod});
 
   if (auto *file = dyn_cast<FileUnit>(dc)) {
+    // Should include both SPI & non-SPI
     file->getImportedModules(stack,
                              {ModuleDecl::ImportFilterKind::Default,
                               ModuleDecl::ImportFilterKind::ImplementationOnly,

lib/AST/Module.cpp

@@ -1163,6 +1163,13 @@ void SourceFile::lookupPrecedenceGroupDirect(
 
 void ModuleDecl::getImportedModules(SmallVectorImpl<ImportedModule> &modules,
                                     ModuleDecl::ImportFilter filter) const {
+  assert(filter.containsAny(ImportFilter({
+      ModuleDecl::ImportFilterKind::Exported,
+      ModuleDecl::ImportFilterKind::Default,
+      ModuleDecl::ImportFilterKind::ImplementationOnly}))
+    && "filter should have at least one of Exported|Private|ImplementationOnly"
+  );
+
   FORWARD(getImportedModules, (modules, filter));
 }
 
@@ -1187,11 +1194,12 @@ SourceFile::getImportedModules(SmallVectorImpl<ModuleDecl::ImportedModule> &modu
       requiredFilter |= ModuleDecl::ImportFilterKind::Exported;
     else if (desc.importOptions.contains(ImportFlags::ImplementationOnly))
       requiredFilter |= ModuleDecl::ImportFilterKind::ImplementationOnly;
-    else if (desc.importOptions.contains(ImportFlags::SPIAccessControl))
-      requiredFilter |= ModuleDecl::ImportFilterKind::SPIAccessControl;
     else
       requiredFilter |= ModuleDecl::ImportFilterKind::Default;
 
+    if (desc.importOptions.contains(ImportFlags::SPIAccessControl))
+      requiredFilter |= ModuleDecl::ImportFilterKind::SPIAccessControl;
+
     if (!separatelyImportedOverlays.lookup(desc.module.importedModule).empty())
       requiredFilter |= ModuleDecl::ImportFilterKind::ShadowedByCrossImportOverlay;
 

lib/Frontend/ModuleInterfaceSupport.cpp

@@ -116,7 +116,8 @@ static void printImports(raw_ostream &out,
 
     SmallVector<ModuleDecl::ImportedModule, 4> ioiImport;
     M->getImportedModules(ioiImport,
-                          ModuleDecl::ImportFilterKind::ImplementationOnly);
+                          {ModuleDecl::ImportFilterKind::ImplementationOnly,
+                           ModuleDecl::ImportFilterKind::SPIAccessControl});
     ioiImportSet.insert(ioiImport.begin(), ioiImport.end());
   }
 

lib/Serialization/Serialization.cpp

@@ -1039,7 +1039,11 @@ void Serializer::writeInputBlock(const SerializationOptions &options) {
   ImportSet privateImportSet =
       getImportsAsSet(M, ModuleDecl::ImportFilterKind::Default);
   ImportSet spiImportSet =
-      getImportsAsSet(M, ModuleDecl::ImportFilterKind::SPIAccessControl);
+      getImportsAsSet(M, {
+          ModuleDecl::ImportFilterKind::Exported,
+          ModuleDecl::ImportFilterKind::Default,
+          ModuleDecl::ImportFilterKind::SPIAccessControl
+      });
 
   auto clangImporter =
     static_cast<ClangImporter *>(M->getASTContext().getClangModuleLoader());