[analysis_server] Move CodeActionKind filter earlier in production of code actions

This removes a filter of CodeActionKinds from the final step (and a class named `_CodeActionSorter`!) and instead applies it earlier during building of the actions. This will help apply the filter in the case where we return bare Commands (which don't have `kind`s) instead of `CodeActionLiteral`s.

I added some TODOs because I still don't think this is filtering early enough (because in the case of invoking an action via the command, we need to not have to produce _all_ code actions just to locate the _one_ we want to execute), but it's a step closer (and easier to review this without it being lumped in with the CL that supports returning Commands).

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

Author: DanTup

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

@@ -63,20 +63,26 @@ abstract class AbstractCodeActionsProducer
 
   bool get supportsLiterals => capabilities.literalCodeActions;
 
-  /// Creates a CodeAction to apply this assist. Note: This code will fetch the
-  /// version of each document being modified so it's important to call this
-  /// immediately after computing edits to ensure the document is not modified
-  /// before the version number is read.
+  /// Creates a CodeAction to apply this assist.
+  ///
+  /// This code will fetch the version of each document being modified so it's
+  /// important to call this immediately after computing edits to ensure the
+  /// document is not modified before the version number is read.
   @protected
   CodeActionLiteral createAssistAction(
     protocol.SourceChange change,
+    CodeActionKind kind,
     String? loggedAssistId,
     String path,
     LineInfo lineInfo,
   ) {
+    assert(
+      kind == CodeActionKind.Refactor ||
+          '$kind'.startsWith('${CodeActionKind.Refactor}.'),
+    );
     return CodeActionLiteral(
       title: change.message,
-      kind: toCodeActionKind(change.id, CodeActionKind.Refactor),
+      kind: kind,
       diagnostics: const [],
       command: createLogActionCommand(loggedAssistId),
       edit: createWorkspaceEdit(
@@ -106,21 +112,27 @@ abstract class AbstractCodeActionsProducer
     );
   }
 
-  /// Creates a CodeAction to apply this fix. Note: This code will fetch the
-  /// version of each document being modified so it's important to call this
-  /// immediately after computing edits to ensure the document is not modified
-  /// before the version number is read.
+  /// Creates a CodeAction to apply this fix.
+  ///
+  /// This code will fetch the version of each document being modified so it's
+  /// important to call this immediately after computing edits to ensure the
+  /// document is not modified before the version number is read.
   @protected
   CodeActionLiteral createFixAction(
     protocol.SourceChange change,
+    CodeActionKind kind,
     String? loggedFixId,
     Diagnostic diagnostic,
     String path,
     LineInfo lineInfo,
   ) {
+    assert(
+      kind == CodeActionKind.QuickFix ||
+          '$kind'.startsWith('${CodeActionKind.QuickFix}.'),
+    );
     return CodeActionLiteral(
       title: change.message,
-      kind: toCodeActionKind(change.id, CodeActionKind.QuickFix),
+      kind: kind,
       diagnostics: [diagnostic],
       command: createLogActionCommand(loggedFixId),
       edit: createWorkspaceEdit(

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

@@ -6,6 +6,7 @@ import 'dart:async';
 
 import 'package:analysis_server/lsp_protocol/protocol.dart';
 import 'package:analysis_server/src/lsp/handlers/code_actions/abstract_code_actions_producer.dart';
+import 'package:analysis_server/src/lsp/mapping.dart';
 import 'package:analysis_server/src/services/correction/fix/analysis_options/fix_generator.dart';
 import 'package:analyzer/source/file_source.dart';
 import 'package:analyzer/source/line_info.dart';
@@ -95,15 +96,22 @@ class AnalysisOptionsCodeActionsProducer extends AbstractCodeActionsProducer {
       var diagnostic = createDiagnostic(lineInfo, result, error);
       codeActions.addAll(
         fixes.map((fix) {
+          var kind = toCodeActionKind(fix.change.id, CodeActionKind.QuickFix);
+          // TODO(dantup): Find a way to filter these earlier, so we don't
+          //  compute fixes we will filter out.
+          if (!shouldIncludeKind(kind)) {
+            return null;
+          }
           var action = createFixAction(
             fix.change,
+            kind,
             fix.change.id,
             diagnostic,
             path,
             lineInfo,
           );
           return (action: action, priority: fix.kind.priority);
-        }),
+        }).nonNulls,
       );
     }
 

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

@@ -162,15 +162,28 @@ class DartCodeActionsProducer extends AbstractCodeActionsProducer {
         assists = await computeAssists(context);
       }
 
-      return assists.map((assist) {
-        var action = createAssistAction(
-          assist.change,
-          assist.change.id,
-          unitResult.path,
-          unitResult.lineInfo,
-        );
-        return (action: action, priority: assist.kind.priority);
-      }).toList();
+      return assists
+          .map((assist) {
+            var kind = toCodeActionKind(
+              assist.change.id,
+              CodeActionKind.Refactor,
+            );
+            // TODO(dantup): Find a way to filter these earlier, so we don't
+            //  compute fixes we will filter out.
+            if (!shouldIncludeKind(kind)) {
+              return null;
+            }
+            var action = createAssistAction(
+              assist.change,
+              kind,
+              assist.change.id,
+              unitResult.path,
+              unitResult.lineInfo,
+            );
+            return (action: action, priority: assist.kind.priority);
+          })
+          .nonNulls
+          .toList();
     } on InconsistentAnalysisException {
       // If an InconsistentAnalysisException occurs, it's likely the user modified
       // the source and therefore is no longer interested in the results, so
@@ -247,15 +260,25 @@ class DartCodeActionsProducer extends AbstractCodeActionsProducer {
           );
           codeActions.addAll(
             fixes.map((fix) {
+              var kind = toCodeActionKind(
+                fix.change.id,
+                CodeActionKind.QuickFix,
+              );
+              // TODO(dantup): Find a way to filter these earlier, so we don't
+              //  compute fixes we will filter out.
+              if (!shouldIncludeKind(kind)) {
+                return null;
+              }
               var action = createFixAction(
                 fix.change,
+                kind,
                 fix.change.id,
                 diagnostic,
                 path,
                 lineInfo,
               );
               return (action: action, priority: fix.kind.priority);
-            }),
+            }).nonNulls,
           );
         }
       }
@@ -300,7 +323,11 @@ class DartCodeActionsProducer extends AbstractCodeActionsProducer {
         performance: performanceTracker,
       );
       var actions = await processor.compute();
-      refactorActions.addAll(actions.map(CodeAction.t1));
+      refactorActions.addAll(
+        actions
+            .where((literal) => shouldIncludeKind(literal.kind))
+            .map(CodeAction.t1),
+      );
 
       // Extracts
       if (shouldIncludeKind(CodeActionKind.RefactorExtract)) {

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

@@ -48,6 +48,7 @@ class PluginCodeActionsProducer extends AbstractCodeActionsProducer {
         .map((response) => plugin.EditGetAssistsResult.fromResponse(response))
         .expand((response) => response.assists)
         .map(_convertAssist)
+        .nonNulls
         .toList();
   }
 
@@ -78,10 +79,20 @@ class PluginCodeActionsProducer extends AbstractCodeActionsProducer {
   @override
   Future<List<CodeAction>> getSourceActions() async => [];
 
-  CodeActionWithPriority _convertAssist(plugin.PrioritizedSourceChange assist) {
+  CodeActionWithPriority? _convertAssist(
+    plugin.PrioritizedSourceChange assist,
+  ) {
+    var kind = toCodeActionKind(assist.change.id, CodeActionKind.Refactor);
+    // TODO(dantup): Find a way to filter these earlier, so we don't
+    //  compute fixes we will filter out.
+    if (!shouldIncludeKind(kind)) {
+      return null;
+    }
+
     return (
       action: createAssistAction(
         assist.change,
+        kind,
         'assist from plugin',
         path,
         lineInfo,
@@ -100,18 +111,25 @@ class PluginCodeActionsProducer extends AbstractCodeActionsProducer {
       supportedTags: supportedDiagnosticTags,
       clientSupportsCodeDescription: supportsCodeDescription,
     );
-    return fixes.fixes.map(
-      (fix) => (
+    return fixes.fixes.map((fix) {
+      var kind = toCodeActionKind(fix.change.id, CodeActionKind.QuickFix);
+      // TODO(dantup): Find a way to filter these earlier, so we don't
+      //  compute fixes we will filter out.
+      if (!shouldIncludeKind(kind)) {
+        return null;
+      }
+      return (
         action: createFixAction(
           fix.change,
+          kind,
           'fix from plugin',
           diagnostic,
           path,
           lineInfo,
         ),
         priority: fix.priority,
-      ),
-    );
+      );
+    }).nonNulls;
   }
 
   Future<List<plugin.Response>> _sendPluginRequest(

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

@@ -6,6 +6,7 @@ import 'dart:async';
 
 import 'package:analysis_server/lsp_protocol/protocol.dart';
 import 'package:analysis_server/src/lsp/handlers/code_actions/abstract_code_actions_producer.dart';
+import 'package:analysis_server/src/lsp/mapping.dart';
 import 'package:analysis_server/src/services/correction/fix/pubspec/fix_generator.dart';
 import 'package:analyzer/source/file_source.dart';
 import 'package:analyzer/source/line_info.dart';
@@ -81,15 +82,22 @@ class PubspecCodeActionsProducer extends AbstractCodeActionsProducer {
       var diagnostic = createDiagnostic(lineInfo, result, error);
       codeActions.addAll(
         fixes.map((fix) {
+          var kind = toCodeActionKind(fix.change.id, CodeActionKind.QuickFix);
+          // TODO(dantup): Find a way to filter these earlier, so we don't
+          //  compute fixes we will filter out.
+          if (!shouldIncludeKind(kind)) {
+            return null;
+          }
           var action = createFixAction(
             fix.change,
+            kind,
             fix.change.id,
             diagnostic,
             path,
             lineInfo,
           );
           return (action: action, priority: fix.kind.priority);
-        }),
+        }).nonNulls,
       );
     }
 

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

@@ -218,7 +218,7 @@ class CodeActionHandler
             analysisOptions: analysisOptions,
           ),
         ];
-        var sorter = _CodeActionSorter(params.range, shouldIncludeKind);
+        var sorter = _CodeActionSorter(params.range);
 
         var allActions = <CodeAction>[
           // Like-kinded actions are grouped (and prioritized) together
@@ -303,9 +303,8 @@ class CodeActionRegistrations extends FeatureRegistration
 /// the one nearest [range].
 class _CodeActionSorter {
   final Range range;
-  final bool Function(CodeActionKind?) shouldIncludeKind;
 
-  _CodeActionSorter(this.range, this.shouldIncludeKind);
+  _CodeActionSorter(this.range);
 
   List<CodeAction> sort(List<CodeActionWithPriority> actions) {
     var dedupedActions = _dedupeActions(actions, range.start);
@@ -323,7 +322,6 @@ class _CodeActionSorter {
     dedupedActionsWithIndex.sort(_compareCodeActions);
 
     return dedupedActionsWithIndex
-        .where((action) => shouldIncludeKind(action.action.kind))
         .map((action) => CodeAction.t1(action.action))
         .toList();
   }

pkg/analysis_server/test/lsp/code_actions_fixes_test.dart

@@ -312,9 +312,10 @@ Future foo;
     ofKind(CodeActionKind kind) =>
         getCodeActions(mainFileUri, range: code.range.range, kinds: [kind]);
 
-    // The code above will return a quickfix.remove.unusedImport
+    // The code above will return a 'quickfix.remove.unusedImport'.
     expect(await ofKind(CodeActionKind.QuickFix), isNotEmpty);
     expect(await ofKind(CodeActionKind('quickfix.remove')), isNotEmpty);
+    expect(await ofKind(CodeActionKind('quickfix.remove.foo')), isEmpty);
     expect(await ofKind(CodeActionKind('quickfix.other')), isEmpty);
     expect(await ofKind(CodeActionKind.Refactor), isEmpty);
   }

pkg/analysis_server/test/lsp/code_actions_refactor_test.dart

@@ -12,7 +12,6 @@ import 'package:test/test.dart';
 import 'package:test_reflective_loader/test_reflective_loader.dart';
 
 import '../tool/lsp_spec/matchers.dart';
-import '../utils/lsp_protocol_extensions.dart';
 import '../utils/test_code_extensions.dart';
 import 'code_actions_abstract.dart';
 import 'request_helpers_mixin.dart';
@@ -255,22 +254,12 @@ void f() {
     ofKind(CodeActionKind kind) =>
         getCodeActions(mainFileUri, range: code.range.range, kinds: [kind]);
 
-    // Helper that requests CodeActions for [kind] and ensures all results
-    // returned have either an equal kind, or a kind that is prefixed with the
-    // requested kind followed by a dot.
-    Future<void> checkResults(CodeActionKind kind) async {
-      var results = await ofKind(kind);
-      for (var result in results) {
-        var resultKind = result.asCodeActionLiteral.kind;
-        expect('$resultKind', anyOf([equals('$kind'), startsWith('$kind.')]));
-      }
-    }
-
-    // Check a few of each that will produces multiple matches and no matches.
-    await checkResults(CodeActionKind.Refactor);
-    await checkResults(CodeActionKind.RefactorExtract);
-    await checkResults(CodeActionKind('refactor.extract.foo'));
-    await checkResults(CodeActionKind.RefactorRewrite);
+    // The code above will return a 'refactor.extract' (as well as some other
+    // refactors, but not rewrite).
+    expect(await ofKind(CodeActionKind.Refactor), isNotEmpty);
+    expect(await ofKind(CodeActionKind.RefactorExtract), isNotEmpty);
+    expect(await ofKind(CodeActionKind('refactor.extract.foo')), isEmpty);
+    expect(await ofKind(CodeActionKind.RefactorRewrite), isEmpty);
   }
 
   Future<void> test_generatesNames() async {

pkg/analysis_server/test/lsp/request_helpers_mixin.dart

@@ -18,6 +18,7 @@ import 'package:path/path.dart' as path;
 import 'package:test/test.dart' as test show expect;
 import 'package:test/test.dart';
 
+import '../utils/lsp_protocol_extensions.dart';
 import 'change_verifier.dart';
 
 /// A mixin with helpers for applying LSP edits to strings.
@@ -346,7 +347,7 @@ mixin LspRequestHelpersMixin {
     List<CodeActionKind>? kinds,
     CodeActionTriggerKind? triggerKind,
     ProgressToken? workDoneToken,
-  }) {
+  }) async {
     range ??=
         position != null
             ? Range(start: position, end: position)
@@ -367,7 +368,8 @@ mixin LspRequestHelpersMixin {
         workDoneToken: workDoneToken,
       ),
     );
-    return expectSuccessfulResponseTo(
+
+    var actions = await expectSuccessfulResponseTo(
       request,
       _fromJsonList(
         _generateFromJsonFor(
@@ -378,6 +380,26 @@ mixin LspRequestHelpersMixin {
         ),
       ),
     );
+
+    // As an additional check, ensure all returned values are either exact
+    // matches or sub-kinds of the requested kind(s).
+    if (kinds != null && kinds.isNotEmpty) {
+      // Kinds must either by an exact match, or start with the
+      // requested value followed by a dot (a sub-kind).
+      var allowedKinds =
+          kinds
+              .expand((kind) => [equals('$kind'), startsWith('$kind.')])
+              .toList();
+
+      // Only CodeActionLiterals can be checked because bare commands do not
+      // have CodeActionKinds (once they've left the server).
+      var literals = actions.where((action) => action.isCodeActionLiteral);
+      for (var result in literals) {
+        expect(result.asCodeActionLiteral.kind.toString(), anyOf(allowedKinds));
+      }
+    }
+
+    return actions;
   }
 
   Future<TextDocumentCodeLensResult> getCodeLens(Uri uri) {