Skip to content

Migrate ASTScope to CharSourceRanges #34207

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Oct 8, 2020
Merged

Migrate ASTScope to CharSourceRanges #34207

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4252659
ASTScope: Add a getCharSourceRangeOfScope() method to ASTScopeImpl
slavapestov Oct 2, 2020
771fd6e
ASTScope: Redo assertions to look at CharSourceRanges
slavapestov Oct 2, 2020
e52413f
ASTScope: Use CharSourceRanges for lookup
slavapestov Oct 2, 2020
996100c
ASTScope: Remove old SourceRange machinery
slavapestov Oct 6, 2020
fbb97b9
AST: Fix an assertion in UnqualifiedLookup to use CharSourceRanges
slavapestov Oct 6, 2020
e59069f
Add a couple of crashers that are now fixed
slavapestov Oct 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
96 changes: 8 additions & 88 deletions include/swift/AST/ASTScope.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,21 +141,14 @@ 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;

/// 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<SourceRange> 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<CharSourceRange> cachedCharSourceRange;

#pragma mark - constructor / destructor
public:
Expand Down Expand Up @@ -192,9 +185,6 @@ class ASTScopeImpl {
public:
void addChild(ASTScopeImpl *child, ASTContext &);

private:
NullablePtr<ASTScopeImpl> getPriorSibling() const;

public:
void preOrderDo(function_ref<void(ASTScopeImpl *)>);
/// Like preorderDo but without myself.
Expand All @@ -203,78 +193,26 @@ 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;

/// 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<void()>);

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();
CharSourceRange getCharSourceRangeOfScope(SourceManager &SM,
bool omitAssertions = false) const;
bool isCharSourceRangeCached() const;

public: // public for debugging
/// Returns source range of this node alone, without factoring in any
/// children.
virtual SourceRange
getSourceRangeOfThisASTNode(bool omitAssertions = false) const = 0;

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:
Expand Down Expand Up @@ -329,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<ASTScopeImpl> insertionPointForDeferredExpansion();
virtual SourceRange sourceRangeForDeferredExpansion() const;

private:
virtual ScopeCreator &getScopeCreator();
Expand Down Expand Up @@ -545,8 +475,6 @@ class Portion {

virtual NullablePtr<ASTScopeImpl>
insertionPointForDeferredExpansion(IterableTypeScope *) const = 0;
virtual SourceRange
sourceRangeForDeferredExpansion(const IterableTypeScope *) const = 0;
};

// For the whole Decl scope of a GenericType or an Extension
Expand All @@ -570,8 +498,6 @@ class Portion {

NullablePtr<ASTScopeImpl>
insertionPointForDeferredExpansion(IterableTypeScope *) const override;
SourceRange
sourceRangeForDeferredExpansion(const IterableTypeScope *) const override;
};

/// GenericTypeOrExtension = GenericType or Extension
Expand Down Expand Up @@ -603,8 +529,6 @@ class GenericTypeOrExtensionWherePortion final

NullablePtr<ASTScopeImpl>
insertionPointForDeferredExpansion(IterableTypeScope *) const override;
SourceRange
sourceRangeForDeferredExpansion(const IterableTypeScope *) const override;
};

/// Behavior specific to representing the Body of a NominalTypeDecl or
Expand All @@ -622,8 +546,6 @@ class IterableTypeBodyPortion final

NullablePtr<ASTScopeImpl>
insertionPointForDeferredExpansion(IterableTypeScope *) const override;
SourceRange
sourceRangeForDeferredExpansion(const IterableTypeScope *) const override;
};

/// GenericType or Extension scope
Expand Down Expand Up @@ -720,7 +642,6 @@ class IterableTypeScope : public GenericTypeScope {

public:
NullablePtr<ASTScopeImpl> insertionPointForDeferredExpansion() override;
SourceRange sourceRangeForDeferredExpansion() const override;

void countBodies(ScopeCreator &) const;
};
Expand Down Expand Up @@ -929,7 +850,6 @@ class FunctionBodyScope : public ASTScopeImpl {
public:
std::string getClassName() const override;
NullablePtr<ASTScopeImpl> insertionPointForDeferredExpansion() override;
SourceRange sourceRangeForDeferredExpansion() const override;
};

class DefaultArgumentInitializerScope final : public ASTScopeImpl {
Expand Down
21 changes: 1 addition & 20 deletions lib/AST/ASTScope.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,23 +197,4 @@ void ASTScopeImpl::postOrderDo(function_ref<void(ASTScopeImpl *)> fn) {
for (auto *child : getChildren())
child->postOrderDo(fn);
fn(this);
}

unsigned ASTScopeImpl::countDescendants() const {
unsigned count = 0;
const_cast<ASTScopeImpl *>(this)->preOrderDo(
[&](ASTScopeImpl *) { ++count; });
return count - 1;
}

// Can fail if a subscope is lazy and not reexpanded
void ASTScopeImpl::assertThatTreeDoesNotShrink(function_ref<void()> fn) {
#ifndef NDEBUG
unsigned beforeCount = countDescendants();
#endif
fn();
#ifndef NDEBUG
unsigned afterCount = countDescendants();
ASTScopeAssert(beforeCount <= afterCount, "shrank?!");
#endif
}
}
20 changes: 9 additions & 11 deletions lib/AST/ASTScopeCreation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,8 +334,6 @@ class ScopeCreator final {

ASTScopeImpl *insertionPoint =
child->expandAndBeCurrentDetectingRecursion(*this);
ASTScopeAssert(child->verifyThatThisNodeComeAfterItsPriorSibling(),
"Ensure search will work");
return insertionPoint;
}

Expand Down Expand Up @@ -574,7 +572,6 @@ class NodeAdder
#define VISIT_AND_IGNORE(What) \
NullablePtr<ASTScopeImpl> visit##What(What *w, ASTScopeImpl *p, \
ScopeCreator &) { \
p->widenSourceRangeForIgnoredASTNode(w); \
return p; \
}

Expand Down Expand Up @@ -768,10 +765,9 @@ class NodeAdder

NullablePtr<ASTScopeImpl> visitExpr(Expr *expr, ASTScopeImpl *p,
ScopeCreator &scopeCreator) {
if (expr) {
p->widenSourceRangeForIgnoredASTNode(expr);
if (expr)
scopeCreator.addExprToScopeTree(expr, p);
}

return p;
}
};
Expand Down Expand Up @@ -881,6 +877,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.
Expand All @@ -889,9 +892,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();
}

#pragma mark implementations of expansion
Expand Down Expand Up @@ -926,8 +926,6 @@ ASTScopeImpl *ASTScopeImpl::expandAndBeCurrent(ScopeCreator &scopeCreator) {

setWasExpanded();

ASTScopeAssert(checkSourceRangeAfterExpansion(scopeCreator.getASTContext()),
"Bad range.");
return insertionPoint;
}

Expand Down
26 changes: 10 additions & 16 deletions lib/AST/ASTScopeLookup.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,22 +72,14 @@ 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,
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;
}
}
Expand All @@ -101,17 +93,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;
}

Expand Down
4 changes: 1 addition & 3 deletions lib/AST/ASTScopePrinting.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}

Expand Down
Loading