ASTScope: Local pattern bindings are now introduced by a new scope

Repurpose PatternEntryDeclScope for local bindings. The initializer
expression is now outside of this scope, which begins immediately
after the initializer (or the pattern, if there is no initializer)
ends.

The scope becomes a new insertion point for the parent BraceStmt,
since local pattern bindings become visible until the end of the
enclosing lexical scope.

Author: slavapestov

include/swift/AST/ASTScope.h

@@ -344,9 +344,6 @@ class ASTScopeImpl {
   virtual NullablePtr<ASTScopeImpl> insertionPointForDeferredExpansion();
   virtual SourceRange sourceRangeForDeferredExpansion() const;
 
-public:
-  bool isATypeDeclScope() const;
-
 private:
   virtual ScopeCreator &getScopeCreator();
 
@@ -1018,31 +1015,25 @@ class AttachedPropertyWrapperScope final : public ASTScopeImpl {
 /// PatternBindingDecl's (PBDs) are tricky (See the comment for \c
 /// PatternBindingDecl):
 ///
-/// A PBD contains a list of "patterns", e.g.
-///   var (a, b) = foo(), (c,d) = bar() which has two patterns.
+/// A PBD contains a list of "pattern binding entries", eg
+///   var (a, b) = foo(), (c,d) = bar() has two pattern bindings.
 ///
-/// For each pattern, there will be potentially three scopes:
-/// always one for the declarations, maybe one for the initializers, and maybe
-/// one for users of that pattern.
+/// A pattern binding entry consists of a "pattern" which binds zero or more
+/// names, together with an optional initializer expression.
 ///
-/// If a PBD occurs in code, its initializer can access all prior declarations.
-/// Thus, a new scope must be created, nested in the scope of the PBD.
-/// In contrast, if a PBD occurs in a type declaration body, its initializer
-/// cannot access prior declarations in that body.
+/// The initializer expression gets its own scope. This scope can introduce
+/// a 'self' binding. Currently this is only used by initializer expressions
+/// of 'lazy' properties, which can access the containing 'self' binding.
 ///
-/// As a further complication, we get VarDecls and their accessors in deferred
-/// which really must go into one of the PBD scopes. So we discard them in
-/// createIfNeeded, and special-case their creation in
-/// addVarDeclScopesAndTheirAccessors.
-
+/// Pattern binding entries in local context also define a scope to
+/// introduce the bindings. This scope begins immediately after the
+/// initializer expression (or parent) ends.
 class AbstractPatternEntryScope : public ASTScopeImpl {
 public:
   PatternBindingDecl *const decl;
   const unsigned patternEntryIndex;
-  const DeclVisibilityKind vis;
 
-  AbstractPatternEntryScope(PatternBindingDecl *, unsigned entryIndex,
-                            DeclVisibilityKind);
+  AbstractPatternEntryScope(PatternBindingDecl *, unsigned entryIndex);
   virtual ~AbstractPatternEntryScope() {}
 
   const PatternBindingEntry &getPatternEntry() const;
@@ -1057,37 +1048,43 @@ class AbstractPatternEntryScope : public ASTScopeImpl {
 };
 
 class PatternEntryDeclScope final : public AbstractPatternEntryScope {
+  /// The first source location where this pattern binding entry's
+  /// bindings become visible.
+  SourceLoc loc;
+
 public:
   PatternEntryDeclScope(PatternBindingDecl *pbDecl, unsigned entryIndex,
-                        DeclVisibilityKind vis)
-      : AbstractPatternEntryScope(pbDecl, entryIndex, vis) {}
+                        SourceLoc loc)
+      : AbstractPatternEntryScope(pbDecl, entryIndex), loc(loc) {}
   virtual ~PatternEntryDeclScope() {}
 
 protected:
   ASTScopeImpl *expandSpecifically(ScopeCreator &scopeCreator) override;
 
-private:
-  AnnotatedInsertionPoint
-  expandAScopeThatCreatesANewInsertionPoint(ScopeCreator &);
-
 public:
   std::string getClassName() const override;
   SourceRange
   getSourceRangeOfThisASTNode(bool omitAssertions = false) const override;
 
   NullablePtr<const void> getReferrent() const override;
 
+  static SourceLoc
+  getStartLocForBinding(SourceManager &SM,
+                        ASTScopeImpl *parent,
+                        PatternBindingDecl *decl,
+                        unsigned entryIndex);
+
 protected:
   bool lookupLocalsOrMembers(DeclConsumer) const override;
+  bool isLabeledStmtLookupTerminator() const override;
 };
 
 class PatternEntryInitializerScope final : public AbstractPatternEntryScope {
   Expr *initAsWrittenWhenCreated;
 
 public:
-  PatternEntryInitializerScope(PatternBindingDecl *pbDecl, unsigned entryIndex,
-                               DeclVisibilityKind vis)
-      : AbstractPatternEntryScope(pbDecl, entryIndex, vis),
+  PatternEntryInitializerScope(PatternBindingDecl *pbDecl, unsigned entryIndex)
+      : AbstractPatternEntryScope(pbDecl, entryIndex),
         initAsWrittenWhenCreated(pbDecl->getOriginalInit(entryIndex)) {}
   virtual ~PatternEntryInitializerScope() {}
 

lib/AST/ASTScopeCreation.cpp

@@ -38,11 +38,6 @@
 using namespace swift;
 using namespace ast_scope;
 
-/// If true, nest scopes so a variable is out of scope before its declaration
-/// Does not handle capture rules for local functions properly.
-/// If false don't push uses down into subscopes after decls.
-static const bool handleUseBeforeDef = false;
-
 #pragma mark source range utilities
 static bool rangeableIsIgnored(const Decl *d) { return d->isImplicit(); }
 static bool rangeableIsIgnored(const Expr *d) {
@@ -215,6 +210,10 @@ class ScopeCreator final {
   ScopeCreator(const ScopeCreator &) = delete;  // ensure no copies
   ScopeCreator(const ScopeCreator &&) = delete; // ensure no moves
 
+  SourceManager &getSourceManager() const {
+    return ctx.SourceMgr;
+  }
+
   /// Given an array of ASTNodes or Decl pointers, add them
   /// Return the resultant insertionPoint
   ASTScopeImpl *
@@ -283,7 +282,7 @@ class ScopeCreator final {
     /// \endcode
     /// I'm seeing a dumped AST include:
     /// (pattern_binding_decl range=[test.swift:13:8 - line:12:29]
-    const auto &SM = d->getASTContext().SourceMgr;
+    const auto &SM = getSourceManager();
 
     // Once we allow invalid PatternBindingDecls (see
     // isWorthTryingToCreateScopeFor), then
@@ -746,22 +745,62 @@ class NodeAdder
     if (auto *var = patternBinding->getSingleVar())
       scopeCreator.addChildrenForKnownAttributes(var, parentScope);
 
-    const bool isInTypeDecl = parentScope->isATypeDeclScope();
+    const bool isLocalBinding = patternBinding->getDeclContext()->isLocalContext();
 
-    const DeclVisibilityKind vis =
-        isInTypeDecl ? DeclVisibilityKind::MemberOfCurrentNominal
-                     : DeclVisibilityKind::LocalVariable;
     auto *insertionPoint = parentScope;
     for (auto i : range(patternBinding->getNumPatternEntries())) {
-      insertionPoint =
+      // Create a child for the initializer, if present.
+      // Cannot trust the source range given in the ASTScopeImpl for the end of the
+      // initializer (because of InterpolatedLiteralStrings and EditorPlaceHolders),
+      // so compute it ourselves.
+      // Even if this predicate fails, there may be an initContext but
+      // we cannot make a scope for it, since no source range.
+      if (auto *expr = patternBinding->getOriginalInit(i)) {
+        if (isLocalizable(expr)) {
+          ASTScopeAssert(
+              !scopeCreator.getSourceManager().isBeforeInBuffer(
+                  expr->getStartLoc(), patternBinding->getStartLoc()),
+              "Original inits are always after the '='");
           scopeCreator
-              .ifUniqueConstructExpandAndInsert<PatternEntryDeclScope>(
-                  insertionPoint, patternBinding, i, vis)
-              .getPtrOr(insertionPoint);
+              .constructExpandAndInsertUncheckable<PatternEntryInitializerScope>(
+                  insertionPoint, patternBinding, i);
+        }
+      }
+
+      // Add accessors for the variables in this pattern.
+      patternBinding->getPattern(i)->forEachVariable([&](VarDecl *var) {
+        scopeCreator.addChildrenForAllLocalizableAccessorsInSourceOrder(
+            var, insertionPoint);
+      });
+
+      // For local bindings, introduce the scope where the names become
+      // visible.
+      if(isLocalBinding) {
+        // Don't create the scope if the start is past the end of the buffer.
+        //
+        // This can happen with invalid code missing an end delimiter, eg
+        //
+        // func foo() { var x = 123<EOF>
+        //
+        // The PatternEntryDeclScope for 'x' should begin immediately after
+        // the 123, but that is not a valid location, so we drop the scope
+        // since we're not going to be able to see it anyway.
+        auto loc = PatternEntryDeclScope::getStartLocForBinding(
+            scopeCreator.getSourceManager(),
+            insertionPoint, patternBinding, i);
+        if (loc.isValid()) {
+          insertionPoint =
+              scopeCreator
+                  .ifUniqueConstructExpandAndInsert<PatternEntryDeclScope>(
+                      insertionPoint, patternBinding, i, loc)
+                  .getPtrOr(insertionPoint);
+        }
+      }
     }
+
     // If in a type decl, the type search will find these,
     // but if in a brace stmt, must continue under the last binding.
-    return isInTypeDecl ? parentScope : insertionPoint;
+    return isLocalBinding ? insertionPoint : parentScope;
   }
 
   NullablePtr<ASTScopeImpl> visitEnumElementDecl(EnumElementDecl *eed,
@@ -947,7 +986,6 @@ ASTScopeImpl *ASTScopeImpl::expandAndBeCurrent(ScopeCreator &scopeCreator) {
 CREATES_NEW_INSERTION_POINT(ASTSourceFileScope)
 CREATES_NEW_INSERTION_POINT(ConditionalClauseScope)
 CREATES_NEW_INSERTION_POINT(GuardStmtScope)
-CREATES_NEW_INSERTION_POINT(PatternEntryDeclScope)
 CREATES_NEW_INSERTION_POINT(GenericTypeOrExtensionScope)
 CREATES_NEW_INSERTION_POINT(BraceStmtScope)
 CREATES_NEW_INSERTION_POINT(TopLevelCodeScope)
@@ -976,6 +1014,7 @@ NO_NEW_INSERTION_POINT(SwitchStmtScope)
 NO_NEW_INSERTION_POINT(WhileStmtScope)
 
 NO_EXPANSION(GenericParamScope)
+NO_EXPANSION(PatternEntryDeclScope)
 NO_EXPANSION(SpecializeAttributeScope)
 NO_EXPANSION(DifferentiableAttributeScope)
 NO_EXPANSION(ConditionalClausePatternUseScope)
@@ -1013,41 +1052,6 @@ ParameterListScope::expandAScopeThatDoesNotCreateANewInsertionPoint(
   }
 }
 
-AnnotatedInsertionPoint
-PatternEntryDeclScope::expandAScopeThatCreatesANewInsertionPoint(
-    ScopeCreator &scopeCreator) {
-  // Initializers come before VarDecls, e.g. PCMacro/didSet.swift 19
-  auto patternEntry = getPatternEntry();
-
-  // Create a child for the initializer, if present.
-  // Cannot trust the source range given in the ASTScopeImpl for the end of the
-  // initializer (because of InterpolatedLiteralStrings and EditorPlaceHolders),
-  // so compute it ourselves.
-  // Even if this predicate fails, there may be an initContext but
-  // we cannot make a scope for it, since no source range.
-  if (patternEntry.getOriginalInit() &&
-      isLocalizable(patternEntry.getOriginalInit())) {
-    ASTScopeAssert(
-        !getSourceManager().isBeforeInBuffer(
-            patternEntry.getOriginalInit()->getStartLoc(), decl->getStartLoc()),
-        "Original inits are always after the '='");
-    scopeCreator
-        .constructExpandAndInsertUncheckable<PatternEntryInitializerScope>(
-            this, decl, patternEntryIndex, vis);
-  }
-
-  // Add accessors for the variables in this pattern.
-  patternEntry.getPattern()->forEachVariable([&](VarDecl *var) {
-    scopeCreator.addChildrenForAllLocalizableAccessorsInSourceOrder(var, this);
-  });
-
-  ASTScopeAssert(!handleUseBeforeDef,
-                 "next line is wrong otherwise; would need a use scope");
-
-  return {getParent().get(), "When not handling use-before-def, succeeding "
-                             "code just goes in the same scope as this one"};
-}
-
 void
 PatternEntryInitializerScope::expandAScopeThatDoesNotCreateANewInsertionPoint(
     ScopeCreator &scopeCreator) {
@@ -1417,18 +1421,12 @@ ASTScopeImpl *LabeledConditionalStmtScope::createNestedConditionalClauseScopes(
 }
 
 AbstractPatternEntryScope::AbstractPatternEntryScope(
-    PatternBindingDecl *declBeingScoped, unsigned entryIndex,
-    DeclVisibilityKind vis)
-    : decl(declBeingScoped), patternEntryIndex(entryIndex), vis(vis) {
+    PatternBindingDecl *declBeingScoped, unsigned entryIndex)
+    : decl(declBeingScoped), patternEntryIndex(entryIndex) {
   ASTScopeAssert(entryIndex < declBeingScoped->getPatternList().size(),
                  "out of bounds");
 }
 
-bool ASTScopeImpl::isATypeDeclScope() const {
-  Decl *const pd = getDeclIfAny().getPtrOrNull();
-  return pd && (isa<NominalTypeDecl>(pd) || isa<ExtensionDecl>(pd));
-}
-
 #pragma mark new operators
 void *ASTScopeImpl::operator new(size_t bytes, const ASTContext &ctx,
                                  unsigned alignment) {
@@ -1481,8 +1479,6 @@ ScopeCreator &ASTSourceFileScope::getScopeCreator() { return *scopeCreator; }
   }
 
 GET_REFERRENT(AbstractFunctionDeclScope, getDecl())
-// If the PatternBindingDecl is a dup, detect it for the first
-// PatternEntryDeclScope; the others are subscopes.
 GET_REFERRENT(PatternEntryDeclScope, getPattern())
 GET_REFERRENT(TopLevelCodeScope, getDecl())
 GET_REFERRENT(SubscriptDeclScope, getDecl())

lib/AST/ASTScopeLookup.cpp

@@ -279,9 +279,10 @@ bool GenericParamScope::lookupLocalsOrMembers(DeclConsumer consumer) const {
 }
 
 bool PatternEntryDeclScope::lookupLocalsOrMembers(DeclConsumer consumer) const {
-  if (vis != DeclVisibilityKind::LocalVariable)
-    return false; // look in self type will find this later
-  return lookupLocalBindingsInPattern(getPattern(), vis, consumer);
+  ASTScopeAssert(decl->getDeclContext()->isLocalContext(),
+                 "Only local bindings need their own scope");
+  return lookupLocalBindingsInPattern(getPattern(), DeclVisibilityKind::LocalVariable,
+                                      consumer);
 }
 
 bool ForEachPatternScope::lookupLocalsOrMembers(DeclConsumer consumer) const {
@@ -510,6 +511,10 @@ bool CaseStmtBodyScope::isLabeledStmtLookupTerminator() const {
   return false;
 }
 
+bool PatternEntryDeclScope::isLabeledStmtLookupTerminator() const {
+  return false;
+}
+
 llvm::SmallVector<LabeledStmt *, 4>
 ASTScopeImpl::lookupLabeledStmts(SourceFile *sourceFile, SourceLoc loc) {
   // Find the innermost scope from which to start our search.

lib/AST/ASTScopeSourceRange.cpp

@@ -247,7 +247,23 @@ SourceRange DefaultArgumentInitializerScope::getSourceRangeOfThisASTNode(
 
 SourceRange PatternEntryDeclScope::getSourceRangeOfThisASTNode(
     const bool omitAssertions) const {
-  return getPatternEntry().getSourceRange();
+  return loc;
+}
+
+SourceLoc
+PatternEntryDeclScope::getStartLocForBinding(SourceManager &SM,
+                                             ASTScopeImpl *parent,
+                                             PatternBindingDecl *decl,
+                                             unsigned entryIndex) {
+  // FIXME: This feels more complicated than it should be.
+  auto end = decl->getPatternList()[entryIndex].getSourceRange().End;
+  auto next = Lexer::getLocForEndOfToken(SM, end);
+
+  auto endOfParent = parent->getSourceRangeOfThisASTNode(true).End;
+  if (SM.isBeforeInBuffer(endOfParent, next))
+    return SourceLoc();
+
+  return next;
 }
 
 SourceRange PatternEntryInitializerScope::getSourceRangeOfThisASTNode(

test/NameLookup/scope_map_top_level.swift

@@ -31,8 +31,7 @@ var i: Int = b.my_identity()
 // CHECK-EXPANDED-NEXT:   `-NominalTypeBodyScope {{.*}}, [4:11 - 4:13]
 // CHECK-EXPANDED-NEXT: `-TopLevelCodeScope {{.*}}, [6:1 - 21:28]
 // CHECK-EXPANDED-NEXT:   `-BraceStmtScope {{.*}}, [6:1 - 21:28]
-// CHECK-EXPANDED-NEXT:     |-PatternEntryDeclScope {{.*}}, [6:5 - 6:15] entry 0 'a'
-// CHECK-EXPANDED-NEXT:       `-PatternEntryInitializerScope {{.*}}, [6:15 - 6:15] entry 0 'a'
+// CHECK-EXPANDED-NEXT:     |-PatternEntryInitializerScope {{.*}}, [6:15 - 6:15] entry 0 'a'
 // CHECK-EXPANDED-NEXT:     `-TopLevelCodeScope {{.*}}, [8:1 - 21:28]
 // CHECK-EXPANDED-NEXT:       `-BraceStmtScope {{.*}}, [8:1 - 21:28]
 // CHECK-EXPANDED-NEXT:         `-GuardStmtScope {{.*}}, [8:1 - 21:28]
@@ -46,8 +45,7 @@ var i: Int = b.my_identity()
 // CHECK-EXPANDED-NEXT:                 `-BraceStmtScope {{.*}}, [11:18 - 11:19]
 // CHECK-EXPANDED-NEXT:             `-TopLevelCodeScope {{.*}}, [13:1 - 21:28]
 // CHECK-EXPANDED-NEXT:               `-BraceStmtScope {{.*}}, [13:1 - 21:28]
-// CHECK-EXPANDED-NEXT:                 |-PatternEntryDeclScope {{.*}}, [13:5 - 13:9] entry 0 'c'
-// CHECK-EXPANDED-NEXT:                   `-PatternEntryInitializerScope {{.*}}, [13:9 - 13:9] entry 0 'c'
+// CHECK-EXPANDED-NEXT:                 |-PatternEntryInitializerScope {{.*}}, [13:9 - 13:9] entry 0 'c'
 // CHECK-EXPANDED-NEXT:                 |-TypeAliasDeclScope {{.*}}, [15:1 - 15:15]
 // CHECK-EXPANDED-NEXT:                 |-ExtensionDeclScope {{.*}}, [17:14 - 19:1]
 // CHECK-EXPANDED-NEXT:                   `-ExtensionBodyScope {{.*}}, [17:15 - 19:1]
@@ -56,8 +54,7 @@ var i: Int = b.my_identity()
 // CHECK-EXPANDED-NEXT:                         `-BraceStmtScope {{.*}}, [18:29 - 18:43]
 // CHECK-EXPANDED-NEXT:                 `-TopLevelCodeScope {{.*}}, [21:1 - 21:28]
 // CHECK-EXPANDED-NEXT:                   `-BraceStmtScope {{.*}}, [21:1 - 21:28]
-// CHECK-EXPANDED-NEXT:                     `-PatternEntryDeclScope {{.*}}, [21:5 - 21:28] entry 0 'i'
-// CHECK-EXPANDED-NEXT:                       `-PatternEntryInitializerScope {{.*}}, [21:14 - 21:28] entry 0 'i'
+// CHECK-EXPANDED-NEXT:                     `-PatternEntryInitializerScope {{.*}}, [21:14 - 21:28] entry 0 'i'
 
 
 // REQUIRES: asserts