Optimize Condition Resolution (#6279)

CodaFi · web-flow · commit dfa41d625b35 · 2017-01-05T12:08:54.000-07:00
* Pack the bits for IfConfigDecls into Decl

* Don't open symbols into a module when evaluating canImport statements

The module loaders now have API to check whether a given module can be
imported without importing the referenced module.  This provides a
significant speed boost to condition resolution and no longer
introduces symbols from the referenced module into the current context
without the user explicitly requesting it.

The definition of ‘canImport’ does not necessarily mean that a full
import without error is possible, merely that the path to the import is
visible to the compiler and the module is loadable in some form or
another.

Note that this means this check is insufficient to guarantee that you
are on one platform or another.  For those kinds of checks, use
‘os(OSNAME)’.
diff --git a/include/swift/AST/ASTContext.h b/include/swift/AST/ASTContext.h
@@ -580,6 +580,13 @@ class ASTContext {
   /// Does nothing in non-asserts (NDEBUG) builds.
   void verifyAllLoadedModules() const;
 
+  /// \brief Check whether the module with a given name can be imported without
+  /// importing it.
+  ///
+  /// Note that even if this check succeeds, errors may still occur if the
+  /// module is loaded in full.
+  bool canImportModule(std::pair<Identifier, SourceLoc> ModulePath);
+
   /// \returns a module with a given name that was already loaded.  If the
   /// module was not loaded, returns nullptr.
   ModuleDecl *getLoadedModule(
diff --git a/include/swift/AST/Decl.h b/include/swift/AST/Decl.h
@@ -578,6 +578,20 @@ class alignas(1 << DeclAlignInBits) Decl {
   enum { NumExtensionDeclBits = NumDeclBits + 6 };
   static_assert(NumExtensionDeclBits <= 32, "fits in an unsigned");
 
+  class IfConfigDeclBitfields {
+    friend class IfConfigDecl;
+    unsigned : NumDeclBits;
+
+    /// Whether this decl is missing its closing '#endif'.
+    unsigned HadMissingEnd : 1;
+
+    /// Whether this condition has been resolved either statically by Parse or
+    /// later by Condition Resolution.
+    unsigned HasBeenResolved : 1;
+  };
+  enum { NumIfConfigDeclBits = NumDeclBits + 2 };
+  static_assert(NumIfConfigDeclBits <= 32, "fits in an unsigned");
+
 protected:
   union {
     DeclBitfields DeclBits;
@@ -601,6 +615,7 @@ class alignas(1 << DeclAlignInBits) Decl {
     PrecedenceGroupDeclBitfields PrecedenceGroupDeclBits;
     ImportDeclBitfields ImportDeclBits;
     ExtensionDeclBitfields ExtensionDeclBits;
+    IfConfigDeclBitfields IfConfigDeclBits;
     uint32_t OpaqueBits;
   };
 
@@ -1931,14 +1946,16 @@ class IfConfigDecl : public Decl {
   /// The array is ASTContext allocated.
   ArrayRef<IfConfigDeclClause> Clauses;
   SourceLoc EndLoc;
-  bool HadMissingEnd;
-  bool HasBeenResolved = false;
+
 public:
   
   IfConfigDecl(DeclContext *Parent, ArrayRef<IfConfigDeclClause> Clauses,
                SourceLoc EndLoc, bool HadMissingEnd)
-    : Decl(DeclKind::IfConfig, Parent), Clauses(Clauses),
-      EndLoc(EndLoc), HadMissingEnd(HadMissingEnd) {}
+    : Decl(DeclKind::IfConfig, Parent), Clauses(Clauses), EndLoc(EndLoc)
+  {
+    IfConfigDeclBits.HadMissingEnd = HadMissingEnd;
+    IfConfigDeclBits.HasBeenResolved = false;
+  }
 
   ArrayRef<IfConfigDeclClause> getClauses() const { return Clauses; }
 
@@ -1955,13 +1972,13 @@ class IfConfigDecl : public Decl {
     return {};
   }
 
-  bool isResolved() const { return HasBeenResolved; }
-  void setResolved() { HasBeenResolved = true; }
+  bool isResolved() const { return IfConfigDeclBits.HasBeenResolved; }
+  void setResolved() { IfConfigDeclBits.HasBeenResolved = true; }
   
   SourceLoc getEndLoc() const { return EndLoc; }
   SourceLoc getLoc() const { return Clauses[0].Loc; }
 
-  bool hadMissingEnd() const { return HadMissingEnd; }
+  bool hadMissingEnd() const { return IfConfigDeclBits.HadMissingEnd; }
   
   SourceRange getSourceRange() const;
   
diff --git a/include/swift/AST/ModuleLoader.h b/include/swift/AST/ModuleLoader.h
@@ -74,6 +74,13 @@ class ModuleLoader {
 public:
   virtual ~ModuleLoader() = default;
 
+  /// \brief Check whether the module with a given name can be imported without
+  /// importing it.
+  ///
+  /// Note that even if this check succeeds, errors may still occur if the
+  /// module is loaded in full.
+  virtual bool canImportModule(std::pair<Identifier, SourceLoc> named) = 0;
+
   /// \brief Import a module with the given module path.
   ///
   /// \param importLoc The location of the 'import' keyword.
diff --git a/include/swift/ClangImporter/ClangImporter.h b/include/swift/ClangImporter/ClangImporter.h
@@ -94,6 +94,13 @@ class ClangImporter final : public ClangModuleLoader {
 
   ~ClangImporter();
 
+  /// \brief Check whether the module with a given name can be imported without
+  /// importing it.
+  ///
+  /// Note that even if this check succeeds, errors may still occur if the
+  /// module is loaded in full.
+  virtual bool canImportModule(std::pair<Identifier, SourceLoc> named) override;
+
   /// \brief Import a module with the given module path.
   ///
   /// Clang modules will be imported using the Objective-C ARC dialect,
diff --git a/include/swift/Sema/SourceLoader.h b/include/swift/Sema/SourceLoader.h
@@ -48,6 +48,13 @@ class SourceLoader : public ModuleLoader {
   SourceLoader &operator=(const SourceLoader &) = delete;
   SourceLoader &operator=(SourceLoader &&) = delete;
 
+  /// \brief Check whether the module with a given name can be imported without
+  /// importing it.
+  ///
+  /// Note that even if this check succeeds, errors may still occur if the
+  /// module is loaded in full.
+  virtual bool canImportModule(std::pair<Identifier, SourceLoc> named) override;
+
   /// \brief Import a module with the given module path.
   ///
   /// \param importLoc The location of the 'import' keyword.
diff --git a/include/swift/Serialization/SerializedModuleLoader.h b/include/swift/Serialization/SerializedModuleLoader.h
@@ -48,6 +48,13 @@ class SerializedModuleLoader : public ModuleLoader {
   SerializedModuleLoader &operator=(const SerializedModuleLoader &) = delete;
   SerializedModuleLoader &operator=(SerializedModuleLoader &&) = delete;
 
+  /// \brief Check whether the module with a given name can be imported without
+  /// importing it.
+  ///
+  /// Note that even if this check succeeds, errors may still occur if the
+  /// module is loaded in full.
+  virtual bool canImportModule(std::pair<Identifier, SourceLoc> named) override;
+
   /// \brief Import a module with the given module path.
   ///
   /// \param importLoc The location of the 'import' keyword.
diff --git a/lib/AST/ASTContext.cpp b/lib/AST/ASTContext.cpp
@@ -1263,6 +1263,20 @@ GenericEnvironment *ASTContext::getOrCreateCanonicalGenericEnvironment(
   return env;
 }
 
+bool ASTContext::canImportModule(std::pair<Identifier, SourceLoc> ModulePath) {
+  // If this module has already been successfully imported, it is importable.
+  if (getLoadedModule(ModulePath) != nullptr)
+    return true;
+
+  // Otherwise, ask the module loaders.
+  for (auto &importer : Impl.ModuleLoaders) {
+    if (importer->canImportModule(ModulePath)) {
+      return true;
+    }
+  }
+  return false;
+}
+
 Module *
 ASTContext::getModule(ArrayRef<std::pair<Identifier, SourceLoc>> ModulePath) {
   assert(!ModulePath.empty());
diff --git a/lib/ClangImporter/ClangImporter.cpp b/lib/ClangImporter/ClangImporter.cpp
@@ -1022,6 +1022,23 @@ bool ClangImporter::isModuleImported(const clang::Module *M) {
   return M->NameVisibility == clang::Module::NameVisibilityKind::AllVisible;
 }
 
+bool ClangImporter::canImportModule(std::pair<Identifier, SourceLoc> moduleID) {
+  // Look up the top-level module to see if it exists.
+  // FIXME: This only works with top-level modules.
+  auto &clangHeaderSearch = Impl.getClangPreprocessor().getHeaderSearchInfo();
+  clang::Module *clangModule =
+    clangHeaderSearch.lookupModule(moduleID.first.str());
+  if (!clangModule) {
+    return false;
+  }
+
+  clang::Module::Requirement r;
+  clang::Module::UnresolvedHeaderDirective mh;
+  clang::Module *m;
+  auto &ctx = Impl.getClangASTContext();
+  return clangModule->isAvailable(ctx.getLangOpts(), getTargetInfo(), r, mh, m);
+}
+
 Module *ClangImporter::loadModule(
     SourceLoc importLoc,
     ArrayRef<std::pair<Identifier, SourceLoc>> path) {
diff --git a/lib/Parse/ConditionResolver.cpp b/lib/Parse/ConditionResolver.cpp
@@ -59,8 +59,7 @@ namespace {
 
     ConditionClauseResolver(SmallVectorImpl<Decl *> &ExtraTLCDs,
                             SourceFile &SF)
-    : NDF(SF), ExtraTLCDs(ExtraTLCDs), SF(SF),
-    Context(SF.getASTContext()) {}
+      : NDF(SF), ExtraTLCDs(ExtraTLCDs), SF(SF), Context(SF.getASTContext()) {}
 
     void resolveClausesAndInsertMembers(IterableDeclContext *Nom,
                                         IfConfigDecl *Cond) {
@@ -69,9 +68,9 @@ namespace {
         // Evaluate conditions until we find the active clause.
         DiagnosticTransaction DT(Context.Diags);
         auto classification =
-        Parser::classifyConditionalCompilationExpr(clause.Cond, Context,
-                                                   Context.Diags,
-                                                   /*fullCheck*/ true);
+          Parser::classifyConditionalCompilationExpr(clause.Cond, Context,
+                                                     Context.Diags,
+                                                     /*fullCheck*/ true);
         DT.abort();
         if (clause.Cond && !classification.getValueOr(false)) {
           continue;
diff --git a/lib/Parse/ParseStmt.cpp b/lib/Parse/ParseStmt.cpp
@@ -1789,8 +1789,8 @@ Parser::classifyConditionalCompilationExpr(Expr *condition,
           if (!fullCheck) {
             return None;
           }
-          return
-            Context.getModule({ { argumentIdent, UDRE->getLoc() } }) != nullptr;
+
+          return Context.canImportModule({ argumentIdent, UDRE->getLoc() });
         }
 
         if (!fullCheck) {
diff --git a/lib/Sema/SourceLoader.cpp b/lib/Sema/SourceLoader.cpp
@@ -70,6 +70,22 @@ class SkipNonTransparentFunctions : public DelayedParsingCallbacks {
 
 } // unnamed namespace
 
+bool SourceLoader::canImportModule(std::pair<Identifier, SourceLoc> ID) {
+  // Search the memory buffers to see if we can find this file on disk.
+  FileOrError inputFileOrError = findModule(Ctx, ID.first.str(),
+                                            ID.second);
+  if (!inputFileOrError) {
+    auto err = inputFileOrError.getError();
+    if (err != std::errc::no_such_file_or_directory) {
+      Ctx.Diags.diagnose(ID.second, diag::sema_opening_import,
+                         ID.first, err.message());
+    }
+
+    return false;
+  }
+  return true;
+}
+
 Module *SourceLoader::loadModule(SourceLoc importLoc,
                              ArrayRef<std::pair<Identifier, SourceLoc>> path) {
   // FIXME: Swift submodules?
diff --git a/lib/Serialization/SerializedModuleLoader.cpp b/lib/Serialization/SerializedModuleLoader.cpp
@@ -19,6 +19,7 @@
 #include "swift/Basic/SourceManager.h"
 #include "swift/Basic/Version.h"
 #include "llvm/ADT/SmallString.h"
+#include "llvm/Support/FileSystem.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Debug.h"
@@ -39,15 +40,25 @@ SerializedModuleLoader::~SerializedModuleLoader() = default;
 static std::error_code
 openModuleFiles(StringRef DirName, StringRef ModuleFilename,
                 StringRef ModuleDocFilename,
-                std::unique_ptr<llvm::MemoryBuffer> &ModuleBuffer,
-                std::unique_ptr<llvm::MemoryBuffer> &ModuleDocBuffer,
+                std::unique_ptr<llvm::MemoryBuffer> *ModuleBuffer,
+                std::unique_ptr<llvm::MemoryBuffer> *ModuleDocBuffer,
                 llvm::SmallVectorImpl<char> &Scratch) {
+  assert(((ModuleBuffer && ModuleDocBuffer)
+            || (!ModuleBuffer && !ModuleDocBuffer))
+         && "Module and Module Doc buffer must both be initialized or NULL");
   // Try to open the module file first.  If we fail, don't even look for the
   // module documentation file.
   Scratch.clear();
   llvm::sys::path::append(Scratch, DirName, ModuleFilename);
+  // If there are no buffers to load into, simply check for the existence of
+  // the module file.
+  if (!(ModuleBuffer || ModuleDocBuffer)) {
+    return llvm::sys::fs::access(StringRef(Scratch.data(), Scratch.size()),
+                                 llvm::sys::fs::AccessMode::Exist);
+  }
+
   llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> ModuleOrErr =
-    llvm::MemoryBuffer::getFile(StringRef(Scratch.data(), Scratch.size()));
+  llvm::MemoryBuffer::getFile(StringRef(Scratch.data(), Scratch.size()));
   if (!ModuleOrErr)
     return ModuleOrErr.getError();
 
@@ -56,21 +67,23 @@ openModuleFiles(StringRef DirName, StringRef ModuleFilename,
   Scratch.clear();
   llvm::sys::path::append(Scratch, DirName, ModuleDocFilename);
   llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> ModuleDocOrErr =
-    llvm::MemoryBuffer::getFile(StringRef(Scratch.data(), Scratch.size()));
+  llvm::MemoryBuffer::getFile(StringRef(Scratch.data(), Scratch.size()));
   if (!ModuleDocOrErr &&
       ModuleDocOrErr.getError() != std::errc::no_such_file_or_directory) {
     return ModuleDocOrErr.getError();
   }
-  ModuleBuffer = std::move(ModuleOrErr.get());
+
+  *ModuleBuffer = std::move(ModuleOrErr.get());
   if (ModuleDocOrErr)
-    ModuleDocBuffer = std::move(ModuleDocOrErr.get());
+    *ModuleDocBuffer = std::move(ModuleDocOrErr.get());
+
   return std::error_code();
 }
 
 static bool
 findModule(ASTContext &ctx, AccessPathElem moduleID,
-           std::unique_ptr<llvm::MemoryBuffer> &moduleBuffer,
-           std::unique_ptr<llvm::MemoryBuffer> &moduleDocBuffer,
+           std::unique_ptr<llvm::MemoryBuffer> *moduleBuffer,
+           std::unique_ptr<llvm::MemoryBuffer> *moduleDocBuffer,
            bool &isFramework) {
   llvm::SmallString<64> moduleFilename(moduleID.first.str());
   moduleFilename += '.';
@@ -95,7 +108,6 @@ findModule(ASTContext &ctx, AccessPathElem moduleID,
 
   llvm::SmallString<128> scratch;
   llvm::SmallString<128> currPath;
-
   isFramework = false;
   for (auto path : ctx.SearchPathOpts.ImportSearchPaths) {
     auto err = openModuleFiles(path,
@@ -353,6 +365,21 @@ FileUnit *SerializedModuleLoader::loadAST(
   return nullptr;
 }
 
+bool
+SerializedModuleLoader::canImportModule(std::pair<Identifier, SourceLoc> mID) {
+  // First see if we find it in the registered memory buffers.
+  if (!MemoryBuffers.empty()) {
+    auto bufIter = MemoryBuffers.find(mID.first.str());
+    if (bufIter != MemoryBuffers.end()) {
+      return true;
+    }
+  }
+
+  // Otherwise look on disk.
+  bool isFramework = false;
+  return findModule(Ctx, mID, nullptr, nullptr, isFramework);
+}
+
 Module *SerializedModuleLoader::loadModule(SourceLoc importLoc,
                                            Module::AccessPathTy path) {
   // FIXME: Swift submodules?
@@ -378,7 +405,7 @@ Module *SerializedModuleLoader::loadModule(SourceLoc importLoc,
 
   // Otherwise look on disk.
   if (!moduleInputBuffer) {
-    if (!findModule(Ctx, moduleID, moduleInputBuffer, moduleDocInputBuffer,
+    if (!findModule(Ctx, moduleID, &moduleInputBuffer, &moduleDocInputBuffer,
                     isFramework)) {
       return nullptr;
     }
diff --git a/test/ClangImporter/Inputs/missing-requirement/module.modulemap b/test/ClangImporter/Inputs/missing-requirement/module.modulemap
@@ -0,0 +1,3 @@
+module MissingRequirement {
+  requires missing
+}
diff --git a/test/ClangImporter/MixedSource/can_import_objc_idempotent.swift b/test/ClangImporter/MixedSource/can_import_objc_idempotent.swift
@@ -0,0 +1,40 @@
+// RUN: rm -rf %t && mkdir -p %t
+// RUN: cp -R %S/Inputs/mixed-target %t
+
+// FIXME: BEGIN -enable-source-import hackaround
+// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk) -emit-module -o %t %clang-importer-sdk-path/swift-modules/CoreGraphics.swift
+// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk) -emit-module -o %t %clang-importer-sdk-path/swift-modules/Foundation.swift
+// FIXME: END -enable-source-import hackaround
+
+// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk-nosource -I %t) -I %S/../Inputs/custom-modules -import-objc-header %t/mixed-target/header.h -emit-module-path %t/MixedWithHeader.swiftmodule %S/Inputs/mixed-with-header.swift %S/../../Inputs/empty.swift -module-name MixedWithHeader -disable-objc-attr-requires-foundation-module
+// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk-nosource -I %t) -I %S/../Inputs/custom-modules -typecheck %s -verify
+
+// RUN: rm -rf %t/mixed-target/
+// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk-nosource -I %t) -I %S/../Inputs/custom-modules -typecheck %s -verify
+
+// REQUIRES: objc_interop
+
+// Test that 'canImport(Foo)' directives do not open symbols from 'Foo' into the
+// current module.  Only an 'import Foo' statement should do this.
+
+#if canImport(AppKit)
+  class AppKitView : NSView {} // expected-error {{use of undeclared type 'NSView'}}
+#endif
+
+#if canImport(UIKit)
+  class UIKitView : UIView {} // expected-error {{use of undeclared type 'UIView'}}
+#endif
+
+#if canImport(CoreGraphics)
+  let square = CGRect(x: 100, y: 100, width: 100, height: 100) // expected-error {{use of unresolved identifier 'CGRect'}}
+  // expected-error@-1 {{'square' used within its own type}}
+  // expected-error@-2 {{could not infer type for 'square'}}
+  let (r, s) = square.divided(atDistance: 50, from: .minXEdge)
+#endif
+
+#if canImport(MixedWithHeader)
+let object = NSObject() // expected-error {{use of unresolved identifier 'NSObject'}}
+  // expected-error@-1 {{'object' used within its own type}}
+  // expected-error@-2 {{could not infer type for 'object'}}
+let someAPI = Derived() // expected-error {{use of unresolved identifier 'Derived'}}
+#endif
diff --git a/test/ClangImporter/can_import_missing_requirement.swift b/test/ClangImporter/can_import_missing_requirement.swift
@@ -0,0 +1,9 @@
+// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk) -typecheck -I %S/Inputs/missing-requirement %s -verify
+
+// REQUIRES: objc_interop
+
+class Unique {}
+
+#if canImport(MissingRequirement)
+  class Unique {} // No diagnostic
+#endif
diff --git a/test/Parse/ConditionalCompilation/can_import_idempotent.swift b/test/Parse/ConditionalCompilation/can_import_idempotent.swift

Original file line number	Diff line number	Diff line change
`@@ -1789,8 +1789,8 @@ Parser::classifyConditionalCompilationExpr(Expr *condition,`
`1789`	`1789`	`if (!fullCheck) {`
`1790`	`1790`	`return None;`
`1791`	`1791`	`}`
`1792`		`- return`
`1793`		`- Context.getModule({ { argumentIdent, UDRE->getLoc() } }) != nullptr;`
	`1792`	`+`
	`1793`	`+ return Context.canImportModule({ argumentIdent, UDRE->getLoc() });`
`1794`	`1794`	`}`
`1795`	`1795`
`1796`	`1796`	`if (!fullCheck) {`
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+module MissingRequirement {`
	`2`	`+ requires missing`
	`3`	`+}`