From 5e892b3dc950761697a5f2672dbb47c6e8bd1ce3 Mon Sep 17 00:00:00 2001 From: Slava Pestov Date: Tue, 6 Oct 2020 18:57:30 -0400 Subject: [PATCH 1/4] Sema: Eagerly expand scopes in TypeCheckASTNodeAtLocRequest --- lib/Sema/TypeCheckStmt.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/Sema/TypeCheckStmt.cpp b/lib/Sema/TypeCheckStmt.cpp index f320bbe34f09b..97c2846b23dfd 100644 --- a/lib/Sema/TypeCheckStmt.cpp +++ b/lib/Sema/TypeCheckStmt.cpp @@ -20,6 +20,7 @@ #include "MiscDiagnostics.h" #include "swift/Subsystems.h" #include "swift/AST/ASTPrinter.h" +#include "swift/AST/ASTScope.h" #include "swift/AST/ASTWalker.h" #include "swift/AST/ASTVisitor.h" #include "swift/AST/DiagnosticsSema.h" @@ -1892,6 +1893,8 @@ bool TypeCheckASTNodeAtLocRequest::evaluate(Evaluator &evaluator, if (auto *AFD = dyn_cast(DC)) { if (AFD->isBodyTypeChecked()) return false; + + ASTScope::expandFunctionBody(AFD); } // Function builder function doesn't support partial type checking. From 0c11fad5bd8018e18124f3e3cd4333cf99040c5b Mon Sep 17 00:00:00 2001 From: Slava Pestov Date: Tue, 6 Oct 2020 17:03:49 -0400 Subject: [PATCH 2/4] ASTScope: Don't silently drop duplicate AST nodes Instead, let's rely on the existing source range assertions. They will fire if we add two nodes with overlapping or equal source ranges. --- include/swift/AST/ASTScope.h | 21 --- lib/AST/ASTScopeCreation.cpp | 316 ++++++++++------------------------- 2 files changed, 84 insertions(+), 253 deletions(-) diff --git a/include/swift/AST/ASTScope.h b/include/swift/AST/ASTScope.h index 0e1f65e71625d..1e70eb886a34d 100644 --- a/include/swift/AST/ASTScope.h +++ b/include/swift/AST/ASTScope.h @@ -224,7 +224,6 @@ class ASTScopeImpl { virtual NullablePtr getDeclAttributeIfAny() const { return nullptr; } - virtual NullablePtr getReferrent() const { return nullptr; } #pragma mark - debugging and printing @@ -470,9 +469,6 @@ class Portion { virtual NullablePtr getLookupLimitFor(const GenericTypeOrExtensionScope *) const; - virtual const Decl * - getReferrentOfScope(const GenericTypeOrExtensionScope *s) const; - virtual NullablePtr insertionPointForDeferredExpansion(IterableTypeScope *) const = 0; }; @@ -493,9 +489,6 @@ class Portion { NullablePtr getLookupLimitFor(const GenericTypeOrExtensionScope *) const override; - const Decl * - getReferrentOfScope(const GenericTypeOrExtensionScope *s) const override; - NullablePtr insertionPointForDeferredExpansion(IterableTypeScope *) const override; }; @@ -570,7 +563,6 @@ class GenericTypeOrExtensionScope : public ASTScopeImpl { virtual Decl *getDecl() const = 0; NullablePtr getDeclIfAny() const override { return getDecl(); } - NullablePtr getReferrent() const override; private: AnnotatedInsertionPoint @@ -745,7 +737,6 @@ class GenericParamScope final : public ASTScopeImpl { /// Actually holder is always a GenericContext, need to test if /// ProtocolDecl or SubscriptDecl but will refactor later. - NullablePtr getReferrent() const override; std::string getClassName() const override; SourceRange getSourceRangeOfThisASTNode(bool omitAssertions = false) const override; @@ -788,8 +779,6 @@ class AbstractFunctionDeclScope final : public ASTScopeImpl { virtual NullablePtr getDeclIfAny() const override { return decl; } Decl *getDecl() const { return decl; } - NullablePtr getReferrent() const override; - protected: NullablePtr genericParams() const override; }; @@ -902,7 +891,6 @@ class AttachedPropertyWrapperScope final : public ASTScopeImpl { NullablePtr getDeclAttributeIfAny() const override { return attr; } - NullablePtr getReferrent() const override; private: void expandAScopeThatDoesNotCreateANewInsertionPoint(ScopeCreator &); @@ -970,8 +958,6 @@ class PatternEntryDeclScope final : public AbstractPatternEntryScope { SourceRange getSourceRangeOfThisASTNode(bool omitAssertions = false) const override; - NullablePtr getReferrent() const override; - protected: bool lookupLocalsOrMembers(DeclConsumer) const override; bool isLabeledStmtLookupTerminator() const override; @@ -1072,7 +1058,6 @@ class CaptureListScope final : public ASTScopeImpl { getSourceRangeOfThisASTNode(bool omitAssertions = false) const override; NullablePtr getExprIfAny() const override { return expr; } Expr *getExpr() const { return expr; } - NullablePtr getReferrent() const override; bool lookupLocalsOrMembers(DeclConsumer) const override; }; @@ -1094,7 +1079,6 @@ class ClosureParametersScope final : public ASTScopeImpl { } NullablePtr getExprIfAny() const override { return closureExpr; } Expr *getExpr() const { return closureExpr; } - NullablePtr getReferrent() const override; protected: ASTScopeImpl *expandSpecifically(ScopeCreator &scopeCreator) override; @@ -1128,7 +1112,6 @@ class TopLevelCodeScope final : public ASTScopeImpl { getSourceRangeOfThisASTNode(bool omitAssertions = false) const override; virtual NullablePtr getDeclIfAny() const override { return decl; } Decl *getDecl() const { return decl; } - NullablePtr getReferrent() const override; }; /// The \c _@specialize attribute. @@ -1153,7 +1136,6 @@ class SpecializeAttributeScope final : public ASTScopeImpl { NullablePtr getDeclAttributeIfAny() const override { return specializeAttr; } - NullablePtr getReferrent() const override; protected: ASTScopeImpl *expandSpecifically(ScopeCreator &) override; @@ -1183,7 +1165,6 @@ class DifferentiableAttributeScope final : public ASTScopeImpl { NullablePtr getDeclAttributeIfAny() const override { return differentiableAttr; } - NullablePtr getReferrent() const override; protected: ASTScopeImpl *expandSpecifically(ScopeCreator &) override; @@ -1214,7 +1195,6 @@ class SubscriptDeclScope final : public ASTScopeImpl { public: virtual NullablePtr getDeclIfAny() const override { return decl; } Decl *getDecl() const { return decl; } - NullablePtr getReferrent() const override; protected: NullablePtr genericParams() const override; @@ -1244,7 +1224,6 @@ class AbstractStmtScope : public ASTScopeImpl { getSourceRangeOfThisASTNode(bool omitAssertions = false) const override; virtual Stmt *getStmt() const = 0; NullablePtr getStmtIfAny() const override { return getStmt(); } - NullablePtr getReferrent() const override; protected: bool isLabeledStmtLookupTerminator() const override; diff --git a/lib/AST/ASTScopeCreation.cpp b/lib/AST/ASTScopeCreation.cpp index bbee272aed3ae..6dd649c11c747 100644 --- a/lib/AST/ASTScopeCreation.cpp +++ b/lib/AST/ASTScopeCreation.cpp @@ -132,61 +132,6 @@ static std::vector asNodeVector(DeclRange dr) { namespace swift { namespace ast_scope { -namespace { -/// Use me with any ASTNode, Expr*, Decl*, or Stmt* -/// I will yield a void* that is the same, even when given an Expr* and a -/// ClosureExpr* because I take the Expr*, figure its real class, then up -/// cast. -/// Useful for duplicate checking. -class UniquePointerCalculator - : public ASTVisitor { -public: - template const void *visit(const T *x) { - return const_cast(x); - } - - // Call these only from the superclass - void *visitDecl(Decl *e) { return e; } - void *visitStmt(Stmt *e) { return e; } - void *visitExpr(Expr *e) { return e; } - void *visitPattern(Pattern *e) { return e; } - void *visitDeclAttribute(DeclAttribute *e) { return e; } - -// Provide default implementations for statements as ASTVisitor does for Exprs -#define STMT(CLASS, PARENT) \ - void *visit##CLASS##Stmt(CLASS##Stmt *S) { return visitStmt(S); } -#include "swift/AST/StmtNodes.def" - -// Provide default implementations for patterns as ASTVisitor does for Exprs -#define PATTERN(CLASS, PARENT) \ - void *visit##CLASS##Pattern(CLASS##Pattern *S) { return visitPattern(S); } -#include "swift/AST/PatternNodes.def" -}; - -/// A set that does the right pointer calculation for comparing Decls to -/// DeclContexts, and Exprs -class NodeSet { - ::llvm::DenseSet pointers; - -public: - bool contains(const ASTScopeImpl *const s) { - if (auto *r = s->getReferrent().getPtrOrNull()) - return pointers.count(r); - return false; // never exclude a non-checkable scope - } - bool insert(const ASTScopeImpl *const s) { - if (auto *r = s->getReferrent().getPtrOrNull()) - return pointers.insert(r).second; - return true; - } - void erase(const ASTScopeImpl *const s) { - if (auto *r = s->getReferrent().getPtrOrNull()) - pointers.erase(r); - } -}; -} // namespace - #pragma mark ScopeCreator class ScopeCreator final { @@ -198,15 +143,9 @@ class ScopeCreator final { ASTSourceFileScope *const sourceFileScope; ASTContext &getASTContext() const { return ctx; } - /// The AST can have duplicate nodes, and we don't want to create scopes for - /// those. - NodeSet scopedNodes; - ScopeCreator(SourceFile *SF) : ctx(SF->getASTContext()), - sourceFileScope(new (ctx) ASTSourceFileScope(SF, this)) { - ctx.addDestructorCleanup(scopedNodes); - } + sourceFileScope(new (ctx) ASTSourceFileScope(SF, this)) {} ScopeCreator(const ScopeCreator &) = delete; // ensure no copies ScopeCreator(const ScopeCreator &&) = delete; // ensure no moves @@ -222,9 +161,7 @@ class ScopeCreator final { Optional endLoc) { auto *ip = insertionPoint; for (auto nd : nodesOrDeclsToAdd) { - auto *const newIP = - addToScopeTreeAndReturnInsertionPoint(nd, ip, endLoc).getPtrOr(ip); - ip = newIP; + ip = addToScopeTreeAndReturnInsertionPoint(nd, ip, endLoc); } return ip; } @@ -234,12 +171,12 @@ class ScopeCreator final { void addToScopeTree(ASTNode n, ASTScopeImpl *parent) { (void)addToScopeTreeAndReturnInsertionPoint(n, parent, None); } - /// Return new insertion point if the scope was not a duplicate + /// Return new insertion point. /// For ease of searching, don't call unless insertion point is needed /// /// \param endLoc The end location for any "scopes until the end" that /// we introduce here, such as PatternEntryDeclScope and GuardStmtScope - NullablePtr + ASTScopeImpl * addToScopeTreeAndReturnInsertionPoint(ASTNode, ASTScopeImpl *parent, Optional endLoc); @@ -299,31 +236,6 @@ class ScopeCreator final { return true; } - /// Create a new scope of class ChildScope initialized with a ChildElement, - /// expandScope it, - /// add it as a child of the receiver, and return the child and the scope to - /// receive more decls. - template - ASTScopeImpl *constructExpandAndInsertUncheckable(ASTScopeImpl *parent, - Args... args) { - ASTScopeAssert(!Scope(args...).getReferrent(), - "Not checking for duplicate ASTNode but class supports it"); - return constructExpandAndInsert(parent, args...); - } - - template - NullablePtr - ifUniqueConstructExpandAndInsert(ASTScopeImpl *parent, Args... args) { - Scope dryRun(args...); - ASTScopeAssert( - dryRun.getReferrent(), - "Checking for duplicate ASTNode but class does not support it"); - if (scopedNodes.insert(&dryRun)) - return constructExpandAndInsert(parent, args...); - return nullptr; - } - -private: template ASTScopeImpl *constructExpandAndInsert(ASTScopeImpl *parent, Args... args) { auto *child = new (ctx) Scope(args...); @@ -342,15 +254,7 @@ class ScopeCreator final { ASTScopeImpl *constructWithPortionExpandAndInsert(ASTScopeImpl *parent, Args... args) { const Portion *portion = new (ctx) PortionClass(); - return constructExpandAndInsertUncheckable(parent, portion, args...); - } - - template - NullablePtr - ifUniqueConstructWithPortionExpandAndInsert(ASTScopeImpl *parent, - Args... args) { - const Portion *portion = new (ctx) PortionClass(); - return ifUniqueConstructExpandAndInsert(parent, portion, args...); + return constructExpandAndInsert(parent, portion, args...); } void addExprToScopeTree(Expr *expr, ASTScopeImpl *parent) { @@ -370,13 +274,13 @@ class ScopeCreator final { std::pair walkToExprPre(Expr *E) override { if (auto *closure = dyn_cast(E)) { scopeCreator - .ifUniqueConstructExpandAndInsert( + .constructExpandAndInsert( parent, closure); return {false, E}; } if (auto *capture = dyn_cast(E)) { scopeCreator - .ifUniqueConstructExpandAndInsert( + .constructExpandAndInsert( parent, capture); return {false, E}; } @@ -411,9 +315,8 @@ class ScopeCreator final { return parent; auto *s = parent; for (unsigned i : indices(generics->getParams())) - s = ifUniqueConstructExpandAndInsert( - s, parameterizedDecl, generics, i) - .getPtrOr(s); + s = constructExpandAndInsert( + s, parameterizedDecl, generics, i); return s; } @@ -431,7 +334,7 @@ class ScopeCreator final { /// \param endLoc Must be valid iff the pattern binding is in a local /// scope, in which case this is the last source location where the /// pattern bindings are going to be visible. - NullablePtr + ASTScopeImpl * addPatternBindingToScopeTree(PatternBindingDecl *patternBinding, ASTScopeImpl *parent, Optional endLoc); @@ -555,8 +458,8 @@ namespace swift { namespace ast_scope { class NodeAdder - : public ASTVisitor, - NullablePtr, NullablePtr, + : public ASTVisitor { Optional endLoc; @@ -565,13 +468,9 @@ class NodeAdder #pragma mark ASTNodes that do not create scopes - // Even ignored Decls and Stmts must extend the source range of a scope: - // E.g. a braceStmt with some definitions that ends in a statement that - // accesses such a definition must resolve as being IN the scope. - #define VISIT_AND_IGNORE(What) \ - NullablePtr visit##What(What *w, ASTScopeImpl *p, \ - ScopeCreator &) { \ + ASTScopeImpl *visit##What(What *w, ASTScopeImpl *p, \ + ScopeCreator &) { \ return p; \ } @@ -602,9 +501,9 @@ class NodeAdder #pragma mark simple creation ignoring deferred nodes #define VISIT_AND_CREATE(What, ScopeClass) \ - NullablePtr visit##What(What *w, ASTScopeImpl *p, \ - ScopeCreator &scopeCreator) { \ - return scopeCreator.ifUniqueConstructExpandAndInsert(p, w); \ + ASTScopeImpl *visit##What(What *w, ASTScopeImpl *p, \ + ScopeCreator &scopeCreator) { \ + return scopeCreator.constructExpandAndInsert(p, w); \ } VISIT_AND_CREATE(SubscriptDecl, SubscriptDeclScope) @@ -623,9 +522,9 @@ class NodeAdder #pragma mark 2D simple creation (ignoring deferred nodes) #define VISIT_AND_CREATE_WHOLE_PORTION(What, WhatScope) \ - NullablePtr visit##What(What *w, ASTScopeImpl *p, \ - ScopeCreator &scopeCreator) { \ - return scopeCreator.ifUniqueConstructWithPortionExpandAndInsert< \ + ASTScopeImpl *visit##What(What *w, ASTScopeImpl *p, \ + ScopeCreator &scopeCreator) { \ + return scopeCreator.constructWithPortionExpandAndInsert< \ WhatScope, GenericTypeOrExtensionWholePortion>(p, w); \ } @@ -640,8 +539,8 @@ class NodeAdder // This declaration is handled from // addChildrenForAllLocalizableAccessorsInSourceOrder - NullablePtr visitAccessorDecl(AccessorDecl *ad, ASTScopeImpl *p, - ScopeCreator &scopeCreator) { + ASTScopeImpl *visitAccessorDecl(AccessorDecl *ad, ASTScopeImpl *p, + ScopeCreator &scopeCreator) { return visitAbstractFunctionDecl(ad, p, scopeCreator); } @@ -650,17 +549,17 @@ class NodeAdder // Each of the following creates a new scope, so that nodes which were parsed // after them need to be placed in scopes BELOW them in the tree. So pass down // the deferred nodes. - NullablePtr visitGuardStmt(GuardStmt *e, ASTScopeImpl *p, - ScopeCreator &scopeCreator) { + ASTScopeImpl *visitGuardStmt(GuardStmt *e, ASTScopeImpl *p, + ScopeCreator &scopeCreator) { ASTScopeAssert(endLoc.hasValue(), "GuardStmt outside of a BraceStmt?"); - return scopeCreator.ifUniqueConstructExpandAndInsert( + return scopeCreator.constructExpandAndInsert( p, e, *endLoc); } - NullablePtr visitTopLevelCodeDecl(TopLevelCodeDecl *d, - ASTScopeImpl *p, - ScopeCreator &scopeCreator) { + ASTScopeImpl *visitTopLevelCodeDecl(TopLevelCodeDecl *d, + ASTScopeImpl *p, + ScopeCreator &scopeCreator) { ASTScopeAssert(endLoc.hasValue(), "TopLevelCodeDecl in wrong place?"); - return scopeCreator.ifUniqueConstructExpandAndInsert( + return scopeCreator.constructExpandAndInsert( p, d, *endLoc); } @@ -670,21 +569,21 @@ class NodeAdder ASTScope_unreachable("SourceFiles are orphans."); } - NullablePtr visitYieldStmt(YieldStmt *ys, ASTScopeImpl *p, - ScopeCreator &scopeCreator) { + ASTScopeImpl *visitYieldStmt(YieldStmt *ys, ASTScopeImpl *p, + ScopeCreator &scopeCreator) { for (Expr *e : ys->getYields()) visitExpr(e, p, scopeCreator); return p; } - NullablePtr visitDeferStmt(DeferStmt *ds, ASTScopeImpl *p, - ScopeCreator &scopeCreator) { + ASTScopeImpl *visitDeferStmt(DeferStmt *ds, ASTScopeImpl *p, + ScopeCreator &scopeCreator) { visitFuncDecl(ds->getTempDecl(), p, scopeCreator); return p; } - NullablePtr visitBraceStmt(BraceStmt *bs, ASTScopeImpl *p, - ScopeCreator &scopeCreator) { + ASTScopeImpl *visitBraceStmt(BraceStmt *bs, ASTScopeImpl *p, + ScopeCreator &scopeCreator) { if (bs->empty()) return p; @@ -710,17 +609,16 @@ class NodeAdder if (endLoc.hasValue()) endLocForBraceStmt = *endLoc; - auto maybeBraceScope = - scopeCreator.ifUniqueConstructExpandAndInsert( - p, bs, std::move(localFuncsAndTypes), std::move(localVars), - endLocForBraceStmt); if (auto *s = scopeCreator.getASTContext().Stats) ++s->getFrontendCounters().NumBraceStmtASTScopes; - return maybeBraceScope.getPtrOr(p); + return + scopeCreator.constructExpandAndInsert( + p, bs, std::move(localFuncsAndTypes), std::move(localVars), + endLocForBraceStmt); } - NullablePtr + ASTScopeImpl * visitPatternBindingDecl(PatternBindingDecl *patternBinding, ASTScopeImpl *parentScope, ScopeCreator &scopeCreator) { @@ -728,43 +626,43 @@ class NodeAdder patternBinding, parentScope, endLoc); } - NullablePtr visitEnumElementDecl(EnumElementDecl *eed, - ASTScopeImpl *p, - ScopeCreator &scopeCreator) { - scopeCreator.constructExpandAndInsertUncheckable(p, eed); + ASTScopeImpl *visitEnumElementDecl(EnumElementDecl *eed, + ASTScopeImpl *p, + ScopeCreator &scopeCreator) { + scopeCreator.constructExpandAndInsert(p, eed); return p; } - NullablePtr visitIfConfigDecl(IfConfigDecl *icd, - ASTScopeImpl *p, - ScopeCreator &scopeCreator) { + ASTScopeImpl *visitIfConfigDecl(IfConfigDecl *icd, + ASTScopeImpl *p, + ScopeCreator &scopeCreator) { ASTScope_unreachable( "Should be handled inside of " "expandIfConfigClausesThenCullAndSortElementsOrMembers"); } - NullablePtr visitReturnStmt(ReturnStmt *rs, ASTScopeImpl *p, - ScopeCreator &scopeCreator) { + ASTScopeImpl *visitReturnStmt(ReturnStmt *rs, ASTScopeImpl *p, + ScopeCreator &scopeCreator) { if (rs->hasResult()) visitExpr(rs->getResult(), p, scopeCreator); return p; } - NullablePtr visitThrowStmt(ThrowStmt *ts, ASTScopeImpl *p, - ScopeCreator &scopeCreator) { + ASTScopeImpl *visitThrowStmt(ThrowStmt *ts, ASTScopeImpl *p, + ScopeCreator &scopeCreator) { visitExpr(ts->getSubExpr(), p, scopeCreator); return p; } - NullablePtr visitPoundAssertStmt(PoundAssertStmt *pas, - ASTScopeImpl *p, - ScopeCreator &scopeCreator) { + ASTScopeImpl *visitPoundAssertStmt(PoundAssertStmt *pas, + ASTScopeImpl *p, + ScopeCreator &scopeCreator) { visitExpr(pas->getCondition(), p, scopeCreator); return p; } - NullablePtr visitExpr(Expr *expr, ASTScopeImpl *p, - ScopeCreator &scopeCreator) { + ASTScopeImpl *visitExpr(Expr *expr, ASTScopeImpl *p, + ScopeCreator &scopeCreator) { if (expr) scopeCreator.addExprToScopeTree(expr, p); @@ -776,7 +674,7 @@ class NodeAdder // These definitions are way down here so it can call into // NodeAdder -NullablePtr +ASTScopeImpl * ScopeCreator::addToScopeTreeAndReturnInsertionPoint(ASTNode n, ASTScopeImpl *parent, Optional endLoc) { @@ -823,23 +721,23 @@ void ScopeCreator::addChildrenForKnownAttributes(ValueDecl *decl, for (auto *attr : relevantAttrs) { if (auto *diffAttr = dyn_cast(attr)) { - ifUniqueConstructExpandAndInsert( + constructExpandAndInsert( parent, diffAttr, decl); } else if (auto *specAttr = dyn_cast(attr)) { if (auto *afd = dyn_cast(decl)) { - ifUniqueConstructExpandAndInsert( + constructExpandAndInsert( parent, specAttr, afd); } } else if (auto *customAttr = dyn_cast(attr)) { if (auto *vd = dyn_cast(decl)) { - ifUniqueConstructExpandAndInsert( + constructExpandAndInsert( parent, customAttr, vd); } } } } -NullablePtr +ASTScopeImpl * ScopeCreator::addPatternBindingToScopeTree(PatternBindingDecl *patternBinding, ASTScopeImpl *parentScope, Optional endLoc) { @@ -861,10 +759,9 @@ ScopeCreator::addPatternBindingToScopeTree(PatternBindingDecl *patternBinding, } insertionPoint = - ifUniqueConstructExpandAndInsert( + constructExpandAndInsert( insertionPoint, patternBinding, i, - isLocalBinding, endLocForBinding) - .getPtrOr(insertionPoint); + isLocalBinding, endLocForBinding); ASTScopeAssert(isLocalBinding || insertionPoint == parentScope, "Bindings at the top-level or members of types should " @@ -1017,7 +914,7 @@ ParameterListScope::expandAScopeThatDoesNotCreateANewInsertionPoint( for (ParamDecl *pd : params->getArray()) { if (pd->hasDefaultExpr()) scopeCreator - .constructExpandAndInsertUncheckable( + .constructExpandAndInsert( this, pd); } } @@ -1041,7 +938,7 @@ PatternEntryDeclScope::expandAScopeThatCreatesANewInsertionPoint( patternEntry.getOriginalInit()->getStartLoc(), decl->getStartLoc()), "Original inits are always after the '='"); scopeCreator - .constructExpandAndInsertUncheckable( + .constructExpandAndInsert( this, decl, patternEntryIndex); } @@ -1074,7 +971,7 @@ ConditionalClausePatternUseScope::expandAScopeThatCreatesANewInsertionPoint( auto *initializer = sec.getInitializer(); if (!isa(initializer)) { scopeCreator - .constructExpandAndInsertUncheckable( + .constructExpandAndInsert( this, initializer); } @@ -1105,7 +1002,7 @@ GuardStmtScope::expandAScopeThatCreatesANewInsertionPoint(ScopeCreator & auto *body = stmt->getBody(); if (!body->empty()) { scopeCreator - .constructExpandAndInsertUncheckable( + .constructExpandAndInsert( conditionLookupParent, this, stmt->getBody()); } @@ -1142,14 +1039,13 @@ AnnotatedInsertionPoint TopLevelCodeScope::expandAScopeThatCreatesANewInsertionPoint(ScopeCreator & scopeCreator) { - if (auto *body = - scopeCreator - .addToScopeTreeAndReturnInsertionPoint(decl->getBody(), this, endLoc) - .getPtrOrNull()) - return {body, "So next top level code scope and put its decls in its body " - "under a guard statement scope (etc) from the last top level " - "code scope"}; - return {this, "No body"}; + auto *body = + scopeCreator + .addToScopeTreeAndReturnInsertionPoint(decl->getBody(), this, endLoc); + + return {body, "So next top level code scope and put its decls in its body " + "under a guard statement scope (etc) from the last top level " + "code scope"}; } #pragma mark expandAScopeThatDoesNotCreateANewInsertionPoint @@ -1171,7 +1067,7 @@ void AbstractFunctionDeclScope::expandAScopeThatDoesNotCreateANewInsertionPoint( auto *params = decl->getParameters(); if (params->size() > 0) { - scopeCreator.constructExpandAndInsertUncheckable( + scopeCreator.constructExpandAndInsert( leaf, params, nullptr); } } @@ -1180,16 +1076,14 @@ void AbstractFunctionDeclScope::expandAScopeThatDoesNotCreateANewInsertionPoint( // We create body scopes when there is no body for source kit to complete // erroneous code in bodies. if (decl->getBodySourceRange().isValid()) { - scopeCreator.constructExpandAndInsertUncheckable(leaf, - decl); + scopeCreator.constructExpandAndInsert(leaf, decl); } } void EnumElementScope::expandAScopeThatDoesNotCreateANewInsertionPoint( ScopeCreator &scopeCreator) { if (auto *pl = decl->getParameterList()) - scopeCreator.constructExpandAndInsertUncheckable( - this, pl, nullptr); + scopeCreator.constructExpandAndInsert(this, pl, nullptr); // The invariant that the raw value expression can never introduce a new scope // is checked in Parse. However, this guarantee is not future-proof. Compute // and add the raw value expression anyways just to be defensive. @@ -1261,8 +1155,7 @@ void SwitchStmtScope::expandAScopeThatDoesNotCreateANewInsertionPoint( for (auto caseStmt : stmt->getCases()) { if (isLocalizable(caseStmt)) - scopeCreator.ifUniqueConstructExpandAndInsert(this, - caseStmt); + scopeCreator.constructExpandAndInsert(this, caseStmt); } } @@ -1277,8 +1170,7 @@ void ForEachStmtScope::expandAScopeThatDoesNotCreateANewInsertionPoint( // above. if (!stmt->getBody()->isImplicit()) { if (isLocalizable(stmt->getBody())) - scopeCreator.constructExpandAndInsertUncheckable( - this, stmt); + scopeCreator.constructExpandAndInsert(this, stmt); } } @@ -1292,14 +1184,12 @@ void CaseStmtScope::expandAScopeThatDoesNotCreateANewInsertionPoint( ScopeCreator &scopeCreator) { for (auto &item : stmt->getCaseLabelItems()) { if (item.getGuardExpr()) { - scopeCreator.constructExpandAndInsertUncheckable( - this, item); + scopeCreator.constructExpandAndInsert(this, item); } } if (!stmt->getBody()->empty()) { - scopeCreator.constructExpandAndInsertUncheckable( - this, stmt); + scopeCreator.constructExpandAndInsert(this, stmt); } } @@ -1318,7 +1208,7 @@ void SubscriptDeclScope::expandAScopeThatDoesNotCreateANewInsertionPoint( scopeCreator.addChildrenForKnownAttributes(decl, this); auto *leaf = scopeCreator.addNestedGenericParamScopesToTree( decl, decl->getGenericParams(), this); - scopeCreator.constructExpandAndInsertUncheckable( + scopeCreator.constructExpandAndInsert( leaf, decl->getIndices(), decl->getAccessor(AccessorKind::Get)); scopeCreator.addChildrenForAllLocalizableAccessorsInSourceOrder(decl, leaf); } @@ -1327,8 +1217,7 @@ void CaptureListScope::expandAScopeThatDoesNotCreateANewInsertionPoint( ScopeCreator &scopeCreator) { auto *closureExpr = expr->getClosureBody(); scopeCreator - .ifUniqueConstructExpandAndInsert( - this, closureExpr); + .constructExpandAndInsert(this, closureExpr); } void ClosureParametersScope::expandAScopeThatDoesNotCreateANewInsertionPoint( @@ -1454,7 +1343,7 @@ ASTScopeImpl *LabeledConditionalStmtScope::createNestedConditionalClauseScopes( break; case StmtConditionElement::CK_PatternBinding: insertionPoint = - scopeCreator.constructExpandAndInsertUncheckable< + scopeCreator.constructExpandAndInsert< ConditionalClausePatternUseScope>( insertionPoint, sec, endLoc); break; @@ -1512,43 +1401,6 @@ ScopeCreator &ASTScopeImpl::getScopeCreator() { ScopeCreator &ASTSourceFileScope::getScopeCreator() { return *scopeCreator; } -#pragma mark getReferrent - - // These are the scopes whose ASTNodes (etc) might be duplicated in the AST - // getReferrent is the cookie used to dedup them - -#define GET_REFERRENT(Scope, x) \ - NullablePtr Scope::getReferrent() const { \ - return UniquePointerCalculator().visit(x); \ - } - -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()) -GET_REFERRENT(GenericParamScope, paramList->getParams()[index]) -GET_REFERRENT(AbstractStmtScope, getStmt()) -GET_REFERRENT(CaptureListScope, getExpr()) -GET_REFERRENT(ClosureParametersScope, getExpr()) -GET_REFERRENT(SpecializeAttributeScope, specializeAttr) -GET_REFERRENT(DifferentiableAttributeScope, differentiableAttr) -GET_REFERRENT(AttachedPropertyWrapperScope, attr) -GET_REFERRENT(GenericTypeOrExtensionScope, portion->getReferrentOfScope(this)); - -const Decl * -Portion::getReferrentOfScope(const GenericTypeOrExtensionScope *s) const { - return nullptr; -}; - -const Decl *GenericTypeOrExtensionWholePortion::getReferrentOfScope( - const GenericTypeOrExtensionScope *s) const { - return s->getDecl(); -}; - -#undef GET_REFERRENT - #pragma mark currency NullablePtr ASTScopeImpl::insertionPointForDeferredExpansion() { return nullptr; From ee0d008178d6046e12acaeaa9aad4f5d8647814c Mon Sep 17 00:00:00 2001 From: Slava Pestov Date: Wed, 7 Oct 2020 15:22:49 -0400 Subject: [PATCH 3/4] Parse: Preserve source order when code completion adds delayed declarations --- include/swift/AST/DeclContext.h | 22 +++++-- lib/AST/DeclContext.cpp | 109 +++++++++++++++++++++++++++----- lib/Parse/Parser.cpp | 4 +- 3 files changed, 110 insertions(+), 25 deletions(-) diff --git a/include/swift/AST/DeclContext.h b/include/swift/AST/DeclContext.h index e71f5d96c95fd..c6ef79874d2b9 100644 --- a/include/swift/AST/DeclContext.h +++ b/include/swift/AST/DeclContext.h @@ -782,9 +782,20 @@ class IterableDeclContext { /// abstractions on top of member loading, such as a name lookup table. DeclRange getCurrentMembersWithoutLoading() const; - /// Add a member to this context. If the hint decl is specified, the new decl - /// is inserted immediately after the hint. - void addMember(Decl *member, Decl *hint = nullptr); + /// Add a member to this context. + /// + /// If the hint decl is specified, the new decl is inserted immediately + /// after the hint. + /// + /// If insertAtHead is set, the new decl is inserted at the beginning of + /// the list. + /// + /// Otherwise, it is inserted at the end. + void addMember(Decl *member, Decl *hint = nullptr, bool insertAtHead = false); + + /// Add a member in the right place to preserve source order. This should + /// only be called from the code completion delayed parsing path. + void addMemberPreservingSourceOrder(Decl *member); /// Check whether there are lazily-loaded members. bool hasLazyMembers() const { @@ -862,10 +873,7 @@ class IterableDeclContext { private: /// Add a member to the list for iteration purposes, but do not notify the /// subclass that we have done so. - /// - /// This is used internally when loading members, because loading a - /// member is an invisible addition. - void addMemberSilently(Decl *member, Decl *hint = nullptr) const; + void addMemberSilently(Decl *member, Decl *hint, bool insertAtHead) const; }; /// Define simple_display for DeclContexts but not for subclasses in order to diff --git a/lib/AST/DeclContext.cpp b/lib/AST/DeclContext.cpp index 25224941d99af..2ef4cd00deb9c 100644 --- a/lib/AST/DeclContext.cpp +++ b/lib/AST/DeclContext.cpp @@ -764,10 +764,33 @@ ArrayRef IterableDeclContext::getSemanticMembers() const { ArrayRef()); } +void IterableDeclContext::addMemberPreservingSourceOrder(Decl *member) { + auto &SM = getASTContext().SourceMgr; + + SourceLoc start = member->getStartLoc(); + Decl *hint = nullptr; + + for (auto *existingMember : getMembers()) { + if (existingMember->isImplicit()) + continue; + + if (isa(existingMember) || + isa(existingMember)) + continue; + + if (!SM.isBeforeInBuffer(existingMember->getEndLoc(), start)) + break; + + hint = existingMember; + } + + addMember(member, hint, /*insertAtHead=*/hint == nullptr); +} + /// Add a member to this context. -void IterableDeclContext::addMember(Decl *member, Decl *Hint) { +void IterableDeclContext::addMember(Decl *member, Decl *hint, bool insertAtHead) { // Add the member to the list of declarations without notification. - addMemberSilently(member, Hint); + addMemberSilently(member, hint, insertAtHead); // Notify our parent declaration that we have added the member, which can // be used to update the lookup tables. @@ -790,29 +813,83 @@ void IterableDeclContext::addMember(Decl *member, Decl *Hint) { } } -void IterableDeclContext::addMemberSilently(Decl *member, Decl *hint) const { +void IterableDeclContext::addMemberSilently(Decl *member, Decl *hint, + bool insertAtHead) const { assert(!isa(member) && "Accessors should not be added here"); assert(!member->NextDecl && "Already added to a container"); - // If there is a hint decl that specifies where to add this, just - // link into the chain immediately following it. - if (hint) { +#ifndef NDEBUG + auto checkSourceRange = [&](Decl *prev, Decl *next) { + if (!member->getDeclContext()->getParentSourceFile()) + return; + + auto shouldSkip = [](Decl *d) { + if (isa(d) || isa(d) || isa(d)) + return true; + + if (d->isImplicit()) + return true; + + return false; + }; + + if (shouldSkip(prev) || shouldSkip(next)) + return; + + SourceLoc prevEnd = prev->getEndLoc(); + SourceLoc nextStart = next->getStartLoc(); + + if (!prevEnd.isValid() || !nextStart.isValid()) + return; + + if (getASTContext().SourceMgr.isBeforeInBuffer(prevEnd, nextStart)) + return; + + llvm::errs() << "Source ranges out of order in addMember():\n"; + prev->dump(llvm::errs()); + next->dump(llvm::errs()); + abort(); + }; +#endif + + // Empty list. + if (!FirstDeclAndLazyMembers.getPointer()) { + assert(hint == nullptr); + + FirstDeclAndLazyMembers.setPointer(member); + LastDeclAndKind.setPointer(member); + + // Insertion at the head. + } else if (insertAtHead) { + assert(hint == nullptr); + + member->NextDecl = FirstDeclAndLazyMembers.getPointer(); + FirstDeclAndLazyMembers.setPointer(member); + + // Insertion at the tail. + } else if (hint == nullptr) { + auto *last = LastDeclAndKind.getPointer(); + +#ifndef NDEBUG + checkSourceRange(last, member); +#endif + + last->NextDecl = member; + LastDeclAndKind.setPointer(member); + + // Insertion after 'hint' (which may be the tail). + } else { +#ifndef NDEBUG + checkSourceRange(hint, member); +#endif + member->NextDecl = hint->NextDecl; hint->NextDecl = member; - // If the hint was the last in the parent context's chain, update it. + // Handle case where the 'hint' is the tail. if (LastDeclAndKind.getPointer() == hint) LastDeclAndKind.setPointer(member); - return; - } - - if (auto last = LastDeclAndKind.getPointer()) { - last->NextDecl = member; - assert(last != member && "Simple cycle in decl list"); - } else { - FirstDeclAndLazyMembers.setPointer(member); } - LastDeclAndKind.setPointer(member); } void IterableDeclContext::setMemberLoader(LazyMemberLoader *loader, diff --git a/lib/Parse/Parser.cpp b/lib/Parse/Parser.cpp index 66eeaced9ffa6..4dd6fc627b3b7 100644 --- a/lib/Parse/Parser.cpp +++ b/lib/Parse/Parser.cpp @@ -189,9 +189,9 @@ void Parser::performCodeCompletionSecondPassImpl( parseDecl(ParseDeclOptions(info.Flags), /*IsAtStartOfLineOrPreviousHadSemi=*/true, [&](Decl *D) { if (auto *NTD = dyn_cast(DC)) { - NTD->addMember(D); + NTD->addMemberPreservingSourceOrder(D); } else if (auto *ED = dyn_cast(DC)) { - ED->addMember(D); + ED->addMemberPreservingSourceOrder(D); } else if (auto *SF = dyn_cast(DC)) { SF->addTopLevelDecl(D); } else { From 14620a34db8a92020daec4e09748e491bbfd0eca Mon Sep 17 00:00:00 2001 From: Slava Pestov Date: Tue, 6 Oct 2020 19:01:16 -0400 Subject: [PATCH 4/4] ASTScope: Remove source range sorting --- include/swift/AST/ASTScope.h | 4 ---- lib/AST/ASTScopeCreation.cpp | 31 ++++--------------------------- lib/AST/ASTScopeSourceRange.cpp | 33 +-------------------------------- 3 files changed, 5 insertions(+), 63 deletions(-) diff --git a/include/swift/AST/ASTScope.h b/include/swift/AST/ASTScope.h index 1e70eb886a34d..a1145d3f96e45 100644 --- a/include/swift/AST/ASTScope.h +++ b/include/swift/AST/ASTScope.h @@ -194,10 +194,6 @@ class ASTScopeImpl { #pragma mark - source ranges public: - /// Return signum of ranges. Centralize the invariant that ASTScopes use ends. - static int compare(SourceRange, SourceRange, const SourceManager &, - bool ensureDisjoint); - CharSourceRange getCharSourceRangeOfScope(SourceManager &SM, bool omitAssertions = false) const; bool isCharSourceRangeCached() const; diff --git a/lib/AST/ASTScopeCreation.cpp b/lib/AST/ASTScopeCreation.cpp index 6dd649c11c747..1dfaf3c5eb6ab 100644 --- a/lib/AST/ASTScopeCreation.cpp +++ b/lib/AST/ASTScopeCreation.cpp @@ -359,27 +359,6 @@ class ScopeCreator final { return culled; } - /// Templated to work on either ASTNodes, Decl*'s, or whatnot. - template - std::vector - sortBySourceRange(std::vector toBeSorted) const { - auto compareNodes = [&](Rangeable n1, Rangeable n2) { - return isNotAfter(n1, n2); - }; - std::stable_sort(toBeSorted.begin(), toBeSorted.end(), compareNodes); - return toBeSorted; - } - - template - bool isNotAfter(Rangeable n1, Rangeable n2) const { - const auto r1 = getRangeableSourceRange(n1); - const auto r2 = getRangeableSourceRange(n2); - - const int signum = ASTScopeImpl::compare(r1, r2, ctx.SourceMgr, - /*ensureDisjoint=*/true); - return -1 == signum; - } - SWIFT_DEBUG_DUMP { print(llvm::errs()); } void print(raw_ostream &out) const { @@ -895,8 +874,7 @@ ASTSourceFileScope::expandAScopeThatCreatesANewInsertionPoint( std::vector newNodes(decls.begin(), decls.end()); insertionPoint = scopeCreator.addSiblingsToScopeTree(insertionPoint, - scopeCreator.sortBySourceRange( - scopeCreator.cull(newNodes)), + scopeCreator.cull(newNodes), endLoc); // Too slow to perform all the time: @@ -1024,9 +1002,8 @@ BraceStmtScope::expandAScopeThatCreatesANewInsertionPoint( // elements in source order auto *insertionPoint = scopeCreator.addSiblingsToScopeTree(this, - scopeCreator.sortBySourceRange( - scopeCreator.cull( - stmt->getElements())), + scopeCreator.cull( + stmt->getElements()), endLoc); if (auto *s = scopeCreator.getASTContext().Stats) ++s->getFrontendCounters().NumBraceStmtASTScopeExpansions; @@ -1388,7 +1365,7 @@ void GenericTypeOrExtensionScope::expandBody(ScopeCreator &) {} void IterableTypeScope::expandBody(ScopeCreator &scopeCreator) { auto nodes = asNodeVector(getIterableDeclContext().get()->getMembers()); - nodes = scopeCreator.sortBySourceRange(scopeCreator.cull(nodes)); + nodes = scopeCreator.cull(nodes); scopeCreator.addSiblingsToScopeTree(this, nodes, None); if (auto *s = scopeCreator.getASTContext().Stats) ++s->getFrontendCounters().NumIterableTypeBodyASTScopeExpansions; diff --git a/lib/AST/ASTScopeSourceRange.cpp b/lib/AST/ASTScopeSourceRange.cpp index d8a27d5b749cf..ecd6892623987 100644 --- a/lib/AST/ASTScopeSourceRange.cpp +++ b/lib/AST/ASTScopeSourceRange.cpp @@ -371,35 +371,4 @@ SourceLoc ast_scope::extractNearestSourceLoc( std::tuple scopeAndCreator) { const ASTScopeImpl *scope = std::get<0>(scopeAndCreator); return scope->getSourceRangeOfThisASTNode().Start; -} - -int ASTScopeImpl::compare(const SourceRange lhs, const SourceRange rhs, - const SourceManager &SM, const bool ensureDisjoint) { - ASTScopeAssert(!SM.isBeforeInBuffer(lhs.End, lhs.Start), - "Range is backwards."); - ASTScopeAssert(!SM.isBeforeInBuffer(rhs.End, rhs.Start), - "Range is backwards."); - - auto cmpLoc = [&](const SourceLoc lhs, const SourceLoc rhs) { - return lhs == rhs ? 0 : SM.isBeforeInBuffer(lhs, rhs) ? -1 : 1; - }; - // Establish that we use end locations throughout ASTScopes here - const int endOrder = cmpLoc(lhs.End, rhs.End); - -#ifndef NDEBUG - if (ensureDisjoint) { - const int startOrder = cmpLoc(lhs.Start, rhs.Start); - - if (startOrder * endOrder == -1) { - llvm::errs() << "*** Start order contradicts end order between: ***\n"; - lhs.print(llvm::errs(), SM, false); - llvm::errs() << "\n*** and: ***\n"; - rhs.print(llvm::errs(), SM, false); - } - ASTScopeAssert(startOrder * endOrder != -1, - "Start order contradicts end order"); - } -#endif - - return endOrder; -} +} \ No newline at end of file