Skip to content

Commit

Permalink
Improve the computation of the completion prefix
Browse files Browse the repository at this point in the history
There are a couple of tests where the new code fails to produce a
prefix, but in those cases the prefixes are `""` and `{`, which are
not useful for filtering (and wrong for replacing). The new code does,
however, now find prefixes that the old code missed, which improves the
filtering. The changes to the tests reflect the improved filtering.

Change-Id: I3d48162ee56de1b94ecb3a88d11e8a2167fb8f2f
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/372860
Reviewed-by: Keerti Parthasarathy <[email protected]>
Commit-Queue: Brian Wilkerson <[email protected]>
  • Loading branch information
bwilkerson authored and Commit Queue committed Jun 24, 2024
1 parent 77feaad commit e159bfe
Show file tree
Hide file tree
Showing 13 changed files with 78 additions and 62 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,6 @@ class DartCompletionManager {
required DartCompletionRequest request,
bool suggestOverrides = true,
bool suggestUris = true,
required bool useFilter,
}) async {
request.checkAborted();

Expand All @@ -101,7 +100,8 @@ class DartCompletionManager {
if (selection == null) {
throw AbortCompletion();
}
var targetPrefix = request.targetPrefix;
var tokenData = TokenData.fromSelection(selection);
var targetPrefix = tokenData?.prefix ?? '';
var matcher =
targetPrefix.isEmpty ? NoPrefixMatcher() : FuzzyMatcher(targetPrefix);
var state = CompletionState(request, selection, budget, matcher);
Expand Down Expand Up @@ -164,8 +164,7 @@ class DartCompletionManager {
performance: performance,
request: request,
suggestOverrides: enableOverrideContributor,
suggestUris: enableUriContributor,
useFilter: useFilter);
suggestUris: enableUriContributor);

var builder =
SuggestionBuilder(request, useFilter: useFilter, listener: listener);
Expand Down Expand Up @@ -458,3 +457,78 @@ class NotImportedSuggestions {
/// processed all available libraries, e.g. we ran out of budget.
bool isIncomplete = false;
}

/// Information about the token containing the selection.
class TokenData {
/// The token containing the offset.
///
/// The token can be any token, including a comment token.
final Token token;

/// The prefix before the selection offset.
///
/// This will be an empty string if the token isn't either an identifier or
/// keyword, or if the selection offset is at the beginning of the token.
final String prefix;

TokenData._(this.token, this.prefix);

/// Returns token data representing the token containing the offset of the
/// [selection], or `null` if the offset isn't within any token.
static TokenData? fromSelection(Selection selection) {
var coveringNode = selection.coveringNode;
var selectionOffset = selection.offset;
// Start at the last token in the covering node and walk backward in the
// token stream until we've found the left-most token whose offset is before
// the `selectionOffset`.
var currentToken = coveringNode.endToken;
while ((currentToken.isSynthetic ||
currentToken.offset > selectionOffset ||
(currentToken.offset == selectionOffset &&
!currentToken.isKeywordOrIdentifier)) &&
!currentToken.isEof) {
currentToken = currentToken.previous!;
}
if (currentToken.isEof) {
return null;
}
if (selectionOffset > currentToken.end) {
// The selection is between two tokens. Check to see whether it's inside a
// comment token.
Token? commentToken = currentToken.next!.precedingComments;
while (commentToken != null) {
if (selectionOffset >= commentToken.offset &&
selectionOffset <= commentToken.end) {
return TokenData._(commentToken, '');
}
commentToken = commentToken.next;
}
return null;
}
if (currentToken.isKeywordOrIdentifier) {
var offsetInToken = selectionOffset - currentToken.offset;
var prefix = currentToken.lexeme.substring(0, offsetInToken);
return TokenData._(currentToken, prefix);
} else if (currentToken.type == TokenType.STRING) {
// Compute a prefix inside string literals to support completion of URIs
// in directives.
var lexeme = currentToken.lexeme;
var startOfContent = 1;
if (lexeme.startsWith("r'''") || lexeme.startsWith('r"""')) {
startOfContent = 4;
} else if (lexeme.startsWith("r'") || lexeme.startsWith('r"')) {
startOfContent = 2;
} else if (lexeme.startsWith("'''") || lexeme.startsWith('"""')) {
startOfContent = 3;
}
var offsetInToken = selectionOffset - currentToken.offset;
if (offsetInToken < startOfContent) {
// The cursor is inside the opening quote sequence.
return TokenData._(currentToken, '');
}
var prefix = currentToken.lexeme.substring(startOfContent, offsetInToken);
return TokenData._(currentToken, prefix);
}
return TokenData._(currentToken, '');
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -1821,12 +1821,6 @@ A0 S1;
replacement
left: 1
suggestions
A0
kind: class
S2.B
kind: class
_B0
kind: class
S2
kind: library
''');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -212,8 +212,6 @@ A Sew;
replacement
left: 1
suggestions
S0.B
kind: class
S0
kind: library
''');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2139,12 +2139,6 @@ A0 S1;
replacement
left: 1
suggestions
A0
kind: class
S2.B
kind: class
_B0
kind: class
S2
kind: library
''');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1447,12 +1447,6 @@ A0 S1;
replacement
left: 1
suggestions
A0
kind: class
S2.B
kind: class
_B0
kind: class
S2
kind: library
''');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -412,10 +412,6 @@ suggestions
kind: import
dart:typed_data
kind: import
package:
kind: import
package:test/
kind: import
package:test/test.dart
kind: import
dart:core
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,22 +91,12 @@ suggestions
await computeSuggestions('''
class A e^ implements foo { }
''');
// TODO(brianwilkerson): The keyword `with` should not be suggested when
// using protocol 2 (so this these should require a conditional check).
// The reason it is being suggested is as follows: The `e` is ignored by
// the parser so it doesn't show up in the AST. As a result, the "entity"
// is the implements clause, and the code doesn't find the unattached token
// for `e`. As a result, there is no prefix, so the fuzzy matcher returns 1
// (a perfect match). We need to improve the detection of a prefix in order
// to fix this bug.
assertResponse(r'''
replacement
left: 1
suggestions
extends
kind: keyword
with
kind: keyword
''');
}

Expand All @@ -120,8 +110,6 @@ replacement
suggestions
extends
kind: keyword
with
kind: keyword
''');
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -90,8 +90,6 @@ suggestions
kind: keyword
async*
kind: keyword
sync*
kind: keyword
''');
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,8 +93,6 @@ suggestions
kind: keyword
async*
kind: keyword
sync*
kind: keyword
''');
}

Expand All @@ -110,8 +108,6 @@ suggestions
kind: keyword
async*
kind: keyword
sync*
kind: keyword
''');
}

Expand All @@ -127,8 +123,6 @@ suggestions
kind: keyword
async*
kind: keyword
sync*
kind: keyword
''');
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -245,14 +245,10 @@ import "foo" d^ hide foo;
replacement
left: 1
suggestions
as
kind: keyword
deferred as
kind: keyword
hide
kind: keyword
show
kind: keyword
''');
}

Expand Down Expand Up @@ -323,14 +319,10 @@ import "foo" d^ show foo;
replacement
left: 1
suggestions
as
kind: keyword
deferred as
kind: keyword
hide
kind: keyword
show
kind: keyword
''');
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -281,8 +281,6 @@ suggestions
kind: keyword
async*
kind: keyword
sync*
kind: keyword
''');
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,6 @@ replacement
suggestions
case
kind: keyword
default:
kind: keyword
''');
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,6 @@ void f() { if (v i^ && false) {} }
replacement
left: 1
suggestions
case
kind: keyword
is
kind: keyword
''');
Expand Down

0 comments on commit e159bfe

Please sign in to comment.