Skip to content

[CodeCompletion] Enable ASTScope in code completion #33433

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.

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
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 5 additions & 1 deletion include/swift/AST/SourceFile.h
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ class SourceFile final : public FileUnit {
bool IsPrimary;

/// The scope map that describes this source file.
std::unique_ptr<ASTScope> Scope;
NullablePtr<ASTScope> Scope = nullptr;

/// The set of validated opaque return type decls in the source file.
llvm::SmallVector<OpaqueTypeDecl *, 4> OpaqueReturnTypes;
Expand Down Expand Up @@ -468,6 +468,10 @@ class SourceFile final : public FileUnit {
/// Retrieve the scope that describes this source file.
ASTScope &getScope();

void clearScope() {
Scope = nullptr;
}

/// Retrieves the previously set delayed parser state, asserting that it
/// exists.
PersistentParserState *getDelayedParserState() {
Expand Down
17 changes: 17 additions & 0 deletions include/swift/Basic/SourceManager.h
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,20 @@ class SourceManager {
/// to speed up stats.
mutable llvm::DenseMap<StringRef, llvm::vfs::Status> StatusCache;

struct ReplacedRangeType {
SourceRange Original;
SourceRange New;
ReplacedRangeType() {}
ReplacedRangeType(NoneType) {}
ReplacedRangeType(SourceRange Original, SourceRange New)
: Original(Original), New(New) {
assert(Original.isValid() && New.isValid());
}

explicit operator bool() const { return Original.isValid(); }
};
ReplacedRangeType ReplacedRange;

// \c #sourceLocation directive handling.
struct VirtualFile {
CharSourceRange Range;
Expand Down Expand Up @@ -89,6 +103,9 @@ class SourceManager {

SourceLoc getCodeCompletionLoc() const;

const ReplacedRangeType &getReplacedRange() const { return ReplacedRange; }
void setReplacedRange(const ReplacedRangeType &val) { ReplacedRange = val; }

/// Returns true if \c LHS is before \c RHS in the source buffer.
bool isBeforeInBuffer(SourceLoc LHS, SourceLoc RHS) const {
return LHS.Value.getPointer() < RHS.Value.getPointer();
Expand Down
13 changes: 7 additions & 6 deletions lib/AST/ASTScopeCreation.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -795,7 +795,7 @@ void ASTSourceFileScope::
void ASTSourceFileScope::expandFunctionBody(AbstractFunctionDecl *AFD) {
if (!AFD)
return;
auto sr = AFD->getBodySourceRange();
auto sr = AFD->getOriginalBodySourceRange();
if (sr.isInvalid())
return;
ASTScopeImpl *bodyScope = findInnermostEnclosingScope(sr.Start, nullptr);
Expand Down Expand Up @@ -1596,11 +1596,6 @@ ASTScopeImpl *GenericTypeOrExtensionWholePortion::expandScope(
// Get now in case recursion emancipates scope
auto *const ip = scope->getParent().get();

// Prevent circular request bugs caused by illegal input and
// doing lookups that getExtendedNominal in the midst of getExtendedNominal.
if (scope->shouldHaveABody() && !scope->doesDeclHaveABody())
return ip;

auto *context = scope->getGenericContext();
auto *genericParams = (isa<TypeAliasDecl>(context)
? context->getParsedGenericParams()
Expand All @@ -1609,6 +1604,12 @@ ASTScopeImpl *GenericTypeOrExtensionWholePortion::expandScope(
scope->getDecl(), genericParams, scope);
if (context->getTrailingWhereClause())
scope->createTrailingWhereClauseScope(deepestScope, scopeCreator);

// Prevent circular request bugs caused by illegal input and
// doing lookups that getExtendedNominal in the midst of getExtendedNominal.
if (scope->shouldHaveABody() && !scope->doesDeclHaveABody())
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this happen in non-code completion cases too?

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, yes. For example, for:

extension Collection where Element == Int

current master emits

test.swift:2:42: error: expected '{' in extension
extension Collection where Element == Int
                                         ^
test.swift:2:28: error: cannot find type 'Element' in scope
extension Collection where Element == Int
                           ^~~~~~~
test.swift:2:36: error: generic signature requires types '<<error type>>' and 'Int' to be the same
extension Collection where Element == Int
                                   ^

With this change, it's just:

test.swift:2:42: error: expected '{' in extension
extension Collection where Element == Int
                                         ^

Copy link
Member Author

Choose a reason for hiding this comment

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

Which file can I add a test case for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added to test/decl/ext/extensions.swift

return ip;

scope->createBodyScope(deepestScope, scopeCreator);
return ip;
}
Expand Down
63 changes: 39 additions & 24 deletions lib/AST/ASTScopeLookup.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@
#include "swift/AST/TypeRepr.h"
#include "swift/Basic/STLExtras.h"
#include "llvm/Support/Compiler.h"
#include <algorithm>

using namespace swift;
using namespace namelookup;
Expand Down Expand Up @@ -82,6 +81,15 @@ const ASTScopeImpl *ASTScopeImpl::findStartingScopeForLookup(
// 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a FIXME and file a radar for me or Dave? The ASTScope should be a 'single source of truth', even for invalid code.

Copy link
Member Author

Choose a reason for hiding this comment

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

Filed rdar://problem/66945121

// 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
Expand Down Expand Up @@ -143,33 +151,40 @@ bool ASTScopeImpl::checkSourceRangeOfThisASTNode() const {
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,
SourceLoc loc) {
if (const auto &replacedRange = sourceMgr.getReplacedRange()) {
if (sourceMgr.rangeContainsTokenLoc(replacedRange.New, loc) &&
!sourceMgr.rangeContains(replacedRange.New, range)) {
return replacedRange.Original.Start;
}
}
return loc;
}

NullablePtr<ASTScopeImpl>
ASTScopeImpl::findChildContaining(SourceLoc loc,
SourceManager &sourceMgr) const {
// Use binary search to find the child that contains this location.
struct CompareLocs {
SourceManager &sourceMgr;

bool operator()(const ASTScopeImpl *scope, SourceLoc loc) {
ASTScopeAssert(scope->checkSourceRangeOfThisASTNode(), "Bad range.");
return -1 == ASTScopeImpl::compare(scope->getSourceRangeOfScope(), loc,
sourceMgr,
/*ensureDisjoint=*/false);
}
bool operator()(SourceLoc loc, const ASTScopeImpl *scope) {
ASTScopeAssert(scope->checkSourceRangeOfThisASTNode(), "Bad range.");
// Alternatively, we could check that loc < start-of-scope
return 0 >= ASTScopeImpl::compare(loc, scope->getSourceRangeOfScope(),
sourceMgr,
/*ensureDisjoint=*/false);
}
};
auto *const *child = std::lower_bound(
getChildren().begin(), getChildren().end(), loc, CompareLocs{sourceMgr});

if (child != getChildren().end() &&
sourceMgr.rangeContainsTokenLoc((*child)->getSourceRangeOfScope(), loc))
return *child;
auto *const *child = llvm::lower_bound(
getChildren(), loc,
[&sourceMgr](const ASTScopeImpl *scope, SourceLoc loc) {
ASTScopeAssert(scope->checkSourceRangeOfThisASTNode(), "Bad range.");
auto rangeOfScope = scope->getSourceRangeOfScope();
loc = translateLocForReplacedRange(sourceMgr, rangeOfScope, loc);
return -1 == ASTScopeImpl::compare(rangeOfScope, loc, sourceMgr,
/*ensureDisjoint=*/false);
});

if (child != getChildren().end()) {
auto rangeOfScope = (*child)->getSourceRangeOfScope();
loc = translateLocForReplacedRange(sourceMgr, rangeOfScope, loc);
if (sourceMgr.rangeContainsTokenLoc(rangeOfScope, loc))
return *child;
}
Comment on lines +172 to +187
Copy link
Contributor

Choose a reason for hiding this comment

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

Great! Thanks.


return nullptr;
}
Expand Down
22 changes: 19 additions & 3 deletions lib/AST/ASTScopeSourceRange.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,14 @@ ASTScopeImpl::widenSourceRangeForChildren(const SourceRange 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we try to clean this up as well?

if (const auto &replacedRange = getSourceManager().getReplacedRange()) {
if (getSourceManager().rangeContains(replacedRange.Original, range) &&
getSourceManager().rangeContains(replacedRange.New, childRange))
return r;
}
r.widen(childRange);
return r;
}
Expand Down Expand Up @@ -114,6 +122,14 @@ bool ASTScopeImpl::verifyThatChildrenAreContainedWithin(
getChildren().back()->getSourceRangeOfScope().End);
if (getSourceManager().rangeContains(range, rangeOfChildren))
return true;

// 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;
}

auto &out = verificationError() << "children not contained in its parent\n";
if (getChildren().size() == 1) {
out << "\n***Only Child node***\n";
Expand Down Expand Up @@ -200,7 +216,7 @@ SourceRange DifferentiableAttributeScope::getSourceRangeOfThisASTNode(

SourceRange AbstractFunctionBodyScope::getSourceRangeOfThisASTNode(
const bool omitAssertions) const {
return decl->getBodySourceRange();
return decl->getOriginalBodySourceRange();
}

SourceRange TopLevelCodeScope::getSourceRangeOfThisASTNode(
Expand Down Expand Up @@ -357,7 +373,7 @@ SourceRange AbstractFunctionDeclScope::getSourceRangeOfThisASTNode(
ASTScopeAssert(r.End.isValid(), "Start valid imples end valid.");
return r;
}
return decl->getBodySourceRange();
return decl->getOriginalBodySourceRange();
}

SourceRange ParameterListScope::getSourceRangeOfThisASTNode(
Expand Down Expand Up @@ -609,7 +625,7 @@ SourceRange IterableTypeScope::sourceRangeForDeferredExpansion() const {
return portion->sourceRangeForDeferredExpansion(this);
}
SourceRange AbstractFunctionBodyScope::sourceRangeForDeferredExpansion() const {
const auto bsr = decl->getBodySourceRange();
const auto bsr = decl->getOriginalBodySourceRange();
const SourceLoc endEvenIfNoCloseBraceAndEndsWithInterpolatedStringLiteral =
getLocEncompassingPotentialLookups(getSourceManager(), bsr.End);
return SourceRange(bsr.Start,
Expand Down
2 changes: 1 addition & 1 deletion lib/AST/Module.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2387,7 +2387,7 @@ StringRef SourceFile::getFilename() const {

ASTScope &SourceFile::getScope() {
if (!Scope)
Scope = std::unique_ptr<ASTScope>(new (getASTContext()) ASTScope(this));
Scope = new (getASTContext()) ASTScope(this);
return *Scope.get();
}

Expand Down
5 changes: 2 additions & 3 deletions lib/IDE/CompletionInstance.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -417,6 +417,8 @@ bool CompletionInstance::performCachedOperationIfPossible(

auto *AFD = cast<AbstractFunctionDecl>(DC);
AFD->setBodyToBeReparsed(newBodyRange);
SM.setReplacedRange({AFD->getOriginalBodySourceRange(), newBodyRange});
oldSF->clearScope();

traceDC = AFD;
break;
Expand Down Expand Up @@ -602,9 +604,6 @@ bool swift::ide::CompletionInstance::performOperation(
// We don't need token list.
Invocation.getLangOptions().CollectParsedToken = false;

// FIXME: ASTScopeLookup doesn't support code completion yet.
Invocation.disableASTScopeLookup();

if (EnableASTCaching) {
// Compute the signature of the invocation.
llvm::hash_code ArgsHash(0);
Expand Down
21 changes: 21 additions & 0 deletions test/IDE/complete_attributes.swift
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=TOP_LEVEL_ATTR_1 -code-completion-keywords=false | %FileCheck %s -check-prefix=ERROR_COMMON
// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=MEMBER_DECL_ATTR_1 -code-completion-keywords=false | %FileCheck %s -check-prefix=ERROR_COMMON
// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=ATTRARG_MEMBER | %FileCheck %s -check-prefix=MEMBER_MyValue
// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=ATTRARG_MEMBER_IN_CLOSURE | %FileCheck %s -check-prefix=MEMBER_MyValue

// ERROR_COMMON: found code completion token
// ERROR_COMMON-NOT: Keyword/
Expand All @@ -9,3 +11,22 @@
class MemberDeclAttribute {
@#^MEMBER_DECL_ATTR_1^# func memberDeclAttr1() {}
}

struct MyValue {
init() {}
static var val: Int
}

// MEMBER_MyValue: Begin completions, 4 items
// MEMBER_MyValue-DAG: Keyword[self]/CurrNominal: self[#MyValue.Type#];
// MEMBER_MyValue-DAG: Keyword/CurrNominal: Type[#MyValue.Type#];
// MEMBER_MyValue-DAG: Decl[Constructor]/CurrNominal: init()[#MyValue#];
// MEMBER_MyValue-DAG: Decl[StaticVar]/CurrNominal: val[#Int#];
// MEMBER_MyValue: End completions

class TestUknownDanglingAttr1 {
@UknownAttr(arg: MyValue.#^ATTRARG_MEMBER^#)
}
class TestUknownDanglingAttr2 {
@UknownAttr(arg: { MyValue.#^ATTRARG_MEMBER_IN_CLOSURE^# })
}
7 changes: 7 additions & 0 deletions test/decl/ext/extensions.swift
Original file line number Diff line number Diff line change
Expand Up @@ -349,3 +349,10 @@ struct SR_10466<T> {
extension SR_10466 where T == Never { // expected-note {{requirement specified as 'T' == 'Never' [with T = T]}}
typealias A = Int
}

#if true
protocol Rdar66943328 {
associatedtype Assoc
}
extension Rdar66943328 where Assoc == Int // expected-error {{expected '{' in extension}}
#endif