ASTScope: Remove the ExpandASTScopeRequest

This doesn't really fit the request evaluator model since the
result of evaluating the request is the new insertion point,
but we don't have a way to get the insertion point of an
already-expanded scope.

Instead, let's change the callers of the now-removed
expandAndBeCurrentDetectingRecursion() to instead call
expandAndBeCurrent(), while checking first if the scope was
already expanded.

Also, set the expanded flag before calling expandSpecifically()
from inside expandAndBeCurrent(), to ensure that re-entrant
calls to expandAndBeCurrent() are flagged by the existing
assertion there.

Finally, replace a couple of existing counters, and the
now-gone request counter with a single ASTScopeExpansions
counter to track expansions.

Author: slavapestov

include/swift/AST/ASTScope.h

@@ -247,13 +247,7 @@ 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. Asserts if it was already expanded.
   ASTScopeImpl *expandAndBeCurrent(ScopeCreator &);
 
   bool getWasExpanded() const { return wasExpanded; }
@@ -629,8 +623,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>(
@@ -596,26 +597,17 @@ void ASTScopeImpl::addChild(ASTScopeImpl *child, ASTContext &ctx) {
 
 #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 +616,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 +816,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 +1072,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 +1172,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 +1218,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;