[analysis_server] Support String IDs in log replay + run some LSP integration tests with string IDs

My previous CL made some LSP tests run with String IDs instead of ints, but those tests don't appear to go through the session logger so did not fail with the casts here.

This change adds a base integration test that also uses string IDs, which did fail on the cast, so I've updated the session logger to use `Either2<int, String>` for IDs instead.

Fixes https://github.com/dart-lang/sdk/issues/62442

Change-Id: Iee582e9ce2b8b5a1127120c987670a679d2ca76c
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/473260
Commit-Queue: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Jake Macdonald <jakemac@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>

pkg/analysis_server/integration_test/lsp_server/initialization_test.dart

@@ -3,19 +3,36 @@
 // BSD-style license that can be found in the LICENSE file.
 
 import 'package:analysis_server/lsp_protocol/protocol.dart';
+import 'package:language_server_protocol/json_parsing.dart';
 import 'package:test/test.dart';
 import 'package:test_reflective_loader/test_reflective_loader.dart';
 
+import '../../test/lsp/request_helpers_mixin.dart';
 import 'integration_tests.dart';
 
 void main() {
   defineReflectiveSuite(() {
     defineReflectiveTests(InitializationTest);
+    defineReflectiveTests(StringIdInitializationTest);
   });
 }
 
 @reflectiveTest
 class InitializationTest extends AbstractLspAnalysisServerIntegrationTest {
+  Future<void> test_initialize() async {
+    var response = await initialize();
+    expect(
+      InitializeResult.canParse(response.result, nullLspJsonReporter),
+      isTrue,
+    );
+    var result = InitializeResult.fromJson(
+      response.result as Map<String, Object?>,
+    );
+    // Check we have some expected fields.
+    expect(result.capabilities.textDocumentSync, isNotNull);
+    expect(result.capabilities.codeActionProvider, isNotNull);
+  }
+
   Future<void> test_initialize_invalidParams() async {
     var params = {'processId': 'invalid'};
     var request = RequestMessage(
@@ -30,3 +47,11 @@ class InitializationTest extends AbstractLspAnalysisServerIntegrationTest {
     expect(response.result, isNull);
   }
 }
+
+/// Runs all initialization tests using String IDs instead of integers (both
+/// are valid).
+@reflectiveTest
+class StringIdInitializationTest extends InitializationTest {
+  @override
+  LspMessageIdMode get idMode => LspMessageIdMode.string;
+}

pkg/analysis_server/integration_test/lsp_server/integration_tests.dart

@@ -32,7 +32,7 @@ abstract class AbstractLspAnalysisServerIntegrationTest
   final List<String> vmArgs = [];
   LspServerClient? client;
   InstrumentationService? instrumentationService;
-  final Map<num, Completer<ResponseMessage>> _completers = {};
+  final Map<Either2<int, String>, Completer<ResponseMessage>> _completers = {};
   String dartSdkPath = path.dirname(path.dirname(Platform.resolvedExecutable));
 
   @override
@@ -121,10 +121,7 @@ abstract class AbstractLspAnalysisServerIntegrationTest
   @override
   Future<ResponseMessage> sendRequestToServer(RequestMessage request) {
     var completer = Completer<ResponseMessage>();
-    var id = request.id.map(
-      (number) => number,
-      (string) => throw 'String IDs not supported in tests',
-    );
+    var id = request.id;
     _completers[id] = completer;
 
     channel.sendRequest(request);
@@ -153,11 +150,7 @@ abstract class AbstractLspAnalysisServerIntegrationTest
     await client.start(dartSdkPath: dartSdkPath, vmArgs: vmArgs);
     client.serverToClient.listen((message) {
       if (message is ResponseMessage) {
-        var id = message.id!.map(
-          (number) => number,
-          (string) => throw 'String IDs not supported in tests',
-        );
-
+        var id = message.id;
         var completer = _completers[id];
         if (completer == null) {
           throw 'Response with ID $id was unexpected';

pkg/analysis_server/lib/src/session_logger/log_entry.dart

@@ -5,6 +5,7 @@
 import 'package:analysis_server/src/session_logger/entry_keys.dart' as key;
 import 'package:analysis_server/src/session_logger/entry_kind.dart';
 import 'package:analysis_server/src/session_logger/process_id.dart';
+import 'package:language_server_protocol/protocol_special.dart' show Either2;
 
 /// A representation of an entry in a [Log].
 ///
@@ -39,7 +40,18 @@ extension type LogEntry(JsonMap map) {
 extension type Message(JsonMap map) {
   /// The ID of the message. All request messages have IDs, but notifications
   /// do not.
-  int? get id => map['id'] as int?;
+  Either2<int, String>? get id {
+    // The id in the JSON could be either an int or String (LSP is JSON-RPC 2
+    // which allows either).
+    return switch (map['id']) {
+      int i => Either2<int, String>.t1(i),
+      String s => Either2<int, String>.t2(s),
+      null => null,
+      _ => throw Exception(
+        'Message ID was unexpected type ${map['id'].runtimeType}',
+      ),
+    };
+  }
 
   /// Whether this message is a notification that a file has changed.
   bool get isDidChange => method == 'textDocument/didChange';
@@ -107,5 +119,6 @@ extension type Message(JsonMap map) {
       (params?['textDocument'] as Map<String, Object?>?)?['uri'] as String?;
 
   /// Whether this message is a response to the request with the [requestId].
-  bool isResponseTo(int requestId) => isResponse && id == requestId;
+  bool isResponseTo(Either2<int, String> requestId) =>
+      isResponse && id == requestId;
 }

pkg/analysis_server/lib/src/session_logger/session_logger_sink.dart

@@ -7,6 +7,7 @@ import 'dart:io';
 
 import 'package:analysis_server/src/session_logger/log_entry.dart';
 import 'package:analysis_server/src/session_logger/process_id.dart';
+import 'package:language_server_protocol/protocol_special.dart' show Either2;
 
 /// A sink for a session logger that will write entries to a file.
 class SessionLoggerFileSink extends SessionLoggerSink {
@@ -148,12 +149,11 @@ class SessionLoggerInMemorySink extends SessionLoggerSink {
   ///
   /// This assumes that the entries are all from the same process. If that isn't
   /// true, then the returned list may contain duplicate ids.
-  List<int> _getRequestIds(List<LogEntry> entries) {
+  List<Either2<int, String>> _getRequestIds(List<LogEntry> entries) {
     return entries
         .where((entry) => entry.isMessage)
         .map((entry) => entry.message.id)
-        .where((id) => id != null)
-        .cast<int>()
+        .nonNulls
         .toList();
   }
 

pkg/analysis_server/tool/log_player/log_player.dart

@@ -12,6 +12,7 @@ import 'package:analysis_server/src/session_logger/log_entry.dart';
 import 'package:analysis_server/src/session_logger/process_id.dart';
 import 'package:cli_util/cli_logging.dart';
 import 'package:collection/collection.dart';
+import 'package:language_server_protocol/protocol_special.dart' show Either2;
 
 import 'log.dart';
 import 'message_equality.dart';
@@ -20,7 +21,7 @@ import 'server_driver.dart';
 /// Some messages from the analysis server should just be ignored.
 bool _shouldSkip(Message message) =>
     // This is the response to the initialize request.
-    message.id == 0 ||
+    (message.id?.valueEquals(0) ?? false) ||
     // Notifications, we can skip these.
     message.id == null ||
     // These are unpredictable and noisy, we can silently ignore them for now.
@@ -63,10 +64,10 @@ class LogPlayer {
     var extraServerMessages = <Message>[];
     // Maps the recorded message IDs for messages initiated by the analysis
     // server to the actual message IDs observed for this run.
-    var actualServerMessageIds = <int, int>{};
+    var actualServerMessageIds = <Either2<int, String>, Either2<int, String>>{};
     // Original recorded ids for work progress notifications. We will skip the
     // responses to these and not expect the requests as they are unreliable.
-    var workProgressIds = <int>{};
+    var workProgressIds = <Either2<int, String>>{};
     try {
       while (nextIndex < entries.length) {
         try {
@@ -192,7 +193,7 @@ class LogPlayer {
     Message message,
     ServerDriver? server,
     List<Message> pendingServerMessageExpectations,
-    Map<int, int> actualServerMessageIds,
+    Map<Either2<int, String>, Either2<int, String>> actualServerMessageIds,
     List<Message> extraServerMessages,
   ) {
     var isServerInitiatedRequest = message.method != null;
@@ -339,5 +340,8 @@ extension MessageExtension on Message {
       _messageEquality.equals(this, other, skipMatchId: skipMatchId);
 
   // Can't be a setter https://github.com/dart-lang/language/issues/4334
-  void setId(int newId) => map['id'] = newId;
+  void setId(Either2<int, String> newId) =>
+      // We always store the underlying value in the map, not the Either2,
+      // because that matches what a real JSON map would have.
+      map['id'] = newId.map((i) => i, (s) => s);
 }

third_party/pkg/language_server_protocol/lib/protocol_special.dart

@@ -67,6 +67,7 @@ class Either2<T1, T2> implements ToJsonable {
   @override
   bool operator ==(other) =>
       other is Either2<T1, T2> &&
+      other._which == _which &&
       lspEquals(other._t1, _t1) &&
       lspEquals(other._t2, _t2);
 
@@ -111,6 +112,7 @@ class Either3<T1, T2, T3> implements ToJsonable {
   @override
   bool operator ==(other) =>
       other is Either3<T1, T2, T3> &&
+      other._which == _which &&
       lspEquals(other._t1, _t1) &&
       lspEquals(other._t2, _t2) &&
       lspEquals(other._t3, _t3);
@@ -176,6 +178,7 @@ class Either4<T1, T2, T3, T4> implements ToJsonable {
   @override
   bool operator ==(other) =>
       other is Either4<T1, T2, T3, T4> &&
+      other._which == _which &&
       lspEquals(other._t1, _t1) &&
       lspEquals(other._t2, _t2) &&
       lspEquals(other._t3, _t3) &&