[analysis_server] Support LSP executeCommand over DTD to support code actions
There are a few things in this CL that were difficult to separate testing of (because they're all part of the same feature):
1. sets `requiresTrustedCaller=false` on the `executeCommand` handler so that commands can be called over DTD. It adds the same flag to the command handlers themselves, so a command can control whether it can be called over DTD or not (right now we allow everything except `logAction` and `sendWorkspaceEdit` which are both commands used internally and not appropriate for DTD clients to call).
2. Removes the allow-list on DTD methods, allowing all LSP shared methods to be available over DTD
3. Extends the integration test classes to support reverse-requests so we can verify the edits being sent back to the editor when calling the code actions commands over DTD
4. It also fixes a few bugs where we read the callers capabilities instead of the editors capabilities (which until now would always be the same in those places, but with this change are not).
Change-Id: I6d271ddad6dc1b00a98b10b735763a368c91af7a
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/428784
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Commit-Queue: Brian Wilkerson <brianwilkerson@google.com>
diff --git a/pkg/analysis_server/lib/src/handler/legacy/lsp_over_legacy_handler.dart b/pkg/analysis_server/lib/src/handler/legacy/lsp_over_legacy_handler.dart
index c279c6e8..a1b1302 100644
--- a/pkg/analysis_server/lib/src/handler/legacy/lsp_over_legacy_handler.dart
+++ b/pkg/analysis_server/lib/src/handler/legacy/lsp_over_legacy_handler.dart
@@ -69,6 +69,7 @@
performance: performance,
clientCapabilities: server.editorClientCapabilities,
timeSinceRequest: request.timeSinceRequest,
+ isTrustedCaller: true,
);
ErrorOr<Object?> result;
diff --git a/pkg/analysis_server/lib/src/lsp/handlers/commands/apply_code_action.dart b/pkg/analysis_server/lib/src/lsp/handlers/commands/apply_code_action.dart
index 4aa5f83..d57ba3a 100644
--- a/pkg/analysis_server/lib/src/lsp/handlers/commands/apply_code_action.dart
+++ b/pkg/analysis_server/lib/src/lsp/handlers/commands/apply_code_action.dart
@@ -34,6 +34,9 @@
bool get recordsOwnAnalytics => true;
@override
+ bool get requiresTrustedCaller => false;
+
+ @override
Future<ErrorOr<void>> handle(
MessageInfo message,
Map<String, Object?> parameters,
diff --git a/pkg/analysis_server/lib/src/lsp/handlers/commands/fix_all.dart b/pkg/analysis_server/lib/src/lsp/handlers/commands/fix_all.dart
index d25243a..2483a69 100644
--- a/pkg/analysis_server/lib/src/lsp/handlers/commands/fix_all.dart
+++ b/pkg/analysis_server/lib/src/lsp/handlers/commands/fix_all.dart
@@ -23,6 +23,9 @@
String get commandName => 'Fix All';
@override
+ bool get requiresTrustedCaller => false;
+
+ @override
Future<ErrorOr<void>> handle(
MessageInfo message,
Map<String, Object?> parameters,
diff --git a/pkg/analysis_server/lib/src/lsp/handlers/commands/fix_all_in_workspace.dart b/pkg/analysis_server/lib/src/lsp/handlers/commands/fix_all_in_workspace.dart
index bf19556..70708c6 100644
--- a/pkg/analysis_server/lib/src/lsp/handlers/commands/fix_all_in_workspace.dart
+++ b/pkg/analysis_server/lib/src/lsp/handlers/commands/fix_all_in_workspace.dart
@@ -25,6 +25,9 @@
bool get requireConfirmation;
@override
+ bool get requiresTrustedCaller => false;
+
+ @override
Future<ErrorOr<void>> handle(
MessageInfo message,
Map<String, Object?> parameters,
diff --git a/pkg/analysis_server/lib/src/lsp/handlers/commands/log_action.dart b/pkg/analysis_server/lib/src/lsp/handlers/commands/log_action.dart
index 15aadae..be82642 100644
--- a/pkg/analysis_server/lib/src/lsp/handlers/commands/log_action.dart
+++ b/pkg/analysis_server/lib/src/lsp/handlers/commands/log_action.dart
@@ -16,6 +16,10 @@
bool get recordsOwnAnalytics => true;
@override
+ // We only currently expect this to be called by the editor.
+ bool get requiresTrustedCaller => true;
+
+ @override
Future<ErrorOr<void>> handle(
MessageInfo message,
Map<String, Object?> parameters,
diff --git a/pkg/analysis_server/lib/src/lsp/handlers/commands/organize_imports.dart b/pkg/analysis_server/lib/src/lsp/handlers/commands/organize_imports.dart
index 9f8e557..85967cb 100644
--- a/pkg/analysis_server/lib/src/lsp/handlers/commands/organize_imports.dart
+++ b/pkg/analysis_server/lib/src/lsp/handlers/commands/organize_imports.dart
@@ -17,6 +17,9 @@
String get commandName => 'Organize Imports';
@override
+ bool get requiresTrustedCaller => false;
+
+ @override
Future<ErrorOr<void>> handle(
MessageInfo message,
Map<String, Object?> parameters,
diff --git a/pkg/analysis_server/lib/src/lsp/handlers/commands/perform_refactor.dart b/pkg/analysis_server/lib/src/lsp/handlers/commands/perform_refactor.dart
index b461e59..47f3036 100644
--- a/pkg/analysis_server/lib/src/lsp/handlers/commands/perform_refactor.dart
+++ b/pkg/analysis_server/lib/src/lsp/handlers/commands/perform_refactor.dart
@@ -29,6 +29,9 @@
bool get recordsOwnAnalytics => true;
@override
+ bool get requiresTrustedCaller => false;
+
+ @override
FutureOr<ErrorOr<void>> execute(
String path,
String kind,
@@ -40,6 +43,12 @@
ProgressReporter reporter,
int? docVersion,
) async {
+ var editorCapabilities = server.editorClientCapabilities;
+ if (editorCapabilities == null) {
+ // This should not happen unless a client misbehaves.
+ return serverNotInitializedError;
+ }
+
var actionName = 'dart.refactor.${kind.toLowerCase()}';
server.analyticsManager.executedCommand(actionName);
@@ -102,7 +111,7 @@
return fileModifiedError;
}
- var edit = createWorkspaceEdit(server, clientCapabilities, change);
+ var edit = createWorkspaceEdit(server, editorCapabilities, change);
return await sendWorkspaceEditToClient(edit);
} finally {
manager.end(cancelableToken);
diff --git a/pkg/analysis_server/lib/src/lsp/handlers/commands/refactor_command_handler.dart b/pkg/analysis_server/lib/src/lsp/handlers/commands/refactor_command_handler.dart
index 7943201..6d08468 100644
--- a/pkg/analysis_server/lib/src/lsp/handlers/commands/refactor_command_handler.dart
+++ b/pkg/analysis_server/lib/src/lsp/handlers/commands/refactor_command_handler.dart
@@ -28,6 +28,9 @@
RefactorCommandHandler(super.server, this.commandName, this.generator);
@override
+ bool get requiresTrustedCaller => false;
+
+ @override
Future<ErrorOr<void>> handle(
MessageInfo message,
Map<String, Object?> parameters,
diff --git a/pkg/analysis_server/lib/src/lsp/handlers/commands/send_workspace_edit.dart b/pkg/analysis_server/lib/src/lsp/handlers/commands/send_workspace_edit.dart
index f2abb19..c5cd9d6 100644
--- a/pkg/analysis_server/lib/src/lsp/handlers/commands/send_workspace_edit.dart
+++ b/pkg/analysis_server/lib/src/lsp/handlers/commands/send_workspace_edit.dart
@@ -23,6 +23,11 @@
String get commandName => 'Send Workspace Edit';
@override
+ // This command is used internally to work around some limitations in LSP
+ // and is not expected to ever be called by a DTD client.
+ bool get requiresTrustedCaller => true;
+
+ @override
Future<ErrorOr<void>> handle(
MessageInfo message,
Map<String, Object?> parameters,
diff --git a/pkg/analysis_server/lib/src/lsp/handlers/commands/sort_members.dart b/pkg/analysis_server/lib/src/lsp/handlers/commands/sort_members.dart
index 1640f20..c441716 100644
--- a/pkg/analysis_server/lib/src/lsp/handlers/commands/sort_members.dart
+++ b/pkg/analysis_server/lib/src/lsp/handlers/commands/sort_members.dart
@@ -18,6 +18,9 @@
String get commandName => 'Sort Members';
@override
+ bool get requiresTrustedCaller => false;
+
+ @override
Future<ErrorOr<void>> handle(
MessageInfo message,
Map<String, Object?> parameters,
diff --git a/pkg/analysis_server/lib/src/lsp/handlers/commands/validate_refactor.dart b/pkg/analysis_server/lib/src/lsp/handlers/commands/validate_refactor.dart
index f66ece8..b3b2a76 100644
--- a/pkg/analysis_server/lib/src/lsp/handlers/commands/validate_refactor.dart
+++ b/pkg/analysis_server/lib/src/lsp/handlers/commands/validate_refactor.dart
@@ -23,6 +23,9 @@
bool get recordsOwnAnalytics => true;
@override
+ bool get requiresTrustedCaller => false;
+
+ @override
FutureOr<ErrorOr<ValidateRefactorResult>> execute(
String path,
String kind,
diff --git a/pkg/analysis_server/lib/src/lsp/handlers/handler_execute_command.dart b/pkg/analysis_server/lib/src/lsp/handlers/handler_execute_command.dart
index a61fc1f..6fb658e 100644
--- a/pkg/analysis_server/lib/src/lsp/handlers/handler_execute_command.dart
+++ b/pkg/analysis_server/lib/src/lsp/handlers/handler_execute_command.dart
@@ -2,7 +2,9 @@
// 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/lsp_protocol/protocol.dart';
+import 'dart:async';
+
+import 'package:analysis_server/lsp_protocol/protocol.dart' hide MessageType;
import 'package:analysis_server/src/analysis_server.dart';
import 'package:analysis_server/src/lsp/constants.dart';
import 'package:analysis_server/src/lsp/error_or.dart';
@@ -65,10 +67,9 @@
ExecuteCommandParams.jsonHandler;
@override
- // TODO(dantup): This will need to be relaxed to support calling over DTD,
- // but at that point we must also add this flag to Commands so that we can
- // control which commands require trusted callers.
- bool get requiresTrustedCaller => true;
+ /// This handler does not require a trusted caller, however some of the
+ /// commands might (which are checked on the handler during execution).
+ bool get requiresTrustedCaller => false;
@override
Future<ErrorOr<Object?>> handle(
@@ -84,6 +85,13 @@
);
}
+ if (handler.requiresTrustedCaller && !message.isTrustedCaller) {
+ return error(
+ ServerErrorCodes.UnknownCommand,
+ '${params.command} can only be called by the owning process',
+ );
+ }
+
if (!handler.recordsOwnAnalytics) {
server.analyticsManager.executedCommand(params.command);
}
diff --git a/pkg/analysis_server/lib/src/lsp/handlers/handler_will_rename_files.dart b/pkg/analysis_server/lib/src/lsp/handlers/handler_will_rename_files.dart
index e01c9cb..ff0c3cc 100644
--- a/pkg/analysis_server/lib/src/lsp/handlers/handler_will_rename_files.dart
+++ b/pkg/analysis_server/lib/src/lsp/handlers/handler_will_rename_files.dart
@@ -39,8 +39,10 @@
MessageInfo message,
CancellationToken token,
) async {
- var clientCapabilities = message.clientCapabilities;
- if (clientCapabilities == null) {
+ // Use editor capabilities because this is used to compute to edits to send
+ // to the client.
+ var editorCapabilities = server.editorClientCapabilities;
+ if (editorCapabilities == null) {
return serverNotInitializedError;
}
@@ -61,11 +63,11 @@
pathMapping[oldPath] = newPath;
});
}
- return _renameFiles(clientCapabilities, pathMapping, token);
+ return _renameFiles(editorCapabilities, pathMapping, token);
}
Future<ErrorOr<WorkspaceEdit?>> _renameFiles(
- LspClientCapabilities clientCapabilities,
+ LspClientCapabilities editorCapabilities,
Map<String, String> renames,
CancellationToken token,
) async {
@@ -103,7 +105,7 @@
server.checkConsistency(sessions);
- var edit = createWorkspaceEdit(server, clientCapabilities, change);
+ var edit = createWorkspaceEdit(server, editorCapabilities, change);
return success(edit);
}
}
diff --git a/pkg/analysis_server/lib/src/lsp/handlers/handlers.dart b/pkg/analysis_server/lib/src/lsp/handlers/handlers.dart
index 03b5011..bde6b5f 100644
--- a/pkg/analysis_server/lib/src/lsp/handlers/handlers.dart
+++ b/pkg/analysis_server/lib/src/lsp/handlers/handlers.dart
@@ -50,6 +50,10 @@
/// can record a more specific command name.
bool get recordsOwnAnalytics => false;
+ /// Whether or not this handler can only be called by the owner of the
+ /// analysis server process (for example the editor).
+ bool get requiresTrustedCaller;
+
Future<ErrorOr<Object?>> handle(
MessageInfo message,
Map<String, Object?> parameters,
@@ -455,6 +459,10 @@
/// shown.
final Completer<void>? completer;
+ /// Whether or not this message came from the the owner of the analysis server
+ /// process (for example the editor).
+ final bool isTrustedCaller;
+
MessageInfo({
required this.performance,
// TODO(dantup): Consider a version of this that has a non-nullable
@@ -462,6 +470,7 @@
// is non-null and reject the request. Only a small number of handlers need
// to run without, so it would remove a bunch of boilerplate in the others.
required this.clientCapabilities,
+ required this.isTrustedCaller,
this.timeSinceRequest,
this.completer,
});
diff --git a/pkg/analysis_server/lib/src/lsp/lsp_analysis_server.dart b/pkg/analysis_server/lib/src/lsp/lsp_analysis_server.dart
index 26cb7a4..4dacdac 100644
--- a/pkg/analysis_server/lib/src/lsp/lsp_analysis_server.dart
+++ b/pkg/analysis_server/lib/src/lsp/lsp_analysis_server.dart
@@ -509,6 +509,7 @@
clientCapabilities: editorClientCapabilities,
timeSinceRequest: message.timeSinceRequest,
completer: completer,
+ isTrustedCaller: true,
);
if (message is RequestMessage) {
diff --git a/pkg/analysis_server/lib/src/services/dart_tooling_daemon/dtd_services.dart b/pkg/analysis_server/lib/src/services/dart_tooling_daemon/dtd_services.dart
index bc06431..7a3ed52 100644
--- a/pkg/analysis_server/lib/src/services/dart_tooling_daemon/dtd_services.dart
+++ b/pkg/analysis_server/lib/src/services/dart_tooling_daemon/dtd_services.dart
@@ -7,7 +7,6 @@
import 'package:analysis_server/lsp_protocol/protocol.dart';
import 'package:analysis_server/src/analysis_server.dart';
import 'package:analysis_server/src/lsp/client_capabilities.dart';
-import 'package:analysis_server/src/lsp/constants.dart';
import 'package:analysis_server/src/lsp/error_or.dart';
import 'package:analysis_server/src/lsp/handlers/handler_states.dart';
import 'package:analysis_server/src/lsp/handlers/handlers.dart';
@@ -36,21 +35,6 @@
/// A connection to DTD that exposes some analysis services (such as a subset
/// of LSP) to other DTD clients.
class DtdServices {
- /// The set of LSP methods currently exposed over DTD.
- ///
- /// Eventually all "shared" methods will be exposed, but during initial
- /// development and testing this will be restricted to selected methods (in
- /// particular, those with well defined results that are not affected by
- /// differences in client capabilities).
- static final allowedLspMethods = <Method>{
- // TODO(dantup): Remove this allow list so that all shared methods are
- // available over DTD.
- CustomMethods.experimentalEcho,
- CustomMethods.dartTextDocumentEditableArguments,
- CustomMethods.dartTextDocumentEditArgument,
- Method.textDocument_hover,
- };
-
/// The name of the DTD service that methods will be registered under.
static const _lspServiceName = 'Lsp';
@@ -92,6 +76,7 @@
// capabilities so that the responses don't change in format based on the
// owning editor.
clientCapabilities: fixedBasicLspClientCapabilities,
+ isTrustedCaller: false,
);
var token = NotCancelableToken(); // We don't currently support cancel.
@@ -230,8 +215,7 @@
) async {
await Future.wait([
for (var lspHandler in handler.messageHandlers.values)
- if (allowedLspMethods.contains(lspHandler.handlesMessage) &&
- (registerExperimentalHandlers || !lspHandler.isExperimental))
+ if (registerExperimentalHandlers || !lspHandler.isExperimental)
_registerLspService(lspHandler, dtd),
]);
@@ -240,7 +224,8 @@
await dtd.postEvent(_lspStreamName, 'initialized', {});
}
- /// Registers a single message handler to DTD if it allows untrusted callers.
+ /// Registers a single message handler to DTD only if it allows untrusted
+ /// callers.
Future<void> _registerLspService(
MessageHandler<Object?, Object?, AnalysisServer> messageHandler,
DartToolingDaemon dtd,
diff --git a/pkg/analysis_server/lib/src/services/user_prompts/dart_fix_prompt_manager.dart b/pkg/analysis_server/lib/src/services/user_prompts/dart_fix_prompt_manager.dart
index 57cff2a..814d523 100644
--- a/pkg/analysis_server/lib/src/services/user_prompts/dart_fix_prompt_manager.dart
+++ b/pkg/analysis_server/lib/src/services/user_prompts/dart_fix_prompt_manager.dart
@@ -256,6 +256,7 @@
MessageInfo(
performance: OperationPerformanceImpl(''),
clientCapabilities: clientCapabilities,
+ isTrustedCaller: true,
),
NotCancelableToken(),
);
diff --git a/pkg/analysis_server/test/integration/dtd/dtd_test.dart b/pkg/analysis_server/test/integration/dtd/dtd_test.dart
index 82a4c97..cbf4ebf 100644
--- a/pkg/analysis_server/test/integration/dtd/dtd_test.dart
+++ b/pkg/analysis_server/test/integration/dtd/dtd_test.dart
@@ -33,6 +33,10 @@
Future<void> initializeServer() async {
await standardAnalysisSetup();
await analysisFinished;
+ await sendServerSetClientCapabilities(
+ [],
+ lspCapabilities: editorClientCapabilities.raw,
+ );
}
@override
diff --git a/pkg/analysis_server/test/integration/lsp/abstract_lsp_over_legacy.dart b/pkg/analysis_server/test/integration/lsp/abstract_lsp_over_legacy.dart
index 963facd..eeb87ee 100644
--- a/pkg/analysis_server/test/integration/lsp/abstract_lsp_over_legacy.dart
+++ b/pkg/analysis_server/test/integration/lsp/abstract_lsp_over_legacy.dart
@@ -3,22 +3,38 @@
// BSD-style license that can be found in the LICENSE file.
import 'dart:async';
+import 'dart:io';
import 'package:analysis_server/lsp_protocol/protocol.dart';
+import 'package:analysis_server/protocol/protocol_constants.dart';
+import 'package:analysis_server/src/protocol_server.dart';
import 'package:analyzer_plugin/src/utilities/client_uri_converter.dart';
+import 'package:path/path.dart';
import 'package:test/test.dart';
import '../../lsp/request_helpers_mixin.dart';
+import '../../lsp/server_abstract.dart';
import '../support/integration_tests.dart';
abstract class AbstractLspOverLegacyTest
extends AbstractAnalysisServerIntegrationTest
with
+ ClientCapabilitiesHelperMixin,
LspRequestHelpersMixin,
LspReverseRequestHelpersMixin,
LspNotificationHelpersMixin,
- LspEditHelpersMixin {
- late final testFile = sourcePath('lib/test.dart');
+ LspEditHelpersMixin,
+ LspVerifyEditHelpersMixin {
+ late final testFile = sourcePath('lib/main.dart');
+
+ // TODO(dantup): Support this for LSP-over-Legacy shared tests, possibly by
+ // providing an implementaion of SharedTestInterface for (both kinds of)
+ // integration tests.
+ var failTestOnErrorDiagnostic = false;
+
+ /// Tracks the current overlay content so that when we apply edits they can
+ /// be applied in the same way a real client would apply them.
+ final _overlayContent = <String, String>{};
/// A stream of LSP [NotificationMessage]s from the server.
@override
@@ -30,8 +46,20 @@
);
@override
- Stream<RequestMessage> get requestsFromServer =>
- throw 'Reverse-requests not currently supported for LSP-over-Legacy integration tests';
+ String get projectFolderPath => sourceDirectory.path;
+
+ @override
+ Stream<RequestMessage> get requestsFromServer => serverToClientRequests
+ .where((request) => request.method == LSP_REQUEST_HANDLE)
+ .map((request) {
+ var params = LspHandleParams.fromRequest(
+ request,
+ clientUriConverter: uriConverter,
+ );
+ return RequestMessage.fromJson(
+ params.lspMessage as Map<String, Object?>,
+ );
+ });
/// The URI for the macro-generated content for [testFileUri].
Uri get testFileMacroUri =>
@@ -71,6 +99,43 @@
}
@override
+ String? getCurrentFileContent(Uri uri) {
+ var filePath = fromUri(uri);
+ // First try and overlay the test has set.
+ if (_overlayContent.containsKey(filePath)) {
+ return _overlayContent[filePath];
+ }
+
+ // Otherwise fall back to the disk.
+ try {
+ return File(filePath).readAsStringSync();
+ } catch (_) {
+ return null;
+ }
+ }
+
+ @override
+ Future<AnalysisUpdateContentResult> sendAnalysisUpdateContent(
+ Map<String, Object> files,
+ ) {
+ for (var MapEntry(key: filePath, value: content) in files.entries) {
+ switch (content) {
+ case AddContentOverlay(:var content):
+ _overlayContent[filePath] = content;
+ case ChangeContentOverlay(:var edits):
+ _overlayContent[filePath] = SourceEdit.applySequence(
+ _overlayContent[filePath]!,
+ edits,
+ );
+ case RemoveContentOverlay():
+ _overlayContent.remove(filePath);
+ }
+ }
+
+ return super.sendAnalysisUpdateContent(files);
+ }
+
+ @override
Future<ResponseMessage> sendRequestToServer(RequestMessage message) async {
var legacyResult = await sendLspHandle(message.toJson());
var lspResponseJson = legacyResult.lspResponse as Map<String, Object?>;
@@ -81,6 +146,17 @@
@override
void sendResponseToServer(ResponseMessage response) {
- throw 'Reverse-requests not currently supported for LSP-over-Legacy integration tests';
+ server.sendRaw(
+ Response(
+ // Convert the LSP int-or-string ID to always a string for legacy.
+ response.id!.map((i) => i.toString(), (s) => s),
+ // A client-provided response to an LSP reverse-request is always
+ // a full LSP result payload as the "result". The legacy request should
+ // always succeed and any errors handled as LSP error responses within.
+ result: LspHandleResult(
+ response,
+ ).toJson(clientUriConverter: uriConverter),
+ ).toJson(),
+ );
}
}
diff --git a/pkg/analysis_server/test/integration/lsp/handle_test.dart b/pkg/analysis_server/test/integration/lsp/handle_test.dart
index 1f511c7..aab8122 100644
--- a/pkg/analysis_server/test/integration/lsp/handle_test.dart
+++ b/pkg/analysis_server/test/integration/lsp/handle_test.dart
@@ -94,7 +94,7 @@
```dart
class Aaa
```
-*package:test/test.dart*
+*package:test/main.dart*
---
This is my class.
@@ -116,7 +116,7 @@
```dart
class Aaa
```
-*package:test/test.dart*
+*package:test/main.dart*
---
This is my class.''';
diff --git a/pkg/analysis_server/test/integration/lsp_server/integration_tests.dart b/pkg/analysis_server/test/integration/lsp_server/integration_tests.dart
index 0bb3866..8331681 100644
--- a/pkg/analysis_server/test/integration/lsp_server/integration_tests.dart
+++ b/pkg/analysis_server/test/integration/lsp_server/integration_tests.dart
@@ -11,9 +11,11 @@
import 'package:analysis_server/src/services/pub/pub_command.dart';
import 'package:analyzer/file_system/physical_file_system.dart';
import 'package:analyzer/instrumentation/instrumentation.dart';
+import 'package:analyzer_plugin/src/utilities/client_uri_converter.dart';
import 'package:meta/meta.dart';
-import 'package:path/path.dart';
+import 'package:path/path.dart' as path;
+import '../../constants.dart';
import '../../lsp/request_helpers_mixin.dart';
import '../../lsp/server_abstract.dart';
import '../../support/sdk_paths.dart';
@@ -25,12 +27,18 @@
LspReverseRequestHelpersMixin,
LspNotificationHelpersMixin,
LspEditHelpersMixin,
+ LspVerifyEditHelpersMixin,
LspAnalysisServerTestMixin {
final List<String> vmArgs = [];
LspServerClient? client;
InstrumentationService? instrumentationService;
final Map<num, Completer<ResponseMessage>> _completers = {};
- String dartSdkPath = dirname(dirname(Platform.resolvedExecutable));
+ String dartSdkPath = path.dirname(path.dirname(Platform.resolvedExecutable));
+
+ @override
+ late final ClientUriConverter uriConverter = ClientUriConverter.noop(
+ pathContext,
+ );
/// Tracks the current overlay content so that when we apply edits they can
/// be applied in the same way a real client would apply them.
@@ -39,7 +47,7 @@
LspByteStreamServerChannel get channel => client!.channel!;
@override
- Context get pathContext => PhysicalResourceProvider.INSTANCE.pathContext;
+ path.Context get pathContext => PhysicalResourceProvider.INSTANCE.pathContext;
@override
Stream<Message> get serverToClient => client!.serverToClient;
@@ -132,9 +140,9 @@
.createTempSync('analysisServer')
.resolveSymbolicLinksSync();
newFolder(projectFolderPath);
- newFolder(join(projectFolderPath, 'lib'));
- mainFilePath = join(projectFolderPath, 'lib', 'main.dart');
- analysisOptionsPath = join(projectFolderPath, 'analysis_options.yaml');
+ newFolder(path.join(projectFolderPath, 'lib'));
+ mainFilePath = path.join(projectFolderPath, 'lib', 'main.dart');
+ analysisOptionsPath = path.join(projectFolderPath, 'analysis_options.yaml');
var client = LspServerClient(instrumentationService);
this.client = client;
@@ -184,7 +192,7 @@
Future<int> get exitCode => _process!.exitCode;
- Context get pathContext => PhysicalResourceProvider.INSTANCE.pathContext;
+ path.Context get pathContext => PhysicalResourceProvider.INSTANCE.pathContext;
Stream<Message> get serverToClient => _serverToClient.stream;
@@ -201,7 +209,7 @@
throw Exception('Process already started');
}
- var dartBinary = join(dartSdkPath, 'bin', 'dart');
+ var dartBinary = path.join(dartSdkPath, 'bin', 'dart');
var serverPath = getAnalysisServerPath(dartSdkPath);
var arguments = [...?vmArgs, serverPath, '--lsp', '--suppress-analytics'];
@@ -227,11 +235,12 @@
});
var inputStream = _extractDevToolsLine(process.stdout);
+ var outputStream = process.stdin;
channel = LspByteStreamServerChannel(
inputStream,
- process.stdin,
- instrumentationService ?? InstrumentationService.NULL_SERVICE,
+ outputStream,
+ instrumentationService ?? InstrumentationLogAdapter(PrintableLogger()),
)..listen(_serverToClient.add);
}
@@ -268,7 +277,7 @@
/// An [InstrumentationLogger] that buffers logs until [debugStdio()] is called.
class PrintableLogger extends InstrumentationLogger {
- bool _printLogs = false;
+ bool _printLogs = debugPrintCommunication;
final _buffer = StringBuffer();
void debugStdio() {
diff --git a/pkg/analysis_server/test/integration/support/integration_test_methods.dart b/pkg/analysis_server/test/integration/support/integration_test_methods.dart
index d47e33b..3d4ed28 100644
--- a/pkg/analysis_server/test/integration/support/integration_test_methods.dart
+++ b/pkg/analysis_server/test/integration/support/integration_test_methods.dart
@@ -15,6 +15,7 @@
import 'package:analysis_server/src/protocol/protocol_internal.dart';
import 'package:analyzer_plugin/src/utilities/client_uri_converter.dart';
import 'package:analyzer_plugin/protocol/protocol_common.dart';
+import 'package:path/path.dart' as path;
import 'package:test/test.dart';
import 'integration_tests.dart';
@@ -25,7 +26,7 @@
Server get server;
/// The converter used to convert between URI/Paths in server communication.
- ClientUriConverter? uriConverter;
+ final ClientUriConverter uriConverter = ClientUriConverter.noop(path.context);
/// Return the version number of the analysis server.
///
diff --git a/pkg/analysis_server/test/integration/support/integration_tests.dart b/pkg/analysis_server/test/integration/support/integration_tests.dart
index b0fb602..f2af29a 100644
--- a/pkg/analysis_server/test/integration/support/integration_tests.dart
+++ b/pkg/analysis_server/test/integration/support/integration_tests.dart
@@ -8,17 +8,17 @@
import 'dart:io';
import 'package:analysis_server/protocol/protocol_constants.dart';
-import 'package:analysis_server/protocol/protocol_generated.dart';
+import 'package:analysis_server/src/protocol_server.dart';
import 'package:analysis_server/src/services/pub/pub_command.dart';
import 'package:analyzer/file_system/physical_file_system.dart';
import 'package:analyzer/src/util/file_paths.dart' as file_paths;
-import 'package:analyzer_plugin/protocol/protocol_common.dart';
import 'package:analyzer_testing/mock_packages/mock_packages.dart';
import 'package:analyzer_utilities/testing/test_support.dart';
import 'package:meta/meta.dart';
import 'package:path/path.dart' as path;
import 'package:test/test.dart';
+import '../../constants.dart';
import '../../support/configuration_files.dart';
import '../../support/sdk_paths.dart';
import 'integration_test_methods.dart';
@@ -102,6 +102,9 @@
typedef NotificationProcessor =
void Function(String event, Map<Object?, Object?> params);
+/// Type of callbacks used to process reverse-requests.
+typedef ReverseRequestProcessor = void Function(Request request);
+
/// Base class for analysis server integration tests.
abstract class AbstractAnalysisServerIntegrationTest extends IntegrationTest
with MockPackagesMixin, ConfigurationFilesMixin {
@@ -140,6 +143,9 @@
String dartSdkPath = path.dirname(path.dirname(Platform.resolvedExecutable));
+ StreamController<Request> serverToClientRequestsController =
+ StreamController<Request>.broadcast();
+
/// Return a future which will complete when a 'server.status' notification is
/// received from the server with 'analyzing' set to false.
///
@@ -166,6 +172,9 @@
@override
String get packagesRootPath => packagesDirectory.path;
+ Stream<Request> get serverToClientRequests =>
+ serverToClientRequestsController.stream;
+
@override
String get testPackageRootPath => sourceDirectory.path;
@@ -175,6 +184,10 @@
server.debugStdio();
}
+ void dispatchReverseRequest(Request request) {
+ serverToClientRequestsController.add(request);
+ }
+
/// If there was a set of errors (might be empty) received for the file
/// with the given [path], return it. If no errors - fail.
List<AnalysisError> existingErrorsForFile(String path) {
@@ -232,7 +245,7 @@
fail('${params.message}\n${params.stackTrace}');
});
await startServer();
- server.listenToOutput(dispatchNotification);
+ server.listenToOutput(dispatchNotification, dispatchReverseRequest);
unawaited(
server.exitCode.then((_) {
skipShutdown = true;
@@ -512,7 +525,7 @@
final List<String> _recordedStdio = <String>[];
/// True if we are currently printing out messages exchanged with the server.
- bool _debuggingStdio = false;
+ bool _debuggingStdio = debugPrintCommunication;
/// True if we've received bad data from the server, and we are aborting the
/// test.
@@ -558,8 +571,19 @@
}
/// Start listening to output from the server, and deliver notifications to
- /// [notificationProcessor].
- void listenToOutput(NotificationProcessor notificationProcessor) {
+ /// [notificationProcessor] and reverse requests to [reverseRequestProcessor].
+ void listenToOutput(
+ NotificationProcessor notificationProcessor, [
+ ReverseRequestProcessor? reverseRequestProcessor,
+ ]) {
+ // Provide a default implementation of the reverse request processor that
+ // just throws because there are many tests that don't use reverse-requests.
+ reverseRequestProcessor ??=
+ (_) =>
+ throw UnimplementedError(
+ "A reverse request was received but the test did not provide 'reverseRequestProcessor'",
+ );
+
_process.stdout.transform(utf8.decoder).transform(LineSplitter()).listen((
String line,
) {
@@ -596,7 +620,17 @@
return;
}
outOfTestExpect(message, isMap);
- if (message.containsKey('id')) {
+ if (message.containsKey('id') && message.containsKey('method')) {
+ // Message is a reverse-request from the server.
+ outOfTestExpect(message['id'], isString);
+ var id = message['id'] as String;
+ outOfTestExpect(message['method'], isString);
+ var method = message['method'] as String;
+ reverseRequestProcessor!(
+ Request(id, method, message['params'] as Map<String, Object?>?),
+ );
+ } else if (message.containsKey('id')) {
+ // Message is a response to an outstanding request.
outOfTestExpect(message['id'], isString);
var id = message['id'] as String;
var completer = _pendingCommands[id];
@@ -656,10 +690,15 @@
}
var completer = Completer<Map<String, Object?>?>();
_pendingCommands[id] = completer;
- var line = json.encode(command);
+ sendRaw(command);
+ return completer.future;
+ }
+
+ /// Send a raw object to the server.
+ void sendRaw(Map<String, Object?> data) {
+ var line = json.encode(data);
_recordStdio('==> $line');
_process.stdin.add(utf8.encode('$line\n'));
- return completer.future;
}
/// Start the server. If [profileServer] is `true`, the server will be started
diff --git a/pkg/analysis_server/test/lsp/request_helpers_mixin.dart b/pkg/analysis_server/test/lsp/request_helpers_mixin.dart
index 0394754..e7ed67c 100644
--- a/pkg/analysis_server/test/lsp/request_helpers_mixin.dart
+++ b/pkg/analysis_server/test/lsp/request_helpers_mixin.dart
@@ -1167,6 +1167,9 @@
mixin LspReverseRequestHelpersMixin {
/// A stream of reverse-requests from the server that can be responded to via
/// [sendResponseToServer].
+ ///
+ /// Only LSP message requests (`lsp.handle`) from the server are included
+ /// here.
Stream<RequestMessage> get requestsFromServer;
/// Expects a [method] request from the server after executing [f].
@@ -1280,7 +1283,14 @@
/// information about the project to build paths.
mixin LspVerifyEditHelpersMixin
on LspEditHelpersMixin, ClientCapabilitiesHelperMixin {
- LspClientCapabilities get editorClientCapabilities;
+ LspClientCapabilities get editorClientCapabilities => LspClientCapabilities(
+ ClientCapabilities(
+ workspace: workspaceCapabilities,
+ textDocument: textDocumentCapabilities,
+ window: windowCapabilities,
+ experimental: experimentalCapabilities,
+ ),
+ );
path.Context get pathContext;
diff --git a/pkg/analysis_server/test/lsp_over_legacy/abstract_lsp_over_legacy.dart b/pkg/analysis_server/test/lsp_over_legacy/abstract_lsp_over_legacy.dart
index 568726c..cb15a8d 100644
--- a/pkg/analysis_server/test/lsp_over_legacy/abstract_lsp_over_legacy.dart
+++ b/pkg/analysis_server/test/lsp_over_legacy/abstract_lsp_over_legacy.dart
@@ -203,10 +203,6 @@
Uri get pubspecFileUri => toUri(convertPath(pubspecFilePath));
- /// A stream of [RequestMessage]s from the server.
- ///
- /// Only LSP message requests (`lsp.handle`) from the server are included
- /// here.
@override
Stream<RequestMessage> get requestsFromServer => serverChannel
.serverToClientRequests
diff --git a/pkg/analysis_server/test/shared/shared_dtd_tests.dart b/pkg/analysis_server/test/shared/shared_dtd_tests.dart
index 32578b8..b9ac0a5 100644
--- a/pkg/analysis_server/test/shared/shared_dtd_tests.dart
+++ b/pkg/analysis_server/test/shared/shared_dtd_tests.dart
@@ -10,12 +10,12 @@
import 'package:dtd/dtd.dart';
import 'package:json_rpc_2/json_rpc_2.dart';
import 'package:test/test.dart';
-import 'package:test_reflective_loader/test_reflective_loader.dart';
import '../integration/support/dart_tooling_daemon.dart';
import '../integration/support/web_sockets.dart';
import '../lsp/request_helpers_mixin.dart';
import '../tool/lsp_spec/matchers.dart';
+import '../utils/lsp_protocol_extensions.dart';
import '../utils/test_code_extensions.dart';
/// The name of the DTD service that LSP methods are registered against.
@@ -37,7 +37,15 @@
T Function(R) fromJson,
) async {
var response = await sendRequestToServer(request);
- return fromJson(response.result as R);
+ var error = response.error;
+ if (error != null) {
+ throw error;
+ } else {
+ // response.result should only be null when error != null if T allows null.
+ return response.result == null
+ ? null as T
+ : fromJson(response.result as R);
+ }
}
@override
@@ -59,7 +67,8 @@
/// Shared DTD tests that are used by both LSP and legacy server integration
/// tests.
-mixin SharedDtdTests on LspRequestHelpersMixin {
+mixin SharedDtdTests
+ on LspRequestHelpersMixin, LspEditHelpersMixin, LspVerifyEditHelpersMixin {
/// The name of the DTD service that methods will be registered under.
/// The `dart tooling-daemon` process we've spawned to connect to.
@@ -84,6 +93,9 @@
/// An invalid DTD URI used for testing connection failures.
final invalidUri = Uri.parse('ws://invalid:345/invalid');
+ // TODO(dantup): Support this for LSP-over-Legacy shared tests.
+ set failTestOnErrorDiagnostic(bool value);
+
/// Overridden by test subclasses to provide the path of a file for testing.
String get testFile;
@@ -93,6 +105,44 @@
/// Overridden by test subclasses to create a new file.
void createFile(String path, String content);
+ /// Sets up a file with [code] and expects/returns the [Command] code action
+ /// with [title].
+ Future<Command> expectCommandCodeAction(TestCode code, String title) async {
+ createFile(testFile, code.code);
+ await initializeServer();
+ await sendConnectToDtdRequest();
+
+ // Ensure the codeAction service is available.
+ expectMethod(Method.textDocument_codeAction);
+
+ // Fetch code actions at the marked location.
+ var actions = await dtd.getCodeActions(
+ testFileUri,
+ range: code.range.range,
+ );
+
+ // Ensure all returned actions are Commands (not CodeActionLiterals).
+ var commands = actions.map((action) => action.asCommand).toList();
+
+ // Find the one with the matching title.
+ expect(commands.map((command) => command.title), contains(title));
+ return commands.singleWhere((command) => command.title == title);
+ }
+
+ Future<void> expectedCommandCodeActionEdits(
+ TestCode code,
+ String title,
+ String expected,
+ ) async {
+ var command = await expectCommandCodeAction(code, title);
+
+ // Invoke the command over DTD, expecting edits to be sent back to us
+ // (not over DTD).
+ var verifier = await executeForEdits(() => dtd.executeCommand(command));
+
+ verifier.verifyFiles(expected);
+ }
+
void expectMethod(Method method, {bool available = true}) {
if (available) {
expect(availableMethods, contains((lspServiceName, method)));
@@ -291,7 +341,6 @@
expectMethod(CustomMethods.experimentalEcho);
}
- @SkippedTest(reason: 'Not all shared LSP methods are enabled yet')
Future<void> test_connectToDtd_success_registers_standardLspMethods() async {
await initializeServer();
await sendConnectToDtdRequest();
@@ -304,6 +353,84 @@
expectMethod(Method.textDocument_documentColor);
}
+ Future<void> test_service_codeAction_assist() async {
+ setApplyEditSupport();
+
+ var code = TestCode.parse('''
+var a = [!''!];
+''');
+
+ var title = 'Convert to double quoted string';
+ var expected = r'''
+>>>>>>>>>> lib/main.dart
+var a = "";
+''';
+
+ await expectedCommandCodeActionEdits(code, title, expected);
+ }
+
+ Future<void> test_service_codeAction_fix() async {
+ failTestOnErrorDiagnostic = false;
+ setApplyEditSupport();
+
+ var code = TestCode.parse('''
+Future<void> [!f!]() {}
+''');
+
+ var title = "Add 'async' modifier";
+ var expected = r'''
+>>>>>>>>>> lib/main.dart
+Future<void> f() async {}
+''';
+
+ await expectedCommandCodeActionEdits(code, title, expected);
+ }
+
+ Future<void> test_service_codeAction_refactor() async {
+ setApplyEditSupport();
+
+ var code = TestCode.parse('''
+void f() {
+ [!print('');!]
+}
+''');
+
+ var title = 'Extract Method';
+ var expected = r'''
+>>>>>>>>>> lib/main.dart
+void f() {
+ newMethod();
+}
+
+void newMethod() {
+ print('');
+}
+''';
+
+ await expectedCommandCodeActionEdits(code, title, expected);
+ }
+
+ Future<void> test_service_codeAction_source() async {
+ setApplyEditSupport();
+
+ var code = TestCode.parse('''
+[!!]import 'dart:async';
+import 'dart:io';
+
+FutureOr<void>? a;
+''');
+
+ var title = 'Organize Imports';
+ var expected = r'''
+>>>>>>>>>> lib/main.dart
+import 'dart:async';
+
+FutureOr<void>? a;
+''';
+
+ await expectedCommandCodeActionEdits(code, title, expected);
+ }
+
Future<void> test_service_failure_hover() async {
await initializeServer();
await sendConnectToDtdRequest();
diff --git a/pkg/analysis_server/tool/spec/codegen_inttest_methods.dart b/pkg/analysis_server/tool/spec/codegen_inttest_methods.dart
index 1862ea1..6c45d42 100644
--- a/pkg/analysis_server/tool/spec/codegen_inttest_methods.dart
+++ b/pkg/analysis_server/tool/spec/codegen_inttest_methods.dart
@@ -94,6 +94,7 @@
write(uri);
writeln("';");
}
+ writeln("import 'package:path/path.dart' as path;");
writeln("import 'package:test/test.dart';");
writeln();
writeln("import 'integration_tests.dart';");
@@ -107,7 +108,9 @@
writeln(
'/// The converter used to convert between URI/Paths in server communication.',
);
- writeln('ClientUriConverter? uriConverter;');
+ writeln(
+ 'final ClientUriConverter uriConverter = ClientUriConverter.noop(path.context);',
+ );
super.visitApi();
writeln();
docComment(
