Add LSP support for Extract Method refactor

Change-Id: Id5c9e0657648963d5f96469fbac9269dad4e32a7
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/106348
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Danny Tuppeny <dantup@google.com>
This commit is contained in:
Danny Tuppeny 2019-06-18 15:40:10 +00:00 committed by commit-bot@chromium.org
parent 48f19ef961
commit e32a179922
10 changed files with 339 additions and 156 deletions

View file

@ -43,10 +43,12 @@ abstract class Commands {
sortMembers,
organizeImports,
sendWorkspaceEdit,
performRefactor,
];
static const sortMembers = 'edit.sortMembers';
static const organizeImports = 'edit.organizeImports';
static const sendWorkspaceEdit = 'edit.sendWorkspaceEdit';
static const performRefactor = 'refactor.perform';
}
abstract class CustomMethods {
@ -84,6 +86,7 @@ abstract class ServerErrorCodes {
static const FileHasErrors = const ErrorCodes(-32008);
static const ClientFailedToApplyEdit = const ErrorCodes(-32009);
static const RenameNotValid = const ErrorCodes(-32010);
static const RefactorFailed = const ErrorCodes(-32011);
/// An error raised when the server detects that the server and client are out
/// of sync and cannot recover. For example if a textDocument/didChange notification

View file

@ -0,0 +1,104 @@
// Copyright (c) 2019, the Dart project authors. Please see the AUTHORS file
// 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_generated.dart';
import 'package:analysis_server/lsp_protocol/protocol_special.dart';
import 'package:analysis_server/src/lsp/constants.dart';
import 'package:analysis_server/src/lsp/handlers/commands/simple_edit_handler.dart';
import 'package:analysis_server/src/lsp/lsp_analysis_server.dart';
import 'package:analysis_server/src/lsp/mapping.dart';
import 'package:analysis_server/src/protocol_server.dart';
import 'package:analysis_server/src/services/refactoring/refactoring.dart';
import 'package:analyzer/dart/analysis/results.dart';
class PerformRefactorCommandHandler extends SimpleEditCommandHandler {
PerformRefactorCommandHandler(LspAnalysisServer server) : super(server);
@override
String get commandName => 'Perform Refactor';
@override
Future<ErrorOr<void>> handle(List<dynamic> arguments) async {
if (arguments == null ||
arguments.length != 6 ||
arguments[0] is! String || // kind
arguments[1] is! String || // path
(arguments[2] != null && arguments[2] is! int) || // docVersion
arguments[3] is! int || // offset
arguments[4] is! int || // length
// options
(arguments[5] != null && arguments[5] is! Map<String, dynamic>)) {
// length
return ErrorOr.error(new ResponseError(
ServerErrorCodes.InvalidCommandArguments,
'$commandName requires 6 parameters: RefactoringKind, docVersion, filePath, offset, length, options (optional)',
null,
));
}
String kind = arguments[0];
String path = arguments[1];
int docVersion = arguments[2];
int offset = arguments[3];
int length = arguments[4];
Map<String, dynamic> options = arguments[5];
final result = await requireResolvedUnit(path);
return result.mapResult((result) async {
return _getRefactoring(
RefactoringKind(kind), result, offset, length, options)
.mapResult((refactoring) async {
final status = await refactoring.checkAllConditions();
if (status.hasError) {
return error(ServerErrorCodes.RefactorFailed, status.message);
}
final change = await refactoring.createChange();
// If the file changed while we were validating and preparing the change,
// we should fail to avoid sending bad edits.
if (docVersion != null &&
docVersion != server.getVersionedDocumentIdentifier(path).version) {
return error(ErrorCodes.ContentModified,
'Content was modified before refactor was applied');
}
final edit = createWorkspaceEdit(server, change.edits);
return await sendWorkspaceEditToClient(edit);
});
});
}
ErrorOr<Refactoring> _getRefactoring(
RefactoringKind kind,
ResolvedUnitResult result,
int offset,
int length,
Map<String, dynamic> options,
) {
switch (kind) {
case RefactoringKind.EXTRACT_METHOD:
final refactor = ExtractMethodRefactoring(
server.searchEngine, result, offset, length);
// TODO(dantup): For now we don't have a good way to prompt the user
// for a method name so we just use a placeholder and expect them to
// rename (this is what C#/Omnisharp does), but there's an open request
// to handle this better.
// https://github.com/microsoft/language-server-protocol/issues/764
refactor.name =
(options != null ? options['name'] : null) ?? 'newMethod';
// Defaults to true, but may be surprising if users didn't have an option
// to opt in.
refactor.extractAll = false;
return success(refactor);
default:
return error(ServerErrorCodes.InvalidCommandArguments,
'Unknown RefactoringKind $kind was supplied to $commandName');
}
}
}

View file

@ -13,12 +13,14 @@ import 'package:analysis_server/src/lsp/constants.dart';
import 'package:analysis_server/src/lsp/handlers/handlers.dart';
import 'package:analysis_server/src/lsp/lsp_analysis_server.dart';
import 'package:analysis_server/src/lsp/mapping.dart';
import 'package:analysis_server/src/protocol_server.dart';
import 'package:analysis_server/src/services/correction/assist.dart';
import 'package:analysis_server/src/services/correction/assist_internal.dart';
import 'package:analysis_server/src/services/correction/change_workspace.dart';
import 'package:analysis_server/src/services/correction/fix.dart';
import 'package:analysis_server/src/services/correction/fix/dart/top_level_declarations.dart';
import 'package:analysis_server/src/services/correction/fix_internal.dart';
import 'package:analysis_server/src/services/refactoring/refactoring.dart';
import 'package:analyzer/dart/analysis/results.dart';
import 'package:analyzer/dart/analysis/session.dart'
show InconsistentAnalysisException;
@ -164,7 +166,7 @@ class CodeActionHandler extends MessageHandler<CodeActionParams,
_getSourceActions(
kinds, supportsLiterals, supportsWorkspaceApplyEdit, path),
_getAssistActions(kinds, supportsLiterals, offset, length, unit),
_getRefactorActions(kinds, supportsLiterals, path, range, unit),
_getRefactorActions(kinds, supportsLiterals, path, offset, length, unit),
_getFixActions(kinds, supportsLiterals, range, unit),
]);
final flatResults = results.expand((x) => x).toList();
@ -225,18 +227,56 @@ class CodeActionHandler extends MessageHandler<CodeActionParams,
HashSet<CodeActionKind> clientSupportedCodeActionKinds,
bool clientSupportsLiteralCodeActions,
String path,
Range range,
int offset,
int length,
ResolvedUnitResult unit,
) async {
// We only support these for clients that advertise codeActionLiteralSupport.
if (!clientSupportsLiteralCodeActions ||
// The refactor actions supported are only valid for Dart files.
if (!AnalysisEngine.isDartFileName(path)) {
return const [];
}
// If the client told us what kinds they support but it does not include
// Refactor then don't return any.
if (clientSupportsLiteralCodeActions &&
!clientSupportedCodeActionKinds.contains(CodeActionKind.Refactor)) {
return const [];
}
/// Helper to create refactors that execute commands provided with
/// the current file, location and document version.
createRefactor(
CodeActionKind actionKind,
String name,
RefactoringKind refactorKind, [
Map<String, dynamic> options,
]) {
return _commandOrCodeAction(
clientSupportsLiteralCodeActions,
actionKind,
new Command(name, Commands.performRefactor, [
refactorKind.toJson(),
path,
server.getVersionedDocumentIdentifier(path).version,
offset,
length,
options,
]));
}
try {
// TODO(dantup): Implement refactors.
return [];
final refactorActions = <Either2<Command, CodeAction>>[];
// Extract Method
if (ExtractMethodRefactoring(server.searchEngine, unit, offset, length)
.isAvailable()) {
refactorActions.add(createRefactor(CodeActionKind.RefactorExtract,
'Extract Method', RefactoringKind.EXTRACT_METHOD));
}
// TODO(dantup): Extract Widget
return refactorActions;
} on InconsistentAnalysisException {
// If an InconsistentAnalysisException occurs, it's likely the user modified
// the source and therefore is no longer interested in the results, so

View file

@ -8,6 +8,7 @@ import 'package:analysis_server/lsp_protocol/protocol_generated.dart';
import 'package:analysis_server/lsp_protocol/protocol_special.dart';
import 'package:analysis_server/src/lsp/constants.dart';
import 'package:analysis_server/src/lsp/handlers/commands/organize_imports.dart';
import 'package:analysis_server/src/lsp/handlers/commands/perform_refactor.dart';
import 'package:analysis_server/src/lsp/handlers/commands/send_workspace_edit.dart';
import 'package:analysis_server/src/lsp/handlers/commands/sort_members.dart';
import 'package:analysis_server/src/lsp/handlers/handlers.dart';
@ -22,6 +23,7 @@ class ExecuteCommandHandler
: commandHandlers = {
Commands.sortMembers: new SortMembersCommandHandler(server),
Commands.organizeImports: new OrganizeImportsCommandHandler(server),
Commands.performRefactor: new PerformRefactorCommandHandler(server),
Commands.sendWorkspaceEdit:
new SendWorkspaceEditCommandHandler(server),
},

View file

@ -105,8 +105,8 @@ class InitializeMessageHandler
null,
null,
true, // foldingRangeProvider
new ExecuteCommandOptions(Commands.serverSupportedCommands),
null, // declarationProvider
new ExecuteCommandOptions(Commands.serverSupportedCommands),
new ServerCapabilitiesWorkspace(
new ServerCapabilitiesWorkspaceFolders(true, true)),
null);

View file

@ -41,11 +41,14 @@ abstract class AbstractCodeActionsTest extends AbstractLspAnalysisServerTest {
}
Either2<Command, CodeAction> findCommand(
List<Either2<Command, CodeAction>> actions, String commandID) {
List<Either2<Command, CodeAction>> actions, String commandID,
[String wantedTitle]) {
for (var codeAction in actions) {
final id = codeAction.map(
(cmd) => cmd.command, (action) => action.command.command);
if (id == commandID) {
final title =
codeAction.map((cmd) => cmd.title, (action) => action.title);
if (id == commandID && (wantedTitle == null || wantedTitle == title)) {
return codeAction;
}
}
@ -67,4 +70,64 @@ abstract class AbstractCodeActionsTest extends AbstractLspAnalysisServerTest {
}
return null;
}
/// Verifies that executing the given code actions command on the server
/// results in an edit being sent in the client that updates the file to match
/// the expected content.
Future verifyCodeActionEdits(Either2<Command, CodeAction> codeAction,
String content, String expectedContent,
{bool expectDocumentChanges = false}) async {
final command = codeAction.map(
(command) => command,
(codeAction) => codeAction.command,
);
await verifyCommandEdits(command, content, expectedContent,
expectDocumentChanges: expectDocumentChanges);
}
/// Verifies that executing the given command on the server results in an edit
/// being sent in the client that updates the file to match the expected
/// content.
Future<void> verifyCommandEdits(
Command command, String content, String expectedContent,
{bool expectDocumentChanges = false}) async {
ApplyWorkspaceEditParams editParams;
final commandResponse = await handleExpectedRequest<Object,
ApplyWorkspaceEditParams, ApplyWorkspaceEditResponse>(
Method.workspace_applyEdit,
() => executeCommand(command),
handler: (edit) {
// When the server sends the edit back, just keep a copy and say we
// applied successfully (it'll be verified below).
editParams = edit;
return new ApplyWorkspaceEditResponse(true, null);
},
);
// Successful edits return an empty success() response.
expect(commandResponse, isNull);
// Ensure the edit came back, and using the expected changes.
expect(editParams, isNotNull);
if (expectDocumentChanges) {
expect(editParams.edit.changes, isNull);
expect(editParams.edit.documentChanges, isNotNull);
} else {
expect(editParams.edit.changes, isNotNull);
expect(editParams.edit.documentChanges, isNull);
}
// Ensure applying the changes will give us the expected content.
final contents = {
mainFilePath: withoutMarkers(content),
};
if (expectDocumentChanges) {
applyDocumentChanges(contents, editParams.edit.documentChanges);
} else {
applyChanges(contents, editParams.edit.changes);
}
expect(contents[mainFilePath], equals(expectedContent));
}
}

View file

@ -0,0 +1,64 @@
// Copyright (c) 2019, the Dart project authors. Please see the AUTHORS file
// 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 'code_actions_abstract.dart';
main() {
defineReflectiveSuite(() {
defineReflectiveTests(ExtractMethodRefactorCodeActionsTest);
});
}
@reflectiveTest
class ExtractMethodRefactorCodeActionsTest extends AbstractCodeActionsTest {
final extractMethodTitle = 'Extract Method';
test_appliesCorrectEdits() async {
const content = '''
main() {
print('Test!');
[[print('Test!');]]
}
''';
const expectedContent = '''
main() {
print('Test!');
newMethod();
}
void newMethod() {
print('Test!');
}
''';
await newFile(mainFilePath, content: withoutMarkers(content));
await initialize();
final codeActions = await getCodeActions(mainFileUri.toString(),
range: rangeFromMarkers(content));
final codeAction =
findCommand(codeActions, Commands.performRefactor, extractMethodTitle);
expect(codeAction, isNotNull);
await verifyCodeActionEdits(
codeAction, withoutMarkers(content), expectedContent);
}
test_invalidLocation() async {
const content = '''
import 'dart:convert';
^
main() {}
''';
await newFile(mainFilePath, content: content);
await initialize();
final codeActions = await getCodeActions(mainFileUri.toString());
final codeAction =
findCommand(codeActions, Commands.performRefactor, extractMethodTitle);
expect(codeAction, isNull);
}
}

View file

@ -44,38 +44,8 @@ int minified(int x, int y) => min(x, y);
final codeAction = findCommand(codeActions, Commands.organizeImports);
expect(codeAction, isNotNull);
final command = codeAction.map(
(command) => command,
(codeAction) => codeAction.command,
);
ApplyWorkspaceEditParams editParams;
final commandResponse = await handleExpectedRequest<Object,
ApplyWorkspaceEditParams, ApplyWorkspaceEditResponse>(
Method.workspace_applyEdit,
() => executeCommand(command),
handler: (edit) {
// When the server sends the edit back, just keep a copy and say we
// applied successfully (it'll be verified below).
editParams = edit;
return new ApplyWorkspaceEditResponse(true, null);
},
);
// Successful edits return an empty success() response.
expect(commandResponse, isNull);
// Ensure the edit came back, and using the documentChanges.
expect(editParams, isNotNull);
expect(editParams.edit.documentChanges, isNotNull);
expect(editParams.edit.changes, isNull);
// Ensure applying the changes will give us the expected content.
final contents = {
mainFilePath: withoutMarkers(content),
};
applyDocumentChanges(contents, editParams.edit.documentChanges);
expect(contents[mainFilePath], equals(expectedContent));
await verifyCodeActionEdits(codeAction, content, expectedContent,
expectDocumentChanges: true);
}
test_appliesCorrectEdits_withoutDocumentChangesSupport() async {
@ -103,38 +73,7 @@ int minified(int x, int y) => min(x, y);
final codeAction = findCommand(codeActions, Commands.organizeImports);
expect(codeAction, isNotNull);
final command = codeAction.map(
(command) => command,
(codeAction) => codeAction.command,
);
ApplyWorkspaceEditParams editParams;
final commandResponse = await handleExpectedRequest<Object,
ApplyWorkspaceEditParams, ApplyWorkspaceEditResponse>(
Method.workspace_applyEdit,
() => executeCommand(command),
handler: (edit) {
// When the server sends the edit back, just keep a copy and say we
// applied successfully (it'll be verified below).
editParams = edit;
return new ApplyWorkspaceEditResponse(true, null);
},
);
// Successful edits return an empty success() response.
expect(commandResponse, isNull);
// Ensure the edit came back, and using changes.
expect(editParams, isNotNull);
expect(editParams.edit.changes, isNotNull);
expect(editParams.edit.documentChanges, isNull);
// Ensure applying the changes will give us the expected content.
final contents = {
mainFilePath: withoutMarkers(content),
};
applyChanges(contents, editParams.edit.changes);
expect(contents[mainFilePath], equals(expectedContent));
await verifyCodeActionEdits(codeAction, content, expectedContent);
}
test_availableAsCodeActionLiteral() async {
@ -261,38 +200,8 @@ class SortMembersSourceCodeActionsTest extends AbstractCodeActionsTest {
final codeAction = findCommand(codeActions, Commands.sortMembers);
expect(codeAction, isNotNull);
final command = codeAction.map(
(command) => command,
(codeAction) => codeAction.command,
);
ApplyWorkspaceEditParams editParams;
final commandResponse = await handleExpectedRequest<Object,
ApplyWorkspaceEditParams, ApplyWorkspaceEditResponse>(
Method.workspace_applyEdit,
() => executeCommand(command),
handler: (edit) {
// When the server sends the edit back, just keep a copy and say we
// applied successfully (it'll be verified below).
editParams = edit;
return new ApplyWorkspaceEditResponse(true, null);
},
);
// Successful edits return an empty success() response.
expect(commandResponse, isNull);
// Ensure the edit came back, and using the documentChanges.
expect(editParams, isNotNull);
expect(editParams.edit.documentChanges, isNotNull);
expect(editParams.edit.changes, isNull);
// Ensure applying the changes will give us the expected content.
final contents = {
mainFilePath: withoutMarkers(content),
};
applyDocumentChanges(contents, editParams.edit.documentChanges);
expect(contents[mainFilePath], equals(expectedContent));
await verifyCodeActionEdits(codeAction, content, expectedContent,
expectDocumentChanges: true);
}
test_appliesCorrectEdits_withoutDocumentChangesSupport() async {
@ -313,38 +222,7 @@ class SortMembersSourceCodeActionsTest extends AbstractCodeActionsTest {
final codeAction = findCommand(codeActions, Commands.sortMembers);
expect(codeAction, isNotNull);
final command = codeAction.map(
(command) => command,
(codeAction) => codeAction.command,
);
ApplyWorkspaceEditParams editParams;
final commandResponse = await handleExpectedRequest<Object,
ApplyWorkspaceEditParams, ApplyWorkspaceEditResponse>(
Method.workspace_applyEdit,
() => executeCommand(command),
handler: (edit) {
// When the server sends the edit back, just keep a copy and say we
// applied successfully (it'll be verified below).
editParams = edit;
return new ApplyWorkspaceEditResponse(true, null);
},
);
// Successful edits return an empty success() response.
expect(commandResponse, isNull);
// Ensure the edit came back, and using changes.
expect(editParams, isNotNull);
expect(editParams.edit.changes, isNotNull);
expect(editParams.edit.documentChanges, isNull);
// Ensure applying the changes will give us the expected content.
final contents = {
mainFilePath: withoutMarkers(content),
};
applyChanges(contents, editParams.edit.changes);
expect(contents[mainFilePath], equals(expectedContent));
await verifyCodeActionEdits(codeAction, content, expectedContent);
}
test_availableAsCodeActionLiteral() async {

View file

@ -147,34 +147,51 @@ mixin LspAnalysisServerTestMixin implements ClientCapabilitiesHelperMixin {
// Complex text manipulations are described with an array of TextEdit's,
// representing a single change to the document.
//
// All text edits ranges refer to positions in the original document. Text
// All text edits ranges refer to positions in the original document. Text
// edits ranges must never overlap, that means no part of the original
// document must be manipulated by more than one edit. However, it is possible
// that multiple edits have the same start position: multiple inserts, or any
// number of inserts followed by a single remove or replace edit. If multiple
// inserts have the same position, the order in the array defines the order in
// which the inserted strings appear in the resulting text.
// document must be manipulated by more than one edit. It is possible
// that multiple edits have the same start position (eg. multiple inserts in
// reverse order), however since that involves complicated tracking and we
// only apply edits here sequentially, we don't supported them. We do sort
// edits to ensure we apply the later ones first, so we can assume the locations
// in the edit are still valid against the new string as each edit is applied.
/// Ensures changes are simple enough to apply easily without any complicated
/// logic.
void validateChangesCanBeApplied() {
bool intersectsWithOrComesAfter(Position pos, Position other) =>
pos.line > other.line ||
(pos.line == other.line || pos.character >= other.character);
/// Check if a position is before (but not equal) to another position.
bool isBefore(Position p, Position other) =>
p.line < other.line ||
(p.line == other.line && p.character < other.character);
Position earliestPositionChanged;
for (final change in changes) {
if (earliestPositionChanged != null &&
intersectsWithOrComesAfter(
change.range.end, earliestPositionChanged)) {
throw 'Test helper applyTextEdits does not support applying multiple edits '
'where the edits are not in reverse order.';
/// Check if a position is after (but not equal) to another position.
bool isAfter(Position p, Position other) =>
p.line > other.line ||
(p.line == other.line && p.character > other.character);
// Check if two ranges intersect or touch.
bool rangesIntersect(Range r1, Range r2) {
bool endsBefore = isBefore(r1.end, r2.start);
bool startsAfter = isAfter(r1.start, r2.end);
return !(endsBefore || startsAfter);
}
for (final change1 in changes) {
for (final change2 in changes) {
if (change1 != change2 &&
rangesIntersect(change1.range, change2.range)) {
throw 'Test helper applyTextEdits does not support applying multiple edits '
'where the edits are not in reverse order.';
}
}
earliestPositionChanged = change.range.start;
}
}
validateChangesCanBeApplied();
changes.sort(
(c1, c2) =>
positionCompare(c1.range.start, c2.range.start) *
-1, // Multiply by -1 to get descending sort.
);
for (final change in changes) {
newContent = applyTextEdit(newContent, change);
}
@ -623,6 +640,16 @@ mixin LspAnalysisServerTestMixin implements ClientCapabilitiesHelperMixin {
await pumpEventQueue();
}
int positionCompare(Position p1, Position p2) {
if (p1.line < p2.line) return -1;
if (p1.line > p2.line) return 1;
if (p1.character < p2.character) return -1;
if (p1.character > p2.character) return -1;
return 0;
}
Position positionFromMarker(String contents) =>
positionFromOffset(withoutRangeMarkers(contents).indexOf('^'), contents);

View file

@ -8,9 +8,10 @@ import '../src/lsp/lsp_packet_transformer_test.dart' as lsp_packet_transformer;
import 'analyzer_status_test.dart' as analyzer_status;
import 'cancel_request_test.dart' as cancel_request;
import 'change_workspace_folders_test.dart' as change_workspace_folders;
import 'code_actions_assists_test.dart' as code_actions_assists;
import 'closing_labels_test.dart' as closing_labels;
import 'code_actions_assists_test.dart' as code_actions_assists;
import 'code_actions_fixes_test.dart' as code_actions_fixes;
import 'code_actions_refactor_test.dart' as code_actions_refactor;
import 'code_actions_source_test.dart' as code_actions_source;
import 'completion_test.dart' as completion;
import 'definition_test.dart' as definition;
@ -20,7 +21,6 @@ import 'document_symbols_test.dart' as document_symbols;
import 'file_modification_test.dart' as file_modification;
import 'folding_test.dart' as folding;
import 'format_test.dart' as format;
import 'super_test.dart' as get_super;
import 'hover_test.dart' as hover;
import 'implementation_test.dart' as implementation;
import 'initialization_test.dart' as initialization;
@ -30,6 +30,7 @@ import 'references_test.dart' as references;
import 'rename_test.dart' as rename;
import 'server_test.dart' as server;
import 'signature_help_test.dart' as signature_help;
import 'super_test.dart' as get_super;
import 'workspace_symbols_test.dart' as workspace_symbols;
main() {
@ -41,6 +42,7 @@ main() {
code_actions_assists.main();
code_actions_fixes.main();
code_actions_source.main();
code_actions_refactor.main();
completion.main();
definition.main();
diagnostic.main();