From 216a14b5997841e8a0ed63ff29414496a6707423 Mon Sep 17 00:00:00 2001 From: Danny Tuppeny Date: Fri, 7 Jun 2024 14:34:00 +0000 Subject: [PATCH] [analysis_server] Avoid using stale LineInfos immediately after overlay updates This fixes a bug where a semantic tokens request could use an incorrect LineInfo for mappings if sent immediately after an overlay update because: a) we computed a LineInfo from the source instead of using one consistent with the resolved unit b) we read the file state in a way that did not take into account the latest overlay updates if the analysis driver hadn't handled the changed file The second issue could affect other requests too because server.getLineInfo() is used in a number of places (to support plugins). This change reverts reading the file state directly, but also updates semantic tokens to prefer using the LineInfo from the resolved unit when we have one (only falling back to computing one from current sources for non-Dart files, which can be handled by plugins). Fixes https://github.com/dart-lang/sdk/issues/55084 Change-Id: I77ab6ad63a78ebd062b264d901a9ae8568b9096e Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/370021 Reviewed-by: Sam Rawlins Commit-Queue: Brian Wilkerson Reviewed-by: Brian Wilkerson --- .../lib/src/analysis_server.dart | 17 ++++++----- .../src/lsp/handlers/handler_definition.dart | 26 ++++++++-------- .../lsp/handlers/handler_semantic_tokens.dart | 22 +++++++------- .../test/lsp/semantic_tokens_test.dart | 30 +++++++++++++++++++ 4 files changed, 64 insertions(+), 31 deletions(-) diff --git a/pkg/analysis_server/lib/src/analysis_server.dart b/pkg/analysis_server/lib/src/analysis_server.dart index 5548d4b57c71..7ccc6ae3fc59 100644 --- a/pkg/analysis_server/lib/src/analysis_server.dart +++ b/pkg/analysis_server/lib/src/analysis_server.dart @@ -618,21 +618,22 @@ abstract class AnalysisServer { /// This method supports non-Dart files but uses the current content of the /// file which may not be the latest analyzed version of the file if it was /// recently modified, so using the LineInfo from an analyzed result may be - /// preferable. + /// preferable. This method exists mainly to support plugins which do not + /// provide us a matching [LineInfo] for the content they used. LineInfo? getLineInfo(String path) { try { - // First try to get from the File if it's an analyzed Dart file. + // First try from the overlay because it may be more up-to-date than + // the file state. + var content = resourceProvider.getFile(path).readAsStringSync(); + return LineInfo.fromContent(content); + } on FileSystemException { + // If the file does not exist or cannot be read, try the file state + // because this could be something like a macro-generated file. var result = getAnalysisDriver(path)?.getFileSync(path); if (result is FileResult) { return result.lineInfo; } - // Fall back to reading from the resource provider. - var content = resourceProvider.getFile(path).readAsStringSync(); - return LineInfo.fromContent(content); - } on FileSystemException { - // If the file does not exist or cannot be read, return null to allow - // the caller to decide how to handle this. return null; } } diff --git a/pkg/analysis_server/lib/src/lsp/handlers/handler_definition.dart b/pkg/analysis_server/lib/src/lsp/handlers/handler_definition.dart index 6e58a172069e..90742d8cc54f 100644 --- a/pkg/analysis_server/lib/src/lsp/handlers/handler_definition.dart +++ b/pkg/analysis_server/lib/src/lsp/handlers/handler_definition.dart @@ -51,19 +51,16 @@ class DefinitionHandler extends LspMessageHandler getServerResult( - bool supportsLocationLink, String path, int offset) async { + Future getServerResult(ResolvedUnitResult result, + String path, bool supportsLocationLink, int offset) async { var collector = NavigationCollectorImpl(); - var result = await server.getResolvedUnit(path); - if (result != null) { - computeDartNavigation( - server.resourceProvider, collector, result, offset, 0); - if (supportsLocationLink) { - await _updateTargetsWithCodeLocations(collector); - } - collector.createRegions(); + computeDartNavigation( + server.resourceProvider, collector, result, offset, 0); + if (supportsLocationLink) { + await _updateTargetsWithCodeLocations(collector); } + collector.createRegions(); return AnalysisNavigationParams( path, collector.regions, collector.targets, collector.files); @@ -86,7 +83,10 @@ class DefinitionHandler extends LspMessageHandler } Future> getServerResult( - String path, SourceRange? range) async { - var result = await requireResolvedUnit(path); - return result.map( - (_) => [], // Error, return nothing. - (unit) { - var computer = DartUnitHighlightsComputer(unit.unit, range: range); - return computer.computeSemanticTokens(); - }, - ); + CompilationUnit unit, SourceRange? range) async { + var computer = DartUnitHighlightsComputer(unit, range: range); + return computer.computeSemanticTokens(); } Iterable _filter( @@ -59,7 +54,10 @@ abstract class AbstractSemanticTokensHandler var path = pathOfDoc(textDocument); return path.mapResult((path) async { - var lineInfo = server.getLineInfo(path); + // Always prefer a LineInfo from a resolved unit than server.getLineInfo. + var resolvedUnit = (await requireResolvedUnit(path)).resultOrNull; + var lineInfo = resolvedUnit?.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) { @@ -67,7 +65,9 @@ abstract class AbstractSemanticTokensHandler } return toSourceRangeNullable(lineInfo, range).mapResult((range) async { - var serverTokens = await getServerResult(path, range); + var serverTokens = resolvedUnit != null + ? await getServerResult(resolvedUnit.unit, range) + : []; var pluginHighlightRegions = getPluginResults(path).flattenedToList2; if (token.isCancellationRequested) { diff --git a/pkg/analysis_server/test/lsp/semantic_tokens_test.dart b/pkg/analysis_server/test/lsp/semantic_tokens_test.dart index ba21ef8bea39..20f78ef2e398 100644 --- a/pkg/analysis_server/test/lsp/semantic_tokens_test.dart +++ b/pkg/analysis_server/test/lsp/semantic_tokens_test.dart @@ -802,6 +802,36 @@ extension type E(int i) {} expect(decoded, equals(expected)); } + /// Verify that sending a semantic token request immediately after an overlay + /// update (with no delay) does not result in corrupt semantic tokens because + /// the previous file content was used. + /// + /// https://github.com/dart-lang/sdk/issues/55084 + Future test_immediatelyAfterUpdate() async { + const initialContent = 'class A {}\nclass B {}'; + const updatedContent = 'class Aaaaa {}\nclass Bbbbb {}'; + + newFile(mainFilePath, initialContent); + await initialize(); + + await openFile(mainFileUri, initialContent); + + // Send an edit (don't await), then fetch the tokens and verify the results + // were correct for the final content. If the bug occurs, the strings won't + // match up because the offsets will have been mapped incorrectly. + unawaited(replaceFile(2, mainFileUri, updatedContent)); + var tokens = await getSemanticTokens(mainFileUri); + var decoded = _decodeSemanticTokens(updatedContent, tokens); + expect(decoded, [ + _Token('class', SemanticTokenTypes.keyword), + _Token('Aaaaa', SemanticTokenTypes.class_, + [SemanticTokenModifiers.declaration]), + _Token('class', SemanticTokenTypes.keyword), + _Token('Bbbbb', SemanticTokenTypes.class_, + [SemanticTokenModifiers.declaration]) + ]); + } + Future test_invalidSyntax() async { failTestOnErrorDiagnostic = false;