[analysis_server] Support cancellation + file modification rejection for new refactors For the legacy refactors, we would cancel an in-progress refactor if another refactor started, and we would fail the refactor (don't send edits back to the client) if the initial file was modified while we were computing. This adds support for the same to the new self-described refactors (the tests were added to "Add Constructor Name", but the mechanism is general). Change-Id: I2e637808e0ad9a06da55a8156226b68020d4778e Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/563523 Reviewed-by: Samuel Rawlins <srawlins@google.com> Reviewed-by: Keerti Parthasarathy <keertip@google.com> Commit-Queue: Keerti Parthasarathy <keertip@google.com>
diff --git a/pkg/analysis_server/lib/src/lsp/handlers/commands/abstract_refactor.dart b/pkg/analysis_server/lib/src/lsp/handlers/commands/abstract_refactor.dart index 2282593..63d0158 100644 --- a/pkg/analysis_server/lib/src/lsp/handlers/commands/abstract_refactor.dart +++ b/pkg/analysis_server/lib/src/lsp/handlers/commands/abstract_refactor.dart
@@ -18,8 +18,6 @@ import 'package:analyzer/dart/analysis/results.dart'; import 'package:analyzer/dart/element/element.dart'; -final _manager = LspRefactorManager._(); - /// A base class for the legacy refactoring commands (EXTRACT_WIDGET, etc.) that /// need to create [Refactoring]s from client-supplied arguments. abstract class AbstractRefactorCommandHandler @@ -30,7 +28,9 @@ @override String get commandName => 'Perform Refactor'; - LspRefactorManager get manager => _manager; + /// The (singleton) [LspRefactorManager] used for cancelling refactors when + /// another is started. + LspRefactorManager get manager => LspRefactorManager.instance; FutureOr<ErrorOr<void>> execute( MessageInfo message, @@ -251,6 +251,10 @@ /// Manages a running refactor to help ensure only one refactor runs at a time. class LspRefactorManager { + /// A singleton [LspRefactorManager] used for cancelling refactors when + /// another is started. + static final instance = LspRefactorManager._(); + /// The cancellation token for the current in-progress refactor (or null). CancelableToken? _currentRefactoringCancellationToken;
diff --git a/pkg/analysis_server/lib/src/lsp/handlers/commands/refactor_command_executor.dart b/pkg/analysis_server/lib/src/lsp/handlers/commands/refactor_command_executor.dart index 1725921..4d16ad4 100644 --- a/pkg/analysis_server/lib/src/lsp/handlers/commands/refactor_command_executor.dart +++ b/pkg/analysis_server/lib/src/lsp/handlers/commands/refactor_command_executor.dart
@@ -7,6 +7,7 @@ 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/commands/abstract_refactor.dart'; import 'package:analysis_server/src/lsp/handlers/commands/refactor_command_handler_mixin.dart'; import 'package:analysis_server/src/lsp/handlers/commands/simple_edit_handler.dart'; import 'package:analysis_server/src/lsp/mapping.dart'; @@ -31,6 +32,10 @@ new(super.server, this.commandName, this.generator); + /// The (singleton) [LspRefactorManager] used for cancelling refactors when + /// another is started. + LspRefactorManager get manager => LspRefactorManager.instance; + @override bool get requiresTrustedCaller => false; @@ -43,7 +48,14 @@ RefactoringContext context, List<Object?> arguments, CancellationToken cancellationToken, + int? docVersion, ) async { + // If the token we were given is not cancelable, wrap it with one that + // is for the rest of this request as a future refactor may need to + // cancel this request. + var cancelableToken = cancellationToken.asCancelable(); + manager.begin(cancelableToken); + try { // ignore: unawaited_futures progress.begin('Refactoring…'); @@ -53,9 +65,11 @@ clientCapabilities, context, arguments, - cancellationToken, + cancelableToken, + docVersion, ); } finally { + manager.end(cancelableToken); // ignore: unawaited_futures progress.end(); } @@ -70,6 +84,7 @@ RefactoringContext context, List<Object?> arguments, CancellationToken cancellationToken, + int? docVersion, ) async { var producer = generator(context); var builder = ChangeBuilder( @@ -78,6 +93,10 @@ ); var status = await producer.compute(arguments, builder); + if (cancellationToken.isCancellationRequested) { + return cancelled(cancellationToken); + } + if (status is ComputeStatusFailure) { var reason = status.reason ?? 'Cannot compute the change. No details.'; return ErrorOr.error( @@ -110,6 +129,10 @@ UserPromptActions.cancel, ], cancellationToken); + if (cancellationToken.isCancellationRequested) { + return cancelled(cancellationToken); + } + // Unless they choose to refactor anyway, abort. if (userChoice != UserPromptActions.refactorAnyway) { return success(null); @@ -121,6 +144,12 @@ return success(null); } + // If the file changed while we were validating and preparing the change, + // we should fail to avoid sending bad edits. + if (fileHasBeenModified(unit.path, docVersion)) { + return fileModifiedError; + } + var fileEdits = <FileEditInformation>[]; for (var edit in edits) { var path = edit.file;
diff --git a/pkg/analysis_server/lib/src/lsp/handlers/commands/refactor_command_handler_mixin.dart b/pkg/analysis_server/lib/src/lsp/handlers/commands/refactor_command_handler_mixin.dart index b2ccd1f..dd7cf67 100644 --- a/pkg/analysis_server/lib/src/lsp/handlers/commands/refactor_command_handler_mixin.dart +++ b/pkg/analysis_server/lib/src/lsp/handlers/commands/refactor_command_handler_mixin.dart
@@ -15,10 +15,16 @@ import 'package:analyzer/dart/analysis/results.dart'; import 'package:language_server_protocol/protocol_custom_generated.dart'; import 'package:language_server_protocol/protocol_generated.dart'; +import 'package:meta/meta.dart'; /// A mixin with functionality common to handlers for refactor commands, /// such as [RefactorCommandResolver] and [RefactorCommandExecutor]. mixin RefactorCommandHandlerMixin<T> on HandlerHelperMixin, Handler<T> { + /// A [Future] used by tests to allow inserting a delay between resolving + /// the initial library and the refactor running. + @visibleForTesting + static Future<void>? delayAfterResolveForTests; + Future<ErrorOr<T>> execute( ProgressReporter progress, ResolvedLibraryResult library, @@ -27,6 +33,7 @@ RefactoringContext context, List<Object?> arguments, CancellationToken cancellationToken, + int? docVersion, ); Future<ErrorOr<T>> handle( @@ -64,7 +71,17 @@ ); } + var docVersion = server.getDocumentVersion(filePath); var library = await requireResolvedLibrary(filePath); + if (delayAfterResolveForTests != null) { + await delayAfterResolveForTests; + } + if (cancellationToken.isCancellationRequested) { + return cancelled(cancellationToken); + } + if (fileHasBeenModified(filePath, docVersion)) { + return fileModifiedError; + } return library.mapResult((library) async { var unit = library.unitWithPath(filePath); if (unit == null) { @@ -94,6 +111,7 @@ context, arguments, cancellationToken, + docVersion, ); }); }
diff --git a/pkg/analysis_server/lib/src/lsp/handlers/commands/refactor_command_resolver.dart b/pkg/analysis_server/lib/src/lsp/handlers/commands/refactor_command_resolver.dart index bed5eca..0bcbc8a 100644 --- a/pkg/analysis_server/lib/src/lsp/handlers/commands/refactor_command_resolver.dart +++ b/pkg/analysis_server/lib/src/lsp/handlers/commands/refactor_command_resolver.dart
@@ -44,6 +44,7 @@ RefactoringContext context, List<Object?> arguments, CancellationToken cancellationToken, + int? docVersion, ) async { var producer = generator(context);
diff --git a/pkg/analysis_server/lib/src/scheduler/message_scheduler.dart b/pkg/analysis_server/lib/src/scheduler/message_scheduler.dart index 2c5f046..532c8f0 100644 --- a/pkg/analysis_server/lib/src/scheduler/message_scheduler.dart +++ b/pkg/analysis_server/lib/src/scheduler/message_scheduler.dart
@@ -15,6 +15,7 @@ import 'package:analysis_server/src/lsp/lsp_analysis_server.dart'; import 'package:analysis_server/src/scheduler/scheduled_message.dart'; import 'package:analysis_server/src/server/error_notifier.dart'; +import 'package:analysis_server/src/services/refactoring/framework/refactoring_processor.dart'; import 'package:analyzer/src/utilities/cancellation.dart'; import 'package:language_server_protocol/json_parsing.dart'; @@ -397,7 +398,8 @@ if (message.method == lsp.Method.workspace_executeCommand) { lsp.ExecuteCommandParams? params; params = _getCommandParams(message); - if (params?.command == Commands.performRefactor) { + if (params?.command == Commands.performRefactor || + RefactoringProcessor.generators.containsKey(params?.command)) { return true; } } @@ -457,7 +459,10 @@ if (args.length == 6) { path = args[1] as String?; } else if (args.length == 1 && args[0] is Map<String, Object?>) { - path = (args.single as Map<String, Object?>)['path'] as String?; + var argMap = args.single as Map<String, Object?>; + // The original legacy refactors used `path`, but the newer + // self-describing refactors use `filePath`. + path = (argMap['path'] ?? argMap['filePath']) as String?; } return path != null ? Uri.file(path) : null; @@ -470,7 +475,10 @@ var request = lspMessage.message as lsp.RequestMessage; var execParams = _getCommandParams(request); if (execParams != null && - execParams.command == Commands.performRefactor) { + (execParams.command == Commands.performRefactor || + RefactoringProcessor.generators.containsKey( + execParams.command, + ))) { var args = execParams.arguments ?? []; var refactorUri = getRefactorUri(args); if (refactorUri == documentChangeUri) {
diff --git a/pkg/analysis_server/test/src/services/refactoring/add_constructor_name_test.dart b/pkg/analysis_server/test/src/services/refactoring/add_constructor_name_test.dart index 02e2718..d3b66f3 100644 --- a/pkg/analysis_server/test/src/services/refactoring/add_constructor_name_test.dart +++ b/pkg/analysis_server/test/src/services/refactoring/add_constructor_name_test.dart
@@ -2,9 +2,12 @@ // 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 'dart:async'; + import 'package:analysis_server/lsp_protocol/protocol.dart'; import 'package:analysis_server/src/lsp/constants.dart'; import 'package:analysis_server/src/lsp/extensions/code_action.dart'; +import 'package:analysis_server/src/lsp/handlers/commands/refactor_command_handler_mixin.dart'; import 'package:analysis_server/src/services/refactoring/add_constructor_name.dart'; import 'package:test/test.dart'; import 'package:test_reflective_loader/test_reflective_loader.dart'; @@ -25,6 +28,96 @@ @reflectiveTest class AddConstructorNameInClassTest extends _AddConstructorNameTest with InteractiveFormsTestMixin { + Future<void> test_cancelsInProgress() async { + var originalSource = ''' +class C^() {} + +void f() { + C(); +} +'''; + var expected = ''' +>>>>>>>>>> lib/main.dart +class C.name() {} + +void f() { + C.name(); +} +'''; + + addTestSource(originalSource); + + await initializeServer(); + var action = await expectCodeActionWithTitle(refactoringTitle); + + // Respond to any applyEdit requests from the server with successful + // responses and capturing the last edit. + late WorkspaceEdit edit; + requestsFromServer.listen((request) { + if (request.method == Method.workspace_applyEdit) { + var params = ApplyWorkspaceEditParams.fromJson( + request.params as Map<String, Object?>, + ); + edit = params.edit; + respondTo(request, ApplyWorkspaceEditResult(applied: true)); + } + }); + + // Send two requests together. + var req1 = executeCommand(action.command!); + var req2 = executeCommand(action.command!); + + // Expect the first will have been cancelled by the second. + await expectLater( + req1, + throwsA( + isResponseError( + ErrorCodes.RequestCancelled, + message: 'Another workspace/executeCommand request for a refactor was started', + ), + ), + ); + await req2; + + // Ensure applying the changes will give us the expected content. + verifyEdit(edit, expected); + } + + Future<void> test_contentModified() async { + var originalSource = ''' +class C^() {} + +void f() { + C(); +} +'''; + addTestSource(originalSource); + + await initializeServer(); + await openFile(mainFileUri, testCode.code); + var action = await expectCodeActionWithTitle(refactoringTitle); + + // Use a Completer to control when the refactor handler starts computing. + var completer = Completer<void>(); + RefactorCommandHandlerMixin.delayAfterResolveForTests = completer.future; + try { + // Send an edit request immediately after the refactor request. + var req1 = executeCommand(action.command!); + var req2 = replaceFile(100, mainFileUri, '// new test content'); + completer.complete(); + + // Expect the first to fail because of the modified content. + await expectLater( + req1, + throwsA(isResponseError(ErrorCodes.ContentModified)), + ); + await req2; + } finally { + // Ensure we never leave an incomplete future if anything above throws. + RefactorCommandHandlerMixin.delayAfterResolveForTests = null; + } + } + Future<void> test_inBody_factory_named_onKeyword() async { var originalSource = ''' class C {
diff --git a/pkg/analysis_server/test/src/services/refactoring/refactoring_test_support.dart b/pkg/analysis_server/test/src/services/refactoring/refactoring_test_support.dart index 7a94948..df0bc02 100644 --- a/pkg/analysis_server/test/src/services/refactoring/refactoring_test_support.dart +++ b/pkg/analysis_server/test/src/services/refactoring/refactoring_test_support.dart
@@ -23,12 +23,15 @@ /// The range of characters that were selected. Range? _range; + /// The parsed test source code. + late TestCode testCode; + /// Return the command ID of the refactoring command that is expected to be /// available. String get refactoringCommandId; void addTestSource(String markedCode) { - var testCode = TestCode.parse(markedCode); + testCode = TestCode.parse(markedCode); var positions = testCode.positions; if (positions.isNotEmpty) { _position = positions[0].position;