[analysis_server] Don't return the Fix All code action for legacy server

Currently the commands for Fix All are only supported for a real LSP server, so we shouldn't return a Source CodeAction that would try to run the command when running in the legacy server.

Change-Id: I3256ef5e19c3a45e7fc3f3ec726a59da30d3a30c
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/429642
Commit-Queue: Brian Wilkerson <[email protected]>
Reviewed-by: Samuel Rawlins <[email protected]>
Reviewed-by: Brian Wilkerson <[email protected]>

Author: DanTup

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

@@ -274,4 +274,18 @@ abstract class AbstractCodeActionsProducer
       return null;
     }
   }
+
+  /// Checks whether the server supports a given command.
+  bool serverSupportsCommand(String command) {
+    var handler = server.executeCommandHandler;
+
+    // `null` should never happen, it's set by the constructor of
+    // ExecuteCommandHandler which is invoked as part of initialization.
+    assert(handler != null);
+    if (handler == null) {
+      return false;
+    }
+
+    return handler.commandHandlers[command] != null;
+  }
 }

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

@@ -104,6 +104,10 @@ class DartCodeActionsProducer extends AbstractCodeActionsProducer {
       (() => Commands.serverSupportedCommands.contains(command))(),
       'serverSupportedCommands did not contain $command',
     );
+    assert(
+      (() => serverSupportsCommand(command))(),
+      'This kind of server does not support $command',
+    );
 
     return _maybeWrapCommandInCodeActionLiteral(
       actionKind,
@@ -527,7 +531,8 @@ class DartCodeActionsProducer extends AbstractCodeActionsProducer {
           'Organize Imports',
           Commands.organizeImports,
         ),
-      if (shouldIncludeKind(DartCodeActionKind.FixAll))
+      if (serverSupportsCommand(Commands.fixAll) &&
+          shouldIncludeKind(DartCodeActionKind.FixAll))
         createAction(DartCodeActionKind.FixAll, 'Fix All', Commands.fixAll),
     ];
   }

pkg/analysis_server/test/lsp/code_actions_mixin.dart

@@ -23,6 +23,9 @@ mixin CodeActionsTestMixin
         LspVerifyEditHelpersMixin {
   final String simplePubspecContent = 'name: my_project';
 
+  /// Whether the server supports the "Fix All" command (currently LSP-only).
+  bool get serverSupportsFixAll => true;
+
   /// Initializes the server with some basic configuration and expects to find
   /// a [CodeAction] with [kind]/[command]/[title].
   Future<CodeAction> expectCodeAction(

pkg/analysis_server/test/lsp/code_actions_source_test.dart

@@ -17,6 +17,7 @@ import 'server_abstract.dart';
 
 void main() {
   defineReflectiveSuite(() {
+    defineReflectiveTests(SourceCodeActionsTest);
     defineReflectiveTests(SortMembersSourceCodeActionsTest);
     defineReflectiveTests(OrganizeImportsSourceCodeActionsTest);
     defineReflectiveTests(FixAllSourceCodeActionsTest);
@@ -345,3 +346,10 @@ class SortMembersSourceCodeActionsTest extends AbstractSourceCodeActionsTest
         SharedSourceCodeActionsTestMixin,
         // Tests are defined in a shared mixin.
         SharedSortMembersSourceCodeActionsTests {}
+
+@reflectiveTest
+class SourceCodeActionsTest extends AbstractSourceCodeActionsTest
+    with
+        SharedSourceCodeActionsTestMixin,
+        // Tests are defined in a shared mixin.
+        SharedSourceCodeActionsTests {}

pkg/analysis_server/test/lsp_over_legacy/abstract_code_actions.dart

@@ -2,14 +2,24 @@
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
+import 'package:analysis_server/src/lsp/constants.dart';
+
 import '../lsp/code_actions_mixin.dart';
 import 'abstract_lsp_over_legacy.dart';
 
 class AbstractCodeActionsTest extends SharedLspOverLegacyTest
     with CodeActionsTestMixin {
   @override
+  bool get serverSupportsFixAll =>
+      // Legacy doesn't currently support the fix all command, but read it
+      // directly from the handler so if it changes in future the test starts
+      // running (and fails, prompting updating).
+      server.executeCommandHandler!.commandHandlers[Commands.fixAll] != null;
+
+  @override
   Future<void> initializeServer() async {
     await super.initializeServer();
+    await server.lspInitialized;
 
     // Most CodeActions tests set LSP capabilities so automatically send these
     // to the legacy server as part of initialization.

pkg/analysis_server/test/lsp_over_legacy/code_action_source_test.dart

@@ -2,18 +2,41 @@
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
+import 'package:analysis_server/src/lsp/constants.dart';
+import 'package:test/test.dart';
 import 'package:test_reflective_loader/test_reflective_loader.dart';
 
 import '../shared/shared_code_actions_source_tests.dart';
 import 'abstract_code_actions.dart';
 
 void main() {
   defineReflectiveSuite(() {
+    defineReflectiveTests(SourceCodeActionsTest);
     defineReflectiveTests(OrganizeImportsSourceCodeActionsTest);
     defineReflectiveTests(SortMembersSourceCodeActionsTest);
+    defineReflectiveTests(FixAllSourceCodeActionsTest);
   });
 }
 
+@reflectiveTest
+class FixAllSourceCodeActionsTest extends AbstractCodeActionsTest
+    with SharedSourceCodeActionsTestMixin {
+  /// The fix all command is currently LSP-only, so we shouldn't return code
+  /// actions for it.
+  Future<void> test_unavailable() async {
+    await expectNoAction('// content', command: Commands.fixAll);
+
+    expect(
+      serverSupportsFixAll,
+      isFalse,
+      reason:
+          'Fix All is not currently available for the legacy server. '
+          'If/when that changes, this test must be updated by extracting the '
+          'Fix All tests from LSP into a shared mixin like the others',
+    );
+  }
+}
+
 @reflectiveTest
 class OrganizeImportsSourceCodeActionsTest extends AbstractCodeActionsTest
     with
@@ -27,3 +50,10 @@ class SortMembersSourceCodeActionsTest extends AbstractCodeActionsTest
         SharedSourceCodeActionsTestMixin,
         // Tests are defined in a shared mixin.
         SharedSortMembersSourceCodeActionsTests {}
+
+@reflectiveTest
+class SourceCodeActionsTest extends AbstractCodeActionsTest
+    with
+        SharedSourceCodeActionsTestMixin,
+        // Tests are defined in a shared mixin.
+        SharedSourceCodeActionsTests {}

pkg/analysis_server/test/shared/shared_code_actions_source_tests.dart

@@ -126,20 +126,6 @@ int minified(int x, int y) => min(x, y);
     );
   }
 
-  Future<void> test_filtersCorrectly() async {
-    createFile(testFilePath, '');
-    await initializeServer();
-
-    ofKind(CodeActionKind kind) => getCodeActions(testFileUri, kinds: [kind]);
-
-    expect(await ofKind(CodeActionKind.Source), hasLength(3));
-    expect(await ofKind(CodeActionKind.SourceOrganizeImports), hasLength(1));
-    expect(await ofKind(DartCodeActionKind.SortMembers), hasLength(1));
-    expect(await ofKind(DartCodeActionKind.FixAll), hasLength(1));
-    expect(await ofKind(CodeActionKind('source.foo')), isEmpty);
-    expect(await ofKind(CodeActionKind.Refactor), isEmpty);
-  }
-
   Future<void> test_noEdits() async {
     const content = '''
 import 'dart:async';
@@ -371,3 +357,36 @@ mixin SharedSourceCodeActionsTestMixin
     setSupportedCodeActionKinds([CodeActionKind.Source]);
   }
 }
+
+/// Shared tests used by both LSP + Legacy server tests and/or integration.
+mixin SharedSourceCodeActionsTests
+    on
+        SharedTestInterface,
+        SharedSourceCodeActionsTestMixin,
+        CodeActionsTestMixin,
+        LspRequestHelpersMixin,
+        LspEditHelpersMixin,
+        LspVerifyEditHelpersMixin,
+        ClientCapabilitiesHelperMixin {
+  Future<void> test_filtersCorrectly() async {
+    createFile(testFilePath, '');
+    await initializeServer();
+
+    ofKind(CodeActionKind kind) => getCodeActions(testFileUri, kinds: [kind]);
+
+    var serverSupportsFixAll = this.serverSupportsFixAll;
+
+    expect(
+      await ofKind(CodeActionKind.Source),
+      hasLength(serverSupportsFixAll ? 3 : 2),
+    );
+    expect(await ofKind(CodeActionKind.SourceOrganizeImports), hasLength(1));
+    expect(await ofKind(DartCodeActionKind.SortMembers), hasLength(1));
+    expect(
+      await ofKind(DartCodeActionKind.FixAll),
+      hasLength(serverSupportsFixAll ? 1 : 0),
+    );
+    expect(await ofKind(CodeActionKind('source.foo')), isEmpty);
+    expect(await ofKind(CodeActionKind.Refactor), isEmpty);
+  }
+}