From 4252659a048aa7041fb231d6c289ca23bb8a6a82 Mon Sep 17 00:00:00 2001 From: Slava Pestov Date: Fri, 2 Oct 2020 00:54:56 -0400 Subject: [PATCH 1/6] ASTScope: Add a getCharSourceRangeOfScope() method to ASTScopeImpl --- include/swift/AST/ASTScope.h | 6 ++++++ lib/AST/ASTScopeSourceRange.cpp | 21 +++++++++++++++++++++ 2 files changed, 27 insertions(+) diff --git a/include/swift/AST/ASTScope.h b/include/swift/AST/ASTScope.h index 1d4af06aa2ad0..eae28785f0940 100644 --- a/include/swift/AST/ASTScope.h +++ b/include/swift/AST/ASTScope.h @@ -157,6 +157,8 @@ class ASTScopeImpl { // source range. So include their ranges here SourceRange sourceRangeOfIgnoredASTNodes; + mutable Optional cachedCharSourceRange; + #pragma mark - constructor / destructor public: ASTScopeImpl(){}; @@ -212,6 +214,10 @@ class ASTScopeImpl { SourceRange getSourceRangeOfScope(bool omitAssertions = false) const; + CharSourceRange getCharSourceRangeOfScope(SourceManager &SM, + bool omitAssertions = false) const; + bool isCharSourceRangeCached() const; + /// InterpolatedStringLiteralExprs and EditorPlaceHolders respond to /// getSourceRange with the starting point. But we might be asked to lookup an /// identifer within one of them. So, find the real source range of them here. diff --git a/lib/AST/ASTScopeSourceRange.cpp b/lib/AST/ASTScopeSourceRange.cpp index 258631729f3a5..f0ebd5275887c 100644 --- a/lib/AST/ASTScopeSourceRange.cpp +++ b/lib/AST/ASTScopeSourceRange.cpp @@ -450,6 +450,27 @@ SourceRange GuardStmtBodyScope::getSourceRangeOfThisASTNode( #pragma mark source range caching +CharSourceRange +ASTScopeImpl::getCharSourceRangeOfScope(SourceManager &SM, + bool omitAssertions) const { + if (!isCharSourceRangeCached()) { + auto range = getSourceRangeOfThisASTNode(omitAssertions); + ASTScopeAssert(range.isValid(), "scope has invalid source range"); + ASTScopeAssert(SM.isBeforeInBuffer(range.Start, range.End) || + range.Start == range.End, + "scope source range ends before start"); + + cachedCharSourceRange = + Lexer::getCharSourceRangeFromSourceRange(SM, range); + } + + return *cachedCharSourceRange; +} + +bool ASTScopeImpl::isCharSourceRangeCached() const { + return cachedCharSourceRange.hasValue(); +} + SourceRange ASTScopeImpl::getSourceRangeOfScope(const bool omitAssertions) const { if (!isSourceRangeCached(omitAssertions)) From 771fd6e9d102c2baead48dc8557d5edc24007a89 Mon Sep 17 00:00:00 2001 From: Slava Pestov Date: Fri, 2 Oct 2020 02:44:17 -0400 Subject: [PATCH 2/6] ASTScope: Redo assertions to look at CharSourceRanges --- include/swift/AST/ASTScope.h | 8 +- lib/AST/ASTScopeCreation.cpp | 13 +- lib/AST/ASTScopePrinting.cpp | 4 +- lib/AST/ASTScopeSourceRange.cpp | 164 ++++++---------------- test/NameLookup/scope_map_top_level.swift | 2 +- 5 files changed, 52 insertions(+), 139 deletions(-) diff --git a/include/swift/AST/ASTScope.h b/include/swift/AST/ASTScope.h index eae28785f0940..f02cd3aa3c84a 100644 --- a/include/swift/AST/ASTScope.h +++ b/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: diff --git a/lib/AST/ASTScopeCreation.cpp b/lib/AST/ASTScopeCreation.cpp index eda1e0c69cbb0..f5d5c2ca651b7 100644 --- a/lib/AST/ASTScopeCreation.cpp +++ b/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; } diff --git a/lib/AST/ASTScopePrinting.cpp b/lib/AST/ASTScopePrinting.cpp index d300d24ded763..b9f84266768e4 100644 --- a/lib/AST/ASTScopePrinting.cpp +++ b/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()); } diff --git a/lib/AST/ASTScopeSourceRange.cpp b/lib/AST/ASTScopeSourceRange.cpp index f0ebd5275887c..66e1270da73f3 100644 --- a/lib/AST/ASTScopeSourceRange.cpp +++ b/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::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 diff --git a/test/NameLookup/scope_map_top_level.swift b/test/NameLookup/scope_map_top_level.swift index 07f94a67846bc..a6c3ef3be74a4 100644 --- a/test/NameLookup/scope_map_top_level.swift +++ b/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] From e52413f913aca863884f2ca10c691814b0ef936a Mon Sep 17 00:00:00 2001 From: Slava Pestov Date: Fri, 2 Oct 2020 02:44:27 -0400 Subject: [PATCH 3/6] ASTScope: Use CharSourceRanges for lookup --- lib/AST/ASTScopeLookup.cpp | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/lib/AST/ASTScopeLookup.cpp b/lib/AST/ASTScopeLookup.cpp index 396ddf8f658a3..6a8d25d13c2e9 100644 --- a/lib/AST/ASTScopeLookup.cpp +++ b/lib/AST/ASTScopeLookup.cpp @@ -83,11 +83,11 @@ bool ASTScopeImpl::checkSourceRangeOfThisASTNode() const { /// If the \p loc is in a new buffer but \p range is not, consider the location /// is at the start of replaced range. Otherwise, returns \p loc as is. static SourceLoc translateLocForReplacedRange(SourceManager &sourceMgr, - SourceRange range, + CharSourceRange range, SourceLoc loc) { if (const auto &replacedRange = sourceMgr.getReplacedRange()) { if (sourceMgr.rangeContainsTokenLoc(replacedRange.New, loc) && - !sourceMgr.rangeContains(replacedRange.New, range)) { + !sourceMgr.rangeContainsTokenLoc(replacedRange.New, range.getStart())) { return replacedRange.Original.Start; } } @@ -101,17 +101,19 @@ ASTScopeImpl::findChildContaining(SourceLoc loc, auto *const *child = llvm::lower_bound( getChildren(), loc, [&sourceMgr](const ASTScopeImpl *scope, SourceLoc loc) { - ASTScopeAssert(scope->checkSourceRangeOfThisASTNode(), "Bad range."); - auto rangeOfScope = scope->getSourceRangeOfScope(); + auto rangeOfScope = scope->getCharSourceRangeOfScope(sourceMgr); + ASTScopeAssert(!sourceMgr.isBeforeInBuffer(rangeOfScope.getEnd(), + rangeOfScope.getStart()), + "Source range is backwards"); loc = translateLocForReplacedRange(sourceMgr, rangeOfScope, loc); - return -1 == ASTScopeImpl::compare(rangeOfScope, loc, sourceMgr, - /*ensureDisjoint=*/false); + return (rangeOfScope.getEnd() == loc || + sourceMgr.isBeforeInBuffer(rangeOfScope.getEnd(), loc)); }); if (child != getChildren().end()) { - auto rangeOfScope = (*child)->getSourceRangeOfScope(); + auto rangeOfScope = (*child)->getCharSourceRangeOfScope(sourceMgr); loc = translateLocForReplacedRange(sourceMgr, rangeOfScope, loc); - if (sourceMgr.rangeContainsTokenLoc(rangeOfScope, loc)) + if (rangeOfScope.contains(loc)) return *child; } From 996100cfd1fc93ae92c587a3476988ea9a400bb3 Mon Sep 17 00:00:00 2001 From: Slava Pestov Date: Tue, 6 Oct 2020 15:27:02 -0400 Subject: [PATCH 4/6] ASTScope: Remove old SourceRange machinery --- include/swift/AST/ASTScope.h | 86 +---------- lib/AST/ASTScope.cpp | 21 +-- lib/AST/ASTScopeCreation.cpp | 7 +- lib/AST/ASTScopeLookup.cpp | 8 -- lib/AST/ASTScopeSourceRange.cpp | 245 -------------------------------- 5 files changed, 5 insertions(+), 362 deletions(-) diff --git a/include/swift/AST/ASTScope.h b/include/swift/AST/ASTScope.h index f02cd3aa3c84a..0e1f65e71625d 100644 --- a/include/swift/AST/ASTScope.h +++ b/include/swift/AST/ASTScope.h @@ -141,7 +141,6 @@ class ASTScopeImpl { ASTScopeImpl *parent = nullptr; // null at the root /// Child scopes, sorted by source range. - /// Must clear source range change whenever this changes Children storedChildren; bool wasExpanded = false; @@ -149,14 +148,6 @@ class ASTScopeImpl { /// Can clear storedChildren, so must remember this bool haveAddedCleanup = false; - // Must be updated after last child is added and after last child's source - // position is known - mutable Optional cachedSourceRange; - - // When ignoring ASTNodes in a scope, they still must count towards a scope's - // source range. So include their ranges here - SourceRange sourceRangeOfIgnoredASTNodes; - mutable Optional cachedCharSourceRange; #pragma mark - constructor / destructor @@ -194,9 +185,6 @@ class ASTScopeImpl { public: void addChild(ASTScopeImpl *child, ASTContext &); -private: - NullablePtr getPriorSibling() const; - public: void preOrderDo(function_ref); /// Like preorderDo but without myself. @@ -205,67 +193,15 @@ class ASTScopeImpl { #pragma mark - source ranges -#pragma mark - source range queries - public: /// Return signum of ranges. Centralize the invariant that ASTScopes use ends. static int compare(SourceRange, SourceRange, const SourceManager &, bool ensureDisjoint); - SourceRange getSourceRangeOfScope(bool omitAssertions = false) const; - CharSourceRange getCharSourceRangeOfScope(SourceManager &SM, bool omitAssertions = false) const; bool isCharSourceRangeCached() const; - /// InterpolatedStringLiteralExprs and EditorPlaceHolders respond to - /// getSourceRange with the starting point. But we might be asked to lookup an - /// identifer within one of them. So, find the real source range of them here. - SourceRange getEffectiveSourceRange(ASTNode) const; - - void computeAndCacheSourceRangeOfScope(bool omitAssertions = false) const; - bool isSourceRangeCached(bool omitAssertions = false) const; - - bool checkSourceRangeOfThisASTNode() const; - - /// For debugging - bool doesRangeMatch(unsigned start, unsigned end, StringRef file = "", - StringRef className = ""); - - unsigned countDescendants() const; - - /// Make sure that when the argument is executed, there are as many - /// descendants after as before. - void assertThatTreeDoesNotShrink(function_ref); - -private: - SourceRange computeSourceRangeOfScope(bool omitAssertions = false) const; - SourceRange - computeSourceRangeOfScopeWithChildASTNodes(bool omitAssertions = false) const; - bool ensureNoAncestorsSourceRangeIsCached() const; - -#pragma mark - source range adjustments -private: - SourceRange widenSourceRangeForIgnoredASTNodes(SourceRange range) const; - - /// If the scope refers to a Decl whose source range tells the whole story, - /// for example a NominalTypeScope, it is not necessary to widen the source - /// range by examining the children. In that case we could just return - /// the childlessRange here. - /// But, we have not marked such scopes yet. Doing so would be an - /// optimization. - SourceRange widenSourceRangeForChildren(SourceRange range, - bool omitAssertions) const; - - /// Even ASTNodes that do not form scopes must be included in a Scope's source - /// range. Widen the source range of the receiver to include the (ignored) - /// node. - void widenSourceRangeForIgnoredASTNode(ASTNode); - -private: - void clearCachedSourceRangesOfMeAndAncestors(); - -public: // public for debugging /// Returns source range of this node alone, without factoring in any /// children. virtual SourceRange @@ -331,19 +267,11 @@ class ASTScopeImpl { void setWasExpanded() { wasExpanded = true; } virtual ASTScopeImpl *expandSpecifically(ScopeCreator &) = 0; -private: - /// Compare the pre-expasion range with the post-expansion range and return - /// false if lazyiness couild miss lookups. - bool checkLazySourceRange(const ASTContext &) const; - public: /// Some scopes can be expanded lazily. - /// Such scopes must: not change their source ranges after expansion, and - /// their expansion must return an insertion point outside themselves. - /// After a node is expanded, its source range (getSourceRangeofThisASTNode - /// union children's ranges) must be same as this. + /// Such scopes must return an insertion point outside themselves when + /// expanded. virtual NullablePtr insertionPointForDeferredExpansion(); - virtual SourceRange sourceRangeForDeferredExpansion() const; private: virtual ScopeCreator &getScopeCreator(); @@ -547,8 +475,6 @@ class Portion { virtual NullablePtr insertionPointForDeferredExpansion(IterableTypeScope *) const = 0; - virtual SourceRange - sourceRangeForDeferredExpansion(const IterableTypeScope *) const = 0; }; // For the whole Decl scope of a GenericType or an Extension @@ -572,8 +498,6 @@ class Portion { NullablePtr insertionPointForDeferredExpansion(IterableTypeScope *) const override; - SourceRange - sourceRangeForDeferredExpansion(const IterableTypeScope *) const override; }; /// GenericTypeOrExtension = GenericType or Extension @@ -605,8 +529,6 @@ class GenericTypeOrExtensionWherePortion final NullablePtr insertionPointForDeferredExpansion(IterableTypeScope *) const override; - SourceRange - sourceRangeForDeferredExpansion(const IterableTypeScope *) const override; }; /// Behavior specific to representing the Body of a NominalTypeDecl or @@ -624,8 +546,6 @@ class IterableTypeBodyPortion final NullablePtr insertionPointForDeferredExpansion(IterableTypeScope *) const override; - SourceRange - sourceRangeForDeferredExpansion(const IterableTypeScope *) const override; }; /// GenericType or Extension scope @@ -722,7 +642,6 @@ class IterableTypeScope : public GenericTypeScope { public: NullablePtr insertionPointForDeferredExpansion() override; - SourceRange sourceRangeForDeferredExpansion() const override; void countBodies(ScopeCreator &) const; }; @@ -931,7 +850,6 @@ class FunctionBodyScope : public ASTScopeImpl { public: std::string getClassName() const override; NullablePtr insertionPointForDeferredExpansion() override; - SourceRange sourceRangeForDeferredExpansion() const override; }; class DefaultArgumentInitializerScope final : public ASTScopeImpl { diff --git a/lib/AST/ASTScope.cpp b/lib/AST/ASTScope.cpp index ec6868c2c4412..50c48de5bea68 100644 --- a/lib/AST/ASTScope.cpp +++ b/lib/AST/ASTScope.cpp @@ -197,23 +197,4 @@ void ASTScopeImpl::postOrderDo(function_ref fn) { for (auto *child : getChildren()) child->postOrderDo(fn); fn(this); -} - -unsigned ASTScopeImpl::countDescendants() const { - unsigned count = 0; - const_cast(this)->preOrderDo( - [&](ASTScopeImpl *) { ++count; }); - return count - 1; -} - -// Can fail if a subscope is lazy and not reexpanded -void ASTScopeImpl::assertThatTreeDoesNotShrink(function_ref fn) { -#ifndef NDEBUG - unsigned beforeCount = countDescendants(); -#endif - fn(); -#ifndef NDEBUG - unsigned afterCount = countDescendants(); - ASTScopeAssert(beforeCount <= afterCount, "shrank?!"); -#endif -} +} \ No newline at end of file diff --git a/lib/AST/ASTScopeCreation.cpp b/lib/AST/ASTScopeCreation.cpp index f5d5c2ca651b7..bbee272aed3ae 100644 --- a/lib/AST/ASTScopeCreation.cpp +++ b/lib/AST/ASTScopeCreation.cpp @@ -572,7 +572,6 @@ class NodeAdder #define VISIT_AND_IGNORE(What) \ NullablePtr visit##What(What *w, ASTScopeImpl *p, \ ScopeCreator &) { \ - p->widenSourceRangeForIgnoredASTNode(w); \ return p; \ } @@ -766,10 +765,9 @@ class NodeAdder NullablePtr visitExpr(Expr *expr, ASTScopeImpl *p, ScopeCreator &scopeCreator) { - if (expr) { - p->widenSourceRangeForIgnoredASTNode(expr); + if (expr) scopeCreator.addExprToScopeTree(expr, p); - } + return p; } }; @@ -894,7 +892,6 @@ void ASTScopeImpl::addChild(ASTScopeImpl *child, ASTContext &ctx) { haveAddedCleanup = true; } storedChildren.push_back(child); - clearCachedSourceRangesOfMeAndAncestors(); } #pragma mark implementations of expansion diff --git a/lib/AST/ASTScopeLookup.cpp b/lib/AST/ASTScopeLookup.cpp index 6a8d25d13c2e9..39db7f27c725d 100644 --- a/lib/AST/ASTScopeLookup.cpp +++ b/lib/AST/ASTScopeLookup.cpp @@ -72,14 +72,6 @@ ASTScopeImpl *ASTScopeImpl::findInnermostEnclosingScopeImpl( scopeCreator); } -bool ASTScopeImpl::checkSourceRangeOfThisASTNode() const { - const auto r = getSourceRangeOfThisASTNode(); - (void)r; - ASTScopeAssert(!getSourceManager().isBeforeInBuffer(r.End, r.Start), - "Range is backwards."); - return true; -} - /// If the \p loc is in a new buffer but \p range is not, consider the location /// is at the start of replaced range. Otherwise, returns \p loc as is. static SourceLoc translateLocForReplacedRange(SourceManager &sourceMgr, diff --git a/lib/AST/ASTScopeSourceRange.cpp b/lib/AST/ASTScopeSourceRange.cpp index 66e1270da73f3..d8a27d5b749cf 100644 --- a/lib/AST/ASTScopeSourceRange.cpp +++ b/lib/AST/ASTScopeSourceRange.cpp @@ -36,49 +36,8 @@ using namespace swift; using namespace ast_scope; -static SourceLoc getLocEncompassingPotentialLookups(const SourceManager &, - SourceLoc endLoc); static SourceLoc getLocAfterExtendedNominal(const ExtensionDecl *); -SourceRange ASTScopeImpl::widenSourceRangeForIgnoredASTNodes( - const SourceRange range) const { - if (range.isInvalid()) - return sourceRangeOfIgnoredASTNodes; - auto r = range; - if (sourceRangeOfIgnoredASTNodes.isValid()) - r.widen(sourceRangeOfIgnoredASTNodes); - return r; -} - -SourceRange -ASTScopeImpl::widenSourceRangeForChildren(const SourceRange range, - const bool omitAssertions) const { - if (getChildren().empty()) { - ASTScopeAssert(omitAssertions || range.Start.isValid(), "Bad range."); - return range; - } - const auto childStart = - getChildren().front()->getSourceRangeOfScope(omitAssertions).Start; - const auto childEnd = - getChildren().back()->getSourceRangeOfScope(omitAssertions).End; - auto childRange = SourceRange(childStart, childEnd); - ASTScopeAssert(omitAssertions || childRange.isValid(), "Bad range."); - - if (range.isInvalid()) - return childRange; - auto r = range; - - // HACK: For code completion. If the range of the child is from another - // source buffer, don't widen using that range. - if (const auto &replacedRange = getSourceManager().getReplacedRange()) { - if (getSourceManager().rangeContains(replacedRange.Original, range) && - getSourceManager().rangeContains(replacedRange.New, childRange)) - return r; - } - r.widen(childRange); - return r; -} - void ASTScopeImpl::checkSourceRangeBeforeAddingChild(ASTScopeImpl *child, const ASTContext &ctx) const { auto &sourceMgr = ctx.SourceMgr; @@ -130,24 +89,6 @@ void ASTScopeImpl::checkSourceRangeBeforeAddingChild(ASTScopeImpl *child, } } -#pragma mark validation & debugging - -bool ASTScopeImpl::doesRangeMatch(unsigned start, unsigned end, StringRef file, - StringRef className) { - if (!className.empty() && className != getClassName()) - return false; - const auto &SM = getSourceManager(); - const auto r = getSourceRangeOfScope(true); - if (start && start != SM.getLineAndColumnInBuffer(r.Start).first) - return false; - if (end && end != SM.getLineAndColumnInBuffer(r.End).first) - return false; - if (file.empty()) - return true; - const auto buf = SM.findBufferContainingLoc(r.Start); - return SM.getIdentifierForBuffer(buf).endswith(file); -} - #pragma mark getSourceRangeOfThisASTNode SourceRange SpecializeAttributeScope::getSourceRangeOfThisASTNode( @@ -417,192 +358,6 @@ bool ASTScopeImpl::isCharSourceRangeCached() const { return cachedCharSourceRange.hasValue(); } -SourceRange -ASTScopeImpl::getSourceRangeOfScope(const bool omitAssertions) const { - if (!isSourceRangeCached(omitAssertions)) - computeAndCacheSourceRangeOfScope(omitAssertions); - return *cachedSourceRange; -} - -bool ASTScopeImpl::isSourceRangeCached(const bool omitAssertions) const { - const bool isCached = cachedSourceRange.hasValue(); - ASTScopeAssert(omitAssertions || isCached || - ensureNoAncestorsSourceRangeIsCached(), - "Cached ancestor's range likely is obsolete."); - return isCached; -} - -bool ASTScopeImpl::ensureNoAncestorsSourceRangeIsCached() const { - if (const auto *const p = getParent().getPtrOrNull()) { - auto r = !p->isSourceRangeCached(true) && - p->ensureNoAncestorsSourceRangeIsCached(); - if (!r) - ASTScope_unreachable("found a violation"); - return true; - } - return true; -} - -void ASTScopeImpl::computeAndCacheSourceRangeOfScope( - const bool omitAssertions) const { - // In order to satisfy the invariant that, if my range is uncached, - // my parent's range is uncached, (which is needed to optimize invalidation - // by obviating the need to uncache all the way to the root every time), - // when caching a range, must ensure all children's ranges are cached. - for (auto *c : getChildren()) - c->computeAndCacheSourceRangeOfScope(omitAssertions); - - cachedSourceRange = computeSourceRangeOfScope(omitAssertions); -} - -SourceRange -ASTScopeImpl::computeSourceRangeOfScope(const bool omitAssertions) const { - // If we don't need to consider children, it's cheaper - const auto deferredRange = sourceRangeForDeferredExpansion(); - return deferredRange.isValid() - ? deferredRange - : computeSourceRangeOfScopeWithChildASTNodes(omitAssertions); -} - -SourceRange ASTScopeImpl::computeSourceRangeOfScopeWithChildASTNodes( - const bool omitAssertions) const { - const auto rangeOfJustThisASTNode = - getSourceRangeOfThisASTNode(omitAssertions); - const auto rangeIncludingIgnoredNodes = - widenSourceRangeForIgnoredASTNodes(rangeOfJustThisASTNode); - const auto uncachedSourceRange = - widenSourceRangeForChildren(rangeIncludingIgnoredNodes, omitAssertions); - return uncachedSourceRange; -} - -void ASTScopeImpl::clearCachedSourceRangesOfMeAndAncestors() { - // An optimization: if my range isn't cached, my ancestors must not be - if (!isSourceRangeCached()) - return; - cachedSourceRange = None; - if (auto p = getParent()) - p.get()->clearCachedSourceRangesOfMeAndAncestors(); -} - -#pragma mark compensating for InterpolatedStringLiteralExprs and EditorPlaceHolders - -static bool isInterpolatedStringLiteral(const Token& tok) { - SmallVector Segments; - Lexer::getStringLiteralSegments(tok, Segments, nullptr); - return Segments.size() != 1 || - Segments.front().Kind != Lexer::StringSegment::Literal; -} - -/// If right brace is missing, the source range of the body will end -/// at the last token, which may be a one of the special cases below. -/// This work is only needed for *unexpanded* scopes because unioning the range -/// with the children will do the same thing for an expanded scope. -/// It is also needed for ignored \c ASTNodes, which may be, e.g. \c -/// InterpolatedStringLiterals -static SourceLoc getLocEncompassingPotentialLookups(const SourceManager &SM, - const SourceLoc endLoc) { - const auto tok = Lexer::getTokenAtLocation(SM, endLoc); - switch (tok.getKind()) { - default: - return endLoc; - case tok::string_literal: - if (!isInterpolatedStringLiteral(tok)) - return endLoc; // Just the start of the last token - break; - case tok::identifier: - // subtract one to get a closed-range endpoint from a half-open - if (!Identifier::isEditorPlaceholder(tok.getText())) - return endLoc; - break; - } - return tok.getRange().getEnd().getAdvancedLoc(-1); -} - -SourceRange ASTScopeImpl::sourceRangeForDeferredExpansion() const { - return SourceRange(); -} -SourceRange IterableTypeScope::sourceRangeForDeferredExpansion() const { - return portion->sourceRangeForDeferredExpansion(this); -} -SourceRange FunctionBodyScope::sourceRangeForDeferredExpansion() const { - const auto bsr = decl->getOriginalBodySourceRange(); - const SourceLoc endEvenIfNoCloseBraceAndEndsWithInterpolatedStringLiteral = - getLocEncompassingPotentialLookups(getSourceManager(), bsr.End); - return SourceRange(bsr.Start, - endEvenIfNoCloseBraceAndEndsWithInterpolatedStringLiteral); -} - -SourceRange GenericTypeOrExtensionWholePortion::sourceRangeForDeferredExpansion( - const IterableTypeScope *s) const { - const auto rangeOfThisNodeWithoutChildren = - getChildlessSourceRangeOf(s, false); - const auto rangeExtendedForFinalToken = SourceRange( - rangeOfThisNodeWithoutChildren.Start, - getLocEncompassingPotentialLookups(s->getSourceManager(), - rangeOfThisNodeWithoutChildren.End)); - const auto rangePastExtendedNominal = - s->moveStartPastExtendedNominal(rangeExtendedForFinalToken); - return rangePastExtendedNominal; -} - -SourceRange GenericTypeOrExtensionWherePortion::sourceRangeForDeferredExpansion( - const IterableTypeScope *) const { - return SourceRange(); -} - -SourceRange IterableTypeBodyPortion::sourceRangeForDeferredExpansion( - const IterableTypeScope *s) const { - const auto bracesRange = getChildlessSourceRangeOf(s, false); - return SourceRange(bracesRange.Start, - getLocEncompassingPotentialLookups(s->getSourceManager(), - bracesRange.End)); -} - -SourceRange ASTScopeImpl::getEffectiveSourceRange(const ASTNode n) const { - if (const auto *d = n.dyn_cast()) - return d->getSourceRange(); - if (const auto *s = n.dyn_cast()) - return s->getSourceRange(); - auto *e = n.dyn_cast(); - return getLocEncompassingPotentialLookups(getSourceManager(), e->getEndLoc()); -} - -/// Some nodes (e.g. the error expression) cannot possibly contain anything to -/// be looked up and if included in a parent scope's source range would expand -/// it beyond an ancestor's source range. But if the ancestor is expanded -/// lazily, we check that its source range does not change when expanding it, -/// and this check would fail. -static bool sourceRangeWouldInterfereWithLaziness(const ASTNode n) { - return n.isExpr(ExprKind::Error); -} - -static bool -shouldIgnoredASTNodeSourceRangeWidenEnclosingScope(const ASTNode n) { - if (n.isDecl(DeclKind::Var)) { - // The pattern scopes will include the source ranges for VarDecls. - // Using its range here would cause a pattern initializer scope's range - // to overlap the pattern use scope's range. - return false; - } - if (sourceRangeWouldInterfereWithLaziness(n)) - return false; - return true; -} - -void ASTScopeImpl::widenSourceRangeForIgnoredASTNode(const ASTNode n) { - if (!shouldIgnoredASTNodeSourceRangeWidenEnclosingScope(n)) - return; - - // FIXME: why only do effectiveness bit for *ignored* nodes? - SourceRange r = getEffectiveSourceRange(n); - if (r.isInvalid()) - return; - if (sourceRangeOfIgnoredASTNodes.isInvalid()) - sourceRangeOfIgnoredASTNodes = r; - else - sourceRangeOfIgnoredASTNodes.widen(r); -} - SourceLoc getLocAfterExtendedNominal(const ExtensionDecl *const ext) { const auto *const etr = ext->getExtendedTypeRepr(); if (!etr) From fbb97b9e2650194f38c34f7a6e1fbfd57d78ae42 Mon Sep 17 00:00:00 2001 From: Slava Pestov Date: Tue, 6 Oct 2020 15:35:24 -0400 Subject: [PATCH 5/6] AST: Fix an assertion in UnqualifiedLookup to use CharSourceRanges --- lib/AST/UnqualifiedLookup.cpp | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/lib/AST/UnqualifiedLookup.cpp b/lib/AST/UnqualifiedLookup.cpp index d1fc74b511786..d1267254ca94c 100644 --- a/lib/AST/UnqualifiedLookup.cpp +++ b/lib/AST/UnqualifiedLookup.cpp @@ -27,6 +27,7 @@ #include "swift/Basic/STLExtras.h" #include "swift/Basic/SourceManager.h" #include "swift/Basic/Statistic.h" +#include "swift/Parse/Lexer.h" #include "llvm/ADT/DenseMap.h" #include "llvm/ADT/TinyPtrVector.h" #include "llvm/Support/Debug.h" @@ -147,7 +148,7 @@ namespace { #pragma mark context-based lookup declarations - bool isOutsideBodyOfFunction(const AbstractFunctionDecl *const AFD) const; + bool isInsideBodyOfFunction(const AbstractFunctionDecl *const AFD) const; /// For diagnostic purposes, move aside the unavailables, and put /// them back as a last-ditch effort. @@ -319,11 +320,11 @@ void UnqualifiedLookupFactory::lookUpTopLevelNamesInModuleScopeContext( #pragma mark context-based lookup definitions -bool UnqualifiedLookupFactory::isOutsideBodyOfFunction( +bool UnqualifiedLookupFactory::isInsideBodyOfFunction( const AbstractFunctionDecl *const AFD) const { - return !AFD->isImplicit() && Loc.isValid() && - AFD->getBodySourceRange().isValid() && - !SM.rangeContainsTokenLoc(AFD->getBodySourceRange(), Loc); + auto range = Lexer::getCharSourceRangeFromSourceRange( + SM, AFD->getBodySourceRange()); + return range.contains(Loc); } void UnqualifiedLookupFactory::ResultFinderForTypeContext::findResults( @@ -635,7 +636,7 @@ bool ASTScopeDeclConsumerForUnqualifiedLookup::lookInMembers( NominalTypeDecl *const nominal) { if (candidateSelfDC) { if (auto *afd = dyn_cast(candidateSelfDC)) { - assert(!factory.isOutsideBodyOfFunction(afd) && "Should be inside"); + assert(factory.isInsideBodyOfFunction(afd) && "Should be inside"); } } From e59069f60f34e1d120ed5e750b258945dfce405c Mon Sep 17 00:00:00 2001 From: Slava Pestov Date: Tue, 6 Oct 2020 15:35:01 -0400 Subject: [PATCH 6/6] Add a couple of crashers that are now fixed --- validation-test/compiler_crashers_2_fixed/sr12997.swift | 5 +++++ validation-test/compiler_crashers_2_fixed/sr12999.swift | 5 +++++ 2 files changed, 10 insertions(+) create mode 100644 validation-test/compiler_crashers_2_fixed/sr12997.swift create mode 100644 validation-test/compiler_crashers_2_fixed/sr12999.swift diff --git a/validation-test/compiler_crashers_2_fixed/sr12997.swift b/validation-test/compiler_crashers_2_fixed/sr12997.swift new file mode 100644 index 0000000000000..fa04d7c402fad --- /dev/null +++ b/validation-test/compiler_crashers_2_fixed/sr12997.swift @@ -0,0 +1,5 @@ +// RUN: not %target-swift-frontend -typecheck %s + +func addProperties(b: Int) { + guard true else { + _ = "`\(b)`." diff --git a/validation-test/compiler_crashers_2_fixed/sr12999.swift b/validation-test/compiler_crashers_2_fixed/sr12999.swift new file mode 100644 index 0000000000000..a5f0b4fa09a3e --- /dev/null +++ b/validation-test/compiler_crashers_2_fixed/sr12999.swift @@ -0,0 +1,5 @@ + // RUN: not %target-swift-frontend -typecheck %s + +public final class Foo { + public var a: String { + return "\(backgroundContext)"