[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 {