[analysis_server] Prevent duplicate LSP completion registrations for Dart

... when using plugins that register for Dart files.

Fixes https://github.com/Dart-Code/Dart-Code/issues/3555.

Change-Id: Idadcda0b04f86eb40d11a69e8eadc7120c873ae7
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/213266
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Brian Wilkerson <brianwilkerson@google.com>

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

@@ -293,6 +293,8 @@ class ServerCapabilitiesComputer {
         // interestingFiles. Prefix a `**/` so that the glob matches nested
         // folders as well.
         .map((glob) => DocumentFilter(scheme: 'file', pattern: '**/$glob'));
+    final pluginTypesExcludingDart =
+        pluginTypes.where((filter) => filter.pattern != '**/*.dart');
 
     final fullySupportedTypes = {dartFiles, ...pluginTypes}.toList();
 
@@ -314,7 +316,7 @@ class ServerCapabilitiesComputer {
     final completionSupportedTypesExcludingDart = {
       // Dart is excluded here at it's registered separately with trigger/commit
       // characters.
-      ...pluginTypes,
+      ...pluginTypesExcludingDart,
       pubspecFile,
       analysisOptionsFile,
       fixDataFile,

pkg/analysis_server/test/lsp/initialization_test.dart

@@ -91,6 +91,51 @@ class InitializationTest extends AbstractLspAnalysisServerTest {
     expect(nonDartOptions.triggerCharacters, isNull);
   }
 
+  Future<void> test_completionRegistrations_withDartPlugin() async {
+    // This tests for a bug that occurred with an analysis server plugin
+    // that works on Dart files. When computing completion registrations we
+    // usually have seperate registrations for Dart + non-Dart to account for
+    // different trigger characters. However, the plugin types were all being
+    // included in the non-Dart registration even if they included Dart.
+    //
+    // The result was two registrations including Dart, which caused duplicate
+    // requests for Dart completions, which resulted in duplicate items
+    // appearing in the editor.
+
+    // Track all current registrations.
+    final registrations = <Registration>[];
+
+    // Perform normal registration (without plugins) to get the initial set.
+    await monitorDynamicRegistrations(
+      registrations,
+      () => initialize(
+        textDocumentCapabilities:
+            withAllSupportedTextDocumentDynamicRegistrations(
+                emptyTextDocumentClientCapabilities),
+      ),
+    );
+
+    // Expect only a single registration that includes Dart files.
+    expect(
+      registrationsForDart(registrations, Method.textDocument_completion),
+      hasLength(1),
+    );
+
+    // Monitor the unregistration/new registrations during the plugin activation.
+    await monitorDynamicReregistration(registrations, () async {
+      final plugin = configureTestPlugin();
+      plugin.currentSession = PluginSession(plugin)
+        ..interestingFiles = ['*.dart'];
+      pluginManager.pluginsChangedController.add(null);
+    });
+
+    // Expect that there is still only a single registration for Dart.
+    expect(
+      registrationsForDart(registrations, Method.textDocument_completion),
+      hasLength(1),
+    );
+  }
+
   Future<void> test_dynamicRegistration_containsAppropriateSettings() async {
     // Basic check that the server responds with the capabilities we'd expect,
     // for ex including analysis_options.yaml in text synchronization but not

pkg/analysis_server/test/lsp/server_abstract.dart

@@ -128,19 +128,35 @@ abstract class AbstractLspAnalysisServerTest
     return registrations.singleWhereOrNull((r) => r.method == method.toJson());
   }
 
-  /// Finds the registration for a given LSP method with Dart in its
+  /// Finds a single registration for a given LSP method with Dart in its
   /// documentSelector.
+  ///
+  /// Throws if there is not exactly one match.
   Registration registrationForDart(
     List<Registration> registrations,
     Method method,
+  ) =>
+      registrationsForDart(registrations, method).single;
+
+  /// Finds the registrations for a given LSP method with Dart in their
+  /// documentSelector.
+  List<Registration> registrationsForDart(
+    List<Registration> registrations,
+    Method method,
   ) {
-    return registrations.singleWhere((r) =>
-        r.method == method.toJson() &&
-        (TextDocumentRegistrationOptions.fromJson(
-                    r.registerOptions as Map<String, Object?>)
-                .documentSelector
-                ?.any((selector) => selector.language == dartLanguageId) ??
-            false));
+    bool includesDart(Registration r) {
+      final options = TextDocumentRegistrationOptions.fromJson(
+          r.registerOptions as Map<String, Object?>);
+
+      return options.documentSelector?.any((selector) =>
+              selector.language == dartLanguageId ||
+              (selector.pattern?.contains('.dart') ?? false)) ??
+          false;
+    }
+
+    return registrations
+        .where((r) => r.method == method.toJson() && includesDart(r))
+        .toList();
   }
 
   void resetContextBuildCounter() {
@@ -1390,11 +1406,11 @@ mixin LspAnalysisServerTestMixin implements ClientCapabilitiesHelperMixin {
 
   /// Watches for `client/registerCapability` requests and updates
   /// `registrations`.
-  Future<ResponseMessage> monitorDynamicRegistrations(
+  Future<T> monitorDynamicRegistrations<T>(
     List<Registration> registrations,
-    Future<ResponseMessage> Function() f,
+    Future<T> Function() f,
   ) {
-    return handleExpectedRequest<ResponseMessage, RegistrationParams, void>(
+    return handleExpectedRequest<T, RegistrationParams, void>(
       Method.client_registerCapability,
       RegistrationParams.fromJson,
       f,
@@ -1405,9 +1421,9 @@ mixin LspAnalysisServerTestMixin implements ClientCapabilitiesHelperMixin {
   }
 
   /// Expects both unregistration and reregistration.
-  Future<ResponseMessage> monitorDynamicReregistration(
+  Future<T> monitorDynamicReregistration<T>(
     List<Registration> registrations,
-    Future<ResponseMessage> Function() f,
+    Future<T> Function() f,
   ) =>
       monitorDynamicUnregistrations(
         registrations,
@@ -1416,11 +1432,11 @@ mixin LspAnalysisServerTestMixin implements ClientCapabilitiesHelperMixin {
 
   /// Watches for `client/unregisterCapability` requests and updates
   /// `registrations`.
-  Future<ResponseMessage> monitorDynamicUnregistrations(
+  Future<T> monitorDynamicUnregistrations<T>(
     List<Registration> registrations,
-    Future<ResponseMessage> Function() f,
+    Future<T> Function() f,
   ) {
-    return handleExpectedRequest<ResponseMessage, UnregistrationParams, void>(
+    return handleExpectedRequest<T, UnregistrationParams, void>(
       Method.client_unregisterCapability,
       UnregistrationParams.fromJson,
       f,