From ae2e608ff953eb511f54978c86d4db43fd02441d Mon Sep 17 00:00:00 2001 From: "scheglov@google.com" Date: Thu, 5 Feb 2015 20:42:17 +0000 Subject: [PATCH] Send notificatinos after no-op changes. This should solve the problem when client remove some part of the code, so removes also error markers for it, and then quickly restores it, so for the server it looks as an empty/no-op change. But information is lost, and the server should resend it to the client. R=brianwilkerson@google.com BUG= Review URL: https://codereview.chromium.org//899753004 git-svn-id: https://dart.googlecode.com/svn/branches/bleeding_edge/dart@43521 260f80e4-7a28-3924-810f-c04153c831b5 --- .../lib/src/analysis_server.dart | 43 +++++- .../lib/src/operation/operation_analysis.dart | 122 ++++++++++-------- .../test/analysis/update_content_test.dart | 39 ++++++ pkg/analyzer/lib/src/generated/engine.dart | 74 ++++++++--- pkg/analyzer/test/generated/engine_test.dart | 6 + 5 files changed, 207 insertions(+), 77 deletions(-) diff --git a/pkg/analysis_server/lib/src/analysis_server.dart b/pkg/analysis_server/lib/src/analysis_server.dart index 34531c68402..9bac7d92535 100644 --- a/pkg/analysis_server/lib/src/analysis_server.dart +++ b/pkg/analysis_server/lib/src/analysis_server.dart @@ -589,7 +589,8 @@ class AnalysisServer { } /** - * Returns `true` if there is a subscription for the given [server] and [file]. + * Returns `true` if there is a subscription for the given [service] and + * [file]. */ bool hasAnalysisSubscription(AnalysisService service, String file) { Set files = analysisServices[service]; @@ -699,9 +700,7 @@ class AnalysisServer { */ void scheduleOperation(ServerOperation operation) { addOperation(operation); - if (!performOperationPending) { - _schedulePerformOperation(); - } + _schedulePerformOperation(); } /** @@ -929,6 +928,15 @@ class AnalysisServer { }); } + void test_flushResolvedUnit(String file) { + if (AnalysisEngine.isDartFileName(file)) { + AnalysisContextImpl context = getAnalysisContext(file); + Source source = getSource(file); + DartEntry dartEntry = context.getReadableSourceEntryOrNull(source); + dartEntry.flushAstStructures(); + } + } + /** * Implementation for `analysis.updateContent`. */ @@ -967,6 +975,7 @@ class AnalysisServer { throw new AnalysisException('Illegal change type'); } _overlayState.setContents(source, newContents); + // Update all contexts. for (InternalAnalysisContext context in folderMap.values) { if (context.handleContentsChanged( source, @@ -974,6 +983,28 @@ class AnalysisServer { newContents, true)) { schedulePerformAnalysisOperation(context); + } else { + // When the client sends any change for a source, we should resend + // subscribed notifications, even if there were no changes in the + // source contents. + // TODO(scheglov) consider checking if there are subscriptions. + if (AnalysisEngine.isDartFileName(file)) { + CompilationUnit dartUnit = + context.ensureAnyResolvedDartUnit(source); + if (dartUnit != null) { + AnalysisErrorInfo errorInfo = context.getErrors(source); + scheduleNotificationOperations( + this, + file, + errorInfo.lineInfo, + context, + null, + dartUnit, + errorInfo.errors); + } else { + schedulePerformAnalysisOperation(context); + } + } } } }); @@ -1008,7 +1039,9 @@ class AnalysisServer { * Schedules [performOperation] exection. */ void _schedulePerformOperation() { - assert(!performOperationPending); + if (performOperationPending) { + return; + } /* * TODO (danrubel) Rip out this workaround once the underlying problem * is fixed. Currently, the VM and dart:io do not deliver content diff --git a/pkg/analysis_server/lib/src/operation/operation_analysis.dart b/pkg/analysis_server/lib/src/operation/operation_analysis.dart index 07a4fdc1e0c..ec612a5568f 100644 --- a/pkg/analysis_server/lib/src/operation/operation_analysis.dart +++ b/pkg/analysis_server/lib/src/operation/operation_analysis.dart @@ -20,6 +20,68 @@ import 'package:analyzer/src/generated/html.dart'; import 'package:analyzer/src/generated/source.dart'; +/** + * Schedules sending notifications for the given [file] using the resolved + * [resolvedDartUnit]. + */ +void scheduleNotificationOperations(AnalysisServer server, String file, + LineInfo lineInfo, AnalysisContext context, CompilationUnit parsedDartUnit, + CompilationUnit resolvedDartUnit, List errors) { + // Only send notifications if the current context is the preferred + // context for the file. This avoids redundant notification messages + // being sent to the client (see dartbug.com/22210). + // TODO(paulberry): note that there is a small risk that this will cause + // notifications to be lost if the preferred context for a file changes + // while analysis is in progress (e.g. because the client sent an + // analysis.setAnalysisRoots message). + if (server.getAnalysisContext(file) != context) { + return; + } + // Dart + CompilationUnit dartUnit = + resolvedDartUnit != null ? resolvedDartUnit : parsedDartUnit; + if (resolvedDartUnit != null) { + if (server.hasAnalysisSubscription( + protocol.AnalysisService.HIGHLIGHTS, + file)) { + server.scheduleOperation( + new _DartHighlightsOperation(file, resolvedDartUnit)); + } + if (server.hasAnalysisSubscription( + protocol.AnalysisService.NAVIGATION, + file)) { + server.scheduleOperation( + new _DartNavigationOperation(file, resolvedDartUnit)); + } + if (server.hasAnalysisSubscription( + protocol.AnalysisService.OCCURRENCES, + file)) { + server.scheduleOperation( + new _DartOccurrencesOperation(file, resolvedDartUnit)); + } + if (server.hasAnalysisSubscription( + protocol.AnalysisService.OVERRIDES, + file)) { + server.scheduleOperation( + new _DartOverridesOperation(file, resolvedDartUnit)); + } + } + if (dartUnit != null) { + if (server.hasAnalysisSubscription( + protocol.AnalysisService.OUTLINE, + file)) { + server.scheduleOperation( + new _DartOutlineOperation(file, lineInfo, dartUnit)); + } + } + // errors + if (server.shouldSendErrorsNotificationFor(file)) { + server.scheduleOperation( + new _NotificationErrorsOperation(file, lineInfo, errors)); + } +} + + void sendAnalysisNotificationErrors(AnalysisServer server, String file, LineInfo lineInfo, List errors) { try { @@ -173,61 +235,17 @@ class PerformAnalysisOperation extends ServerOperation { ChangeNotice notice = notices[i]; Source source = notice.source; String file = source.fullName; - // Only send notifications if the current context is the preferred - // context for the file. This avoids redundant notification messages - // being sent to the client (see dartbug.com/22210). - // TODO(paulberry): note that there is a small risk that this will cause - // notifications to be lost if the preferred context for a file changes - // while analysis is in progress (e.g. because the client sent an - // analysis.setAnalysisRoots message). - if (server.getAnalysisContext(file) != context) { - continue; - } // Dart CompilationUnit parsedDartUnit = notice.parsedDartUnit; CompilationUnit resolvedDartUnit = notice.resolvedDartUnit; - CompilationUnit dartUnit = - resolvedDartUnit != null ? resolvedDartUnit : parsedDartUnit; - if (resolvedDartUnit != null) { - if (server.hasAnalysisSubscription( - protocol.AnalysisService.HIGHLIGHTS, - file)) { - server.addOperation( - new _DartHighlightsOperation(file, resolvedDartUnit)); - } - if (server.hasAnalysisSubscription( - protocol.AnalysisService.NAVIGATION, - file)) { - server.addOperation( - new _DartNavigationOperation(file, resolvedDartUnit)); - } - if (server.hasAnalysisSubscription( - protocol.AnalysisService.OCCURRENCES, - file)) { - server.addOperation( - new _DartOccurrencesOperation(file, resolvedDartUnit)); - } - if (server.hasAnalysisSubscription( - protocol.AnalysisService.OVERRIDES, - file)) { - server.addOperation( - new _DartOverridesOperation(file, resolvedDartUnit)); - } - } - if (dartUnit != null) { - if (server.hasAnalysisSubscription( - protocol.AnalysisService.OUTLINE, - file)) { - LineInfo lineInfo = notice.lineInfo; - server.addOperation( - new _DartOutlineOperation(file, lineInfo, dartUnit)); - } - } - // errors - if (server.shouldSendErrorsNotificationFor(file)) { - server.addOperation( - new _NotificationErrorsOperation(file, notice.lineInfo, notice.errors)); - } + scheduleNotificationOperations( + server, + file, + notice.lineInfo, + context, + parsedDartUnit, + resolvedDartUnit, + notice.errors); // done server.fileAnalyzed(notice); } diff --git a/pkg/analysis_server/test/analysis/update_content_test.dart b/pkg/analysis_server/test/analysis/update_content_test.dart index 1fbcaa3fead..f4d5bca234e 100644 --- a/pkg/analysis_server/test/analysis/update_content_test.dart +++ b/pkg/analysis_server/test/analysis/update_content_test.dart @@ -87,4 +87,43 @@ f() {} expect(filesErrors[barPath], isEmpty); }); } + + test_sendNoticesAfterNopChange() async { + createProject(); + addTestFile(''); + await server.onAnalysisComplete; + // add an overlay + server.updateContent('1', { + testFile: new AddContentOverlay('main() {} main() {}') + }); + await server.onAnalysisComplete; + // clear errors and make a no-op change + filesErrors.clear(); + server.updateContent('2', { + testFile: new ChangeContentOverlay([new SourceEdit(0, 4, 'main')]) + }); + await server.onAnalysisComplete; + // errors should have been resent + expect(filesErrors, isNotEmpty); + } + + test_sendNoticesAfterNopChange_flushedUnit() async { + createProject(); + addTestFile(''); + await server.onAnalysisComplete; + // add an overlay + server.updateContent('1', { + testFile: new AddContentOverlay('main() {} main() {}') + }); + await server.onAnalysisComplete; + // clear errors and make a no-op change + filesErrors.clear(); + server.test_flushResolvedUnit(testFile); + server.updateContent('2', { + testFile: new ChangeContentOverlay([new SourceEdit(0, 4, 'main')]) + }); + await server.onAnalysisComplete; + // errors should have been resent + expect(filesErrors, isNotEmpty); + } } diff --git a/pkg/analyzer/lib/src/generated/engine.dart b/pkg/analyzer/lib/src/generated/engine.dart index fbd1d396d42..743353abe9b 100644 --- a/pkg/analyzer/lib/src/generated/engine.dart +++ b/pkg/analyzer/lib/src/generated/engine.dart @@ -1855,6 +1855,39 @@ class AnalysisContextImpl implements InternalAnalysisContext { _pendingFutureSources.clear(); } + @override + CompilationUnit ensureAnyResolvedDartUnit(Source source) { + SourceEntry sourceEntry = _cache.get(source); + if (sourceEntry is! DartEntry) { + return null; + } + DartEntry dartEntry = sourceEntry; + // Check if there is a resolved unit. + CompilationUnit unit = dartEntry.anyResolvedCompilationUnit; + if (unit != null) { + return unit; + } + // Invalidate the flushed RESOLVED_UNIT to force it eventually. + bool shouldBeScheduled = false; + List librariesContaining = dartEntry.containingLibraries; + for (Source librarySource in librariesContaining) { + if (dartEntry.getStateInLibrary(DartEntry.RESOLVED_UNIT, librarySource) == + CacheState.FLUSHED) { + dartEntry.setStateInLibrary( + DartEntry.RESOLVED_UNIT, + librarySource, + CacheState.INVALID); + shouldBeScheduled = true; + } + } + if (shouldBeScheduled) { + _workManager.add(source, SourcePriority.UNKNOWN); + } + // We cannot provide a resolved unit right now, + // but the future analysis will. + return null; + } + @override bool exists(Source source) { if (source == null) { @@ -2184,16 +2217,12 @@ class AnalysisContextImpl implements InternalAnalysisContext { !_tryPoorMansIncrementalResolution(source, newContents)) { _sourceChanged(source); } - if (sourceEntry != null) { - sourceEntry.modificationTime = - _contentCache.getModificationStamp(source); - sourceEntry.setValue(SourceEntry.CONTENT, newContents); - } + sourceEntry.modificationTime = + _contentCache.getModificationStamp(source); + sourceEntry.setValue(SourceEntry.CONTENT, newContents); } else { - if (sourceEntry != null) { - sourceEntry.modificationTime = - _contentCache.getModificationStamp(source); - } + sourceEntry.modificationTime = + _contentCache.getModificationStamp(source); } } else if (originalContents != null) { _incrementalAnalysisCache = @@ -2201,17 +2230,15 @@ class AnalysisContextImpl implements InternalAnalysisContext { changed = newContents != originalContents; // We are removing the overlay for the file, check if the file's // contents is the same as it was in the overlay. - if (sourceEntry != null) { - try { - TimestampedData fileContents = getContents(source); - String fileContentsData = fileContents.data; - if (fileContentsData == originalContents) { - sourceEntry.modificationTime = fileContents.modificationTime; - sourceEntry.setValue(SourceEntry.CONTENT, fileContentsData); - changed = false; - } - } catch (e) { + try { + TimestampedData fileContents = getContents(source); + String fileContentsData = fileContents.data; + if (fileContentsData == originalContents) { + sourceEntry.modificationTime = fileContents.modificationTime; + sourceEntry.setValue(SourceEntry.CONTENT, fileContentsData); + changed = false; } + } catch (e) { } // If not the same content (e.g. the file is being closed without save), // then force analysis. @@ -9702,6 +9729,13 @@ abstract class InternalAnalysisContext implements AnalysisContext { */ CompilationUnit computeResolvableCompilationUnit(Source source); + /** + * Return any resolved [CompilationUnit] for the given [source] if not + * flushed, otherwise return `null` and ensures that the [CompilationUnit] + * will be eventually returned to the client from [performAnalysisTask]. + */ + CompilationUnit ensureAnyResolvedDartUnit(Source source); + /** * Return context that owns the given source. * @@ -9725,7 +9759,7 @@ abstract class InternalAnalysisContext implements AnalysisContext { * is the updated contents. If [notify] is true, a source changed event is * triggered. * - * Normally it should not be necessary for clinets to call this function, + * Normally it should not be necessary for clients to call this function, * since it will be automatically invoked in response to a call to * [applyChanges] or [setContents]. However, if this analysis context is * sharing its content cache with other contexts, then the client must diff --git a/pkg/analyzer/test/generated/engine_test.dart b/pkg/analyzer/test/generated/engine_test.dart index a30c5f63e8b..0089eb8d04e 100644 --- a/pkg/analyzer/test/generated/engine_test.dart +++ b/pkg/analyzer/test/generated/engine_test.dart @@ -6048,6 +6048,11 @@ class TestAnalysisContext implements InternalAnalysisContext { fail("Unexpected invocation of dispose"); } @override + CompilationUnit ensureAnyResolvedDartUnit(Source source) { + fail("Unexpected invocation of ensureAnyResolvedDartUnit"); + return null; + } + @override bool exists(Source source) { fail("Unexpected invocation of exists"); return false; @@ -6210,6 +6215,7 @@ class TestAnalysisContext implements InternalAnalysisContext { void setContents(Source source, String contents) { fail("Unexpected invocation of setContents"); } + @override void visitCacheItems(void callback(Source source, SourceEntry dartEntry, DataDescriptor rowDesc, CacheState state)) {