Skip to content

Commit 0add39a

Browse files
DanTupCommit Bot
authored and
Commit Bot
committed
[analysis_server] Skip analysis work when overlays are created if content matches previous content-
May prevent #48051 from occurring since we will no longer call driver.changeFile() for SDK libraries being opened, although it does not resolve the underlying issue. Change-Id: I5ca53a04842348a584ccc824aaf90ba7f230c002 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/251980 Commit-Queue: Brian Wilkerson <[email protected]> Reviewed-by: Brian Wilkerson <[email protected]> Reviewed-by: Konstantin Shcheglov <[email protected]>
1 parent 188d473 commit 0add39a

File tree

3 files changed

+75
-8
lines changed

3 files changed

+75
-8
lines changed

pkg/analysis_server/lib/src/lsp/handlers/handler_text_document_changes.dart

-4
Original file line numberDiff line numberDiff line change
@@ -103,10 +103,6 @@ class TextDocumentOpenHandler
103103
);
104104
server.onOverlayCreated(path, doc.text);
105105

106-
// If the file did not exist, and is "overlay only", it still should be
107-
// analyzed. Add it to driver to which it should have been added.
108-
server.contextManager.getDriverFor(path)?.addFile(path);
109-
110106
await server.addPriorityFile(path);
111107

112108
return success(null);

pkg/analysis_server/lib/src/lsp/lsp_analysis_server.dart

+32-4
Original file line numberDiff line numberDiff line change
@@ -497,10 +497,33 @@ class LspAnalysisServer extends AbstractAnalysisServer {
497497
}
498498

499499
void onOverlayCreated(String path, String content) {
500+
final currentFile = resourceProvider.getFile(path);
501+
String? currentContent;
502+
503+
try {
504+
currentContent = currentFile.readAsStringSync();
505+
} on FileSystemException {
506+
// It's possible we're creating an overlay for a file that doesn't yet
507+
// exist on disk so must handle missing file exceptions. Checking for
508+
// exists first would introduce a race.
509+
}
510+
500511
resourceProvider.setOverlay(path,
501512
content: content, modificationStamp: overlayModificationStamp++);
502513

503-
_afterOverlayChanged(path, plugin.AddContentOverlay(content));
514+
// If the overlay is exactly the same as the previous content we can skip
515+
// notifying drivers which avoids re-analyzing the same content.
516+
if (content != currentContent) {
517+
_afterOverlayChanged(path, plugin.AddContentOverlay(content));
518+
519+
// If the file did not exist, and is "overlay only", it still should be
520+
// analyzed. Add it to driver to which it should have been added.
521+
contextManager.getDriverFor(path)?.addFile(path);
522+
} else {
523+
// If we skip the work above, we still need to ensure plugins are notified
524+
// of the new overlay (which usually happens in `_afterOverlayChanged`).
525+
_notifyPluginsOverlayChanged(path, plugin.AddContentOverlay(content));
526+
}
504527
}
505528

506529
void onOverlayDestroyed(String path) {
@@ -772,9 +795,7 @@ class LspAnalysisServer extends AbstractAnalysisServer {
772795
for (var driver in driverMap.values) {
773796
driver.changeFile(path);
774797
}
775-
pluginManager.setAnalysisUpdateContentParams(
776-
plugin.AnalysisUpdateContentParams({path: changeForPlugins}),
777-
);
798+
_notifyPluginsOverlayChanged(path, changeForPlugins);
778799

779800
notifyDeclarationsTracker(path);
780801
notifyFlutterWidgetDescriptions(path);
@@ -834,6 +855,13 @@ class LspAnalysisServer extends AbstractAnalysisServer {
834855
}
835856
}
836857

858+
void _notifyPluginsOverlayChanged(
859+
String path, plugin.HasToJson changeForPlugins) {
860+
pluginManager.setAnalysisUpdateContentParams(
861+
plugin.AnalysisUpdateContentParams({path: changeForPlugins}),
862+
);
863+
}
864+
837865
void _onPluginsChanged() {
838866
capabilitiesComputer.performDynamicRegistration();
839867
}

pkg/analysis_server/test/lsp/document_changes_test.dart

+43
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,49 @@ class Bar {
114114
expect(driverForInside.addedFiles, isNot(contains(fileOutsideRootPath)));
115115
}
116116

117+
Future<void> test_documentOpen_contentChanged_analysis() async {
118+
const content = '// original content';
119+
const newContent = '// new content';
120+
newFile(mainFilePath, content);
121+
122+
// Wait for initial analysis to provide diagnostics for the file.
123+
await Future.wait([
124+
waitForDiagnostics(mainFileUri),
125+
initialize(),
126+
]);
127+
128+
// Capture any further diagnostics sent after we open the file.
129+
List<Diagnostic>? diagnostics;
130+
unawaited(waitForDiagnostics(mainFileUri).then((d) => diagnostics = d));
131+
await openFile(mainFileUri, newContent);
132+
await pumpEventQueue(times: 5000);
133+
134+
// Expect diagnostics, because changing the content will have triggered
135+
// analysis.
136+
expect(diagnostics, isNotNull);
137+
}
138+
139+
Future<void> test_documentOpen_contentUnchanged_noAnalysis() async {
140+
const content = '// original content';
141+
newFile(mainFilePath, content);
142+
143+
// Wait for initial analysis to provide diagnostics for the file.
144+
await Future.wait([
145+
waitForDiagnostics(mainFileUri),
146+
initialize(),
147+
]);
148+
149+
// Capture any further diagnostics sent after we open the file.
150+
List<Diagnostic>? diagnostics;
151+
unawaited(waitForDiagnostics(mainFileUri).then((d) => diagnostics = d));
152+
await openFile(mainFileUri, content);
153+
await pumpEventQueue(times: 5000);
154+
155+
// Expect no diagnostics because the file didn't actually change content
156+
// when the overlay was created, so it should not have triggered analysis.
157+
expect(diagnostics, isNull);
158+
}
159+
117160
Future<void> test_documentOpen_createsOverlay() async {
118161
await _initializeAndOpen();
119162

0 commit comments

Comments
 (0)