Merge pull request #34021 from slavapestov/astscope-invalid-pbds

ASTScope: Clean up handling of PatternBindingDecls and AccessorDecls

Author: slavapestov

include/swift/Parse/Parser.h

@@ -1116,7 +1116,7 @@ class Parser {
                            ParsedAccessors &accessors,
                            AbstractStorageDecl *storage,
                            SourceLoc StaticLoc);
-  ParserResult<VarDecl> parseDeclVarGetSet(Pattern *pattern,
+  ParserResult<VarDecl> parseDeclVarGetSet(PatternBindingEntry &entry,
                                            ParseDeclOptions Flags,
                                            SourceLoc StaticLoc,
                                            StaticSpellingKind StaticSpelling,

lib/AST/ASTScopeCreation.cpp

@@ -273,11 +273,6 @@ class ScopeCreator final {
     // Implicit nodes may not have source information for name lookup.
     if (!isLocalizable(d))
       return false;
-    /// In \c Parser::parseDeclVarGetSet fake PBDs are created. Ignore them.
-    /// Example:
-    /// \code
-    /// class SR10903 { static var _: Int { 0 } }
-    /// \endcode
 
     // Commented out for
     // validation-test/compiler_crashers_fixed/27962-swift-rebindselfinconstructorexpr-getcalledconstructor.swift
@@ -502,9 +497,6 @@ class ScopeCreator final {
   std::vector<ASTNode> expandIfConfigClausesThenCullAndSortElementsOrMembers(
       ArrayRef<ASTNode> input) const {
     auto cleanedupNodes = sortBySourceRange(cull(expandIfConfigClauses(input)));
-    // TODO: uncomment when working on not creating two pattern binding decls at
-    // same location.
-    //    findCollidingPatterns(cleanedupNodes);
     return cleanedupNodes;
   }
 
@@ -562,73 +554,6 @@ class ScopeCreator final {
     return culled;
   }
 
-  /// TODO: The parser yields two decls at the same source loc with the same
-  /// kind. TODO:  me when fixing parser's proclivity to create two
-  /// PatternBindingDecls at the same source location, then move this to
-  /// ASTVerifier.
-  ///
-  /// In all cases the first pattern seems to carry the initializer, and the
-  /// second, the accessor
-  void findCollidingPatterns(ArrayRef<ASTNode> input) const {
-    auto dumpPBD = [&](PatternBindingDecl *pbd, const char *which) {
-      llvm::errs() << "*** " << which
-                   << " pbd isImplicit: " << pbd->isImplicit()
-                   << ", #entries: " << pbd->getNumPatternEntries() << " :";
-      pbd->getSourceRange().print(llvm::errs(), pbd->getASTContext().SourceMgr,
-                                  false);
-      llvm::errs() << "\n";
-      llvm::errs() << "init: " << pbd->getInit(0) << "\n";
-      if (pbd->getInit(0)) {
-        llvm::errs() << "SR (init): ";
-        pbd->getInit(0)->getSourceRange().print(
-            llvm::errs(), pbd->getASTContext().SourceMgr, false);
-        llvm::errs() << "\n";
-        pbd->getInit(0)->dump(llvm::errs(), 0);
-      }
-      llvm::errs() << "vars:\n";
-      pbd->getPattern(0)->forEachVariable([&](VarDecl *vd) {
-        llvm::errs() << "  " << vd->getName()
-                     << " implicit: " << vd->isImplicit()
-                     << " #accs: " << vd->getAllAccessors().size()
-                     << "\nSR (var):";
-        vd->getSourceRange().print(llvm::errs(), pbd->getASTContext().SourceMgr,
-                                   false);
-        llvm::errs() << "\nSR (braces)";
-        vd->getBracesRange().print(llvm::errs(), pbd->getASTContext().SourceMgr,
-                                   false);
-        llvm::errs() << "\n";
-        for (auto *a : vd->getAllAccessors()) {
-          llvm::errs() << "SR (acc): ";
-          a->getSourceRange().print(llvm::errs(),
-                                    pbd->getASTContext().SourceMgr, false);
-          llvm::errs() << "\n";
-          a->dump(llvm::errs(), 0);
-        }
-      });
-    };
-
-    Decl *lastD = nullptr;
-    for (auto n : input) {
-      auto *d = n.dyn_cast<Decl *>();
-      if (!d || !lastD || lastD->getStartLoc() != d->getStartLoc() ||
-          lastD->getKind() != d->getKind()) {
-        lastD = d;
-        continue;
-      }
-      if (auto *pbd = dyn_cast<PatternBindingDecl>(lastD))
-        dumpPBD(pbd, "prev");
-      if (auto *pbd = dyn_cast<PatternBindingDecl>(d)) {
-        dumpPBD(pbd, "curr");
-        ASTScope_unreachable("found colliding pattern binding decls");
-      }
-      llvm::errs() << "Two same kind decls at same loc: \n";
-      lastD->dump(llvm::errs());
-      llvm::errs() << "and\n";
-      d->dump(llvm::errs());
-      ASTScope_unreachable("Two same kind decls; unexpected kinds");
-    }
-  }
-
   /// Templated to work on either ASTNodes, Decl*'s, or whatnot.
   template <typename Rangeable>
   std::vector<Rangeable>
@@ -962,24 +887,6 @@ class NodeAdder
                      : DeclVisibilityKind::LocalVariable;
     auto *insertionPoint = parentScope;
     for (auto i : range(patternBinding->getNumPatternEntries())) {
-      // TODO: Won't need to do so much work to avoid creating one without
-      // a SourceRange once parser is fixed to not create two
-      // PatternBindingDecls with same locaiton and getSourceRangeOfThisASTNode
-      // for PatternEntryDeclScope is simplified to use the PatternEntry's
-      // source range.
-      if (!patternBinding->getOriginalInit(i)) {
-        bool found = false;
-        patternBinding->getPattern(i)->forEachVariable([&](VarDecl *vd) {
-          if (!vd->isImplicit())
-            found = true;
-          else
-            found |= llvm::any_of(vd->getAllAccessors(), [&](AccessorDecl *a) {
-              return isLocalizable(a);
-            });
-        });
-        if (!found)
-          continue;
-      }
       insertionPoint =
           scopeCreator
               .ifUniqueConstructExpandAndInsert<PatternEntryDeclScope>(
@@ -1058,15 +965,6 @@ void ScopeCreator::addChildrenForAllLocalizableAccessorsInSourceOrder(
   // Accessors are always nested within their abstract storage
   // declaration. The nesting may not be immediate, because subscripts may
   // have intervening scopes for generics.
-  AbstractStorageDecl *const enclosingAbstractStorageDecl =
-      parent->getEnclosingAbstractStorageDecl().get();
-
-  std::vector<AccessorDecl *> accessorsToScope;
-  // Assume we don't have to deal with inactive clauses of IfConfigs here
-  llvm::copy_if(asd->getAllAccessors(), std::back_inserter(accessorsToScope),
-                [&](AccessorDecl *ad) {
-                  return enclosingAbstractStorageDecl == ad->getStorage();
-                });
 
   // Create scopes for `@differentiable` attributes.
   forEachDifferentiableAttrInSourceOrder(
@@ -1075,9 +973,15 @@ void ScopeCreator::addChildrenForAllLocalizableAccessorsInSourceOrder(
             parent, diffAttr, asd);
       });
 
-  // Sort in order to include synthesized ones, which are out of order.
-  for (auto *accessor : sortBySourceRange(accessorsToScope))
-    addToScopeTree(accessor, parent);
+  AbstractStorageDecl *enclosingAbstractStorageDecl =
+      parent->getEnclosingAbstractStorageDecl().get();
+
+  asd->visitParsedAccessors([&](AccessorDecl *ad) {
+    assert(enclosingAbstractStorageDecl == ad->getStorage());
+    (void) enclosingAbstractStorageDecl;
+
+    this->addToScopeTree(ad, parent);
+  });
 }
 
 #pragma mark creation helpers
@@ -1693,8 +1597,12 @@ AbstractPatternEntryScope::AbstractPatternEntryScope(
 void AbstractPatternEntryScope::forEachVarDeclWithLocalizableAccessors(
     ScopeCreator &scopeCreator, function_ref<void(VarDecl *)> foundOne) const {
   getPatternEntry().getPattern()->forEachVariable([&](VarDecl *var) {
-    if (llvm::any_of(var->getAllAccessors(),
-                     [&](AccessorDecl *a) { return isLocalizable(a); }))
+    bool hasParsedAccessors = false;
+    var->visitParsedAccessors([&](AccessorDecl *) {
+      hasParsedAccessors = true;
+    });
+
+    if (hasParsedAccessors)
       foundOne(var);
   });
 }
@@ -2023,9 +1931,11 @@ class LocalizableDeclContextCollector : public ASTWalker {
       record(pd->getDefaultArgumentInitContext());
     else if (auto *pbd = dyn_cast<PatternBindingDecl>(D))
       recordInitializers(pbd);
-    else if (auto *vd = dyn_cast<VarDecl>(D))
-      for (auto *ad : vd->getAllAccessors())
+    else if (auto *vd = dyn_cast<VarDecl>(D)) {
+      vd->visitParsedAccessors([&](AccessorDecl *ad) {
         ad->walk(*this);
+      });
+    }
     return ASTWalker::walkToDeclPre(D);
   }
 

lib/AST/ASTScopeLookup.cpp

@@ -371,7 +371,7 @@ bool DifferentiableAttributeScope::lookupLocalsOrMembers(
   if (auto *afd = dyn_cast<AbstractFunctionDecl>(attributedDeclaration)) {
     return visitAbstractFunctionDecl(afd);
   } else if (auto *asd = dyn_cast<AbstractStorageDecl>(attributedDeclaration)) {
-    for (auto *accessor : asd->getAllAccessors())
+    if (auto *accessor = asd->getParsedAccessor(AccessorKind::Get))
       if (visitAbstractFunctionDecl(accessor))
         return true;
   }

lib/AST/ASTScopeSourceRange.cpp

@@ -247,16 +247,6 @@ SourceRange DefaultArgumentInitializerScope::getSourceRangeOfThisASTNode(
 
 SourceRange PatternEntryDeclScope::getSourceRangeOfThisASTNode(
     const bool omitAssertions) const {
-  // TODO: Once the creation of two PatternBindingDecls at same location is
-  // eliminated, the following may be able to be simplified.
-  if (!getChildren().empty()) { // why needed???
-    bool hasOne = false;
-    getPattern()->forEachVariable([&](VarDecl *) { hasOne = true; });
-    if (!hasOne)
-      return SourceRange(); // just the init
-    if (!getPatternEntry().getInit())
-      return SourceRange(); // just the var decls
-  }
   return getPatternEntry().getSourceRange();
 }
 

lib/IDE/SyntaxModel.cpp

@@ -990,7 +990,9 @@ bool ModelASTWalker::walkToDeclPre(Decl *D) {
     if (bracesRange.isValid())
       SN.BodyRange = innerCharSourceRangeFromSourceRange(SM, bracesRange);
     SourceLoc NRStart = VD->getNameLoc();
-    SourceLoc NREnd = NRStart.getAdvancedLoc(VD->getName().getLength());
+    SourceLoc NREnd = (!VD->getName().empty()
+                       ? NRStart.getAdvancedLoc(VD->getName().getLength())
+                       : NRStart);
     SN.NameRange = CharSourceRange(SM, NRStart, NREnd);
     SN.TypeRange = charSourceRangeFromSourceRange(SM,
                                         VD->getTypeSourceRangeForDiagnostics());

lib/Parse/ParseDecl.cpp

@@ -5680,14 +5680,16 @@ ParserStatus Parser::parseGetSet(ParseDeclOptions Flags,
 
 /// Parse the brace-enclosed getter and setter for a variable.
 ParserResult<VarDecl>
-Parser::parseDeclVarGetSet(Pattern *pattern, ParseDeclOptions Flags,
+Parser::parseDeclVarGetSet(PatternBindingEntry &entry, ParseDeclOptions Flags,
                            SourceLoc StaticLoc,
                            StaticSpellingKind StaticSpelling,
                            SourceLoc VarLoc, bool hasInitializer,
                            const DeclAttributes &Attributes,
                            SmallVectorImpl<Decl *> &Decls) {
   bool Invalid = false;
-  
+
+  auto *pattern = entry.getPattern();
+
   // The grammar syntactically requires a simple identifier for the variable
   // name. Complain if that isn't what we got. But for recovery purposes,
   // make an effort to look through other things anyway.
@@ -5730,22 +5732,12 @@ Parser::parseDeclVarGetSet(Pattern *pattern, ParseDeclOptions Flags,
                                     VarDecl::Introducer::Var,
                                     VarLoc, Identifier(),
                                     CurDeclContext);
-    storage->setImplicit(true);
     storage->setInvalid();
 
-    Pattern *pattern =
+    pattern =
       TypedPattern::createImplicit(Context, new (Context) NamedPattern(storage),
                                    ErrorType::get(Context));
-    PatternBindingEntry entry(pattern, /*EqualLoc*/ SourceLoc(),
-                              /*Init*/ nullptr, /*InitContext*/ nullptr);
-    auto binding = PatternBindingDecl::create(Context, StaticLoc,
-                                              StaticSpelling,
-                                              VarLoc, entry, CurDeclContext);
-    binding->setInvalid();
-    storage->setParentPatternBinding(binding);
-
-    Decls.push_back(binding);
-    Decls.push_back(storage);
+    entry.setPattern(pattern);
   }
 
   // Parse getter and setter.
@@ -6157,7 +6149,8 @@ Parser::parseDeclVar(ParseDeclOptions Flags,
     if (Tok.is(tok::l_brace)) {
       HasAccessors = true;
       auto boundVar =
-          parseDeclVarGetSet(pattern, Flags, StaticLoc, StaticSpelling, VarLoc,
+          parseDeclVarGetSet(PBDEntries.back(),
+                             Flags, StaticLoc, StaticSpelling, VarLoc,
                              PatternInit != nullptr, Attributes, Decls);
       if (boundVar.hasCodeCompletion())
         return makeResult(makeParserCodeCompletionStatus());

lib/Sema/BuilderTransform.cpp

@@ -1064,6 +1064,8 @@ class BuilderClosureRewriter
     auto pbd = PatternBindingDecl::create(
         ctx, SourceLoc(), StaticSpellingKind::None, temporaryVar->getLoc(),
         pattern, SourceLoc(), initExpr, dc);
+    if (temporaryVar->isImplicit())
+      pbd->setImplicit();
     elements.push_back(temporaryVar);
     elements.push_back(pbd);
   }

test/Index/invalid_code.swift

@@ -1,6 +1,5 @@
 // RUN: %target-swift-ide-test -print-indexed-symbols -include-locals -source-filename %s | %FileCheck %s
 
-// CHECK: [[@LINE+1]]:8 | struct/Swift | Int | {{.*}} | Ref | rel: 0
 var _: Int { get { return 1 } }
 
 func test() {

test/Parse/pattern_without_variables.swift

@@ -40,5 +40,4 @@ func testVarLetPattern(a : SimpleEnum) {
 
 class SR10903 {
   static var _: Int { 0 } //expected-error {{getter/setter can only be defined for a single variable}}
-  //expected-error@-1 {{property declaration does not bind any variables}}
 }

test/SourceKit/DocumentStructure/structure.swift.empty.response

@@ -573,11 +573,10 @@
       key.kind: source.lang.swift.decl.var.global,
       key.accessibility: source.lang.swift.accessibility.internal,
       key.setter_accessibility: source.lang.swift.accessibility.internal,
-      key.name: "Qtys",
       key.offset: 1079,
-      key.length: 15,
-      key.nameoffset: 1089,
-      key.namelength: 4
+      key.length: 3,
+      key.nameoffset: 1079,
+      key.namelength: 0
     },
     {
       key.kind: source.lang.swift.stmt.foreach,