Skip to content

Remove ASTScope starting DeclContext hack #33976

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 4 commits into from
Sep 17, 2020
Merged

Remove ASTScope starting DeclContext hack #33976

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0ddd093
Sema: Fix unqualified lookup performed for DefineMemberBasedOnUninten…
slavapestov Sep 16, 2020
df1b631
ASTScope: Fix source range of ClosureParametersScope
slavapestov Sep 16, 2020
cfffb40
AST: Perform a top-level lookup in directReferencesForUnqualifiedType…
slavapestov Sep 16, 2020
09bb3c0
ASTScope: Don't need to pass in a DeclContext
slavapestov Sep 15, 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
11 changes: 2 additions & 9 deletions include/swift/AST/ASTScope.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -411,8 +411,7 @@ class ASTScopeImpl {

/// Entry point into ASTScopeImpl-land for lookups
static llvm::SmallVector<const ASTScopeImpl *, 0>
unqualifiedLookup(SourceFile *, DeclNameRef, SourceLoc,
const DeclContext *startingContext, DeclConsumer);
unqualifiedLookup(SourceFile *, DeclNameRef, SourceLoc, DeclConsumer);

/// Entry point into ASTScopeImpl-land for labeled statement lookups.
static llvm::SmallVector<LabeledStmt *, 4>
Expand All @@ -429,11 +428,7 @@ class ASTScopeImpl {
private:
static const ASTScopeImpl *findStartingScopeForLookup(SourceFile *,
const DeclNameRef name,
const SourceLoc where,
const DeclContext *ctx);

protected:
virtual bool doesContextMatchStartingContext(const DeclContext *) const;
const SourceLoc where);

protected:
/// Not const because may reexpand some scopes.
Expand Down Expand Up @@ -1006,7 +1001,6 @@ class GenericParamScope final : public ASTScopeImpl {
protected:
bool lookupLocalsOrMembers(ArrayRef<const ASTScopeImpl *>,
DeclConsumer) const override;
bool doesContextMatchStartingContext(const DeclContext *) const override;
Optional<bool>
resolveIsCascadingUseForThisScope(Optional<bool>) const override;
};
Expand Down Expand Up @@ -1622,7 +1616,6 @@ class DifferentiableAttributeScope final : public ASTScopeImpl {
ASTScopeImpl *expandSpecifically(ScopeCreator &) override;
bool lookupLocalsOrMembers(ArrayRef<const ASTScopeImpl *>,
DeclConsumer) const override;
bool doesContextMatchStartingContext(const DeclContext *) const override;
};

class SubscriptDeclScope final : public ASTScopeImpl {
Expand Down
1 change: 0 additions & 1 deletion include/swift/AST/NameLookup.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -691,7 +691,6 @@ class ASTScope {
/// \return the scopes traversed
static llvm::SmallVector<const ast_scope::ASTScopeImpl *, 0>
unqualifiedLookup(SourceFile *, DeclNameRef, SourceLoc,
const DeclContext *startingContext,
namelookup::AbstractASTScopeDeclConsumer &);

static Optional<bool>
Expand Down
4 changes: 1 addition & 3 deletions lib/AST/ASTScope.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,12 +40,10 @@ using namespace ast_scope;

llvm::SmallVector<const ASTScopeImpl *, 0> ASTScope::unqualifiedLookup(
SourceFile *SF, DeclNameRef name, SourceLoc loc,
const DeclContext *startingContext,
namelookup::AbstractASTScopeDeclConsumer &consumer) {
if (auto *s = SF->getASTContext().Stats)
++s->getFrontendCounters().NumASTScopeLookups;
return ASTScopeImpl::unqualifiedLookup(SF, name, loc, startingContext,
consumer);
return ASTScopeImpl::unqualifiedLookup(SF, name, loc, consumer);
}

Optional<bool> ASTScope::computeIsCascadingUse(
Expand Down
153 changes: 4 additions & 149 deletions lib/AST/ASTScopeLookup.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,28 +35,19 @@ using namespace swift;
using namespace namelookup;
using namespace ast_scope;

static bool isLocWithinAnInactiveClause(const SourceLoc loc, SourceFile *SF);

llvm::SmallVector<const ASTScopeImpl *, 0> ASTScopeImpl::unqualifiedLookup(
SourceFile *sourceFile, const DeclNameRef name, const SourceLoc loc,
const DeclContext *const startingContext, DeclConsumer consumer) {
DeclConsumer consumer) {
SmallVector<const ASTScopeImpl *, 0> history;
const auto *start =
findStartingScopeForLookup(sourceFile, name, loc, startingContext);
findStartingScopeForLookup(sourceFile, name, loc);
if (start)
start->lookup(history, nullptr, nullptr, consumer);
return history;
}

const ASTScopeImpl *ASTScopeImpl::findStartingScopeForLookup(
SourceFile *sourceFile, const DeclNameRef name, const SourceLoc loc,
const DeclContext *const startingContext) {
// At present, use legacy code in unqualifiedLookup.cpp to handle module-level
// lookups
// TODO: implement module scope someday
if (startingContext->getContextKind() == DeclContextKind::Module)
return nullptr;

SourceFile *sourceFile, const DeclNameRef name, const SourceLoc loc) {
auto *const fileScope = sourceFile->getScope().impl;
// Parser may have added decls to source file, since previous lookup
if (name.isOperator())
Expand All @@ -66,63 +57,7 @@ const ASTScopeImpl *ASTScopeImpl::findStartingScopeForLookup(
ASTScopeAssert(innermost->getWasExpanded(),
"If looking in a scope, it must have been expanded.");

// The legacy lookup code gets passed both a SourceLoc and a starting context.
// However, our ultimate intent is for clients to not have to pass in a
// DeclContext at all, since the SourceLoc should be enough. While we are
// debugging the new ASTScope lookup code, we can catch bugs by comparing the
// DeclContext of the ASTScope found from the desired SourceLoc to the
// DeclContext passed in by the client.

const auto *startingScope = innermost;
for (; startingScope &&
!startingScope->doesContextMatchStartingContext(startingContext);
startingScope = startingScope->getParent().getPtrOrNull()) {
}
// Someday, just use the assertion below. For now, print out lots of info for
// debugging.
if (!startingScope) {

// Be lenient in code completion mode. There are cases where the decl
// context doesn't match with the ASTScope. e.g. dangling attributes.
// FIXME: Create ASTScope tree even for invalid code.
if (innermost &&
startingContext->getASTContext().SourceMgr.hasCodeCompletionBuffer()) {
return innermost;
}

llvm::errs() << "ASTScopeImpl: resorting to startingScope hack, file: "
<< sourceFile->getFilename() << "\n";
// The check is costly, and inactive lookups will end up here, so don't
// do the check unless we can't find the startingScope.
const bool isInInactiveClause =
isLocWithinAnInactiveClause(loc, sourceFile);
if (isInInactiveClause)
llvm::errs() << " because location is within an inactive clause\n";
llvm::errs() << "'";
name.print(llvm::errs());
llvm::errs() << "' ";
llvm::errs() << "loc: ";
loc.print(llvm::errs(), sourceFile->getASTContext().SourceMgr);
llvm::errs() << "\nstarting context:\n ";
startingContext->printContext(llvm::errs());
// llvm::errs() << "\ninnermost: ";
// innermost->dump();
// llvm::errs() << "in: \n";
// fileScope->dump();
llvm::errs() << "\n\n";

// Might distort things
// if (fileScope->crossCheckWithAST())
// llvm::errs() << "Tree creation missed some DeclContexts.\n";

// Crash compilation even if NDEBUG
if (isInInactiveClause)
llvm::report_fatal_error(
"A lookup was attempted into an inactive clause");
}

ASTScopeAssert(startingScope, "ASTScopeImpl: could not find startingScope");
return startingScope;
return innermost;
}

ASTScopeImpl *
Expand Down Expand Up @@ -189,53 +124,6 @@ ASTScopeImpl::findChildContaining(SourceLoc loc,
return nullptr;
}

#pragma mark doesContextMatchStartingContext
// Match existing UnqualifiedLookupBehavior

bool ASTScopeImpl::doesContextMatchStartingContext(
const DeclContext *context) const {
// Why are we not checking the loc for this--because already did binary search
// on loc to find the start First, try MY DeclContext
if (auto myDCForL = getDeclContext())
return myDCForL == context;
// If I don't have one, ask my parent.
// (Choose innermost scope with matching loc & context.)
if (auto p = getParent())
return p.get()->doesContextMatchStartingContext(context);
// Topmost scope always has a context, the SourceFile.
ASTScope_unreachable("topmost scope always has a context, the SourceFile");
}

// For a SubscriptDecl with generic parameters, the call tries to do lookups
// with startingContext equal to either the get or set subscript
// AbstractFunctionDecls. Since the generic parameters are in the
// SubscriptDeclScope, and not the AbstractFunctionDecl scopes (after all how
// could one parameter be in two scopes?), GenericParamScope intercepts the
// match query here and tests against the accessor DeclContexts.
bool GenericParamScope::doesContextMatchStartingContext(
const DeclContext *context) const {
if (auto *asd = dyn_cast<AbstractStorageDecl>(holder)) {
for (auto accessor : asd->getAllAccessors()) {
if (up_cast<DeclContext>(accessor) == context)
return true;
}
}
return false;
}

bool DifferentiableAttributeScope::doesContextMatchStartingContext(
const DeclContext *context) const {
// Need special logic to handle case where `attributedDeclaration` is an
// `AbstractStorageDecl` (`SubscriptDecl` or `VarDecl`). The initial starting
// context in `ASTScopeImpl::findStartingScopeForLookup` will be an accessor
// of the `attributedDeclaration`.
if (auto *asd = dyn_cast<AbstractStorageDecl>(attributedDeclaration))
for (auto accessor : asd->getAllAccessors())
if (up_cast<DeclContext>(accessor) == context)
return true;
return false;
}

#pragma mark lookup methods that run once per scope

void ASTScopeImpl::lookup(SmallVectorImpl<const ASTScopeImpl *> &history,
Expand Down Expand Up @@ -841,39 +729,6 @@ Optional<bool> PatternEntryInitializerScope::resolveIsCascadingUseForThisScope(
return isCascadingUse;
}

bool isLocWithinAnInactiveClause(const SourceLoc loc, SourceFile *SF) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit surprised you can do away with this.

class InactiveClauseTester : public ASTWalker {
const SourceLoc loc;
const SourceManager &SM;

public:
bool wasFoundWithinInactiveClause = false;

InactiveClauseTester(const SourceLoc loc, const SourceManager &SM)
: loc(loc), SM(SM) {}

bool walkToDeclPre(Decl *D) override {
if (const auto *ifc = dyn_cast<IfConfigDecl>(D)) {
for (const auto &clause : ifc->getClauses()) {
if (clause.isActive)
continue;
for (const auto &n : clause.Elements) {
SourceRange sr = n.getSourceRange();
if (sr.isValid() && SM.rangeContainsTokenLoc(sr, loc)) {
wasFoundWithinInactiveClause = true;
return false;
}
}
}
}
return ASTWalker::walkToDeclPre(D);
}
};
InactiveClauseTester tester(loc, SF->getASTContext().SourceMgr);
SF->walk(tester);
return tester.wasFoundWithinInactiveClause;
}

#pragma mark isLabeledStmtLookupTerminator implementations
bool ASTScopeImpl::isLabeledStmtLookupTerminator() const {
return true;
Expand Down
3 changes: 1 addition & 2 deletions lib/AST/ASTScopeSourceRange.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -456,8 +456,7 @@ SourceRange ClosureParametersScope::getSourceRangeOfThisASTNode(
if (!omitAssertions)
ASTScopeAssert(closureExpr->getInLoc().isValid(),
"We don't create these if no in loc");
return SourceRange(getStartOfFirstParam(closureExpr),
closureExpr->getInLoc());
return SourceRange(closureExpr->getInLoc(), closureExpr->getEndLoc());
}

SourceRange
Expand Down
30 changes: 28 additions & 2 deletions lib/AST/NameLookup.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1946,7 +1946,33 @@ static DirectlyReferencedTypeDecls
directReferencesForUnqualifiedTypeLookup(DeclNameRef name,
SourceLoc loc, DeclContext *dc,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems a bit odd that we still pass in a DC here, esp. since passing in both a location and a DC.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to do a separate PR that redoes the user-facing unqualified lookup entry point to take a SourceFile instead of a DC. The first step was making sure the DC wasn't actually used internally :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good!

LookupOuterResults lookupOuter) {
// In a protocol or protocol extension, the 'where' clause can refer to
// associated types without 'Self' qualification:
//
// protocol MyProto where AssocType : Q { ... }
//
// extension MyProto where AssocType == Int { ... }
//
// For this reason, ASTScope maps source locations inside the 'where'
// clause to a scope that performs the lookup into the protocol or
// protocol extension.
//
// However, protocol and protocol extensions can also put bounds on 'Self',
// for example:
//
// protocol MyProto where Self : MyClass { ... }
//
// We must start searching for 'MyClass' at the top level, otherwise
// we end up with a cycle, because qualified lookup wants to resolve
// 'Self' bounds to build the set of declarations to search inside of.
//
// To make this work, we handle the top-level lookup case explicitly
// here, bypassing unqualified lookup and ASTScope altogether.
if (dc->isModuleScopeContext())
loc = SourceLoc();

Comment on lines +1949 to +1973
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

DirectlyReferencedTypeDecls results;

UnqualifiedLookupOptions options =
UnqualifiedLookupFlags::TypeLookup |
UnqualifiedLookupFlags::AllowProtocolMembers;
Expand All @@ -1958,8 +1984,8 @@ directReferencesForUnqualifiedTypeLookup(DeclNameRef name,
auto lookup = evaluateOrDefault(ctx.evaluator,
UnqualifiedLookupRequest{descriptor}, {});
for (const auto &result : lookup.allResults()) {
if (auto typeDecl = dyn_cast<TypeDecl>(result.getValueDecl()))
results.push_back(typeDecl);
auto typeDecl = cast<TypeDecl>(result.getValueDecl());
results.push_back(typeDecl);
Comment on lines +1987 to +1988
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting! I'd like to understand this change. Was the old check not needed?

Copy link
Contributor Author

@slavapestov slavapestov Sep 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Name lookup only returns TypeDecls if the TypesOnly flag is set, and we already rely on that being the case in other places in the compiler.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

}

return results;
Expand Down
2 changes: 1 addition & 1 deletion lib/AST/UnqualifiedLookup.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -544,7 +544,7 @@ void UnqualifiedLookupFactory::lookInASTScopes() {
#endif

ASTScope::unqualifiedLookup(DC->getParentSourceFile(),
Name, Loc, DC, consumer);
Name, Loc, consumer);
}

bool ASTScopeDeclConsumerForUnqualifiedLookup::consume(
Expand Down
4 changes: 2 additions & 2 deletions lib/Sema/CSSimplify.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7289,8 +7289,8 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyMemberConstraint(

auto descriptor = UnqualifiedLookupDescriptor(
DeclNameRef(param->getName()),
paramDecl->getDeclContext()->getParentForLookup(),
paramDecl->getStartLoc(),
paramDecl->getDeclContext()->getParentSourceFile(),
SourceLoc(),
UnqualifiedLookupFlags::KnownPrivate |
UnqualifiedLookupFlags::TypeLookup);
auto lookup = evaluateOrDefault(
Expand Down