From 3c161d3fd17c19ad2b6d2f272331e31e608cfd41 Mon Sep 17 00:00:00 2001 From: Danny Tuppeny Date: Tue, 6 May 2025 11:03:33 -0700 Subject: [PATCH] [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 Reviewed-by: Brian Wilkerson Commit-Queue: Brian Wilkerson --- .../abstract_code_actions_producer.dart | 32 ++++++++---- .../code_actions/analysis_options.dart | 10 +++- .../src/lsp/handlers/code_actions/dart.dart | 49 ++++++++++++++----- .../lsp/handlers/code_actions/plugins.dart | 28 +++++++++-- .../lsp/handlers/code_actions/pubspec.dart | 10 +++- .../lsp/handlers/handler_code_actions.dart | 6 +-- .../test/lsp/code_actions_fixes_test.dart | 3 +- .../test/lsp/code_actions_refactor_test.dart | 23 +++------ .../test/lsp/request_helpers_mixin.dart | 26 +++++++++- .../test/utils/lsp_protocol_extensions.dart | 16 ++++++ 10 files changed, 151 insertions(+), 52 deletions(-) diff --git a/pkg/analysis_server/lib/src/lsp/handlers/code_actions/abstract_code_actions_producer.dart b/pkg/analysis_server/lib/src/lsp/handlers/code_actions/abstract_code_actions_producer.dart index 9d89a742696..cc7c851445b 100644 --- a/pkg/analysis_server/lib/src/lsp/handlers/code_actions/abstract_code_actions_producer.dart +++ b/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( diff --git a/pkg/analysis_server/lib/src/lsp/handlers/code_actions/analysis_options.dart b/pkg/analysis_server/lib/src/lsp/handlers/code_actions/analysis_options.dart index f8558d4e925..90d74e50cbf 100644 --- a/pkg/analysis_server/lib/src/lsp/handlers/code_actions/analysis_options.dart +++ b/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, ); } diff --git a/pkg/analysis_server/lib/src/lsp/handlers/code_actions/dart.dart b/pkg/analysis_server/lib/src/lsp/handlers/code_actions/dart.dart index 73284c891d6..5848c3effd0 100644 --- a/pkg/analysis_server/lib/src/lsp/handlers/code_actions/dart.dart +++ b/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)) { diff --git a/pkg/analysis_server/lib/src/lsp/handlers/code_actions/plugins.dart b/pkg/analysis_server/lib/src/lsp/handlers/code_actions/plugins.dart index 5e754ab5990..afa59a63eed 100644 --- a/pkg/analysis_server/lib/src/lsp/handlers/code_actions/plugins.dart +++ b/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> 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> _sendPluginRequest( diff --git a/pkg/analysis_server/lib/src/lsp/handlers/code_actions/pubspec.dart b/pkg/analysis_server/lib/src/lsp/handlers/code_actions/pubspec.dart index a2da418423f..1cd09c62609 100644 --- a/pkg/analysis_server/lib/src/lsp/handlers/code_actions/pubspec.dart +++ b/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, ); } diff --git a/pkg/analysis_server/lib/src/lsp/handlers/handler_code_actions.dart b/pkg/analysis_server/lib/src/lsp/handlers/handler_code_actions.dart index 12cd6a754bf..856cbb671a8 100644 --- a/pkg/analysis_server/lib/src/lsp/handlers/handler_code_actions.dart +++ b/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 = [ // 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 sort(List 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(); } diff --git a/pkg/analysis_server/test/lsp/code_actions_fixes_test.dart b/pkg/analysis_server/test/lsp/code_actions_fixes_test.dart index bccff896796..958ef43d2c6 100644 --- a/pkg/analysis_server/test/lsp/code_actions_fixes_test.dart +++ b/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); } diff --git a/pkg/analysis_server/test/lsp/code_actions_refactor_test.dart b/pkg/analysis_server/test/lsp/code_actions_refactor_test.dart index baf5eec14aa..ed71795bac9 100644 --- a/pkg/analysis_server/test/lsp/code_actions_refactor_test.dart +++ b/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 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 test_generatesNames() async { diff --git a/pkg/analysis_server/test/lsp/request_helpers_mixin.dart b/pkg/analysis_server/test/lsp/request_helpers_mixin.dart index ce31cf65e6c..94be420afec 100644 --- a/pkg/analysis_server/test/lsp/request_helpers_mixin.dart +++ b/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? 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 getCodeLens(Uri uri) { diff --git a/pkg/analysis_server/test/utils/lsp_protocol_extensions.dart b/pkg/analysis_server/test/utils/lsp_protocol_extensions.dart index 0248e86476e..5fabfbbd4b0 100644 --- a/pkg/analysis_server/test/utils/lsp_protocol_extensions.dart +++ b/pkg/analysis_server/test/utils/lsp_protocol_extensions.dart @@ -27,6 +27,22 @@ extension CodeActionExtensions on CodeAction { return map((literal) => literal.command, (command) => command); } + /// Whether this [CodeAction] is a [CodeActionLiteral]. + bool get isCodeActionLiteral { + return map( + (_) => true, // literal + (_) => false, // command + ); + } + + /// Whether this [CodeAction] is a [Command]. + bool get isCommand { + return map( + (_) => false, // literal + (_) => true, // command + ); + } + /// The title for this [CodeAction], whether it's a [CodeActionLiteral] /// or a [Command]. String get title {