[analysis_server] Handle unexpected types in LSP experimental client capabilities LSP gracefully

DanTup · Commit Queue · commit 1ea71b4b9b57 · 2024-06-13T17:29:49.000Z
Previously if you sent values in experimental capabilities that are different types to what we'd expect, in some cases we'd throw an exception and fail to initialize the server. Since the experimental section is not specified (it's specifically for off-spec things) the behaviour of sending values should (IMO) be considered server-specific. However, reporting a useful message and continuing to function is clearly better than reporting a cryptic error and failing to initialize. At least one client (`kak-lsp`) appears to be sending values in `"commands"` intended for the Rust LSP server which uses a different format to we're using (a nested Map where we use a List). This change will prevent the failure for them and instead report a warning. My recommendation is that their client is also updated to not send Rust experimental capabilities to non-Rust servers. Fixes #55935 Change-Id: I8cba1aa3a5beebf884ff247180f809ffd33b3111 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/371400 Reviewed-by: Brian Wilkerson <brianwilkerson@google.com> Reviewed-by: Sam Rawlins <srawlins@google.com> Commit-Queue: Brian Wilkerson <brianwilkerson@google.com>
diff --git a/pkg/analysis_server/lib/src/lsp/client_capabilities.dart b/pkg/analysis_server/lib/src/lsp/client_capabilities.dart
@@ -118,6 +118,9 @@ class LspClientCapabilities {
   /// A set of commands that exist on the client that the server may call.
   final Set<String> supportedCommands;
 
+  /// User-friendly error messages from parsing the experimental capabilities.
+  final List<String> experimentalCapabilitiesErrors;
+
   factory LspClientCapabilities(ClientCapabilities raw) {
     var workspace = raw.workspace;
     var workspaceEdit = workspace?.workspaceEdit;
@@ -137,8 +140,6 @@ class LspClientCapabilities {
     var definition = textDocument?.definition;
     var typeDefinition = textDocument?.typeDefinition;
     var workspaceSymbol = workspace?.symbol;
-    var experimental = _mapOrEmpty(raw.experimental);
-    var experimentalActions = _mapOrEmpty(experimental['dartCodeAction']);
 
     var applyEdit = workspace?.applyEdit ?? false;
     var codeActionKinds =
@@ -184,30 +185,8 @@ class LspClientCapabilities {
     var workDoneProgress = raw.window?.workDoneProgress ?? false;
     var workspaceSymbolKinds = _listToSet(workspaceSymbol?.symbolKind?.valueSet,
         defaults: defaultSupportedSymbolKinds);
-    var experimentalSnippetTextEdit = experimental['snippetTextEdit'] == true;
-    var commandParameterSupport =
-        _mapOrEmpty(experimentalActions['commandParameterSupport']);
-    var commandParameterSupportedKinds =
-        _listToSet(commandParameterSupport['supportedKinds'] as List?)
-            .cast<String>();
-    var supportsDartExperimentalTextDocumentContentProvider =
-        (experimental[dartExperimentalTextDocumentContentProviderKey] ??
-                experimental[
-                    dartExperimentalTextDocumentContentProviderLegacyKey]) !=
-            null;
-    var supportedCommands =
-        _listToSet(experimental['commands'] as List?).cast<String>();
 
-    /// At the time of writing (2023-02-01) there is no official capability for
-    /// supporting 'showMessageRequest' because LSP assumed all clients
-    /// supported it.
-    ///
-    /// This turned out to not be the case, so to avoid sending prompts that
-    /// might not be seen, we will only use this functionality if we _know_ the
-    /// client supports it via a custom flag in 'experimental' that is passed by
-    /// the Dart-Code VS Code extension since version v3.58.0 (2023-01-25).
-    var supportsShowMessageRequest =
-        experimental['supportsWindowShowMessageRequest'] == true;
+    var experimental = _ExperimentalClientCapabilities.parse(raw.experimental);
 
     return LspClientCapabilities._(
       raw,
@@ -241,12 +220,14 @@ class LspClientCapabilities {
       completionLabelDetails: completionLabelDetails,
       completionDefaultEditRange: completionDefaultEditRange,
       completionDefaultTextMode: completionDefaultTextMode,
-      experimentalSnippetTextEdit: experimentalSnippetTextEdit,
-      codeActionCommandParameterSupportedKinds: commandParameterSupportedKinds,
-      supportsShowMessageRequest: supportsShowMessageRequest,
+      experimentalSnippetTextEdit: experimental.snippetTextEdit,
+      codeActionCommandParameterSupportedKinds:
+          experimental.commandParameterKinds,
+      supportsShowMessageRequest: experimental.showMessageRequest,
       supportsDartExperimentalTextDocumentContentProvider:
-          supportsDartExperimentalTextDocumentContentProvider,
-      supportedCommands: supportedCommands,
+          experimental.dartTextDocumentContentProvider,
+      supportedCommands: experimental.commands,
+      experimentalCapabilitiesErrors: experimental.errors,
     );
   }
 
@@ -287,6 +268,7 @@ class LspClientCapabilities {
     required this.supportsShowMessageRequest,
     required this.supportsDartExperimentalTextDocumentContentProvider,
     required this.supportedCommands,
+    required this.experimentalCapabilitiesErrors,
   });
 
   /// Converts a list to a `Set`, returning null if the list is null.
@@ -300,8 +282,120 @@ class LspClientCapabilities {
   static Set<T> _listToSet<T>(List<T>? items, {Set<T> defaults = const {}}) {
     return items != null ? {...items} : defaults;
   }
+}
+
+/// A helper for parsing experimental capabilities and collecting any errors
+/// because their values do not match the types expected by the server.
+class _ExperimentalClientCapabilities {
+  /// User-friendly error messages from parsing the experimental capabilities.
+  final List<String> errors;
+
+  final bool snippetTextEdit;
+  final Set<String> commandParameterKinds;
+  final bool dartTextDocumentContentProvider;
+  final Set<String> commands;
+  final bool showMessageRequest;
+
+  _ExperimentalClientCapabilities({
+    required this.snippetTextEdit,
+    required this.commandParameterKinds,
+    required this.dartTextDocumentContentProvider,
+    required this.commands,
+    required this.showMessageRequest,
+    required this.errors,
+  });
+
+  /// Parse the experimental capabilities.
+  ///
+  /// Unlike the capabilities above the spec doesn't define any types for
+  /// these, so we may see types we don't expect (whereas the above would have
+  /// failed to deserialize if the types are invalid). So, check the types
+  /// carefully and report a warning to the client if something looks wrong.
+  ///
+  /// Example: https://github.com/dart-lang/sdk/issues/55935
+  factory _ExperimentalClientCapabilities.parse(Object? raw) {
+    var errors = <String>[];
+
+    /// Helper to ensure [object] is type [T] and otherwise records an error in
+    /// [errors] and returns `null`.
+    T? expectType<T>(String suffix, Object? object, [String? typeDescription]) {
+      if (object is! T) {
+        errors.add(
+            'ClientCapabilities.experimental$suffix must be a ${typeDescription ?? T}');
+        return null;
+      }
+      return object;
+    }
+
+    var expectMap = expectType<Map<String, Object?>?>;
+    var expectBool = expectType<bool?>;
+    var expectString = expectType<String>;
+
+    /// Helper to expect a nullable list of strings and return them as a set.
+    Set<String>? expectNullableStringSet(String name, Object? object) {
+      return expectType<List<Object?>?>(name, object, 'List<String>?')
+          ?.map((item) => expectString('$name[]', item))
+          .nonNulls
+          .toSet();
+    }
+
+    var experimental = expectMap('', raw) ?? const {};
 
-  static Map<String, Object?> _mapOrEmpty(Object? item) {
-    return item is Map<String, Object?> ? item : const {};
+    // Snippets.
+    var snippetTextEdit = expectBool(
+      '.snippetTextEdit',
+      experimental['snippetTextEdit'],
+    );
+
+    // Refactor command parameters.
+    var experimentalActions = expectMap(
+      '.dartCodeAction',
+      experimental['dartCodeAction'],
+    );
+    experimentalActions ??= const {};
+    var commandParameters = expectMap(
+      '.dartCodeAction.commandParameterSupport',
+      experimentalActions['commandParameterSupport'],
+    );
+    commandParameters ??= {};
+    var commandParameterKinds = expectNullableStringSet(
+      '.dartCodeAction.commandParameterSupport.supportedKinds',
+      commandParameters['supportedKinds'],
+    );
+
+    // Macro/Augmentation content.
+    var dartContentValue =
+        experimental[dartExperimentalTextDocumentContentProviderKey] ??
+            experimental[dartExperimentalTextDocumentContentProviderLegacyKey];
+    var dartTextDocumentContentProvider = expectBool(
+      '.$dartExperimentalTextDocumentContentProviderKey',
+      dartContentValue,
+    );
+
+    // Executable commands.
+    var commands =
+        expectNullableStringSet('.commands', experimental['commands']);
+
+    /// At the time of writing (2023-02-01) there is no official capability for
+    /// supporting 'showMessageRequest' because LSP assumed all clients
+    /// supported it.
+    ///
+    /// This turned out to not be the case, so to avoid sending prompts that
+    /// might not be seen, we will only use this functionality if we _know_ the
+    /// client supports it via a custom flag in 'experimental' that is passed by
+    /// the Dart-Code VS Code extension since version v3.58.0 (2023-01-25).
+    var showMessageRequest = expectBool(
+      '.supportsWindowShowMessageRequest',
+      experimental['supportsWindowShowMessageRequest'],
+    );
+
+    return _ExperimentalClientCapabilities(
+      snippetTextEdit: snippetTextEdit ?? false,
+      commandParameterKinds: commandParameterKinds ?? {},
+      dartTextDocumentContentProvider: dartTextDocumentContentProvider ?? false,
+      commands: commands ?? {},
+      showMessageRequest: showMessageRequest ?? false,
+      errors: errors,
+    );
   }
 }
diff --git a/pkg/analysis_server/lib/src/lsp/lsp_analysis_server.dart b/pkg/analysis_server/lib/src/lsp/lsp_analysis_server.dart
@@ -396,6 +396,22 @@ class LspAnalysisServer extends AnalysisServer {
 
     _checkAnalytics();
     enableSurveys();
+
+    // Notify the client of any issues parsing experimental capabilities.
+    var errors = _clientCapabilities?.experimentalCapabilitiesErrors;
+    if (errors != null && errors.isNotEmpty) {
+      var message = 'There were errors parsing your experimental '
+          'client capabilities:\n${errors.join(', ')}';
+
+      var params =
+          ShowMessageParams(type: MessageType.warning.forLsp, message: message);
+
+      channel.sendNotification(NotificationMessage(
+        method: Method.window_showMessage,
+        params: params,
+        jsonrpc: jsonRpcVersion,
+      ));
+    }
   }
 
   /// Handles a response from the client by invoking the completer that the
diff --git a/pkg/analysis_server/test/lsp/initialization_test.dart b/pkg/analysis_server/test/lsp/initialization_test.dart
@@ -5,7 +5,7 @@
 import 'dart:async';
 
 import 'package:analysis_server/lsp_protocol/protocol.dart';
-import 'package:analysis_server/src/analysis_server.dart';
+import 'package:analysis_server/src/analysis_server.dart' hide MessageType;
 import 'package:analysis_server/src/lsp/constants.dart';
 import 'package:analysis_server/src/lsp/server_capabilities_computer.dart';
 import 'package:analysis_server/src/plugin/plugin_manager.dart';
@@ -57,6 +57,27 @@ class InitializationTest extends AbstractLspAnalysisServerTest {
     expect(registeredMethods, equals(result));
   }
 
+  /// Expects successful initialization, but a window/showMessage warning
+  /// [expectedMessage] for the given [experimentalCapabilities].
+  Future<void> expectInvalidExperimentalParams(
+    Map<String, Object?> experimentalCapabilities,
+    String expectedMessage,
+  ) async {
+    var warning = showMessageNotifications
+        .firstWhere((m) => m.type == MessageType.Warning);
+
+    await initialize(experimentalCapabilities: experimentalCapabilities);
+    var message = (await warning).message;
+
+    expect(
+      message,
+      allOf(
+        contains('errors parsing your experimental client capabilities'),
+        contains(expectedMessage),
+      ),
+    );
+  }
+
   TextDocumentRegistrationOptions registrationOptionsFor(
     List<Registration> registrations,
     Method method,
@@ -936,6 +957,57 @@ class InitializationTest extends AbstractLspAnalysisServerTest {
     expect(server.contextManager.includedPaths, equals([projectFolderPath]));
   }
 
+  Future<void> test_invalidExperimental_commands() async {
+    await expectInvalidExperimentalParams(
+      {'commands': 1},
+      'ClientCapabilities.experimental.commands must be a List<String>?',
+    );
+  }
+
+  Future<void> test_invalidExperimental_dartCodeAction() async {
+    await expectInvalidExperimentalParams(
+      {'dartCodeAction': 1},
+      'ClientCapabilities.experimental.dartCodeAction must be a Map<String, Object?>?',
+    );
+  }
+
+  Future<void>
+      test_invalidExperimental_dartCodeAction_commandParameterSupport() async {
+    await expectInvalidExperimentalParams(
+      {
+        'dartCodeAction': {'commandParameterSupport': 1}
+      },
+      'ClientCapabilities.experimental.dartCodeAction.commandParameterSupport must be a Map<String, Object?>?',
+    );
+  }
+
+  Future<void>
+      test_invalidExperimental_dartCodeAction_commandParameterSupport_supportedKinds() async {
+    await expectInvalidExperimentalParams(
+      {
+        'dartCodeAction': {
+          'commandParameterSupport': {'supportedKinds': 1}
+        }
+      },
+      'ClientCapabilities.experimental.dartCodeAction.commandParameterSupport.supportedKinds must be a List<String>?',
+    );
+  }
+
+  Future<void> test_invalidExperimental_snippetTextEdit() async {
+    await expectInvalidExperimentalParams(
+      {'snippetTextEdit': 1},
+      'ClientCapabilities.experimental.snippetTextEdit must be a bool?',
+    );
+  }
+
+  Future<void>
+      test_invalidExperimental_supportsDartTextDocumentContentProvider() async {
+    await expectInvalidExperimentalParams(
+      {'supportsDartTextDocumentContentProvider': 1},
+      'ClientCapabilities.experimental.supportsDartTextDocumentContentProvider must be a bool?',
+    );
+  }
+
   Future<void> test_nonFileScheme_rootUri() async {
     // We expect an error notification about the invalid file we try to open.
     failTestOnAnyErrorNotification = false;
diff --git a/pkg/analysis_server/test/lsp/server_abstract.dart b/pkg/analysis_server/test/lsp/server_abstract.dart
@@ -903,6 +903,14 @@ mixin LspAnalysisServerTestMixin
 
   Stream<Message> get serverToClient;
 
+  /// A stream of [ShowMessageParams] for any `window/logMessage` notifications.
+  Stream<ShowMessageParams> get showMessageNotifications =>
+      notificationsFromServer
+          .where((notification) =>
+              notification.method == Method.window_showMessage)
+          .map((message) => ShowMessageParams.fromJson(
+              message.params as Map<String, Object?>));
+
   String get testPackageRootPath => projectFolderPath;
 
   Future<void> changeFile(
@@ -1598,7 +1606,14 @@ mixin LspAnalysisServerTestMixin
   /// ensure no errors come from the server in response to notifications (which
   /// don't have their own responses).
   bool _isErrorNotification(NotificationMessage notification) {
-    return notification.method == Method.window_logMessage ||
-        notification.method == Method.window_showMessage;
+    var method = notification.method;
+    var params = notification.params as Map<String, Object?>?;
+    if (method == Method.window_logMessage && params != null) {
+      return LogMessageParams.fromJson(params).type == MessageType.Error;
+    } else if (method == Method.window_showMessage && params != null) {
+      return ShowMessageParams.fromJson(params).type == MessageType.Error;
+    } else {
+      return false;
+    }
   }
 }
