LSP requests must always be responded to, even if optional

Change-Id: Ieb3314028436a566c5fe4c2786c6b9196a03e26a
Reviewed-on: https://dart-review.googlesource.com/c/87800
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Danny Tuppeny <dantup@google.com>

pkg/analysis_server/lib/src/lsp/handlers/handler_states.dart

@@ -63,8 +63,7 @@ class InitializingStateMessageHandler extends ServerStateMessageHandler {
     return failure(
         ErrorCodes.ServerNotInitialized,
         'Unable to handle ${message.method} before the server is initialized '
-        'and the client has sent the initialized notification',
-        null);
+        'and the client has sent the initialized notification');
   }
 }
 
@@ -79,9 +78,7 @@ class UninitializedStateMessageHandler extends ServerStateMessageHandler {
     if (message is! RequestMessage) {
       return success();
     }
-    return failure(
-        ErrorCodes.ServerNotInitialized,
-        'Unable to handle ${message.method} before client has sent initialize request',
-        null);
+    return failure(ErrorCodes.ServerNotInitialized,
+        'Unable to handle ${message.method} before client has sent initialize request');
   }
 }

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

@@ -84,8 +84,8 @@ abstract class ServerStateMessageHandler {
     registerHandler(new ExitMessageHandler(server));
   }
 
-  ErrorOr<Object> failure<Object>(
-          ErrorCodes code, String message, Object data) =>
+  ErrorOr<Object> failure<Object>(ErrorCodes code, String message,
+          [Object data]) =>
       new ErrorOr<Object>.error(new ResponseError(code, message, data));
 
   /// Handle the given [message]. If the [message] is a [RequestMessage], then the
@@ -99,14 +99,13 @@ abstract class ServerStateMessageHandler {
   }
 
   FutureOr<ErrorOr<Object>> handleUnknownMessage(IncomingMessage message) {
-    // TODO(dantup): How should we handle unknown notifications that do
-    // *not* start with $/?
-    // https://github.com/Microsoft/language-server-protocol/issues/608
-    if (!_isOptionalRequest(message)) {
-      return failure(
-          ErrorCodes.MethodNotFound, 'Unknown method ${message.method}', null);
-    }
-    return success();
+    // If it's an optional *Notification* we can ignore it (return success).
+    // Otherwise respond with failure. Optional Requests must still be responded
+    // to so they don't leave open requests on the client.
+    return _isOptionalNotification(message)
+        ? success()
+        : failure(
+            ErrorCodes.MethodNotFound, 'Unknown method ${message.method}');
   }
 
   registerHandler(MessageHandler handler) {
@@ -124,9 +123,13 @@ abstract class ServerStateMessageHandler {
 
   ErrorOr<Object> success<Object>([Object t]) => new ErrorOr<Object>.success(t);
 
-  bool _isOptionalRequest(IncomingMessage message) {
-    // Messages that start with $/ are optional and can be silently ignored
-    // if we don't know how to handle them.
+  bool _isOptionalNotification(IncomingMessage message) {
+    // Not a notification.
+    if (message is! NotificationMessage) {
+      return false;
+    }
+
+    // Messages that start with $/ are optional.
     final stringValue = message.method.toJson();
     return stringValue is String && stringValue.startsWith(r'$/');
   }

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

@@ -288,7 +288,7 @@ class LspAnalysisServer extends AbstractAnalysisServer {
       // also ensure the error is logged to the client.
       logError(error.message);
     } else if (message is ResponseMessage) {
-      // For notifications where we couldn't respond with an error, send it as
+      // For bad response messages where we can't respond with an error, send it as
       // show instead of log.
       showError(error.message);
     } else {

pkg/analysis_server/test/lsp/server_test.dart

@@ -33,7 +33,23 @@ class ServerTest extends AbstractLspAnalysisServerTest {
     expect(response.result, isNull);
   }
 
-  test_unknownNotifications_silentlyDropped() async {
+  test_unknownNotifications_logError() async {
+    await initialize();
+
+    final notification =
+        makeNotification(new Method.fromJson(r'some/randomNotification'), null);
+
+    final notificationParams = await expectErrorNotification<ShowMessageParams>(
+      () => channel.sendNotificationToServer(notification),
+    );
+    expect(notificationParams, isNotNull);
+    expect(
+      notificationParams.message,
+      contains('Unknown method some/randomNotification'),
+    );
+  }
+
+  test_unknownOptionalNotifications_silentlyDropped() async {
     await initialize();
     final notification =
         makeNotification(new Method.fromJson(r'$/randomNotification'), null);
@@ -63,11 +79,13 @@ class ServerTest extends AbstractLspAnalysisServerTest {
     expect(response.result, isNull);
   }
 
-  @failingTest
-  test_unknownRequest_silentlyDropped /*??*/ () async {
-    // TODO(dantup): Fix this test up when we know how we're supposed to handle
-    // unknown $/ requests.
-    // https://github.com/Microsoft/language-server-protocol/issues/607
-    fail('TODO(dantup)');
+  test_unknownOptionalRequest_rejected() async {
+    await initialize();
+    final request = makeRequest(new Method.fromJson(r'$/randomRequest'), null);
+    final response = await channel.sendRequestToServer(request);
+    expect(response.id, equals(request.id));
+    expect(response.error, isNotNull);
+    expect(response.error.code, equals(ErrorCodes.MethodNotFound));
+    expect(response.result, isNull);
   }
 }