Skip to content

Commit 28523a5

Browse files
DanTupcommit-bot@chromium.org
DanTup
authored and
[email protected]
committed
[Analyzer] Fix race condition in LSP semantic tokens + re-enable
Change-Id: I52c4eb18dd09e3334f21bbf5fac9ce4e716ea234 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/176660 Reviewed-by: Brian Wilkerson <[email protected]>
1 parent 0a63d23 commit 28523a5

File tree

5 files changed

+62
-78
lines changed

5 files changed

+62
-78
lines changed

‎pkg/analysis_server/lib/src/lsp/handlers/handler_semantic_tokens.dart

Lines changed: 5 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -54,28 +54,20 @@ class SemanticTokensHandler
5454
return success(null);
5555
}
5656

57-
// We need to be able to split multiline tokens up if a client does not
58-
// support them. Doing this correctly requires access to the line endings
59-
// and indenting so we must get a copy of the file contents. Although this
60-
// is on the Dart unit result, we may also need this for files being
61-
// handled by plugins.
62-
final file = server.resourceProvider.getFile(path);
63-
if (!file.exists) {
64-
return success(null);
65-
}
66-
final fileContents = file.readAsStringSync();
67-
6857
final allResults = [
6958
await getServerResult(path),
7059
...getPluginResults(path),
7160
];
7261

62+
if (token.isCancellationRequested) {
63+
return cancelled();
64+
}
65+
7366
final merger = ResultMerger();
7467
final mergedResults = merger.mergeHighlightRegions(allResults);
7568

7669
final encoder = SemanticTokenEncoder();
77-
final tokens =
78-
encoder.convertHighlights(mergedResults, lineInfo, fileContents);
70+
final tokens = encoder.convertHighlights(mergedResults, lineInfo);
7971
final semanticTokens = encoder.encodeTokens(tokens);
8072

8173
return success(semanticTokens);

‎pkg/analysis_server/lib/src/lsp/semantic_tokens/encoder.dart

Lines changed: 9 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ class SemanticTokenEncoder {
1818
/// Converts [regions]s into LSP [SemanticTokenInfo], splitting multiline tokens
1919
/// and nested tokens if required.
2020
List<SemanticTokenInfo> convertHighlights(
21-
List<HighlightRegion> regions, LineInfo lineInfo, String fileContent) {
21+
List<HighlightRegion> regions, LineInfo lineInfo) {
2222
// LSP is zero-based but server is 1-based.
2323
const lspPositionOffset = -1;
2424

@@ -39,8 +39,8 @@ class SemanticTokenEncoder {
3939
.where((region) => highlightRegionTokenTypes.containsKey(region.type));
4040

4141
if (!allowMultilineTokens) {
42-
translatedRegions = translatedRegions.expand(
43-
(region) => _splitMultilineRegions(region, lineInfo, fileContent));
42+
translatedRegions = translatedRegions
43+
.expand((region) => _splitMultilineRegions(region, lineInfo));
4444
}
4545

4646
if (!allowOverlappingTokens) {
@@ -129,9 +129,10 @@ class SemanticTokenEncoder {
129129
}
130130

131131
/// Splits multiline regions into multiple regions for clients that do not support
132-
/// multiline tokens.
132+
/// multiline tokens. Multiline tokens will be split at the end of the line and
133+
/// line endings and indenting will be included in the tokens.
133134
Iterable<HighlightRegion> _splitMultilineRegions(
134-
HighlightRegion region, LineInfo lineInfo, String fileContent) sync* {
135+
HighlightRegion region, LineInfo lineInfo) sync* {
135136
final start = lineInfo.getLocation(region.offset);
136137
final end = lineInfo.getLocation(region.offset + region.length);
137138

@@ -141,30 +142,13 @@ class SemanticTokenEncoder {
141142
lineNumber++) {
142143
final isFirstLine = lineNumber == start.lineNumber;
143144
final isLastLine = lineNumber == end.lineNumber;
144-
final isSingleLine = start.lineNumber == end.lineNumber;
145145
final lineOffset = lineInfo.getOffsetOfLine(lineNumber - 1);
146146

147-
var startOffset = isFirstLine ? start.columnNumber - 1 : 0;
148-
var endOffset = isLastLine
147+
final startOffset = isFirstLine ? start.columnNumber - 1 : 0;
148+
final endOffset = isLastLine
149149
? end.columnNumber - 1
150150
: lineInfo.getOffsetOfLine(lineNumber) - lineOffset;
151-
var length = endOffset - startOffset;
152-
153-
// When we split multiline tokens, we may end up with leading/trailing
154-
// whitespace which doesn't make sense to include in the token. Examine
155-
// the content to remove this.
156-
if (!isSingleLine) {
157-
final tokenContent = fileContent.substring(
158-
lineOffset + startOffset, lineOffset + endOffset);
159-
final leadingWhitespaceCount =
160-
tokenContent.length - tokenContent.trimLeft().length;
161-
final trailingWhitespaceCount =
162-
tokenContent.length - tokenContent.trimRight().length;
163-
164-
startOffset += leadingWhitespaceCount;
165-
endOffset -= trailingWhitespaceCount;
166-
length = endOffset - startOffset;
167-
}
151+
final length = endOffset - startOffset;
168152

169153
yield HighlightRegion(region.type, lineOffset + startOffset, length);
170154
}

‎pkg/analysis_server/lib/src/lsp/server_capabilities_computer.dart

Lines changed: 12 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,6 @@ import 'package:analysis_server/src/lsp/constants.dart';
88
import 'package:analysis_server/src/lsp/lsp_analysis_server.dart';
99
import 'package:analysis_server/src/lsp/semantic_tokens/legend.dart';
1010

11-
/// Semantic tokens temporarily disabled due to a race condition.
12-
const enableSemanticTokens = false;
13-
1411
/// Helper for reading client dynamic registrations which may be ommitted by the
1512
/// client.
1613
class ClientDynamicRegistrations {
@@ -44,7 +41,7 @@ class ClientDynamicRegistrations {
4441
Method.workspace_willRenameFiles,
4542
// Sematic tokens are all registered under a single "method" as the
4643
// actual methods are controlled by the server capabilities.
47-
if (enableSemanticTokens) CustomMethods.semanticTokenDynamicRegistration,
44+
CustomMethods.semanticTokenDynamicRegistration,
4845
];
4946
final ClientCapabilities _capabilities;
5047

@@ -236,18 +233,17 @@ class ServerCapabilitiesComputer {
236233
FoldingRangeRegistrationOptions>.t1(
237234
true,
238235
),
239-
semanticTokensProvider:
240-
dynamicRegistrations.semanticTokens || !enableSemanticTokens
241-
? null
242-
: Either2<SemanticTokensOptions,
243-
SemanticTokensRegistrationOptions>.t1(
244-
SemanticTokensOptions(
245-
legend: semanticTokenLegend.lspLegend,
246-
full: Either2<bool, SemanticTokensOptionsFull>.t2(
247-
SemanticTokensOptionsFull(delta: false),
248-
),
249-
),
236+
semanticTokensProvider: dynamicRegistrations.semanticTokens
237+
? null
238+
: Either2<SemanticTokensOptions,
239+
SemanticTokensRegistrationOptions>.t1(
240+
SemanticTokensOptions(
241+
legend: semanticTokenLegend.lspLegend,
242+
full: Either2<bool, SemanticTokensOptionsFull>.t2(
243+
SemanticTokensOptionsFull(delta: false),
250244
),
245+
),
246+
),
251247
executeCommandProvider: ExecuteCommandOptions(
252248
commands: Commands.serverSupportedCommands,
253249
workDoneProgress: true,
@@ -450,7 +446,7 @@ class ServerCapabilitiesComputer {
450446
Method.workspace_didChangeConfiguration,
451447
);
452448
register(
453-
dynamicRegistrations.semanticTokens && enableSemanticTokens,
449+
dynamicRegistrations.semanticTokens,
454450
CustomMethods.semanticTokenDynamicRegistration,
455451
SemanticTokensRegistrationOptions(
456452
documentSelector: fullySupportedTypes,

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

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -131,8 +131,7 @@ class InitializationTest extends AbstractLspAnalysisServerTest {
131131
expect(initResult.capabilities.foldingRangeProvider, isNotNull);
132132
expect(initResult.capabilities.workspace.fileOperations.willRename,
133133
equals(ServerCapabilitiesComputer.fileOperationRegistrationOptions));
134-
expect(initResult.capabilities.semanticTokensProvider,
135-
enableSemanticTokens ? isNotNull : isNull);
134+
expect(initResult.capabilities.semanticTokensProvider, isNotNull);
136135

137136
expect(didGetRegisterCapabilityRequest, isFalse);
138137
}

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

Lines changed: 35 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -413,11 +413,11 @@ class SemanticTokensTest extends AbstractLspAnalysisServerTest {
413413
*/''';
414414

415415
final expected = [
416-
_Token('/**', SemanticTokenTypes.comment,
416+
_Token('/**\n', SemanticTokenTypes.comment,
417417
[SemanticTokenModifiers.documentation]),
418-
_Token('* Trailing comment', SemanticTokenTypes.comment,
418+
_Token(' * Trailing comment\n', SemanticTokenTypes.comment,
419419
[SemanticTokenModifiers.documentation]),
420-
_Token('*/', SemanticTokenTypes.comment,
420+
_Token(' */', SemanticTokenTypes.comment,
421421
[SemanticTokenModifiers.documentation]),
422422
];
423423

@@ -524,27 +524,27 @@ import 'dart:async';
524524

525525
Future<void> test_multilineRegions() async {
526526
final content = '''
527-
/**
528-
* This is my class comment
529-
*
530-
* There are
531-
* multiple lines
532-
*/
533-
class MyClass {}
527+
/**
528+
* This is my class comment
529+
*
530+
* There are
531+
* multiple lines
532+
*/
533+
class MyClass {}
534534
''';
535535

536536
final expected = [
537-
_Token('/**', SemanticTokenTypes.comment,
537+
_Token('/**\n', SemanticTokenTypes.comment,
538538
[SemanticTokenModifiers.documentation]),
539-
_Token('* This is my class comment', SemanticTokenTypes.comment,
539+
_Token(' * This is my class comment\n', SemanticTokenTypes.comment,
540540
[SemanticTokenModifiers.documentation]),
541-
_Token('*', SemanticTokenTypes.comment,
541+
_Token(' *\n', SemanticTokenTypes.comment,
542542
[SemanticTokenModifiers.documentation]),
543-
_Token('* There are', SemanticTokenTypes.comment,
543+
_Token(' * There are\n', SemanticTokenTypes.comment,
544544
[SemanticTokenModifiers.documentation]),
545-
_Token('* multiple lines', SemanticTokenTypes.comment,
545+
_Token(' * multiple lines\n', SemanticTokenTypes.comment,
546546
[SemanticTokenModifiers.documentation]),
547-
_Token('*/', SemanticTokenTypes.comment,
547+
_Token(' */', SemanticTokenTypes.comment,
548548
[SemanticTokenModifiers.documentation]),
549549
_Token('class', SemanticTokenTypes.keyword),
550550
_Token('MyClass', SemanticTokenTypes.class_),
@@ -559,12 +559,17 @@ import 'dart:async';
559559
}
560560

561561
Future<void> test_strings() async {
562-
final content = r'''
563-
String foo(String c) => c;
564-
const string1 = 'test';
565-
const string2 = 'test1 $string1 test2 ${foo('test3')}';
566-
const string3 = r'$string1 ${string1.length}';
567-
''';
562+
final content = '''
563+
String foo(String c) => c;
564+
const string1 = 'test';
565+
const string2 = 'test1 \$string1 test2 \${foo('test3')}';
566+
const string3 = r'\$string1 \${string1.length}';
567+
const string4 = \'\'\'
568+
multi
569+
line
570+
string
571+
\'\'\';
572+
''';
568573

569574
final expected = [
570575
_Token('String', SemanticTokenTypes.class_),
@@ -592,6 +597,14 @@ import 'dart:async';
592597
_Token('string3', SemanticTokenTypes.variable,
593598
[SemanticTokenModifiers.declaration]),
594599
_Token(r"r'$string1 ${string1.length}'", SemanticTokenTypes.string),
600+
_Token('const', SemanticTokenTypes.keyword),
601+
_Token('string4', SemanticTokenTypes.variable,
602+
[SemanticTokenModifiers.declaration]),
603+
_Token("'''\n", SemanticTokenTypes.string),
604+
_Token('multi\n', SemanticTokenTypes.string),
605+
_Token(' line\n', SemanticTokenTypes.string),
606+
_Token(' string\n', SemanticTokenTypes.string),
607+
_Token("'''", SemanticTokenTypes.string),
595608
];
596609

597610
await initialize();

0 commit comments

Comments
 (0)