-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[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
Changes from all commits
8a39c8d
513fed7
bc20856
b070e85
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great! Thanks. |
||
|
||
return nullptr; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
|
@@ -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"; | ||
|
@@ -200,7 +216,7 @@ SourceRange DifferentiableAttributeScope::getSourceRangeOfThisASTNode( | |
|
||
SourceRange AbstractFunctionBodyScope::getSourceRangeOfThisASTNode( | ||
const bool omitAssertions) const { | ||
return decl->getBodySourceRange(); | ||
return decl->getOriginalBodySourceRange(); | ||
} | ||
|
||
SourceRange TopLevelCodeScope::getSourceRangeOfThisASTNode( | ||
|
@@ -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( | ||
|
@@ -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, | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
current master emits
With this change, it's just:
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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