ASTScope: Redo assertions to look at CharSourceRanges

Author: slavapestov

include/swift/AST/ASTScope.h

@@ -273,14 +273,10 @@ class ASTScopeImpl {
 
 protected:
   SourceManager &getSourceManager() const;
-  bool hasValidSourceRange() const;
-  bool hasValidSourceRangeOfIgnoredASTNodes() const;
-  bool precedesInSource(const ASTScopeImpl *) const;
-  bool verifyThatChildrenAreContainedWithin(SourceRange) const;
-  bool verifyThatThisNodeComeAfterItsPriorSibling() const;
 
 private:
-  bool checkSourceRangeAfterExpansion(const ASTContext &) const;
+  void checkSourceRangeBeforeAddingChild(ASTScopeImpl *child,
+                                         const ASTContext &ctx) const;
 
 #pragma mark common queries
 public:

lib/AST/ASTScopeCreation.cpp

@@ -334,8 +334,6 @@ class ScopeCreator final {
 
     ASTScopeImpl *insertionPoint =
         child->expandAndBeCurrentDetectingRecursion(*this);
-    ASTScopeAssert(child->verifyThatThisNodeComeAfterItsPriorSibling(),
-                   "Ensure search will work");
     return insertionPoint;
   }
 
@@ -881,6 +879,13 @@ ScopeCreator::addPatternBindingToScopeTree(PatternBindingDecl *patternBinding,
 #pragma mark creation helpers
 
 void ASTScopeImpl::addChild(ASTScopeImpl *child, ASTContext &ctx) {
+  ASTScopeAssert(!child->getParent(), "child should not already have parent");
+  child->parent = 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.
@@ -889,8 +894,6 @@ void ASTScopeImpl::addChild(ASTScopeImpl *child, ASTContext &ctx) {
     haveAddedCleanup = true;
   }
   storedChildren.push_back(child);
-  ASTScopeAssert(!child->getParent(), "child should not already have parent");
-  child->parent = this;
   clearCachedSourceRangesOfMeAndAncestors();
 }
 
@@ -926,8 +929,6 @@ ASTScopeImpl *ASTScopeImpl::expandAndBeCurrent(ScopeCreator &scopeCreator) {
 
   setWasExpanded();
 
-  ASTScopeAssert(checkSourceRangeAfterExpansion(scopeCreator.getASTContext()),
-                 "Bad range.");
   return insertionPoint;
 }
 

lib/AST/ASTScopePrinting.cpp

@@ -123,9 +123,7 @@ static void printSourceRange(llvm::raw_ostream &out, const SourceRange range,
 }
 
 void ASTScopeImpl::printRange(llvm::raw_ostream &out) const {
-  if (!isSourceRangeCached(true))
-    out << "(uncached) ";
-  SourceRange range = computeSourceRangeOfScope(/*omitAssertions=*/true);
+  SourceRange range = getSourceRangeOfThisASTNode(/*omitAssertions=*/true);
   printSourceRange(out, range, getSourceManager());
 }
 

lib/AST/ASTScopeSourceRange.cpp

@@ -79,113 +79,59 @@ ASTScopeImpl::widenSourceRangeForChildren(const SourceRange range,
   return r;
 }
 
-bool ASTScopeImpl::checkSourceRangeAfterExpansion(const ASTContext &ctx) const {
-  ASTScopeAssert(getSourceRangeOfThisASTNode().isValid() ||
-                     !getChildren().empty(),
-                 "need to be able to find source range");
-  ASTScopeAssert(verifyThatChildrenAreContainedWithin(getSourceRangeOfScope()),
-                 "Search will fail");
-  ASTScopeAssert(
-      checkLazySourceRange(ctx),
-      "Lazy scopes must have compatible ranges before and after expansion");
+void ASTScopeImpl::checkSourceRangeBeforeAddingChild(ASTScopeImpl *child,
+                                                     const ASTContext &ctx) const {
+  auto &sourceMgr = ctx.SourceMgr;
 
-  return true;
-}
-
-#pragma mark validation & debugging
+  auto range = getCharSourceRangeOfScope(sourceMgr);
 
-bool ASTScopeImpl::hasValidSourceRange() const {
-  const auto sourceRange = getSourceRangeOfScope();
-  return sourceRange.Start.isValid() && sourceRange.End.isValid() &&
-         !getSourceManager().isBeforeInBuffer(sourceRange.End,
-                                              sourceRange.Start);
-}
+  auto childCharRange = child->getCharSourceRangeOfScope(sourceMgr);
 
-bool ASTScopeImpl::hasValidSourceRangeOfIgnoredASTNodes() const {
-  return sourceRangeOfIgnoredASTNodes.isValid();
-}
+  bool childContainedInParent = [&]() {
+    // HACK: For code completion. Handle replaced range.
+    if (const auto &replacedRange = sourceMgr.getReplacedRange()) {
+      auto originalRange = Lexer::getCharSourceRangeFromSourceRange(
+          sourceMgr, replacedRange.Original);
+      auto newRange = Lexer::getCharSourceRangeFromSourceRange(
+          sourceMgr, replacedRange.New);
 
-bool ASTScopeImpl::precedesInSource(const ASTScopeImpl *next) const {
-  if (!hasValidSourceRange() || !next->hasValidSourceRange())
-    return false;
-  return !getSourceManager().isBeforeInBuffer(
-      next->getSourceRangeOfScope().Start, getSourceRangeOfScope().End);
-}
+      if (range.contains(originalRange) &&
+          newRange.contains(childCharRange))
+        return true;
+    }
 
-bool ASTScopeImpl::verifyThatChildrenAreContainedWithin(
-    const SourceRange range) const {
-  // assumes children are already in order
-  if (getChildren().empty())
-    return true;
-  const SourceRange rangeOfChildren =
-      SourceRange(getChildren().front()->getSourceRangeOfScope().Start,
-                  getChildren().back()->getSourceRangeOfScope().End);
-  if (getSourceManager().rangeContains(range, rangeOfChildren))
-    return true;
+    return range.contains(childCharRange);
+  }();
 
-  // HACK: For code completion. Handle replaced range.
-  if (const auto &replacedRange = getSourceManager().getReplacedRange()) {
-    if (getSourceManager().rangeContains(replacedRange.Original, range) &&
-        getSourceManager().rangeContains(replacedRange.New, rangeOfChildren))
-      return true;
+  if (!childContainedInParent) {
+    auto &out = verificationError() << "child not contained in its parent:\n";
+    child->print(out);
+    out << "\n***Parent node***\n";
+    this->print(out);
+    abort();
   }
 
-  auto &out = verificationError() << "children not contained in its parent\n";
-  if (getChildren().size() == 1) {
-    out << "\n***Only Child node***\n";
-    getChildren().front()->print(out);
-  } else {
-    out << "\n***First Child node***\n";
-    getChildren().front()->print(out);
-    out << "\n***Last Child node***\n";
-    getChildren().back()->print(out);
-  }
-  out << "\n***Parent node***\n";
-  this->print(out);
-  abort();
-}
-
-bool ASTScopeImpl::verifyThatThisNodeComeAfterItsPriorSibling() const {
-  auto priorSibling = getPriorSibling();
-  if (!priorSibling)
-    return true;
-  if (priorSibling.get()->precedesInSource(this))
-    return true;
-  auto &out = verificationError() << "unexpected out-of-order nodes\n";
-  out << "\n***Penultimate child node***\n";
-  priorSibling.get()->print(out);
-  out << "\n***Last Child node***\n";
-  print(out);
-  out << "\n***Parent node***\n";
-  getParent().get()->print(out);
-  //  llvm::errs() << "\n\nsource:\n"
-  //               << getSourceManager()
-  //                      .getRangeForBuffer(
-  //                          getSourceFile()->getBufferID().getValue())
-  //                      .str();
-  ASTScope_unreachable("unexpected out-of-order nodes");
-  return false;
-}
-
-NullablePtr<ASTScopeImpl> ASTScopeImpl::getPriorSibling() const {
-  auto parent = getParent();
-  if (!parent)
-    return nullptr;
-  auto const &siblingsAndMe = parent.get()->getChildren();
-  // find myIndex, which is probably the last one
-  int myIndex = -1;
-  for (int i = siblingsAndMe.size() - 1; i >= 0; --i) {
-    if (siblingsAndMe[i] == this) {
-      myIndex = i;
-      break;
+  if (!storedChildren.empty()) {
+    auto previousChild = storedChildren.back();
+    auto endOfPreviousChild = previousChild->getCharSourceRangeOfScope(
+        sourceMgr).getEnd();
+
+    if (childCharRange.getStart() != endOfPreviousChild &&
+        !sourceMgr.isBeforeInBuffer(endOfPreviousChild,
+                                    childCharRange.getStart())) {
+      auto &out = verificationError() << "child overlaps previous child:\n";
+      child->print(out);
+      out << "\n***Previous child\n";
+      previousChild->print(out);
+      out << "\n***Parent node***\n";
+      this->print(out);
+      abort();
     }
   }
-  ASTScopeAssert(myIndex != -1, "I have been disowned!");
-  if (myIndex == 0)
-    return nullptr;
-  return siblingsAndMe[myIndex - 1];
 }
 
+#pragma mark validation & debugging
+
 bool ASTScopeImpl::doesRangeMatch(unsigned start, unsigned end, StringRef file,
                                   StringRef className) {
   if (!className.empty() && className != getClassName())
@@ -509,34 +455,6 @@ void ASTScopeImpl::computeAndCacheSourceRangeOfScope(
   cachedSourceRange = computeSourceRangeOfScope(omitAssertions);
 }
 
-bool ASTScopeImpl::checkLazySourceRange(const ASTContext &ctx) const {
-  const auto unexpandedRange = sourceRangeForDeferredExpansion();
-  const auto expandedRange = computeSourceRangeOfScopeWithChildASTNodes();
-  if (unexpandedRange.isInvalid() || expandedRange.isInvalid())
-    return true;
-  if (unexpandedRange == expandedRange)
-    return true;
-
-  llvm::errs() << "*** Lazy range problem. Parent unexpanded: ***\n";
-  unexpandedRange.print(llvm::errs(), getSourceManager(), false);
-  llvm::errs() << "\n";
-  if (!getChildren().empty()) {
-    llvm::errs() << "*** vs last child: ***\n";
-    auto b = getChildren().back()->computeSourceRangeOfScope();
-    b.print(llvm::errs(), getSourceManager(), false);
-    llvm::errs() << "\n";
-  }
-  else if (hasValidSourceRangeOfIgnoredASTNodes()) {
-    llvm::errs() << "*** vs ignored AST nodes: ***\n";
-    sourceRangeOfIgnoredASTNodes.print(llvm::errs(), getSourceManager(), false);
-    llvm::errs() << "\n";
-  }
-  print(llvm::errs(), 0, false);
-  llvm::errs() << "\n";
-
-  return false;
-}
-
 SourceRange
 ASTScopeImpl::computeSourceRangeOfScope(const bool omitAssertions) const {
   // If we don't need to consider children, it's cheaper

test/NameLookup/scope_map_top_level.swift

@@ -26,7 +26,7 @@ var i: Int = b.my_identity()
 
 
 // CHECK-EXPANDED:      ***Complete scope map***
-// CHECK-EXPANDED-NEXT: ASTSourceFileScope {{.*}}, (uncached) [1:1 - 61:1] 'SOURCE_DIR{{[/\\]}}test{{[/\\]}}NameLookup{{[/\\]}}scope_map_top_level.swift'
+// CHECK-EXPANDED-NEXT: ASTSourceFileScope {{.*}} [1:1 - 61:1] 'SOURCE_DIR{{[/\\]}}test{{[/\\]}}NameLookup{{[/\\]}}scope_map_top_level.swift'
 // CHECK-EXPANDED-NEXT: |-NominalTypeDeclScope {{.*}}, [4:1 - 4:13]
 // CHECK-EXPANDED-NEXT:   `-NominalTypeBodyScope {{.*}}, [4:11 - 4:13]
 // CHECK-EXPANDED-NEXT: `-TopLevelCodeScope {{.*}}, [6:1 - 61:1]