[analysis_server] [lsp] Fix resolving completions with no insertText and stop using insertText entirely
Fixes https://github.com/Dart-Code/Dart-Code/issues/4027.
Change-Id: I60d4e9aabdd125771cd8f3e9f63367cf21eea5fa
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/249489
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Brian Wilkerson <brianwilkerson@google.com>
diff --git a/pkg/analysis_server/lib/src/lsp/handlers/handler_completion_resolve.dart b/pkg/analysis_server/lib/src/lsp/handlers/handler_completion_resolve.dart
index ccea475..07ab252 100644
--- a/pkg/analysis_server/lib/src/lsp/handlers/handler_completion_resolve.dart
+++ b/pkg/analysis_server/lib/src/lsp/handlers/handler_completion_resolve.dart
@@ -85,7 +85,9 @@
return cancelled();
}
- var newInsertText = item.insertText ?? item.label;
+ var newInsertText = item.textEdit
+ ?.map((edit) => edit.newText, (edit) => edit.newText) ??
+ item.label;
final builder = ChangeBuilder(session: session);
await builder.addDartFileEdit(file, (builder) {
final result = builder.importLibraryElement(libraryUri);
@@ -151,7 +153,6 @@
preselect: item.preselect,
sortText: item.sortText,
filterText: item.filterText,
- insertText: newInsertText,
insertTextFormat: item.insertTextFormat,
insertTextMode: item.insertTextMode,
textEdit: supportsInsertReplace && insertionRange != replacementRange
@@ -262,7 +263,6 @@
preselect: item.preselect,
sortText: item.sortText,
filterText: item.filterText,
- insertText: item.insertText,
insertTextFormat: item.insertTextFormat,
textEdit: item.textEdit,
additionalTextEdits: item.additionalTextEdits,
diff --git a/pkg/analysis_server/lib/src/lsp/mapping.dart b/pkg/analysis_server/lib/src/lsp/mapping.dart
index 518bf16..77a4353 100644
--- a/pkg/analysis_server/lib/src/lsp/mapping.dart
+++ b/pkg/analysis_server/lib/src/lsp/mapping.dart
@@ -360,9 +360,6 @@
filterText: completion != label
? completion
: null, // filterText uses label if not set
- insertText: insertText != label
- ? insertText
- : null, // insertText uses label if not set
insertTextFormat: insertTextFormat != lsp.InsertTextFormat.PlainText
? insertTextFormat
: null, // Defaults to PlainText if not supplied
@@ -1124,8 +1121,8 @@
LspClientCapabilities capabilities,
server.LineInfo lineInfo,
server.CompletionSuggestion suggestion, {
- Range? replacementRange,
- Range? insertionRange,
+ required Range replacementRange,
+ required Range insertionRange,
required bool includeCommitCharacters,
required bool completeFunctionCalls,
CompletionItemResolutionInfo? resolutionData,
@@ -1234,31 +1231,26 @@
filterText: filterText != label
? filterText
: null, // filterText uses label if not set
- insertText: insertText != label
- ? insertText
- : null, // insertText uses label if not set
insertTextFormat: insertTextFormat != lsp.InsertTextFormat.PlainText
? insertTextFormat
: null, // Defaults to PlainText if not supplied
insertTextMode: supportsAsIsInsertMode && isMultilineCompletion
? InsertTextMode.asIs
: null,
- textEdit: (insertionRange == null || replacementRange == null)
- ? null
- : supportsInsertReplace && insertionRange != replacementRange
- ? Either2<InsertReplaceEdit, TextEdit>.t1(
- InsertReplaceEdit(
- insert: insertionRange,
- replace: replacementRange,
- newText: insertText,
- ),
- )
- : Either2<InsertReplaceEdit, TextEdit>.t2(
- TextEdit(
- range: replacementRange,
- newText: insertText,
- ),
- ),
+ textEdit: supportsInsertReplace && insertionRange != replacementRange
+ ? Either2<InsertReplaceEdit, TextEdit>.t1(
+ InsertReplaceEdit(
+ insert: insertionRange,
+ replace: replacementRange,
+ newText: insertText,
+ ),
+ )
+ : Either2<InsertReplaceEdit, TextEdit>.t2(
+ TextEdit(
+ range: replacementRange,
+ newText: insertText,
+ ),
+ ),
);
}
diff --git a/pkg/analysis_server/test/lsp/completion_dart_test.dart b/pkg/analysis_server/test/lsp/completion_dart_test.dart
index f55c2bd..9010d16 100644
--- a/pkg/analysis_server/test/lsp/completion_dart_test.dart
+++ b/pkg/analysis_server/test/lsp/completion_dart_test.dart
@@ -32,7 +32,7 @@
with CompletionTestMixin {
Future<void> checkCompleteFunctionCallInsertText(
String content, String completion,
- {required String? insertText, InsertTextFormat? insertTextFormat}) async {
+ {required String? editText, InsertTextFormat? insertTextFormat}) async {
await provideConfig(
() => initialize(
textDocumentCapabilities: withCompletionItemSnippetSupport(
@@ -51,12 +51,13 @@
);
expect(item.insertTextFormat, equals(insertTextFormat));
- expect(item.insertText, equals(insertText));
+ // We always expect `insertText` to be `null` now, as we always use
+ // `textEdit`.
+ expect(item.insertText, isNull);
+ // And the expected text should be in the `textEdit`.
final textEdit = toTextEdit(item.textEdit!);
- // newText in the edit will always be set, so if insertText is null we need
- // fall back to item.label for the expected value.
- expect(textEdit.newText, equals(item.insertText ?? item.label));
+ expect(textEdit.newText, equals(editText));
expect(textEdit.range, equals(rangeFromMarkers(content)));
}
@@ -188,7 +189,7 @@
''',
'Aaaaa(…)',
insertTextFormat: InsertTextFormat.Snippet,
- insertText: r'Aaaaa(${0:a})',
+ editText: r'Aaaaa(${0:a})',
);
Future<void> test_completeFunctionCalls_escapesDollarArgs() =>
@@ -201,7 +202,7 @@
'myFunction(…)',
insertTextFormat: InsertTextFormat.Snippet,
// The dollar should have been escaped.
- insertText: r'myFunction(${1:a\$a}, ${2:b})',
+ editText: r'myFunction(${1:a\$a}, ${2:b})',
);
Future<void> test_completeFunctionCalls_escapesDollarName() =>
@@ -214,7 +215,7 @@
r'myFunc$tion(…)',
insertTextFormat: InsertTextFormat.Snippet,
// The dollar should have been escaped.
- insertText: r'myFunc\$tion(${1:a}, ${2:b})',
+ editText: r'myFunc\$tion(${1:a}, ${2:b})',
);
Future<void> test_completeFunctionCalls_existingArgList_constructor() =>
@@ -228,7 +229,7 @@
}
''',
'Aaaaa(…)',
- insertText: 'Aaaaa',
+ editText: 'Aaaaa',
);
Future<void> test_completeFunctionCalls_existingArgList_expression() =>
@@ -239,7 +240,7 @@
}
''',
'myFunction(…)',
- insertText: 'myFunction',
+ editText: 'myFunction',
);
Future<void> test_completeFunctionCalls_existingArgList_member_noPrefix() =>
@@ -254,7 +255,7 @@
}
''',
'foo(…)',
- insertText: 'foo',
+ editText: 'foo',
);
Future<void> test_completeFunctionCalls_existingArgList_namedConstructor() =>
@@ -268,7 +269,7 @@
}
''',
'foo(…)',
- insertText: 'foo',
+ editText: 'foo',
);
Future<void> test_completeFunctionCalls_existingArgList_statement() =>
@@ -279,7 +280,7 @@
}
''',
'f(…)',
- insertText: 'f',
+ editText: 'f',
);
Future<void> test_completeFunctionCalls_existingArgList_suggestionSets() =>
@@ -290,7 +291,7 @@
}
''',
'print(…)',
- insertText: 'print',
+ editText: 'print',
);
Future<void> test_completeFunctionCalls_existingPartialArgList() =>
@@ -304,7 +305,7 @@
}
''',
'Aaaaa(…)',
- insertText: 'Aaaaa',
+ editText: 'Aaaaa',
);
Future<void> test_completeFunctionCalls_expression() =>
@@ -316,7 +317,7 @@
''',
'myFunction(…)',
insertTextFormat: InsertTextFormat.Snippet,
- insertText: r'myFunction(${1:a}, ${2:b})',
+ editText: r'myFunction(${1:a}, ${2:b})',
);
Future<void> test_completeFunctionCalls_flutterSetState() async {
@@ -360,9 +361,9 @@
// Ensure the snippet comes through in the expected format with the expected
// placeholders.
expect(item.insertTextFormat, equals(InsertTextFormat.Snippet));
- expect(item.insertText, equals('setState(() {\n \$0\n });'));
+ expect(item.insertText, isNull);
final textEdit = toTextEdit(item.textEdit!);
- expect(textEdit.newText, equals(item.insertText));
+ expect(textEdit.newText, 'setState(() {\n \$0\n });');
expect(textEdit.range, equals(rangeFromMarkers(content)));
}
@@ -378,7 +379,7 @@
''',
'foo(…)',
insertTextFormat: InsertTextFormat.Snippet,
- insertText: r'foo(${0:a})',
+ editText: r'foo(${0:a})',
);
Future<void> test_completeFunctionCalls_noParameters() async {
@@ -393,10 +394,7 @@
await checkCompleteFunctionCallInsertText(
content,
'myFunction()',
- // With no params, we don't put a tab stop inside the parens. This results
- // in the insertText being the same as the label, which means it will be
- // set to null so that it falls back without needing to repeat the value.
- insertText: null,
+ editText: 'myFunction()',
insertTextFormat: InsertTextFormat.Snippet,
);
}
@@ -414,7 +412,7 @@
content,
'myFunction(…)',
// With optional params, there should still be parens/tab stop inside.
- insertText: r'myFunction($0)',
+ editText: r'myFunction($0)',
insertTextFormat: InsertTextFormat.Snippet,
);
}
@@ -443,9 +441,9 @@
// Ensure the snippet comes through in the expected format with the expected
// placeholders.
expect(item.insertTextFormat, equals(InsertTextFormat.Snippet));
- expect(item.insertText, equals(r'myFunction(${1:a}, ${2:b}, c: ${3:c})'));
+ expect(item.insertText, isNull);
final textEdit = toTextEdit(item.textEdit!);
- expect(textEdit.newText, equals(item.insertText));
+ expect(textEdit.newText, r'myFunction(${1:a}, ${2:b}, c: ${3:c})');
expect(textEdit.range, equals(rangeFromMarkers(content)));
}
@@ -479,19 +477,67 @@
// Ensure the snippet comes through in the expected format with the expected
// placeholders.
expect(item.insertTextFormat, equals(InsertTextFormat.Snippet));
- expect(item.insertText, equals(r'myFunction(${1:a}, ${2:b}, c: ${3:c})'));
+ expect(item.insertText, isNull);
expect(item.textEdit, isNotNull);
final originalTextEdit = item.textEdit;
// Ensure the item can be resolved and retains the correct textEdit (since
// textEdit may be recomputed during resolve).
final resolved = await resolveCompletion(item);
+ expect(resolved.insertText, isNull);
expect(resolved.textEdit, originalTextEdit);
final textEdit = toTextEdit(resolved.textEdit!);
- expect(textEdit.newText, equals(item.insertText));
+ expect(textEdit.newText, r'myFunction(${1:a}, ${2:b}, c: ${3:c})');
expect(textEdit.range, equals(rangeFromMarkers(content)));
}
+ Future<void>
+ test_completeFunctionCalls_resolve_producesCorrectEditWithoutInsertText() async {
+ // Ensure our `resolve` call does not rely on the presence of `insertText`
+ // to compute the correct edits. This is something we did incorrectly in the
+ // past and broke with
+ // https://github.com/dart-lang/sdk/commit/40e25ebad0bd008615b1c1d8021cb27839f00dcd
+ // because the way these are combined in the VS Code LSP client means we are
+ // not provided both `insertText` and `textEdit` back in the resolve call.
+ //
+ // Now, we never supply `insertText` and always use `textEdit`.
+ final content = '''
+final a = Stri^
+ ''';
+
+ /// Helper to verify a completion is as expected.
+ void expectCorrectCompletion(CompletionItem item) {
+ // Ensure this completion looks as we'd expect.
+ expect(item.label, 'String.fromCharCode(…)');
+ expect(item.insertText, isNull);
+ expect(
+ item.textEdit!.map((edit) => edit.newText, (edit) => edit.newText),
+ r'String.fromCharCode(${0:charCode})',
+ );
+ }
+
+ final initialAnalysis = waitForAnalysisComplete();
+ await provideConfig(
+ () => initialize(
+ textDocumentCapabilities: withCompletionItemSnippetSupport(
+ emptyTextDocumentClientCapabilities),
+ workspaceCapabilities:
+ withConfigurationSupport(emptyWorkspaceClientCapabilities),
+ ),
+ {'completeFunctionCalls': true},
+ );
+ await openFile(mainFileUri, withoutMarkers(content));
+ await initialAnalysis;
+ final res = await getCompletion(mainFileUri, positionFromMarker(content));
+
+ final completion =
+ res.singleWhere((c) => c.label == 'String.fromCharCode(…)');
+ expectCorrectCompletion(completion);
+
+ final resolved = await resolveCompletion(completion);
+ expectCorrectCompletion(resolved);
+ }
+
Future<void> test_completeFunctionCalls_show() async {
final content = '''
import 'dart:math' show mi^
@@ -512,9 +558,9 @@
// The insert text should be a simple string with no parens/args and
// no need for snippets.
expect(item.insertTextFormat, isNull);
- expect(item.insertText, equals(r'min'));
+ expect(item.insertText, isNull);
final textEdit = toTextEdit(item.textEdit!);
- expect(textEdit.newText, equals(item.insertText));
+ expect(textEdit.newText, r'min');
}
Future<void> test_completeFunctionCalls_statement() =>
@@ -526,7 +572,7 @@
''',
'f(…)',
insertTextFormat: InsertTextFormat.Snippet,
- insertText: r'f(${0:a})',
+ editText: r'f(${0:a})',
);
Future<void> test_completeFunctionCalls_suggestionSets() =>
@@ -538,7 +584,7 @@
''',
'print(…)',
insertTextFormat: InsertTextFormat.Snippet,
- insertText: r'print(${0:object})',
+ editText: r'print(${0:object})',
);
Future<void> test_completionKinds_default() async {
@@ -726,7 +772,9 @@
expect(item, isNotNull);
expect(item!.label, equals('name => …'));
expect(item.filterText, isNull); // Falls back to label
- expect(item.insertText, equals('''@override
+ expect(item.insertText, isNull);
+ final textEdit = toTextEdit(item.textEdit!);
+ expect(textEdit.newText, equals('''@override
// TODO: implement name
String get name => throw UnimplementedError();'''));
}
@@ -936,7 +984,9 @@
final item = res.singleWhere((c) => c.label.startsWith('setState'));
// Multiline completions should always set insertTextMode.asIs.
- expect(item.insertText, contains('\n'));
+ expect(item.insertText, isNull);
+ final textEdit = toTextEdit(item.textEdit!);
+ expect(textEdit.newText, contains('\n'));
expect(item.insertTextMode, equals(InsertTextMode.asIs));
}
@@ -956,7 +1006,9 @@
// Single line completions should never set insertTextMode.asIs to
// avoid bloating payload size where it wouldn't matter.
- expect(item.insertText, isNot(contains('\n')));
+ expect(item.insertText, isNull);
+ final textEdit = toTextEdit(item.textEdit!);
+ expect(textEdit.newText, isNot(contains('\n')));
expect(item.insertTextMode, isNull);
}
@@ -1386,7 +1438,9 @@
final item = res.singleWhere((c) => c.label == 'one: ');
expect(item.insertTextFormat,
anyOf(equals(InsertTextFormat.PlainText), isNull));
- expect(item.insertText, anyOf(equals('test'), isNull));
+ expect(item.insertText, isNull);
+ final textEdit = toTextEdit(item.textEdit!);
+ expect(textEdit.newText, item.label);
final updated = applyTextEdits(
withoutMarkers(content),
[toTextEdit(item.textEdit!)],
@@ -1444,7 +1498,7 @@
// Ensure the snippet comes through in the expected format with the expected
// placeholder.
expect(item.insertTextFormat, equals(InsertTextFormat.Snippet));
- expect(item.insertText, equals(r'one: $0,'));
+ expect(item.insertText, isNull);
final textEdit = toTextEdit(item.textEdit!);
expect(textEdit.newText, equals(r'one: $0,'));
expect(
@@ -1483,7 +1537,7 @@
expect(completion.detail, '(int? a, [int b = 1]) → String?');
}
- Future<void> test_parensNotInFilterTextInsertText() async {
+ Future<void> test_parensNotInFilterTextOrEditText() async {
final content = '''
class MyClass {}
@@ -1497,8 +1551,10 @@
final res = await getCompletion(mainFileUri, positionFromMarker(content));
expect(res.any((c) => c.label == 'MyClass()'), isTrue);
final item = res.singleWhere((c) => c.label == 'MyClass()');
- expect(item.filterText, equals('MyClass'));
- expect(item.insertText, equals('MyClass'));
+ expect(item.filterText, 'MyClass');
+ expect(item.insertText, isNull);
+ final textEdit = toTextEdit(item.textEdit!);
+ expect(textEdit.newText, 'MyClass');
}
Future<void> test_plainText() async {