Use SeparateCaching

Author: David Ungar

include/swift/AST/ASTScope.h

@@ -343,6 +343,12 @@ class ASTScopeImpl {
   /// expandScope me, sending deferred nodes to my descendants.
   /// Return the scope into which to place subsequent decls
   ASTScopeImpl *expandAndBeCurrentDetectingRecursion(ScopeCreator &);
+
+  /// Expand or reexpand the scope if unexpanded or if not current.
+  /// There are several places in the compiler that mutate the AST after the
+  /// fact, above and beyond adding Decls to the SourceFile. These are
+  /// documented in: rdar://53018839, rdar://53027266, rdar://53027733,
+  /// rdar://53028050
   ASTScopeImpl *expandAndBeCurrent(ScopeCreator &);
 
   unsigned getASTAncestorScopeCount() const { return astAncestorScopeCount; }
@@ -354,6 +360,12 @@ class ASTScopeImpl {
   void setWasExpanded() { wasExpanded = true; }
   virtual ASTScopeImpl *expandSpecifically(ScopeCreator &) = 0;
   virtual void beCurrent();
+  virtual bool doesExpansionOnlyAddNewDeclsAtEnd() const;
+
+public:
+  bool isExpansionNeeded(const ScopeCreator &) const;
+
+protected:
   bool isCurrent() const;
   virtual bool isCurrentIfWasExpanded() const;
 
@@ -384,16 +396,7 @@ class ASTScopeImpl {
 
   bool isATypeDeclScope() const;
 
-  /// There are several places in the compiler that mutate the AST after the
-  /// fact, above and beyond adding Decls to the SourceFile. These are
-  /// documented in: rdar://53018839, rdar://53027266, rdar://53027733,
-  /// rdar://53028050
-  /// Return true if did reexpand
-  bool reexpandIfObsolete(ScopeCreator &);
-
 private:
-  void reexpand(ScopeCreator &);
-
   virtual ScopeCreator &getScopeCreator();
 
 #pragma mark - - creation queries
@@ -544,7 +547,7 @@ class ASTSourceFileScope final : public ASTScopeImpl {
   /// Since parsing can be interleaved with type-checking, on every
   /// lookup, look at creating scopes for any \c Decls beyond this number.
   /// rdar://55562483 Unify with numberOfChildrenWhenLastExpanded
-  int numberOfDeclsAlreadySeen = 0;
+  size_t numberOfDeclsAlreadySeen = 0;
 
   ASTSourceFileScope(SourceFile *SF, ScopeCreator *scopeCreator);
 
@@ -558,7 +561,6 @@ class ASTSourceFileScope final : public ASTScopeImpl {
 public:
   NullablePtr<DeclContext> getDeclContext() const override;
 
-  void addNewDeclsToScopeTree();
   void buildFullyExpandedTree();
   void
   buildEnoughOfTreeForTopLevelExpressionsButDontRequestGenericsOrExtendedNominals();
@@ -569,11 +571,15 @@ class ASTSourceFileScope final : public ASTScopeImpl {
 
 protected:
   ASTScopeImpl *expandSpecifically(ScopeCreator &scopeCreator) override;
+  bool isCurrentIfWasExpanded() const override;
+  void beCurrent() override;
+  bool doesExpansionOnlyAddNewDeclsAtEnd() const override;
 
   ScopeCreator &getScopeCreator() override;
 
 private:
-  void expandAScopeThatDoesNotCreateANewInsertionPoint(ScopeCreator &);
+  AnnotatedInsertionPoint
+  expandAScopeThatCreatesANewInsertionPoint(ScopeCreator &);
 };
 
 class Portion {

include/swift/AST/NameLookupRequests.h

@@ -333,7 +333,7 @@ class ExpandASTScopeRequest
     : public SimpleRequest<ExpandASTScopeRequest,
                            ast_scope::ASTScopeImpl *(ast_scope::ASTScopeImpl *,
                                                      ast_scope::ScopeCreator *),
-                           CacheKind::Uncached> {
+                           CacheKind::SeparatelyCached> {
 public:
   using SimpleRequest::SimpleRequest;
 
@@ -346,8 +346,10 @@ class ExpandASTScopeRequest
            ast_scope::ScopeCreator *) const;
 
 public:
-  // Caching
-  bool isCached() const { return true; }
+  // Separate caching.
+  bool isCached() const;
+  Optional<ast_scope::ASTScopeImpl *> getCachedResult() const;
+  void cacheResult(ast_scope::ASTScopeImpl *) const {}
 };
 
 #define SWIFT_TYPEID_ZONE NameLookup

include/swift/AST/NameLookupTypeIDZone.def

@@ -20,7 +20,7 @@ SWIFT_REQUEST(NameLookup, CustomAttrNominalRequest,
               NoLocationInfo)
 SWIFT_REQUEST(NameLookup, ExpandASTScopeRequest,
               ast_scope::ASTScopeImpl* (ast_scope::ASTScopeImpl*, ast_scope::ScopeCreator*),
-              Uncached,
+              SeparatelyCached,
               NoLocationInfo)
 SWIFT_REQUEST(NameLookup, ExtendedNominalRequest,
               NominalTypeDecl *(ExtensionDecl *), SeparatelyCached,

lib/AST/ASTScopeCreation.cpp

@@ -654,9 +654,8 @@ class ScopeCreator final {
   bool containsAllDeclContextsFromAST() {
     auto allDeclContexts = findLocalizableDeclContextsInAST();
     llvm::DenseMap<const DeclContext *, const ASTScopeImpl *> bogusDCs;
-    bool rebuilt = false;
     sourceFileScope->preOrderDo([&](ASTScopeImpl *scope) {
-      rebuilt |= scope->reexpandIfObsolete(*this);
+      scope->expandAndBeCurrentDetectingRecursion(*this);
     });
     sourceFileScope->postOrderDo([&](ASTScopeImpl *scope) {
       if (auto *dc = scope->getDeclContext().getPtrOrNull()) {
@@ -673,8 +672,6 @@ class ScopeCreator final {
       d->getSourceRange().dump(ctx.SourceMgr);
       llvm::errs() << " : ";
       d->dump(llvm::errs());
-      if (rebuilt)
-        llvm::errs() << " (rebuilt)";
       llvm::errs() << "\n";
     };
     bool foundOmission = false;
@@ -750,35 +747,17 @@ ASTSourceFileScope *ASTScope::createScopeTree(SourceFile *SF) {
 }
 
 void ASTSourceFileScope::buildFullyExpandedTree() {
-  addNewDeclsToScopeTree();
-  preOrderChildrenDo(
-      [&](ASTScopeImpl *s) { s->reexpandIfObsolete(*scopeCreator); });
+  expandAndBeCurrentDetectingRecursion(*scopeCreator);
+  preOrderChildrenDo([&](ASTScopeImpl *s) {
+    s->expandAndBeCurrentDetectingRecursion(*scopeCreator);
+  });
 }
 
 void ASTSourceFileScope::
     buildEnoughOfTreeForTopLevelExpressionsButDontRequestGenericsOrExtendedNominals() {
-  addNewDeclsToScopeTree();
+      expandAndBeCurrentDetectingRecursion(*scopeCreator);
 }
 
-void ASTSourceFileScope::addNewDeclsToScopeTree() {
-  ASTScopeAssert(SF && scopeCreator,
-                 "Must already have a SourceFile and a ScopeCreator.");
-  ArrayRef<Decl *> decls = SF->Decls;
-  // Assume that decls are only added at the end, in source order
-  ArrayRef<Decl *> newDecls = decls.slice(numberOfDeclsAlreadySeen);
-  std::vector<ASTNode> newNodes(newDecls.begin(), newDecls.end());
-  insertionPoint =
-      scopeCreator->addSiblingsToScopeTree(insertionPoint, this, newNodes);
-
-  // TODO: use regular expansion machinery for ASTSourceFileScope
-  // rdar://55562483
-  numberOfDeclsAlreadySeen = SF->Decls.size();
-  setWasExpanded();
-
-  // Too slow to perform all the time:
-  //    ASTScopeAssert(scopeCreator->containsAllDeclContextsFromAST(),
-  //           "ASTScope tree missed some DeclContexts or made some up");
-}
 
 ASTSourceFileScope::ASTSourceFileScope(SourceFile *SF,
                                        ScopeCreator *scopeCreator)
@@ -1094,10 +1073,33 @@ ASTScopeImpl::expandAndBeCurrentDetectingRecursion(ScopeCreator &scopeCreator) {
 llvm::Expected<ASTScopeImpl *>
 ExpandASTScopeRequest::evaluate(Evaluator &evaluator, ASTScopeImpl *parent,
                                 ScopeCreator *scopeCreator) const {
-  return parent->expandAndBeCurrent(*scopeCreator);
+  auto *insertionPoint = parent->expandAndBeCurrent(*scopeCreator);
+  ASTScopeAssert(insertionPoint,
+                 "Used to return a null pointer if the insertion point would "
+                 "not be used, but it breaks the request dependency hashing");
+  return insertionPoint;
+}
+
+bool ASTScopeImpl::doesExpansionOnlyAddNewDeclsAtEnd() const { return false; }
+bool ASTSourceFileScope::doesExpansionOnlyAddNewDeclsAtEnd() const {
+  return true;
 }
 
 ASTScopeImpl *ASTScopeImpl::expandAndBeCurrent(ScopeCreator &scopeCreator) {
+
+  // We might be reexpanding, so save any scopes that were inserted here from
+  // above it in the AST
+  auto astAncestorScopes = rescueASTAncestorScopesForReuseFromMeOrDescendants();
+  ASTScopeAssert(astAncestorScopes.empty() ||
+                     !doesExpansionOnlyAddNewDeclsAtEnd(),
+                 "ASTSourceFileScope has no ancestors to be rescued.");
+
+  // If reexpanding, we need to remove descendant decls from the duplication set
+  // in order to re-add them as sub-scopes. Since expansion only adds new Decls
+  // at end, don't bother with descendants
+  if (!doesExpansionOnlyAddNewDeclsAtEnd())
+    disownDescendants(scopeCreator);
+
   auto *insertionPoint = expandSpecifically(scopeCreator);
   if (scopeCreator.shouldBeLazy()) {
     ASTScopeAssert(!insertionPointForDeferredExpansion() ||
@@ -1107,6 +1109,7 @@ ASTScopeImpl *ASTScopeImpl::expandAndBeCurrent(ScopeCreator &scopeCreator) {
                    "accurate before expansion, the insertion point before "
                    "expansion must be the same as after expansion.");
   }
+  replaceASTAncestorScopes(astAncestorScopes);
   setWasExpanded();
   beCurrent();
   ASTScopeAssert(checkSourceRangeAfterExpansion(scopeCreator.getASTContext()),
@@ -1132,6 +1135,7 @@ ASTScopeImpl *ASTScopeImpl::expandAndBeCurrent(ScopeCreator &scopeCreator) {
 #define NO_EXPANSION(Scope)                                                    \
   ASTScopeImpl *Scope::expandSpecifically(ScopeCreator &) { return this; }
 
+CREATES_NEW_INSERTION_POINT(ASTSourceFileScope)
 CREATES_NEW_INSERTION_POINT(ParameterListScope)
 CREATES_NEW_INSERTION_POINT(ConditionalClauseScope)
 CREATES_NEW_INSERTION_POINT(GuardStmtScope)
@@ -1163,7 +1167,6 @@ NO_NEW_INSERTION_POINT(WhileStmtScope)
 NO_NEW_INSERTION_POINT(WholeClosureScope)
 
 NO_EXPANSION(GenericParamScope)
-NO_EXPANSION(ASTSourceFileScope)
 NO_EXPANSION(ClosureParametersScope)
 NO_EXPANSION(SpecializeAttributeScope)
 NO_EXPANSION(ConditionalClausePatternUseScope)
@@ -1172,6 +1175,22 @@ NO_EXPANSION(LookupParentDiversionScope)
 #undef CREATES_NEW_INSERTION_POINT
 #undef NO_NEW_INSERTION_POINT
 
+AnnotatedInsertionPoint
+ASTSourceFileScope::expandAScopeThatCreatesANewInsertionPoint(
+    ScopeCreator &scopeCreator) {
+  ASTScopeAssert(SF, "Must already have a SourceFile.");
+  ArrayRef<Decl *> decls = SF->Decls;
+  // Assume that decls are only added at the end, in source order
+  ArrayRef<Decl *> newDecls = decls.slice(numberOfDeclsAlreadySeen);
+  std::vector<ASTNode> newNodes(newDecls.begin(), newDecls.end());
+  insertionPoint =
+      scopeCreator.addSiblingsToScopeTree(insertionPoint, this, newNodes);
+  // Too slow to perform all the time:
+  //    ASTScopeAssert(scopeCreator->containsAllDeclContextsFromAST(),
+  //           "ASTScope tree missed some DeclContexts or made some up");
+  return {insertionPoint, "Next time decls are added they go here."};
+}
+
 AnnotatedInsertionPoint
 ParameterListScope::expandAScopeThatCreatesANewInsertionPoint(
     ScopeCreator &scopeCreator) {
@@ -1230,7 +1249,8 @@ PatternEntryInitializerScope::expandAScopeThatCreatesANewInsertionPoint(
                   "get its endpoint in order to push back start of "
                   "PatternEntryUseScope"};
 
-  return {nullptr, "Unused"};
+  // null pointer here blows up request printing
+  return {getParent().get(), "Unused"};
 }
 
 AnnotatedInsertionPoint
@@ -1308,11 +1328,6 @@ TopLevelCodeScope::expandAScopeThatCreatesANewInsertionPoint(ScopeCreator &
 
 #pragma mark expandAScopeThatDoesNotCreateANewInsertionPoint
 
-void ASTSourceFileScope::expandAScopeThatDoesNotCreateANewInsertionPoint(
-    ScopeCreator &scopeCreator) {
-  ASTScope_unreachable("expanded by addNewDeclsToScopeTree()");
-}
-
 // Create child scopes for every declaration in a body.
 
 void AbstractFunctionDeclScope::expandAScopeThatDoesNotCreateANewInsertionPoint(
@@ -1752,27 +1767,6 @@ void IterableTypeScope::expandBody(ScopeCreator &scopeCreator) {
     ++s->getFrontendCounters().NumIterableTypeBodyASTScopeExpansions;
 }
 
-#pragma mark - reexpandIfObsolete
-
-bool ASTScopeImpl::reexpandIfObsolete(ScopeCreator &scopeCreator) {
-  if (isCurrent() &&
-      !scopeCreator.getASTContext().LangOpts.StressASTScopeLookup) {
-    ASTScopeAssert(getWasExpanded(), "Cannot be current if unexpanded.");
-    return false;
-  }
-  reexpand(scopeCreator);
-  return true;
-}
-
-void ASTScopeImpl::reexpand(ScopeCreator &scopeCreator) {
-  auto astAncestorScopes = rescueASTAncestorScopesForReuseFromMeOrDescendants();
-  disownDescendants(scopeCreator);
-  // If the expansion recurses back into the tree for lookup, the ASTAncestor
-  // scopes will have already been rescued and won't be found!
-  expandAndBeCurrentDetectingRecursion(scopeCreator);
-  replaceASTAncestorScopes(astAncestorScopes);
-}
-
 #pragma mark getScopeCreator
 ScopeCreator &ASTScopeImpl::getScopeCreator() {
   return getParent().get()->getScopeCreator();
@@ -1847,13 +1841,25 @@ IterableTypeBodyPortion::insertionPointForDeferredExpansion(
   return s->getParent().get();
 }
 
+bool ASTScopeImpl::isExpansionNeeded(const ScopeCreator &scopeCreator) const {
+  return !isCurrent() ||
+         scopeCreator.getASTContext().LangOpts.StressASTScopeLookup;
+}
+
 bool ASTScopeImpl::isCurrent() const {
   return getWasExpanded() && isCurrentIfWasExpanded();
 }
 
 void ASTScopeImpl::beCurrent() {}
 bool ASTScopeImpl::isCurrentIfWasExpanded() const { return true; }
 
+void ASTSourceFileScope::beCurrent() {
+  numberOfDeclsAlreadySeen = SF->Decls.size();
+}
+bool ASTSourceFileScope::isCurrentIfWasExpanded() const {
+  return SF->Decls.size() == numberOfDeclsAlreadySeen;
+}
+
 void IterableTypeScope::beCurrent() { portion->beCurrent(this); }
 bool IterableTypeScope::isCurrentIfWasExpanded() const {
   return portion->isCurrentIfWasExpanded(this);
@@ -2092,3 +2098,17 @@ void ast_scope::simple_display(llvm::raw_ostream &out,
                                const ScopeCreator *scopeCreator) {
   scopeCreator->print(out);
 }
+
+//----------------------------------------------------------------------------//
+// ExpandASTScopeRequest computation.
+//----------------------------------------------------------------------------//
+
+bool ExpandASTScopeRequest::isCached() const {
+  ASTScopeImpl *scope = std::get<0>(getStorage());
+  ScopeCreator *scopeCreator = std::get<1>(getStorage());
+  return !scope->isExpansionNeeded(*scopeCreator);
+}
+
+Optional<ASTScopeImpl *> ExpandASTScopeRequest::getCachedResult() const {
+  return std::get<0>(getStorage());
+}

lib/AST/ASTScopeLookup.cpp

@@ -40,7 +40,6 @@ llvm::SmallVector<const ASTScopeImpl *, 0> ASTScopeImpl::unqualifiedLookup(
     SourceFile *sourceFile, const DeclName name, const SourceLoc loc,
     const DeclContext *const startingContext, DeclConsumer consumer) {
   SmallVector<const ASTScopeImpl *, 0> history;
-
   const auto *start =
       findStartingScopeForLookup(sourceFile, name, loc, startingContext);
   if (start)
@@ -59,7 +58,6 @@ const ASTScopeImpl *ASTScopeImpl::findStartingScopeForLookup(
 
   auto *const fileScope = sourceFile->getScope().impl;
   // Parser may have added decls to source file, since previous lookup
-  fileScope->addNewDeclsToScopeTree();
   if (name.isOperator())
     return fileScope; // operators always at file scope
 
@@ -116,7 +114,7 @@ ASTScopeImpl::findInnermostEnclosingScope(SourceLoc loc,
 const ASTScopeImpl *ASTScopeImpl::findInnermostEnclosingScopeImpl(
     SourceLoc loc, NullablePtr<raw_ostream> os, SourceManager &sourceMgr,
     ScopeCreator &scopeCreator) {
-  reexpandIfObsolete(scopeCreator);
+  expandAndBeCurrentDetectingRecursion(scopeCreator);
   auto child = findChildContaining(loc, sourceMgr);
   if (!child)
     return this;

lib/AST/ASTScopePrinting.cpp

@@ -204,5 +204,9 @@ bool IterableTypeScope::doesDeclHaveABody() const {
 
 void ast_scope::simple_display(llvm::raw_ostream &out,
                                const ASTScopeImpl *scope) {
-  scope->print(out);
+  // Cannot call scope->print(out) because printing an ASTFunctionBodyScope
+  // gets the source range which can cause a request to parse it.
+  // That in turn causes the request dependency printing code to blow up
+  // as the AnyRequest ends up with a null.
+  out << scope->getClassName() << "\n";
 }