Convert a few more methods in KeywordContributor
Change-Id: I905a4855f67db227ab654c3d4dc7441b7ee2bf58
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/315540
Commit-Queue: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Samuel Rawlins <srawlins@google.com>
diff --git a/pkg/analysis_server/lib/src/services/completion/dart/completion_manager.dart b/pkg/analysis_server/lib/src/services/completion/dart/completion_manager.dart
index 30474ea..a90fed6 100644
--- a/pkg/analysis_server/lib/src/services/completion/dart/completion_manager.dart
+++ b/pkg/analysis_server/lib/src/services/completion/dart/completion_manager.dart
@@ -305,7 +305,7 @@
}
var state = CompletionState(request.libraryElement, selection);
var pass = InScopeCompletionPass(state: state, collector: collector);
- selection.coveringNode.accept(pass);
+ pass.computeSuggestions();
builder.suggestFromCandidates(collector.suggestions);
}
}
diff --git a/pkg/analysis_server/lib/src/services/completion/dart/in_scope_completion_pass.dart b/pkg/analysis_server/lib/src/services/completion/dart/in_scope_completion_pass.dart
index 5e7dd47..d472f36 100644
--- a/pkg/analysis_server/lib/src/services/completion/dart/in_scope_completion_pass.dart
+++ b/pkg/analysis_server/lib/src/services/completion/dart/in_scope_completion_pass.dart
@@ -5,8 +5,10 @@
import 'package:analysis_server/src/services/completion/dart/completion_state.dart';
import 'package:analysis_server/src/services/completion/dart/keyword_helper.dart';
import 'package:analysis_server/src/services/completion/dart/suggestion_collector.dart';
+import 'package:analysis_server/src/utilities/extensions/ast.dart';
import 'package:analyzer/dart/analysis/features.dart';
import 'package:analyzer/dart/ast/ast.dart';
+import 'package:analyzer/dart/ast/token.dart';
import 'package:analyzer/dart/ast/visitor.dart';
/// A completion pass that will create candidate suggestions based on the
@@ -39,12 +41,99 @@
/// Return the offset at which completion was requested.
int get offset => state.selection.offset;
+ /// Return the node that should be used as the context in which completion is
+ /// occurring.
+ ///
+ /// This is normally the covering node, but if the covering node begins with
+ /// an identifier (or keyword) and the [offset] is covered by the identifier,
+ /// then we look for the highest node that also begins with the same token and
+ /// use the parent of that node.
+ ///
+ /// This allows us more context for completing what the user might be trying
+ /// to write and also reduces the complexity of the visitor and reduces the
+ /// amount of code duplication.
+ AstNode get _completionNode {
+ var selection = state.selection;
+ var coveringNode = selection.coveringNode;
+ var beginToken = coveringNode.beginToken;
+ if (!beginToken.isKeywordOrIdentifier ||
+ !selection.isCoveredByToken(beginToken)) {
+ return coveringNode;
+ }
+ var child = coveringNode;
+ var parent = child.parent;
+ while (parent != null && parent.beginToken == beginToken) {
+ child = parent;
+ parent = child.parent;
+ }
+ // The [child] is now the highest node that starts with the [beginToken].
+ if (parent != null) {
+ return parent;
+ }
+ return child;
+ }
+
+ /// Compute the candidate suggestions associated with this pass.
+ void computeSuggestions() {
+ _completionNode.accept(this);
+ }
+
@override
void visitAdjacentStrings(AdjacentStrings node) {
_visitParentIfAtOrBeforeNode(node);
}
@override
+ void visitAssignmentExpression(AssignmentExpression node) {
+ collector.completionLocation = 'AssignmentExpression_rightHandSide';
+ _forExpression(node);
+ }
+
+ @override
+ void visitAwaitExpression(AwaitExpression node) {
+ collector.completionLocation = 'AwaitExpression_expression';
+ _forExpression(node);
+ }
+
+ @override
+ void visitBinaryExpression(BinaryExpression node) {
+ var operator = node.operator.lexeme;
+ collector.completionLocation = 'BinaryExpression_${operator}_rightOperand';
+ _forExpression(node);
+ }
+
+ @override
+ void visitClassDeclaration(ClassDeclaration node) {
+ if (offset < node.classKeyword.offset) {
+ keywordHelper.addClassModifiers(node);
+ return;
+ }
+ if (offset <= node.classKeyword.end) {
+ keywordHelper.addKeyword(Keyword.CLASS);
+ return;
+ }
+ if (offset <= node.name.end) {
+ // TODO(brianwilkerson) Suggest a name for the class.
+ return;
+ }
+ if (offset <= node.leftBracket.offset) {
+ keywordHelper.addClassDeclarationKeywords(node);
+ return;
+ }
+ if (offset >= node.leftBracket.end && offset <= node.rightBracket.offset) {
+ collector.completionLocation = 'ClassDeclaration_member';
+ _forClassMember();
+ var element = node.members.elementBefore(offset);
+ if (element is MethodDeclaration) {
+ var body = element.body;
+ if (body.isEmpty) {
+ keywordHelper.addFunctionBodyModifiers(body);
+ }
+ }
+ }
+ }
+
+ @override
void visitConstantPattern(ConstantPattern node) {
var expression = node.expression;
if (expression is SimpleIdentifier) {
@@ -155,8 +244,9 @@
}
@override
- void visitRethrowExpression(RethrowExpression node) {
- _forExpression(node);
+ void visitReturnStatement(ReturnStatement node) {
+ collector.completionLocation = 'ReturnStatement_expression';
+ _forExpression(node.expression ?? node);
}
@override
@@ -169,21 +259,29 @@
}
@override
- void visitSimpleIdentifier(SimpleIdentifier node) {
- node.parent?.accept(this);
- }
-
- @override
void visitSimpleStringLiteral(SimpleStringLiteral node) {
_visitParentIfAtOrBeforeNode(node);
}
@override
+ void visitSpreadElement(SpreadElement node) {
+ collector.completionLocation = 'SpreadElement_expression';
+ _forExpression(node);
+ }
+
+ @override
void visitStringInterpolation(StringInterpolation node) {
_visitParentIfAtOrBeforeNode(node);
}
/// Add the suggestions that are appropriate when the selection is at the
+ /// beginning of a class member.
+ void _forClassMember() {
+ keywordHelper.addClassMemberKeywords();
+ // TODO(brianwilkerson) Suggest type names.
+ }
+
+ /// Add the suggestions that are appropriate when the selection is at the
/// beginning of an element in a collection [literal], with the given
/// [elements].
void _forCollectionElement(
diff --git a/pkg/analysis_server/lib/src/services/completion/dart/keyword_contributor.dart b/pkg/analysis_server/lib/src/services/completion/dart/keyword_contributor.dart
index e40283b..8add169 100644
--- a/pkg/analysis_server/lib/src/services/completion/dart/keyword_contributor.dart
+++ b/pkg/analysis_server/lib/src/services/completion/dart/keyword_contributor.dart
@@ -128,21 +128,6 @@
}
@override
- void visitAssignmentExpression(AssignmentExpression node) {
- _addExpressionKeywords(node);
- }
-
- @override
- void visitAwaitExpression(AwaitExpression node) {
- _addExpressionKeywords(node);
- }
-
- @override
- void visitBinaryExpression(BinaryExpression node) {
- _addExpressionKeywords(node);
- }
-
- @override
void visitBlock(Block node) {
var prevStmt = OpType.getPreviousStatement(node, entity);
if (prevStmt is TryStatement) {
@@ -201,70 +186,6 @@
}
@override
- void visitClassDeclaration(ClassDeclaration node) {
- final entity = this.entity;
- // Don't suggest class name
- if (entity == node.name) {
- return;
- }
- if (entity == node.classKeyword) {
- var previous = node.findPrevious(node.classKeyword);
- if (previous != null) {
- if (previous.keyword != Keyword.BASE &&
- previous.keyword != Keyword.FINAL &&
- previous.keyword != Keyword.INTERFACE &&
- previous.keyword != Keyword.MIXIN &&
- previous.keyword != Keyword.SEALED) {
- if (previous.keyword != Keyword.ABSTRACT) {
- if (request.featureSet.isEnabled(Feature.sealed_class)) {
- _addSuggestion(Keyword.SEALED);
- }
- } else {
- // abstract ^ class A {}
- if (request.featureSet.isEnabled(Feature.class_modifiers)) {
- _addSuggestions([
- Keyword.BASE,
- Keyword.FINAL,
- Keyword.INTERFACE,
- Keyword.MIXIN
- ]);
- }
- }
- }
- if (request.featureSet.isEnabled(Feature.class_modifiers) &&
- previous.keyword == Keyword.BASE) {
- // base ^ class A {}
- // abstract base ^ class A {}
- _addSuggestion(Keyword.MIXIN);
- }
- }
- } else if (entity == node.mixinKeyword) {
- var previous = node.findPrevious(node.mixinKeyword!);
- if (previous != null) {
- if (previous.keyword != Keyword.BASE) {
- // abstract ^ mixin class A {}
- if (request.featureSet.isEnabled(Feature.class_modifiers)) {
- _addSuggestion(Keyword.BASE);
- }
- }
- }
- } else if (entity == node.rightBracket) {
- _addClassBodyKeywords();
- } else if (entity is ClassMember) {
- _addClassBodyKeywords();
- var index = node.members.indexOf(entity);
- var previous = index > 0 ? node.members[index - 1] : null;
- if (previous is MethodDeclaration && previous.body.isEmpty) {
- _addSuggestion(Keyword.ASYNC);
- _addSuggestion2(ASYNC_STAR);
- _addSuggestion2(SYNC_STAR);
- }
- } else {
- _addClassDeclarationKeywords(node);
- }
- }
-
- @override
void visitCompilationUnit(CompilationUnit node) {
SyntacticEntity? previousMember;
for (var member in node.childEntities) {
@@ -911,13 +832,6 @@
}
@override
- void visitReturnStatement(ReturnStatement node) {
- if (entity == node.expression) {
- _addExpressionKeywords(node);
- }
- }
-
- @override
void visitSimpleFormalParameter(SimpleFormalParameter node) {
var entity = this.entity;
if (node.type == entity && entity is GenericFunctionType) {
@@ -939,16 +853,6 @@
}
@override
- void visitSpreadElement(SpreadElement node) {
- _addExpressionKeywords(node);
- }
-
- @override
- void visitSuperExpression(SuperExpression node) {
- _addExpressionKeywords(node);
- }
-
- @override
void visitSwitchCase(SwitchCase node) {
_addStatementKeywords(node);
}
diff --git a/pkg/analysis_server/lib/src/services/completion/dart/keyword_helper.dart b/pkg/analysis_server/lib/src/services/completion/dart/keyword_helper.dart
index aa7db85..94889a1 100644
--- a/pkg/analysis_server/lib/src/services/completion/dart/keyword_helper.dart
+++ b/pkg/analysis_server/lib/src/services/completion/dart/keyword_helper.dart
@@ -31,8 +31,10 @@
/// declaration between the name of the class and the body. The [node] is the
/// class declaration containing the selection point.
void addClassDeclarationKeywords(ClassDeclaration node) {
- // Very simplistic suggestion because analyzer will warn if the extends,
- // with, and implements keywords are out of order.
+ // We intentionally add all keywords, even when they would be out of order,
+ // in order to help users discover what keywords are available. If the
+ // keywords are in the wrong order a diagnostic (and fix) will help them get
+ // the keywords in the correct location.
if (node.extendsClause == null) {
addKeyword(Keyword.EXTENDS);
}
@@ -67,18 +69,6 @@
/// declaration before the `class` keyword. The [node] is the class
/// declaration containing the selection point.
void addClassModifiers(ClassDeclaration node) {
- // TODO(brianwilkerson) These next 10 lines duplicate
- // `addClassDeclarationKeywords`, and should probably be removed.
- // Very simplistic suggestion because analyzer will warn if
- // the extends / with / implements keywords are out of order
- if (node.extendsClause == null) {
- addKeyword(Keyword.EXTENDS);
- } else if (node.withClause == null) {
- addKeyword(Keyword.WITH);
- }
- if (node.implementsClause == null) {
- addKeyword(Keyword.IMPLEMENTS);
- }
if (featureSet.isEnabled(Feature.class_modifiers) &&
featureSet.isEnabled(Feature.sealed_class)) {
if (node.baseKeyword == null &&
@@ -96,7 +86,7 @@
addKeyword(Keyword.MIXIN);
}
}
- if (node.baseKeyword != null) {
+ if (node.baseKeyword != null && node.mixinKeyword == null) {
// base ^ class A {}
// abstract base ^ class A {}
addKeyword(Keyword.MIXIN);
@@ -178,8 +168,10 @@
/// declaration between the name of the enum and the body. The [node] is the
/// enum declaration containing the selection point.
void addEnumDeclarationKeywords(EnumDeclaration node) {
- // Very simplistic suggestion because analyzer will warn if the with and
- // implements keywords are out of order.
+ // We intentionally add all keywords, even when they would be out of order,
+ // in order to help users discover what keywords are available. If the
+ // keywords are in the wrong order a diagnostic (and fix) will help them get
+ // the keywords in the correct location.
if (node.withClause == null) {
addKeyword(Keyword.WITH);
}
@@ -207,11 +199,19 @@
/// beginning of an expression. The [node] provides context to determine which
/// keywords to include.
void addExpressionKeywords(AstNode? node) {
+ /// Return `true` if `const` should be suggested for the given [node].
+ bool constIsValid(AstNode? node) {
+ if (node is CollectionElement && node is! Expression) {
+ node = node.parent;
+ }
+ return node is Expression && !node.inConstantContext;
+ }
+
addKeyword(Keyword.FALSE);
addKeyword(Keyword.NULL);
addKeyword(Keyword.TRUE);
if (node != null) {
- if (node is Expression && !node.inConstantContext) {
+ if (constIsValid(node)) {
addKeyword(Keyword.CONST);
}
if (node.inClassMemberBody) {
@@ -314,8 +314,10 @@
/// declaration between the name of the mixin and the body. The [node] is the
/// mixin declaration containing the selection point.
void addMixinDeclarationKeywords(MixinDeclaration node) {
- // Very simplistic suggestion because analyzer will warn if the on and
- //implements clauses are out of order.
+ // We intentionally add all keywords, even when they would be out of order,
+ // in order to help users discover what keywords are available. If the
+ // keywords are in the wrong order a diagnostic (and fix) will help them get
+ // the keywords in the correct location.
if (node.onClause == null) {
addKeyword(Keyword.ON);
}
diff --git a/pkg/analysis_server/test/services/completion/dart/location/class_declaration_test.dart b/pkg/analysis_server/test/services/completion/dart/location/class_declaration_test.dart
index 205f142..17fed24 100644
--- a/pkg/analysis_server/test/services/completion/dart/location/class_declaration_test.dart
+++ b/pkg/analysis_server/test/services/completion/dart/location/class_declaration_test.dart
@@ -108,6 +108,8 @@
suggestions
extends
kind: keyword
+ with
+ kind: keyword
''');
}
@@ -124,12 +126,22 @@
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
''');
}
@@ -143,6 +155,8 @@
suggestions
extends
kind: keyword
+ with
+ kind: keyword
''');
}
@@ -165,6 +179,8 @@
kind: keyword
implements
kind: keyword
+ with
+ kind: keyword
''');
}
@@ -239,6 +255,8 @@
kind: keyword
implements
kind: keyword
+ with
+ kind: keyword
''');
}
}
diff --git a/pkg/analysis_server/test/services/completion/dart/location/compilation_unit_member_test.dart b/pkg/analysis_server/test/services/completion/dart/location/compilation_unit_member_test.dart
index 9a72553..749a746 100644
--- a/pkg/analysis_server/test/services/completion/dart/location/compilation_unit_member_test.dart
+++ b/pkg/analysis_server/test/services/completion/dart/location/compilation_unit_member_test.dart
@@ -813,6 +813,8 @@
''');
assertResponse(r'''
suggestions
+ base
+ kind: keyword
''');
}
diff --git a/pkg/analysis_server/test/services/completion/dart/location/object_pattern_test.dart b/pkg/analysis_server/test/services/completion/dart/location/object_pattern_test.dart
index ccce124..71332b7 100644
--- a/pkg/analysis_server/test/services/completion/dart/location/object_pattern_test.dart
+++ b/pkg/analysis_server/test/services/completion/dart/location/object_pattern_test.dart
@@ -118,10 +118,14 @@
kind: field
f11
kind: field
+ final
+ kind: keyword
g01
kind: getter
g11
kind: getter
+ var
+ kind: keyword
''');
}
}
@@ -216,10 +220,14 @@
kind: field
f11
kind: field
+ final
+ kind: keyword
g01
kind: getter
g11
kind: getter
+ var
+ kind: keyword
''');
}
}
