Skip to content

Commit 7088dc4

Browse files
DanTupCommit Queue
DanTup
authored and
Commit Queue
committed
[analysis_server] Ensure progress notifications are sent for new interactive refactors
Fixes Dart-Code/Dart-Code#5235 Change-Id: I538f62631b5dc972cb3395ccbaf8a24d6accb5d2 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/389161 Reviewed-by: Brian Wilkerson <[email protected]> Reviewed-by: Samuel Rawlins <[email protected]> Commit-Queue: Brian Wilkerson <[email protected]>
1 parent 254b166 commit 7088dc4

File tree

4 files changed

+188
-84
lines changed

4 files changed

+188
-84
lines changed

‎pkg/analysis_server/lib/src/lsp/handlers/commands/refactor_command_handler.dart‎

Lines changed: 64 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
// BSD-style license that can be found in the LICENSE file.
44

55
import 'package:analysis_server/lsp_protocol/protocol.dart';
6+
import 'package:analysis_server/src/lsp/client_capabilities.dart';
67
import 'package:analysis_server/src/lsp/constants.dart';
78
import 'package:analysis_server/src/lsp/error_or.dart';
89
import 'package:analysis_server/src/lsp/handlers/commands/simple_edit_handler.dart';
@@ -64,54 +65,73 @@ class RefactorCommandHandler extends SimpleEditCommandHandler {
6465
return error(ErrorCodes.InternalError,
6566
'The library containing a path did not contain the path.');
6667
}
67-
var context = RefactoringContext(
68-
server: server,
69-
startSessions: await server.currentSessions,
70-
resolvedLibraryResult: library,
71-
resolvedUnitResult: unit,
72-
clientCapabilities: clientCapabilities,
73-
selectionOffset: offset,
74-
selectionLength: length,
75-
includeExperimental:
76-
server.lspClientConfiguration.global.experimentalRefactors,
77-
);
78-
var producer = generator(context);
79-
var builder = ChangeBuilder(
80-
workspace: context.workspace, eol: context.utils.endOfLine);
81-
var status = await producer.compute(arguments, builder);
82-
83-
if (status is ComputeStatusFailure) {
84-
var reason = status.reason ?? 'Cannot compute the change. No details.';
85-
return ErrorOr.error(
86-
ResponseError(
87-
code: ServerErrorCodes.RefactoringComputeStatusFailure,
88-
message: reason,
89-
),
90-
);
68+
try {
69+
progress.begin('Refactoring…');
70+
return await _performRefactor(
71+
library, unit, clientCapabilities, offset, length, arguments);
72+
} finally {
73+
progress.end();
9174
}
75+
});
76+
}
9277

93-
var edits = builder.sourceChange.edits;
94-
if (edits.isEmpty) {
95-
return success(null);
96-
}
78+
/// Performs the refactor, including sending the edits to the client and
79+
/// waiting for them to be applied.
80+
Future<ErrorOr<void>> _performRefactor(
81+
ResolvedLibraryResult library,
82+
ResolvedUnitResult unit,
83+
LspClientCapabilities clientCapabilities,
84+
int offset,
85+
int length,
86+
List<Object?> arguments,
87+
) async {
88+
var context = RefactoringContext(
89+
server: server,
90+
startSessions: await server.currentSessions,
91+
resolvedLibraryResult: library,
92+
resolvedUnitResult: unit,
93+
clientCapabilities: clientCapabilities,
94+
selectionOffset: offset,
95+
selectionLength: length,
96+
includeExperimental:
97+
server.lspClientConfiguration.global.experimentalRefactors,
98+
);
99+
var producer = generator(context);
100+
var builder = ChangeBuilder(
101+
workspace: context.workspace, eol: context.utils.endOfLine);
102+
var status = await producer.compute(arguments, builder);
97103

98-
var fileEdits = <FileEditInformation>[];
99-
for (var edit in edits) {
100-
var path = edit.file;
101-
var fileResult = context.session.getFile(path);
102-
if (fileResult is! FileResult) {
103-
return ErrorOr.error(ResponseError(
104-
code: ServerErrorCodes.FileAnalysisFailed,
105-
message: 'Could not access "$path".'));
106-
}
107-
var docIdentifier = server.getVersionedDocumentIdentifier(path);
108-
fileEdits.add(FileEditInformation(
109-
docIdentifier, fileResult.lineInfo, edit.edits,
110-
newFile: edit.fileStamp == -1));
104+
if (status is ComputeStatusFailure) {
105+
var reason = status.reason ?? 'Cannot compute the change. No details.';
106+
return ErrorOr.error(
107+
ResponseError(
108+
code: ServerErrorCodes.RefactoringComputeStatusFailure,
109+
message: reason,
110+
),
111+
);
112+
}
113+
114+
var edits = builder.sourceChange.edits;
115+
if (edits.isEmpty) {
116+
return success(null);
117+
}
118+
119+
var fileEdits = <FileEditInformation>[];
120+
for (var edit in edits) {
121+
var path = edit.file;
122+
var fileResult = context.session.getFile(path);
123+
if (fileResult is! FileResult) {
124+
return ErrorOr.error(ResponseError(
125+
code: ServerErrorCodes.FileAnalysisFailed,
126+
message: 'Could not access "$path".'));
111127
}
112-
var workspaceEdit = toWorkspaceEdit(clientCapabilities, fileEdits);
113-
return sendWorkspaceEditToClient(workspaceEdit);
114-
});
128+
var docIdentifier = server.getVersionedDocumentIdentifier(path);
129+
fileEdits.add(FileEditInformation(
130+
docIdentifier, fileResult.lineInfo, edit.edits,
131+
newFile: edit.fileStamp == -1));
132+
}
133+
var workspaceEdit = toWorkspaceEdit(clientCapabilities, fileEdits);
134+
return sendWorkspaceEditToClient(workspaceEdit);
115135
}
116136

117137
/// If the [arguments] is a list, then return it. Otherwise, return `null`

‎pkg/analysis_server/test/lsp/code_actions_refactor_test.dart‎

Lines changed: 5 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -8,13 +8,13 @@ import 'package:analysis_server/lsp_protocol/protocol.dart';
88
import 'package:analysis_server/src/lsp/constants.dart';
99
import 'package:analysis_server/src/lsp/handlers/commands/perform_refactor.dart';
1010
import 'package:analyzer/src/test_utilities/test_code_format.dart';
11-
import 'package:language_server_protocol/json_parsing.dart';
1211
import 'package:test/test.dart';
1312
import 'package:test_reflective_loader/test_reflective_loader.dart';
1413

1514
import '../tool/lsp_spec/matchers.dart';
1615
import '../utils/test_code_extensions.dart';
1716
import 'code_actions_abstract.dart';
17+
import 'request_helpers_mixin.dart';
1818

1919
void main() {
2020
defineReflectiveSuite(() {
@@ -113,43 +113,10 @@ void f() {
113113
}
114114

115115
@reflectiveTest
116-
class ExtractMethodRefactorCodeActionsTest extends RefactorCodeActionsTest {
116+
class ExtractMethodRefactorCodeActionsTest extends RefactorCodeActionsTest
117+
with LspProgressNotificationsMixin {
117118
final extractMethodTitle = 'Extract Method';
118119

119-
/// A stream of strings (CREATE, BEGIN, END) corresponding to progress
120-
/// requests and notifications for convenience in testing.
121-
///
122-
/// Analyzing statuses are not included.
123-
Stream<String> get progressUpdates {
124-
var controller = StreamController<String>();
125-
126-
requestsFromServer
127-
.where((r) => r.method == Method.window_workDoneProgress_create)
128-
.listen((request) async {
129-
var params = WorkDoneProgressCreateParams.fromJson(
130-
request.params as Map<String, Object?>);
131-
if (params.token != analyzingProgressToken) {
132-
controller.add('CREATE');
133-
}
134-
}, onDone: controller.close);
135-
notificationsFromServer
136-
.where((n) => n.method == Method.progress)
137-
.listen((notification) {
138-
var params =
139-
ProgressParams.fromJson(notification.params as Map<String, Object?>);
140-
if (params.token != analyzingProgressToken) {
141-
if (WorkDoneProgressBegin.canParse(params.value, nullLspJsonReporter)) {
142-
controller.add('BEGIN');
143-
} else if (WorkDoneProgressEnd.canParse(
144-
params.value, nullLspJsonReporter)) {
145-
controller.add('END');
146-
}
147-
}
148-
});
149-
150-
return controller.stream;
151-
}
152-
153120
Future<void> test_appliesCorrectEdits() async {
154121
const content = '''
155122
void f() {
@@ -460,7 +427,8 @@ void newMethod() {
460427
}
461428
''';
462429

463-
// Ensure the progress messages come through and in the correct order.
430+
// Expect create/begin/end progress updates, because in this case the server
431+
// generates the token.
464432
expect(progressUpdates, emitsInOrder(['CREATE', 'BEGIN', 'END']));
465433

466434
setWorkDoneProgressSupport();

‎pkg/analysis_server/test/lsp/request_helpers_mixin.dart‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,46 @@ mixin LspEditHelpersMixin {
8787
}
8888
}
8989

90+
mixin LspProgressNotificationsMixin {
91+
Stream<NotificationMessage> get notificationsFromServer;
92+
93+
/// A stream of strings (CREATE, BEGIN, END) corresponding to progress
94+
/// requests and notifications for convenience in testing.
95+
///
96+
/// Analyzing statuses are not included.
97+
Stream<String> get progressUpdates {
98+
var controller = StreamController<String>();
99+
100+
requestsFromServer
101+
.where((r) => r.method == Method.window_workDoneProgress_create)
102+
.listen((request) async {
103+
var params = WorkDoneProgressCreateParams.fromJson(
104+
request.params as Map<String, Object?>);
105+
if (params.token != analyzingProgressToken) {
106+
controller.add('CREATE');
107+
}
108+
}, onDone: controller.close);
109+
notificationsFromServer
110+
.where((n) => n.method == Method.progress)
111+
.listen((notification) {
112+
var params =
113+
ProgressParams.fromJson(notification.params as Map<String, Object?>);
114+
if (params.token != analyzingProgressToken) {
115+
if (WorkDoneProgressBegin.canParse(params.value, nullLspJsonReporter)) {
116+
controller.add('BEGIN');
117+
} else if (WorkDoneProgressEnd.canParse(
118+
params.value, nullLspJsonReporter)) {
119+
controller.add('END');
120+
}
121+
}
122+
});
123+
124+
return controller.stream;
125+
}
126+
127+
Stream<RequestMessage> get requestsFromServer;
128+
}
129+
90130
/// Helpers to simplify building LSP requests for use in tests.
91131
///
92132
/// The actual sending of requests must be supplied by the implementing class

‎pkg/analysis_server/test/src/services/refactoring/move_top_level_to_file_test.dart‎

Lines changed: 79 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,8 +5,10 @@
55
import 'package:analysis_server/lsp_protocol/protocol.dart';
66
import 'package:analysis_server/src/services/refactoring/move_top_level_to_file.dart';
77
import 'package:analyzer/src/test_utilities/test_code_format.dart';
8+
import 'package:test/test.dart';
89
import 'package:test_reflective_loader/test_reflective_loader.dart';
910

11+
import '../../../lsp/request_helpers_mixin.dart';
1012
import 'refactoring_test_support.dart';
1113

1214
void main() {
@@ -16,7 +18,8 @@ void main() {
1618
}
1719

1820
@reflectiveTest
19-
class MoveTopLevelToFileTest extends RefactoringTest {
21+
class MoveTopLevelToFileTest extends RefactoringTest
22+
with LspProgressNotificationsMixin {
2023
/// Simple file content with a single class named 'A'.
2124
static const simpleClassContent = '''
2225
class ^A {}
@@ -1282,6 +1285,72 @@ import 'package:test/setter.dart';
12821285
);
12831286
}
12841287

1288+
Future<void> test_progress_clientProvided() async {
1289+
var originalSource = 'class A^ {}';
1290+
var declarationName = 'A';
1291+
1292+
var expected = '''
1293+
>>>>>>>>>> lib/a.dart created
1294+
class A {}<<<<<<<<<<
1295+
>>>>>>>>>> lib/main.dart empty
1296+
''';
1297+
1298+
// Expect begin/end progress updates without a create, since the
1299+
// token was supplied by us (the client).
1300+
expect(progressUpdates, emitsInOrder(['BEGIN', 'END']));
1301+
1302+
await _singleDeclaration(
1303+
originalSource: originalSource,
1304+
expected: expected,
1305+
declarationName: declarationName,
1306+
commandWorkDoneToken: clientProvidedTestWorkDoneToken,
1307+
);
1308+
}
1309+
1310+
Future<void> test_progress_notSupported() async {
1311+
var originalSource = 'class A^ {}';
1312+
var declarationName = 'A';
1313+
1314+
var expected = '''
1315+
>>>>>>>>>> lib/a.dart created
1316+
class A {}<<<<<<<<<<
1317+
>>>>>>>>>> lib/main.dart empty
1318+
''';
1319+
1320+
var didGetProgressNotifications = false;
1321+
progressUpdates.listen((_) => didGetProgressNotifications = true);
1322+
1323+
await _singleDeclaration(
1324+
originalSource: originalSource,
1325+
expected: expected,
1326+
declarationName: declarationName,
1327+
);
1328+
1329+
expect(didGetProgressNotifications, isFalse);
1330+
}
1331+
1332+
Future<void> test_progress_serverGenerated() async {
1333+
var originalSource = 'class A^ {}';
1334+
var declarationName = 'A';
1335+
1336+
var expected = '''
1337+
>>>>>>>>>> lib/a.dart created
1338+
class A {}<<<<<<<<<<
1339+
>>>>>>>>>> lib/main.dart empty
1340+
''';
1341+
1342+
// Expect create/begin/end progress updates, because in this case the server
1343+
// generates the token.
1344+
expect(progressUpdates, emitsInOrder(['CREATE', 'BEGIN', 'END']));
1345+
1346+
setWorkDoneProgressSupport();
1347+
await _singleDeclaration(
1348+
originalSource: originalSource,
1349+
expected: expected,
1350+
declarationName: declarationName,
1351+
);
1352+
}
1353+
12851354
Future<void>
12861355
test_protocol_available_withClientCommandParameterSupport() async {
12871356
addTestSource(simpleClassContent);
@@ -1858,10 +1927,11 @@ class B {}
18581927
required String expected,
18591928
String? otherFilePath,
18601929
String? otherFileContent,
1930+
ProgressToken? commandWorkDoneToken,
18611931
}) async {
18621932
if (originalSource.contains('>>>>') ||
18631933
(otherFileContent?.contains('>>>>>') ?? false)) {
1864-
throw '>>>>>';
1934+
throw 'File content must not include >>>>>';
18651935
}
18661936
addTestSource(originalSource);
18671937
if (otherFilePath != null) {
@@ -1870,7 +1940,11 @@ class B {}
18701940

18711941
await initializeServer();
18721942
var action = await expectCodeAction(actionTitle);
1873-
await verifyCommandEdits(action.command!, expected);
1943+
await verifyCommandEdits(
1944+
action.command!,
1945+
expected,
1946+
workDoneToken: commandWorkDoneToken,
1947+
);
18741948
}
18751949

18761950
Future<void> _singleDeclaration({
@@ -1879,13 +1953,15 @@ class B {}
18791953
required String expected,
18801954
String? otherFilePath,
18811955
String? otherFileContent,
1956+
ProgressToken? commandWorkDoneToken,
18821957
}) async {
18831958
await _refactor(
18841959
originalSource: originalSource,
18851960
expected: expected,
18861961
actionTitle: "Move '$declarationName' to file",
18871962
otherFilePath: otherFilePath,
18881963
otherFileContent: otherFileContent,
1964+
commandWorkDoneToken: commandWorkDoneToken,
18891965
);
18901966
}
18911967

0 commit comments

Comments
 (0)