Merge pull request swiftlang#35162 from slavapestov/remove-expand-astscope-request

Remove the ExpandASTScopeRequest

Author: slavapestov

include/swift/AST/ASTScope.h

@@ -136,18 +136,16 @@ class ASTScopeImpl {
   /// storage declaration or is directly descended from it.
 
 private:
-  /// Always set by the constructor, so that when creating a child
-  /// the parent chain is available.
-  ASTScopeImpl *parent = nullptr; // null at the root
+  /// The pointer:
+  /// - Always set by the constructor, so that when creating a child
+  ///   the parent chain is available. Null at the root.
+  /// The int:
+  /// - A flag indicating if the scope has been expanded yet or not.
+  llvm::PointerIntPair<ASTScopeImpl *, 1> parentAndWasExpanded;
 
   /// Child scopes, sorted by source range.
   Children storedChildren;
 
-  bool wasExpanded = false;
-
-  /// Can clear storedChildren, so must remember this
-  bool haveAddedCleanup = false;
-
   mutable Optional<CharSourceRange> cachedCharSourceRange;
 
 #pragma mark - constructor / destructor
@@ -177,8 +175,12 @@ class ASTScopeImpl {
 
 #pragma mark - tree declarations
 protected:
-  NullablePtr<ASTScopeImpl> getParent() { return parent; }
-  NullablePtr<const ASTScopeImpl> getParent() const { return parent; }
+  NullablePtr<ASTScopeImpl> getParent() {
+    return parentAndWasExpanded.getPointer();
+  }
+  NullablePtr<const ASTScopeImpl> getParent() const {
+    return parentAndWasExpanded.getPointer();
+  }
 
   const Children &getChildren() const { return storedChildren; }
 
@@ -247,19 +249,13 @@ class ASTScopeImpl {
 
 #pragma mark - Scope tree creation
 public:
-  /// 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.
+  /// Expand the scope if unexpanded.
   ASTScopeImpl *expandAndBeCurrent(ScopeCreator &);
 
-  bool getWasExpanded() const { return wasExpanded; }
+  bool getWasExpanded() const { return parentAndWasExpanded.getInt(); }
 
 protected:
-  void setWasExpanded() { wasExpanded = true; }
+  void setWasExpanded() { parentAndWasExpanded.setInt(1); }
   virtual ASTScopeImpl *expandSpecifically(ScopeCreator &) = 0;
 
 public:
@@ -360,7 +356,7 @@ class ASTScopeImpl {
   /// what obtaines for scoping. However, guards are different. The scope after
   /// the guard else must hop into the innermoset scope of the guard condition.
   virtual NullablePtr<const ASTScopeImpl> getLookupParent() const {
-    return parent;
+    return getParent();
   }
 
 #pragma mark - - lookup- local bindings
@@ -629,8 +625,6 @@ class IterableTypeScope : public GenericTypeScope {
 
 public:
   NullablePtr<ASTScopeImpl> insertionPointForDeferredExpansion() override;
-
-  void countBodies(ScopeCreator &) const;
 };
 
 class NominalTypeScope final : public IterableTypeScope {

include/swift/AST/NameLookupRequests.h

@@ -350,30 +350,6 @@ class GenericParamListRequest :
   void cacheResult(GenericParamList *value) const;
 };
 
-/// Expand the given ASTScope. Requestified to detect recursion.
-class ExpandASTScopeRequest
-    : public SimpleRequest<ExpandASTScopeRequest,
-                           ast_scope::ASTScopeImpl *(ast_scope::ASTScopeImpl *,
-                                                     ast_scope::ScopeCreator *),
-                           RequestFlags::SeparatelyCached> {
-public:
-  using SimpleRequest::SimpleRequest;
-
-private:
-  friend SimpleRequest;
-
-  // Evaluation.
-  ast_scope::ASTScopeImpl *
-  evaluate(Evaluator &evaluator, ast_scope::ASTScopeImpl *,
-           ast_scope::ScopeCreator *) const;
-
-public:
-  // Separate caching.
-  bool isCached() const;
-  Optional<ast_scope::ASTScopeImpl *> getCachedResult() const;
-  void cacheResult(ast_scope::ASTScopeImpl *) const {}
-};
-
 /// The input type for an unqualified lookup request.
 class UnqualifiedLookupDescriptor {
   using LookupOptions = OptionSet<UnqualifiedLookupFlags>;

include/swift/AST/NameLookupTypeIDZone.def

@@ -31,10 +31,6 @@ SWIFT_REQUEST(NameLookup, DirectOperatorLookupRequest,
 SWIFT_REQUEST(NameLookup, DirectPrecedenceGroupLookupRequest,
               TinyPtrVector<PrecedenceGroupDecl *>(OperatorLookupDescriptor),
               Uncached, NoLocationInfo)
-SWIFT_REQUEST(NameLookup, ExpandASTScopeRequest,
-              ast_scope::ASTScopeImpl* (ast_scope::ASTScopeImpl*, ast_scope::ScopeCreator*),
-              SeparatelyCached,
-              NoLocationInfo)
 SWIFT_REQUEST(NameLookup, ExtendedNominalRequest,
               NominalTypeDecl *(ExtensionDecl *), SeparatelyCached,
               NoLocationInfo)

include/swift/Basic/Statistics.def

@@ -144,21 +144,12 @@ FRONTEND_STATISTIC(AST, NumModuleLookupValue)
 /// AnyObject lookup.
 FRONTEND_STATISTIC(AST, NumModuleLookupClassMember)
 
-/// Number of body scopes for iterable types
-FRONTEND_STATISTIC(AST, NumIterableTypeBodyASTScopes)
-
-/// Number of expansions of body scopes for iterable types
-FRONTEND_STATISTIC(AST, NumIterableTypeBodyASTScopeExpansions)
-
-/// Number of brace statment scopes for iterable types
-FRONTEND_STATISTIC(AST, NumBraceStmtASTScopes)
-
-/// Number of expansions of brace statement scopes for iterable types
-FRONTEND_STATISTIC(AST, NumBraceStmtASTScopeExpansions)
-
 /// Number of ASTScope lookups
 FRONTEND_STATISTIC(AST, NumASTScopeLookups)
 
+/// Number of ASTScope expansions
+FRONTEND_STATISTIC(AST, NumASTScopeExpansions)
+
 /// Number of lookups of the cached import graph for a module or
 /// source file.
 FRONTEND_STATISTIC(AST, ImportSetFoldHit)

lib/AST/ASTScopeCreation.cpp

@@ -82,8 +82,7 @@ class ScopeCreator final {
     if (auto *ip = child->insertionPointForDeferredExpansion().getPtrOrNull())
       return ip;
 
-    ASTScopeImpl *insertionPoint =
-        child->expandAndBeCurrentDetectingRecursion(*this);
+    ASTScopeImpl *insertionPoint = child->expandAndBeCurrent(*this);
     return insertionPoint;
   }
 
@@ -224,15 +223,18 @@ ASTSourceFileScope *ASTScope::createScopeTree(SourceFile *SF) {
 }
 
 void ASTSourceFileScope::buildFullyExpandedTree() {
-  expandAndBeCurrentDetectingRecursion(*scopeCreator);
+  if (!getWasExpanded())
+    expandAndBeCurrent(*scopeCreator);
   preOrderChildrenDo([&](ASTScopeImpl *s) {
-    s->expandAndBeCurrentDetectingRecursion(*scopeCreator);
+    if (!s->getWasExpanded())
+      s->expandAndBeCurrent(*scopeCreator);
   });
 }
 
 void ASTSourceFileScope::
     buildEnoughOfTreeForTopLevelExpressionsButDontRequestGenericsOrExtendedNominals() {
-      expandAndBeCurrentDetectingRecursion(*scopeCreator);
+  if (!getWasExpanded())
+    expandAndBeCurrent(*scopeCreator);
 }
 
 void ASTSourceFileScope::expandFunctionBody(AbstractFunctionDecl *AFD) {
@@ -242,7 +244,8 @@ void ASTSourceFileScope::expandFunctionBody(AbstractFunctionDecl *AFD) {
   if (sr.isInvalid())
     return;
   ASTScopeImpl *bodyScope = findInnermostEnclosingScope(sr.Start, nullptr);
-  bodyScope->expandAndBeCurrentDetectingRecursion(*scopeCreator);
+  if (!bodyScope->getWasExpanded())
+    bodyScope->expandAndBeCurrent(*scopeCreator);
 }
 
 ASTSourceFileScope::ASTSourceFileScope(SourceFile *SF,
@@ -411,8 +414,6 @@ class NodeAdder
       endLocForBraceStmt = *endLoc;
 
     ASTContext &ctx = scopeCreator.getASTContext();
-    if (auto *s = ctx.Stats)
-      ++s->getFrontendCounters().NumBraceStmtASTScopes;
 
     return
         scopeCreator.constructExpandAndInsert<BraceStmtScope>(
@@ -578,44 +579,33 @@ ScopeCreator::addPatternBindingToScopeTree(PatternBindingDecl *patternBinding,
 
 void ASTScopeImpl::addChild(ASTScopeImpl *child, ASTContext &ctx) {
   ASTScopeAssert(!child->getParent(), "child should not already have parent");
-  child->parent = this;
+  child->parentAndWasExpanded.setPointer(this);
 
 #ifndef NDEBUG
   checkSourceRangeBeforeAddingChild(child, ctx);
 #endif
 
   // If this is the first time we've added children, notify the ASTContext
   // that there's a SmallVector that needs to be cleaned up.
-  // FIXME: If we had access to SmallVector::isSmall(), we could do better.
-  if (storedChildren.empty() && !haveAddedCleanup) {
+  if (storedChildren.empty())
     ctx.addDestructorCleanup(storedChildren);
-    haveAddedCleanup = true;
-  }
+
   storedChildren.push_back(child);
 }
 
 #pragma mark implementations of expansion
 
-ASTScopeImpl *
-ASTScopeImpl::expandAndBeCurrentDetectingRecursion(ScopeCreator &scopeCreator) {
-  return evaluateOrDefault(scopeCreator.getASTContext().evaluator,
-                           ExpandASTScopeRequest{this, &scopeCreator}, nullptr);
-}
-
-ASTScopeImpl *
-ExpandASTScopeRequest::evaluate(Evaluator &evaluator, ASTScopeImpl *parent,
-                                ScopeCreator *scopeCreator) const {
-  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;
-}
-
 ASTScopeImpl *ASTScopeImpl::expandAndBeCurrent(ScopeCreator &scopeCreator) {
   ASTScopeAssert(!getWasExpanded(),
                  "Cannot expand the same scope twice");
 
+  // Set the flag before we actually expand, to detect re-entrant calls
+  // via the above assertion.
+  setWasExpanded();
+
+  if (auto *s = scopeCreator.getASTContext().Stats)
+    ++s->getFrontendCounters().NumASTScopeExpansions;
+
   auto *insertionPoint = expandSpecifically(scopeCreator);
   ASTScopeAssert(!insertionPointForDeferredExpansion() ||
                      insertionPointForDeferredExpansion().get() ==
@@ -624,8 +614,6 @@ ASTScopeImpl *ASTScopeImpl::expandAndBeCurrent(ScopeCreator &scopeCreator) {
                  "accurate before expansion, the insertion point before "
                  "expansion must be the same as after expansion.");
 
-  setWasExpanded();
-
   return insertionPoint;
 }
 
@@ -826,9 +814,6 @@ BraceStmtScope::expandAScopeThatCreatesANewInsertionPoint(
         nd, insertionPoint, endLoc);
   }
 
-  if (auto *s = scopeCreator.getASTContext().Stats)
-    ++s->getFrontendCounters().NumBraceStmtASTScopeExpansions;
-
   return {
       insertionPoint,
       "For top-level code decls, need the scope under, say a guard statment."};
@@ -1085,24 +1070,17 @@ ASTScopeImpl *GenericTypeOrExtensionWherePortion::expandScope(
 
 #pragma mark createBodyScope
 
-void IterableTypeScope::countBodies(ScopeCreator &scopeCreator) const {
-  if (auto *s = scopeCreator.getASTContext().Stats)
-    ++s->getFrontendCounters().NumIterableTypeBodyASTScopes;
-}
-
 void ExtensionScope::createBodyScope(ASTScopeImpl *leaf,
                                      ScopeCreator &scopeCreator) {
   scopeCreator.constructWithPortionExpandAndInsert<ExtensionScope,
                                                    IterableTypeBodyPortion>(
       leaf, decl);
-  countBodies(scopeCreator);
 }
 void NominalTypeScope::createBodyScope(ASTScopeImpl *leaf,
                                        ScopeCreator &scopeCreator) {
   scopeCreator.constructWithPortionExpandAndInsert<NominalTypeScope,
                                                    IterableTypeBodyPortion>(
       leaf, decl);
-  countBodies(scopeCreator);
 }
 
 #pragma mark createTrailingWhereClauseScope
@@ -1192,9 +1170,6 @@ void GenericTypeOrExtensionScope::expandBody(ScopeCreator &) {}
 void IterableTypeScope::expandBody(ScopeCreator &scopeCreator) {
   for (auto *d : getIterableDeclContext().get()->getMembers())
     scopeCreator.addToScopeTree(ASTNode(d), this);
-
-  if (auto *s = scopeCreator.getASTContext().Stats)
-    ++s->getFrontendCounters().NumIterableTypeBodyASTScopeExpansions;
 }
 
 #pragma mark getScopeCreator
@@ -1241,16 +1216,3 @@ 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());
-  return scope->getWasExpanded();
-}
-
-Optional<ASTScopeImpl *> ExpandASTScopeRequest::getCachedResult() const {
-  return std::get<0>(getStorage());
-}

lib/AST/ASTScopeLookup.cpp

@@ -64,7 +64,8 @@ ASTScopeImpl::findInnermostEnclosingScope(SourceLoc loc,
 ASTScopeImpl *ASTScopeImpl::findInnermostEnclosingScopeImpl(
     SourceLoc loc, NullablePtr<raw_ostream> os, SourceManager &sourceMgr,
     ScopeCreator &scopeCreator) {
-  expandAndBeCurrentDetectingRecursion(scopeCreator);
+  if (!getWasExpanded())
+    expandAndBeCurrent(scopeCreator);
   auto child = findChildContaining(loc, sourceMgr);
   if (!child)
     return this;