[analysis_server] Minor refactors to enable running EditArgument thro…

…ugh legacy server

This is some minor refactoring to support the next CL that will enable the EditArgument request (the one that actually edits arguments, not the one that gets the list of arguments) over legacy.

It moves some code for sending LSP reverse-requests through the legacy server from test code into the actual server (and fixes that they weren't correctly wrapped in the 'lsp.handle' protocol classes) and updates the tests to use the shared interface ready.

Change-Id: I06492138645538072fb12f2fc2d424e214f8055b
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/404824
Reviewed-by: Brian Wilkerson <[email protected]>
Reviewed-by: Konstantin Shcheglov <[email protected]>
Commit-Queue: Brian Wilkerson <[email protected]>

pkg/analysis_server/lib/src/legacy_analysis_server.dart

@@ -76,6 +76,7 @@ import 'package:analysis_server/src/handler/legacy/server_shutdown.dart';
 import 'package:analysis_server/src/handler/legacy/unsupported_request.dart';
 import 'package:analysis_server/src/lsp/client_capabilities.dart' as lsp;
 import 'package:analysis_server/src/lsp/client_configuration.dart' as lsp;
+import 'package:analysis_server/src/lsp/constants.dart' as lsp;
 import 'package:analysis_server/src/lsp/handlers/handler_states.dart';
 import 'package:analysis_server/src/operation/operation_analysis.dart';
 import 'package:analysis_server/src/plugin/notification_manager.dart';
@@ -716,6 +717,51 @@ class LegacyAnalysisServer extends AnalysisServer {
     );
   }
 
+  /// Sends an LSP request to the server (wrapped in 'lsp.handle') and unwraps
+  /// the LSP response from the result of the legacy response.
+  Future<lsp.ResponseMessage> sendLspRequest(
+    lsp.Method method,
+    Object params,
+  ) async {
+    var id = nextServerRequestId++;
+
+    // Build the complete LSP RequestMessage to send.
+    var lspMessage = lsp.RequestMessage(
+      id: lsp.Either2<int, String>.t1(id),
+      jsonrpc: lsp.jsonRpcVersion,
+      method: method,
+      params: params,
+    );
+
+    // Wrap the LSP message inside a call to lsp.handle.
+    var response = await sendRequest(
+      Request(
+        id.toString(),
+        LSP_REQUEST_HANDLE,
+        LspHandleParams(lspMessage).toJson(clientUriConverter: uriConverter),
+      ),
+    );
+
+    // Unwrap the LSP response from the legacy response.
+    var result = LspHandleResult.fromResponse(
+      response,
+      clientUriConverter: uriConverter,
+    );
+    var lspResponse = result.lspResponse;
+
+    return lspResponse is Map<String, Object?>
+        ? lsp.ResponseMessage.fromJson(lspResponse)
+        : lsp.ResponseMessage(
+          jsonrpc: lsp.jsonRpcVersion,
+          error: lsp.ResponseError(
+            code: lsp.ServerErrorCodes.UnhandledError,
+            message:
+                "The client responded to a '$method' LSP request but"
+                ' did not include a valid response in the lspResponse field',
+          ),
+        );
+  }
+
   /// Send the given [notification] to the client.
   void sendNotification(Notification notification) {
     channel.sendNotification(notification);

pkg/analysis_server/lib/src/lsp/handlers/commands/simple_edit_handler.dart

@@ -67,7 +67,7 @@ abstract class SimpleEditCommandHandler
   ) async {
     // Send the edit to the client via a applyEdit request (this is a request
     // from server -> client and the client will provide a response).
-    var editResponse = await server.sendRequest(
+    var editResponse = await server.sendLspRequest(
       Method.workspace_applyEdit,
       ApplyWorkspaceEditParams(label: commandName, edit: workspaceEdit),
     );

pkg/analysis_server/lib/src/lsp/handlers/custom/editable_arguments/handler_edit_argument.dart

@@ -295,7 +295,7 @@ class EditArgumentHandler extends SharedMessageHandler<EditArgumentParams, Null>
     }
 
     var editDescription = 'Edit argument';
-    var editResponse = await server.sendRequest(
+    var editResponse = await server.sendLspRequest(
       Method.workspace_applyEdit,
       ApplyWorkspaceEditParams(label: editDescription, edit: workspaceEdit),
     );

pkg/analysis_server/lib/src/lsp/lsp_analysis_server.dart

@@ -329,7 +329,7 @@ class LspAnalysisServer extends AnalysisServer {
       // Fetch all configuration we care about from the client. This is just
       // "dart" for now, but in future this may be extended to include
       // others (for example "flutter").
-      var response = await sendRequest(
+      var response = await sendLspRequest(
         Method.workspace_configuration,
         ConfigurationParams(
           items: [
@@ -864,7 +864,7 @@ class LspAnalysisServer extends AnalysisServer {
   /// Sends a request with the given [params] to the client and wait for a
   /// response. Completes with the raw [ResponseMessage] which could be an
   /// error response.
-  Future<ResponseMessage> sendRequest(Method method, Object params) {
+  Future<ResponseMessage> sendLspRequest(Method method, Object params) {
     var requestId = nextRequestId++;
     var completer = Completer<ResponseMessage>();
     completers[requestId] = completer;
@@ -1020,7 +1020,7 @@ class LspAnalysisServer extends AnalysisServer {
     List<MessageActionItem> actions,
   ) async {
     assert(supportsShowMessageRequest);
-    var response = await sendRequest(
+    var response = await sendLspRequest(
       Method.window_showMessageRequest,
       ShowMessageRequestParams(
         type: type.forLsp,

pkg/analysis_server/lib/src/lsp/progress.dart

@@ -63,7 +63,7 @@ class _ServerCreatedProgressReporter extends _TokenProgressReporter {
     // begin is sent (which could happen because create is async), end will
     // not be sent/return too early.
     _tokenBeginRequest = _server
-        .sendRequest(
+        .sendLspRequest(
           Method.window_workDoneProgress_create,
           WorkDoneProgressCreateParams(token: _token),
         )

pkg/analysis_server/lib/src/lsp/server_capabilities_computer.dart

@@ -305,7 +305,7 @@ class ServerCapabilitiesComputer {
       // we cannot re-enter this method until we have sent both the unregister
       // and register requests to the client atomically.
       // https://github.com/dart-lang/sdk/issues/47851#issuecomment-988093109
-      unregistrationRequest = _server.sendRequest(
+      unregistrationRequest = _server.sendLspRequest(
         Method.client_unregisterCapability,
         UnregistrationParams(unregisterations: unregistrations),
       );
@@ -316,7 +316,7 @@ class ServerCapabilitiesComputer {
     // otherwise we don't know that the client supports registerCapability).
     if (registrationsToAdd.isNotEmpty) {
       registrationRequest = _server
-          .sendRequest(
+          .sendLspRequest(
             Method.client_registerCapability,
             RegistrationParams(registrations: registrationsToAdd),
           )

pkg/analysis_server/lib/src/utilities/mocks.dart

@@ -18,16 +18,31 @@ import 'package:watcher/watcher.dart';
 class MockServerChannel implements ServerCommunicationChannel {
   /// A controller for the stream of requests and responses from the client to
   /// the server.
+  ///
+  /// Messages added to this stream should be converted to/from JSON to ensure
+  /// they are fully serialized/deserialized as they would be in a real server
+  /// otherwise tests may receive real instances where in reality they would be
+  /// maps.
   StreamController<RequestOrResponse> requestController =
       StreamController<RequestOrResponse>();
 
   /// A controller for the stream of requests and responses from the server to
   /// the client.
+  ///
+  /// Messages added to this stream should be converted to/from JSON to ensure
+  /// they are fully serialized/deserialized as they would be in a real server
+  /// otherwise tests may receive real instances where in reality they would be
+  /// maps.
   StreamController<RequestOrResponse> responseController =
       StreamController<RequestOrResponse>.broadcast();
 
   /// A controller for the stream of notifications from the server to the
   /// client.
+  ///
+  /// Unlike [requestController] and [responseController], notifications added
+  /// here are not round-tripped through JSON but instead have real class
+  /// instances as their `params`. This is because they are only used by tests
+  /// which will cast and/or switch on the types for convenience.
   StreamController<Notification> notificationController =
       StreamController<Notification>.broadcast(sync: true);
 
@@ -67,7 +82,10 @@ class MockServerChannel implements ServerCommunicationChannel {
     if (_closed) {
       return;
     }
+
     notificationsReceived.add(notification);
+    notificationController.add(notification);
+
     var errorCompleter = this.errorCompleter;
     if (errorCompleter != null && notification.event == 'server.error') {
       var params = notification.params!;
@@ -77,14 +95,17 @@ class MockServerChannel implements ServerCommunicationChannel {
         StackTrace.fromString(params['stackTrace'] as String),
       );
     }
-    // Wrap send notification in future to simulate websocket
-    // TODO(scheglov): ask Dan why and decide what to do
-    //    new Future(() => notificationController.add(notification));
-    notificationController.add(notification);
   }
 
   @override
   void sendRequest(Request request) {
+    // Round-trip via JSON to ensure all types are fully serialized as they
+    // would be in a real setup.
+    request =
+        Request.fromJson(
+          jsonDecode(jsonEncode(request.toJson())) as Map<String, Object?>,
+        )!;
+
     serverRequestsSent.add(request);
     responseController.add(request);
   }
@@ -95,9 +116,16 @@ class MockServerChannel implements ServerCommunicationChannel {
     if (_closed) {
       return;
     }
+
+    // Round-trip via JSON to ensure all types are fully serialized as they
+    // would be in a real setup.
+    response =
+        Response.fromJson(
+          jsonDecode(jsonEncode(response.toJson())) as Map<String, Object?>,
+        )!;
+
     responsesReceived.add(response);
-    // Wrap send response in future to simulate WebSocket.
-    Future(() => responseController.add(response));
+    responseController.add(response);
   }
 
   /// Send the given [request] to the server as if it had been sent from the
@@ -117,8 +145,7 @@ class MockServerChannel implements ServerCommunicationChannel {
           jsonDecode(jsonEncode(request)) as Map<String, Object?>,
         )!;
 
-    // Wrap send request in future to simulate WebSocket.
-    unawaited(Future(() => requestController.add(request)));
+    requestController.add(request);
     var response = await waitForResponse(request);
 
     // Round-trip via JSON to ensure all types are fully serialized as they
@@ -133,13 +160,20 @@ class MockServerChannel implements ServerCommunicationChannel {
 
   /// Send the given [response] to the server as if it had been sent from the
   /// client.
-  Future<void> simulateResponseFromClient(Response response) {
+  void simulateResponseFromClient(Response response) {
     // No further requests should be sent after the connection is closed.
     if (_closed) {
       throw Exception('simulateRequestFromClient after connection closed');
     }
-    // Wrap send request in future to simulate WebSocket.
-    return Future(() => requestController.add(response));
+
+    // Round-trip via JSON to ensure all types are fully serialized as they
+    // would be in a real setup.
+    response =
+        Response.fromJson(
+          jsonDecode(jsonEncode(response)) as Map<String, Object?>,
+        )!;
+
+    requestController.add(response);
   }
 
   /// Return a future that will complete when a response associated with the

pkg/analysis_server/test/domain_analysis_test.dart

@@ -1787,6 +1787,7 @@ analyzer:
     newFile(path, '<manifest/>');
 
     await setRoots(included: [workspaceRootPath], excluded: []);
+    await waitForTasksFinished();
 
     // No touch-screen.
     assertNotificationsText(r'''

pkg/analysis_server/test/domain_server_test.dart

@@ -92,7 +92,7 @@ class ServerDomainTest extends PubPackageAnalysisServerTest {
 
     // Simulate the response.
     var request = serverChannel.serverRequestsSent[0];
-    await serverChannel.simulateResponseFromClient(
+    serverChannel.simulateResponseFromClient(
       ServerOpenUrlRequestResult().toResponse(
         request.id,
         clientUriConverter: server.uriConverter,
@@ -383,7 +383,7 @@ class ServerDomainTest extends PubPackageAnalysisServerTest {
 
     // Simulate the response.
     var request = serverChannel.serverRequestsSent[0];
-    await serverChannel.simulateResponseFromClient(
+    serverChannel.simulateResponseFromClient(
       ServerShowMessageRequestResult(
         action: 'a',
       ).toResponse(request.id, clientUriConverter: server.uriConverter),
@@ -405,7 +405,7 @@ class ServerDomainTest extends PubPackageAnalysisServerTest {
 
     // Simulate the response.
     var request = serverChannel.serverRequestsSent[0];
-    await serverChannel.simulateResponseFromClient(
+    serverChannel.simulateResponseFromClient(
       ServerShowMessageRequestResult().toResponse(
         request.id,
         clientUriConverter: server.uriConverter,

pkg/analysis_server/test/integration/server/message_scheduler_test.dart

@@ -57,6 +57,7 @@ class LegacyServerMessageSchedulerTest extends PubPackageAnalysisServerTest {
 
   Future<void> test_initialize() async {
     await setRoots(included: [workspaceRootPath], excluded: []);
+    await waitForTasksFinished();
     _assertLogContents(messageScheduler, r'''
 Incoming LegacyMessage: analysis.setAnalysisRoots
 Entering process messages loop
@@ -74,6 +75,7 @@ Exit process messages loop
     ).toRequest('0', clientUriConverter: server.uriConverter);
     futures.add(handleSuccessfulRequest(request));
     await Future.wait(futures);
+    await waitForTasksFinished();
     _assertLogContents(messageScheduler, r'''
 Incoming LegacyMessage: analysis.setAnalysisRoots
 Entering process messages loop

pkg/analysis_server/test/lsp/edit_argument_test.dart

@@ -218,7 +218,7 @@ class ComputeStringValueTest {
 }
 
 @reflectiveTest
-class EditArgumentTest extends AbstractLspAnalysisServerTest {
+class EditArgumentTest extends SharedAbstractLspAnalysisServerTest {
   late TestCode code;
 
   @override
@@ -848,14 +848,14 @@ const myConst = E.one;
     bool open = true,
   }) async {
     code = TestCode.parse(content);
-    newFile(mainFilePath, code.code);
-    await initialize();
+    createFile(testFilePath, code.code);
+    await initializeServer();
     if (open) {
-      await openFile(mainFileUri, code.code);
+      await openFile(testFileUri, code.code);
     }
-    await initialAnalysis;
+    await currentAnalysis;
     var verifier = await executeForEdits(
-      () => editArgument(mainFileUri, code.position.position, edit),
+      () => editArgument(testFileUri, code.position.position, edit),
     );
 
     verifier.verifyFiles(expectedContent);
@@ -885,12 +885,12 @@ class MyWidget extends StatelessWidget {
 ''';
 
     code = TestCode.parse(content);
-    newFile(mainFilePath, code.code);
-    await initialize();
-    await initialAnalysis;
+    createFile(testFilePath, code.code);
+    await initializeServer();
+    await currentAnalysis;
 
     await expectLater(
-      editArgument(mainFileUri, code.position.position, edit),
+      editArgument(testFileUri, code.position.position, edit),
       throwsA(isResponseError(ErrorCodes.RequestFailed, message: message)),
     );
   }
@@ -917,7 +917,7 @@ class MyWidget extends StatelessWidget {
 }
 ''';
     var expectedContent = '''
->>>>>>>>>> lib/main.dart
+>>>>>>>>>> lib/test.dart
 import 'package:flutter/widgets.dart';
 
 $additionalCode

pkg/analysis_server/test/lsp/server_abstract.dart

@@ -249,8 +249,8 @@ abstract class AbstractLspAnalysisServerTest
     _previousContextBuilds = server.contextBuilds;
   }
 
-  Future<ResponseMessage> sendLspRequestToClient(Method method, Object params) {
-    return server.sendRequest(method, params);
+  Future<ResponseMessage> sendLspRequest(Method method, Object params) {
+    return server.sendLspRequest(method, params);
   }
 
   @override
@@ -1707,5 +1707,6 @@ abstract class SharedAbstractLspAnalysisServerTest
   @override
   Future<void> initializeServer() async {
     await initialize();
+    await currentAnalysis;
   }
 }

pkg/analysis_server/test/lsp/workspace_apply_edit_test.dart

@@ -19,12 +19,6 @@ class WorkspaceApplyEditTest extends SharedAbstractLspAnalysisServerTest
         // Tests are defined in SharedWorkspaceApplyEditTests because they
         // are shared and run for both LSP and Legacy servers.
         SharedWorkspaceApplyEditTests {
-  @override
-  Future<void> initializeServer() async {
-    await initialize();
-    await currentAnalysis;
-  }
-
   @override
   Future<void> setUp() async {
     super.setUp();