[CodeCompletion] Enable ASTScope in code completion #33433

rintaro · 2020-08-12T21:26:35Z

Enable ASTScope lookup (by default) in code completion.
rdar://problem/58939376

For fast-completion where the body of AbstractFunctionDecl might be replaced with another body parsed from another source buffer:

Use getOriginalBodySourceRange() for AbstractFunctionBodyScope
Remember the replaced range in SourceManager
Translate source ranges for finding scopes when the lookup location is in the replaced range (findInnermostEnclosingScope())
Bypass source range checks when the child scope from the replaced range (widenSourceRangeForChildren() and verifyThatChildrenAreContainedWithin )
Clear ASTScope for each completion

Unrelated to fast completion:

Add generic params and where clause scope even with missing body
Be lenient with ASTScope / DeclContext mismatch in code completion mode

rintaro · 2020-08-12T21:33:30Z

@swift-ci Please smoke test

rintaro · 2020-08-12T21:36:04Z

@swift-ci Please smoke test

slavapestov · 2020-08-12T21:37:15Z

lib/AST/ASTScopeLookup.cpp

@@ -82,6 +82,14 @@ 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.


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.

Filed rdar://problem/66945121

slavapestov · 2020-08-12T21:37:45Z

lib/AST/ASTScopeSourceRange.cpp

@@ -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.


Can we try to clean this up as well?

slavapestov · 2020-08-12T21:38:20Z

lib/AST/ASTScopeCreation.cpp

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


Can this happen in non-code completion cases too?

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 ^

Which file can I add a test case for this?

Added to test/decl/ext/extensions.swift

slavapestov · 2020-08-12T21:39:42Z

This is great, even with the hacks, which I think we can try to sort out later. The only remaining usage of non-ASTScope lookup is now in lldb.

* Re-create `ASTScope` for each completion * Add generic params and where clause scope even without missing body * Use `getOriginalBodySourceRange()` for `AbstractFunctionBodyScope` * Source range translations for replaced ranges when finding scopes * Bypass source range checks when the completion happens in the replaced range * Be lenient with ASTScope / DeclContext mismatch in code completion

rdar://problem/66943328

rintaro · 2020-08-13T00:03:11Z

@swift-ci Please smoke test

davidungar

Overall, looks pretty good. Just one comment where I could use some reassurance, because the code seems to assume that !(a<b) == b<a . Thanks for doing this!

davidungar · 2020-08-13T02:10:40Z

lib/AST/ASTScopeLookup.cpp

-      return 0 >= ASTScopeImpl::compare(loc, scope->getSourceRangeOfScope(),
-                                        sourceMgr,
-                                        /*ensureDisjoint=*/false);
+      return !(*this)(scope, loc);


Could you please double-check this? (Or maybe just reassure me) What about the == case?

Thanks for notifying! I think your concern is legitimate (the comparison function should not return true for == case).
But, it turned out that this function is not called. According to https://en.cppreference.com/w/cpp/algorithm/lower_bound , comparison happens only by comp(element, value) , but not comp(value, element).

The signature of the predicate function should be equivalent to the following:

bool pred(const Type1 &a, const Type2 &b);

The type Type1 must be such that an object of type ForwardIt can be dereferenced and then implicitly converted to Type1. The type Type2 must be such that an object of type T can be implicitly converted to Type2

So I rewrote this block to:

auto *const *child = llvm::lower_bound( getChildren(), loc, [&sourceMgr](const ASTScopeImpl *scope, SourceLoc loc) { ... });

Any concern with this?

Thank you for addressing it. I worry that a future maintainer, (or even myself) will get confused. If the function on line 183 is not called, do we still need to supply it? If so, can we just put an llvm unreachable in it? Otherwise, how about a comment around line 184 explaining away the potential confusion?

Hmm, since I removed operator()(SourceLoc loc, const ASTScopeImpl *scope) and rewrote CompareLocs with a lambda expression, I believe there's no unreachable code anymore.

The comparision happens only by 'comp(element, value)' not 'comp(value, element)'. We only need `operator()(const ASTScopeImpl *, SourceLoc)`. Use llvm::lower_bound() instead of std::lower_bound().

rintaro · 2020-08-13T16:13:35Z

@swift-ci Please smoke test

rintaro · 2020-08-13T16:50:28Z

@swift-ci Please smoke test

davidungar · 2020-08-13T19:44:54Z

lib/AST/ASTScopeLookup.cpp

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


Great! Thanks.

davidungar

Great! Thanks.

[CodeCompletion Remember the replaced source range in SourceManager

8a39c8d

rintaro force-pushed the ide-completion-astscope-rdar58939376-pt2 branch 2 times, most recently from 36fc543 to ca2f996 Compare August 12, 2020 21:33

rintaro requested review from davidungar and benlangmuir August 12, 2020 21:36

slavapestov approved these changes Aug 12, 2020

View reviewed changes

benlangmuir approved these changes Aug 12, 2020

View reviewed changes

rintaro added 2 commits August 12, 2020 16:36

[Test] Add a test case for extension where clause with missing body

bc20856

rdar://problem/66943328

rintaro force-pushed the ide-completion-astscope-rdar58939376-pt2 branch from ca2f996 to bc20856 Compare August 12, 2020 23:37

davidungar reviewed Aug 13, 2020

View reviewed changes

[ASTScope] Simplify binary search code in findChildContaining()

b070e85

The comparision happens only by 'comp(element, value)' not 'comp(value, element)'. We only need `operator()(const ASTScopeImpl *, SourceLoc)`. Use llvm::lower_bound() instead of std::lower_bound().

davidungar reviewed Aug 13, 2020

View reviewed changes

davidungar approved these changes Aug 13, 2020

View reviewed changes

rintaro merged commit 21b91cd into swiftlang:master Aug 13, 2020

rintaro deleted the ide-completion-astscope-rdar58939376-pt2 branch August 13, 2020 19:48

[CodeCompletion] Enable ASTScope in code completion #33433

[CodeCompletion] Enable ASTScope in code completion #33433

Uh oh!

Conversation

rintaro commented Aug 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rintaro commented Aug 12, 2020

Uh oh!

rintaro commented Aug 12, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slavapestov commented Aug 12, 2020

Uh oh!

rintaro commented Aug 13, 2020

Uh oh!

davidungar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rintaro Aug 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rintaro commented Aug 13, 2020

Uh oh!

rintaro commented Aug 13, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidungar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rintaro commented Aug 12, 2020 •

edited

Loading

rintaro Aug 13, 2020 •

edited

Loading