[analysis_server] Wait for analysis context rebuilds when obtaining parsed/resolved results in LSP handlers
Fixes https://github.com/Dart-Code/Dart-Code/issues/3929.
Change-Id: I3176a603785b5e9546a4c1d7c3f818396eaec4bd
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/241606
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Brian Wilkerson <brianwilkerson@google.com>
diff --git a/pkg/analysis_server/lib/src/lsp/handlers/handler_select_range.dart b/pkg/analysis_server/lib/src/lsp/handlers/handler_select_range.dart
index 3c4a7a4..c567939 100644
--- a/pkg/analysis_server/lib/src/lsp/handlers/handler_select_range.dart
+++ b/pkg/analysis_server/lib/src/lsp/handlers/handler_select_range.dart
@@ -10,7 +10,6 @@
import 'package:analysis_server/src/lsp/mapping.dart';
import 'package:analyzer/dart/analysis/results.dart';
import 'package:analyzer/dart/ast/ast.dart';
-import 'package:analyzer/source/line_info.dart';
class SelectionRangeHandler
extends MessageHandler<SelectionRangeParams, List<SelectionRange>?> {
@@ -31,26 +30,20 @@
final path = pathOfDoc(params.textDocument);
return path.mapResult((path) async {
- final lineInfo = server.getLineInfo(path);
- // If there is no lineInfo, the request cannot be translated from LSP
- // line/col to server offset/length.
- if (lineInfo == null) {
- return success(null);
- }
-
final unit = await requireUnresolvedUnit(path);
final positions = params.positions;
final offsets = await unit.mapResult((unit) =>
ErrorOr.all(positions.map((pos) => toOffset(unit.lineInfo, pos))));
- final allRanges = await offsets.mapResult((offsets) =>
- success(_getSelectionRangesForOffsets(offsets, unit, lineInfo)));
+ final allRanges = await offsets.mapResult(
+ (offsets) => success(_getSelectionRangesForOffsets(offsets, unit)));
return allRanges;
});
}
SelectionRange _getSelectionRangesForOffset(
- CompilationUnit unit, LineInfo lineInfo, int offset) {
+ CompilationUnit unit, int offset) {
+ final lineInfo = unit.lineInfo;
final computer = DartSelectionRangeComputer(unit, offset);
final ranges = computer.compute();
// Loop through the items starting at the end (the outermost range), using
@@ -64,20 +57,19 @@
);
}
- // It's not clear how to respond if a subset of the results
- // do not have results, so for now if the list is empty just return a single
- // range that is exactly the same as the position.
- // TODO(dantup): Update this based on the response to
- // https://github.com/microsoft/language-server-protocol/issues/1270
-
+ // When there is no range for a given position, return an empty range at the
+ // requested position. From the LSP spec:
+ //
+ // "To allow for results where some positions have selection ranges and
+ // others do not, result[i].range is allowed to be the empty range at
+ // positions[i]."
return last ?? SelectionRange(range: toRange(lineInfo, offset, 0));
}
List<SelectionRange> _getSelectionRangesForOffsets(
- List<int> offsets, ErrorOr<ParsedUnitResult> unit, LineInfo lineInfo) {
+ List<int> offsets, ErrorOr<ParsedUnitResult> unit) {
return offsets
- .map((offset) =>
- _getSelectionRangesForOffset(unit.result.unit, lineInfo, offset))
+ .map((offset) => _getSelectionRangesForOffset(unit.result.unit, offset))
.toList();
}
}
diff --git a/pkg/analysis_server/lib/src/lsp/handlers/handlers.dart b/pkg/analysis_server/lib/src/lsp/handlers/handlers.dart
index 1880af3..6a8fd79 100644
--- a/pkg/analysis_server/lib/src/lsp/handlers/handlers.dart
+++ b/pkg/analysis_server/lib/src/lsp/handlers/handlers.dart
@@ -49,12 +49,18 @@
LspAnalysisServer get server;
+ ErrorOr<T> analysisFailedError<T>(String path) => error<T>(
+ ServerErrorCodes.FileAnalysisFailed, 'Analysis failed for file', path);
+
bool fileHasBeenModified(String path, num? clientVersion) {
final serverDocIdentifier = server.getVersionedDocumentIdentifier(path);
return clientVersion != null &&
clientVersion != serverDocIdentifier.version;
}
+ ErrorOr<T> fileNotAnalyzedError<T>(String path) => error<T>(
+ ServerErrorCodes.FileNotAnalyzed, 'File is not being analyzed', path);
+
ErrorOr<LineInfo> getLineInfo(String path) {
final lineInfo = server.getLineInfo(path);
@@ -66,19 +72,30 @@
}
}
- Future<ErrorOr<ResolvedUnitResult>> requireResolvedUnit(String path) async {
+ /// Attempts to get a [ResolvedUnitResult] for [path] or an error.
+ ///
+ /// When [waitForInProgressContextRebuilds] is `true` and the file appears to not be
+ /// analyzed but analysis roots are currently being discovered, will wait for
+ /// discovery to complete and then try again (once) to get a result.
+ Future<ErrorOr<ResolvedUnitResult>> requireResolvedUnit(
+ String path, {
+ bool waitForInProgressContextRebuilds = true,
+ }) async {
final result = await server.getResolvedUnit(path);
if (result == null) {
- if (server.isAnalyzed(path)) {
- // If the file was being analyzed and we got a null result, that usually
- // indicators a parser or analysis failure, so provide a more specific
- // message.
- return error(ServerErrorCodes.FileAnalysisFailed,
- 'Analysis failed for file', path);
- } else {
- return error(ServerErrorCodes.FileNotAnalyzed,
- 'File is not being analyzed', path);
+ // Handle retry if allowed.
+ if (waitForInProgressContextRebuilds) {
+ await server.analysisContextsRebuilt;
+ return requireResolvedUnit(path,
+ waitForInProgressContextRebuilds: false);
}
+
+ // If the file was being analyzed and we got a null result, that usually
+ // indicates a parser or analysis failure, so provide a more specific
+ // message.
+ return server.isAnalyzed(path)
+ ? analysisFailedError(path)
+ : fileNotAnalyzedError(path);
} else if (!result.exists) {
return error(
ServerErrorCodes.InvalidFilePath, 'File does not exist', path);
@@ -86,19 +103,25 @@
return success(result);
}
- Future<ErrorOr<ParsedUnitResult>> requireUnresolvedUnit(String path) async {
+ Future<ErrorOr<ParsedUnitResult>> requireUnresolvedUnit(
+ String path, {
+ bool waitForInProgressContextRebuilds = true,
+ }) async {
final result = await server.getParsedUnit(path);
if (result == null) {
- if (server.isAnalyzed(path)) {
- // If the file was being analyzed and we got a null result, that usually
- // indicators a parser or analysis failure, so provide a more specific
- // message.
- return error(ServerErrorCodes.FileAnalysisFailed,
- 'Analysis failed for file', path);
- } else {
- return error(ServerErrorCodes.FileNotAnalyzed,
- 'File is not being analyzed', path);
+ // Handle retry if allowed.
+ if (waitForInProgressContextRebuilds) {
+ await server.analysisContextsRebuilt;
+ return requireUnresolvedUnit(path,
+ waitForInProgressContextRebuilds: false);
}
+
+ // If the file was being analyzed and we got a null result, that usually
+ // indicates a parser or analysis failure, so provide a more specific
+ // message.
+ return server.isAnalyzed(path)
+ ? analysisFailedError(path)
+ : fileNotAnalyzedError(path);
}
return success(result);
}
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 4667875..dba5fc2 100644
--- a/pkg/analysis_server/lib/src/lsp/lsp_analysis_server.dart
+++ b/pkg/analysis_server/lib/src/lsp/lsp_analysis_server.dart
@@ -120,6 +120,11 @@
/// The subscription to the stream of incoming messages from the client.
late StreamSubscription<void> _channelSubscription;
+ /// A completer that tracks in-progress analysis context rebuilds.
+ ///
+ /// Starts completed and will be replaced each time a context rebuild starts.
+ Completer<void> _analysisContextRebuildCompleter = Completer()..complete();
+
/// Initialize a newly created server to send and receive messages to the
/// given [channel].
LspAnalysisServer(
@@ -163,6 +168,14 @@
pluginManager.pluginsChanged.listen((_) => _onPluginsChanged());
}
+ /// A [Future] that completes when any in-progress analysis context rebuild
+ /// completes.
+ ///
+ /// If no context rebuild is in progress, will return an already complete
+ /// [Future].
+ Future<void> get analysisContextsRebuilt =>
+ _analysisContextRebuildCompleter.future;
+
/// The capabilities of the LSP client. Will be null prior to initialization.
LspClientCapabilities? get clientCapabilities => _clientCapabilities;
@@ -788,10 +801,15 @@
(root) => resourceProvider.pathContext.join(root, excludePath)))
.toSet();
- notificationManager.setAnalysisRoots(
- includedPaths.toList(), excludedPaths.toList());
- await contextManager.setRoots(
- includedPaths.toList(), excludedPaths.toList());
+ final completer = _analysisContextRebuildCompleter = Completer();
+ try {
+ notificationManager.setAnalysisRoots(
+ includedPaths.toList(), excludedPaths.toList());
+ await contextManager.setRoots(
+ includedPaths.toList(), excludedPaths.toList());
+ } finally {
+ completer.complete();
+ }
}
void _updateDriversAndPluginsPriorityFiles() {
diff --git a/pkg/analysis_server/test/lsp/document_symbols_test.dart b/pkg/analysis_server/test/lsp/document_symbols_test.dart
index 2fb6196..43b129b 100644
--- a/pkg/analysis_server/test/lsp/document_symbols_test.dart
+++ b/pkg/analysis_server/test/lsp/document_symbols_test.dart
@@ -27,7 +27,7 @@
newFile(mainFilePath, content);
await initialize();
- final result = await getDocumentSymbols(mainFileUri.toString());
+ final result = await getDocumentSymbols(mainFileUri);
final symbols = result.map(
(docsymbols) => throw 'Expected SymbolInformations, got DocumentSymbols',
(symbolInfos) => symbolInfos,
@@ -61,7 +61,7 @@
),
);
- final result = await getDocumentSymbols(mainFileUri.toString());
+ final result = await getDocumentSymbols(mainFileUri);
final symbols = result.map(
(docsymbols) => throw 'Expected SymbolInformations, got DocumentSymbols',
(symbolInfos) => symbolInfos,
@@ -93,7 +93,7 @@
newFile(mainFilePath, content);
await initialize();
- final result = await getDocumentSymbols(mainFileUri.toString());
+ final result = await getDocumentSymbols(mainFileUri);
final symbols = result.map(
(docsymbols) => throw 'Expected SymbolInformations, got DocumentSymbols',
(symbolInfos) => symbolInfos,
@@ -148,7 +148,7 @@
textDocumentCapabilities: withHierarchicalDocumentSymbolSupport(
emptyTextDocumentClientCapabilities));
- final result = await getDocumentSymbols(mainFileUri.toString());
+ final result = await getDocumentSymbols(mainFileUri);
final symbols = result.map(
(docsymbols) => docsymbols,
(symbolInfos) => throw 'Expected DocumentSymbols, got SymbolInformations',
@@ -186,7 +186,7 @@
await initialize(allowEmptyRootUri: true);
await openFile(mainFileUri, content);
- final result = await getDocumentSymbols(mainFileUri.toString());
+ final result = await getDocumentSymbols(mainFileUri);
final symbols = result.map(
(docsymbols) => throw 'Expected SymbolInformations, got DocumentSymbols',
(symbolInfos) => symbolInfos,
@@ -207,7 +207,7 @@
await initialize(allowEmptyRootUri: true);
await expectLater(
- getDocumentSymbols(mainFileUri.toString()),
+ getDocumentSymbols(mainFileUri),
throwsA(isResponseError(ServerErrorCodes.FileNotAnalyzed,
message: 'File is not being analyzed')));
}
@@ -216,7 +216,7 @@
newFile(pubspecFilePath, simplePubspecContent);
await initialize();
- final result = await getDocumentSymbols(pubspecFileUri.toString());
+ final result = await getDocumentSymbols(pubspecFileUri);
// Since the list is empty, it will deserialise into whatever the first
// type is, so just accept both types.
final symbols = result.map(
diff --git a/pkg/analysis_server/test/lsp/initialization_test.dart b/pkg/analysis_server/test/lsp/initialization_test.dart
index a05138c..68fc852 100644
--- a/pkg/analysis_server/test/lsp/initialization_test.dart
+++ b/pkg/analysis_server/test/lsp/initialization_test.dart
@@ -2,12 +2,15 @@
// 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/json_parsing.dart';
import 'package:analysis_server/src/lsp/server_capabilities_computer.dart';
import 'package:analysis_server/src/plugin/plugin_manager.dart';
+import 'package:analyzer/file_system/memory_file_system.dart';
import 'package:test/test.dart';
import 'package:test_reflective_loader/test_reflective_loader.dart';
@@ -16,6 +19,7 @@
void main() {
defineReflectiveSuite(() {
defineReflectiveTests(InitializationTest);
+ defineReflectiveTests(SlowInitializationTest);
});
}
@@ -495,6 +499,54 @@
);
}
+ /// Tests that when there are no explicit analysis roots (instead they are
+ /// implied by open files), requests for open files are successful even if
+ /// sent _immediately_ after opening the file.
+ ///
+ /// https://github.com/Dart-Code/Dart-Code/issues/3929
+ ///
+ /// This test uses getDocumentSymbols which requires a resolved result.
+ Future<void>
+ test_emptyAnalysisRoots_handlesFileRequestsImmediately_resolved() async {
+ const content = 'void f() {}';
+ final file1 = join(projectFolderPath, 'file1.dart');
+ final file1Uri = Uri.file(file1);
+ newFile(file1, content);
+ newPubspecYamlFile(projectFolderPath, '');
+
+ await initialize(allowEmptyRootUri: true);
+
+ unawaited(openFile(file1Uri, content)); // Don't wait
+ final result = await getDocumentSymbols(file1Uri);
+ final symbols = result.map(
+ (docSymbols) => docSymbols,
+ (symbolInfos) => symbolInfos,
+ );
+ expect(symbols, hasLength(1));
+ }
+
+ /// Tests that when there are no explicit analysis roots (instead they are
+ /// implied by open files), requests for open files are successful even if
+ /// sent _immediately_ after opening the file.
+ ///
+ /// https://github.com/Dart-Code/Dart-Code/issues/3929
+ ///
+ /// This test uses getSelectionRanges which requires only a parsed result.
+ Future<void>
+ test_emptyAnalysisRoots_handlesFileRequestsImmediately_unresolved() async {
+ const content = 'void f() {}';
+ final file1 = join(projectFolderPath, 'file1.dart');
+ final file1Uri = Uri.file(file1);
+ newFile(file1, content);
+ newPubspecYamlFile(projectFolderPath, '');
+
+ await initialize(allowEmptyRootUri: true);
+
+ unawaited(openFile(file1Uri, content)); // Don't wait
+ final result = await getSelectionRanges(file1Uri, [startOfDocPos]);
+ expect(result, hasLength(1));
+ }
+
Future<void> test_emptyAnalysisRoots_multipleOpenFiles() async {
final file1 = join(projectFolderPath, 'file1.dart');
final file1Uri = Uri.file(file1);
@@ -509,12 +561,14 @@
// Opening both files should only add the project folder once.
await openFile(file1Uri, '');
await openFile(file2Uri, '');
+ await pumpEventQueue(times: 5000);
expect(server.contextManager.includedPaths, equals([projectFolderPath]));
// Closing only one of the files should not remove the project folder
// since there are still open files.
resetContextBuildCounter();
await closeFile(file1Uri);
+ await pumpEventQueue(times: 5000);
expect(server.contextManager.includedPaths, equals([projectFolderPath]));
expectNoContextBuilds();
@@ -522,6 +576,7 @@
// the context.
resetContextBuildCounter();
await closeFile(file2Uri);
+ await pumpEventQueue(times: 5000);
expect(server.contextManager.includedPaths, equals([]));
expect(server.contextManager.driverMap, hasLength(0));
expectContextBuilds();
@@ -556,6 +611,7 @@
// Opening a file nested within the project should add the project folder.
await openFile(nestedFileUri, '');
+ await pumpEventQueue(times: 500);
expect(server.contextManager.includedPaths, equals([projectFolderPath]));
// Ensure the file was cached in each driver. This happens as a result of
@@ -795,12 +851,14 @@
// Opening both files should only add the project folder once.
await openFile(file1Uri, '');
await openFile(file2Uri, '');
+ await pumpEventQueue(times: 5000);
expect(server.contextManager.includedPaths, equals([projectFolderPath]));
expect(server.contextManager.driverMap, hasLength(1));
// Closing only one of the files should not remove the root or rebuild the context.
resetContextBuildCounter();
await closeFile(file1Uri);
+ await pumpEventQueue(times: 5000);
expect(server.contextManager.includedPaths, equals([projectFolderPath]));
expect(server.contextManager.driverMap, hasLength(1));
expectNoContextBuilds();
@@ -809,6 +867,7 @@
// the context.
resetContextBuildCounter();
await closeFile(file2Uri);
+ await pumpEventQueue(times: 5000);
expect(server.contextManager.includedPaths, equals([]));
expect(server.contextManager.driverMap, hasLength(0));
expectContextBuilds();
@@ -907,3 +966,15 @@
return uri.replace(path: '${uri.path}/');
}
}
+
+/// Runs all initialization tests with a resource provider that slowly
+/// initializes watchers to simulate delays in real fs watchers.
+@reflectiveTest
+class SlowInitializationTest extends InitializationTest {
+ @override
+ MemoryResourceProvider resourceProvider = MemoryResourceProvider(
+ // Force the in-memory file watchers to be slowly initialized to emulate
+ // the physical watchers (for test_concurrentContextRebuilds).
+ delayWatcherInitialization: Duration(milliseconds: 1),
+ );
+}
diff --git a/pkg/analysis_server/test/lsp/server_abstract.dart b/pkg/analysis_server/test/lsp/server_abstract.dart
index 4277d90..a5e88ea 100644
--- a/pkg/analysis_server/test/lsp/server_abstract.dart
+++ b/pkg/analysis_server/test/lsp/server_abstract.dart
@@ -1156,11 +1156,11 @@
}
Future<Either2<List<DocumentSymbol>, List<SymbolInformation>>>
- getDocumentSymbols(String fileUri) {
+ getDocumentSymbols(Uri uri) {
final request = makeRequest(
Method.textDocument_documentSymbol,
DocumentSymbolParams(
- textDocument: TextDocumentIdentifier(uri: fileUri),
+ textDocument: TextDocumentIdentifier(uri: uri.toString()),
),
);
return expectSuccessfulResponseTo(
