Handle ChangeType.REMOVE for any file, flush it.

Change-Id: Idb61bcb96ad5b6d29d46063fe46a9ae7413679a3
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/186440
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>

pkg/analysis_server/lib/src/analysis_server.dart

@@ -632,8 +632,7 @@ class ServerContextManagerCallbacks extends ContextManagerCallbacks {
 
   ServerContextManagerCallbacks(this.analysisServer, this.resourceProvider);
 
-  @override
+  NotificationManager get _notificationManager =>
-  NotificationManager get notificationManager =>
       analysisServer.notificationManager;
 
   @override
@@ -675,7 +674,7 @@ class ServerContextManagerCallbacks extends ContextManagerCallbacks {
       var path = result.path;
       filesToFlush.add(path);
       if (analysisServer.shouldSendErrorsNotificationFor(path)) {
-        notificationManager.recordAnalysisErrors(NotificationManager.serverId,
+        _notificationManager.recordAnalysisErrors(NotificationManager.serverId,
             path, server.doAnalysisError_listFromEngine(result));
       }
       var unit = result.unit;
@@ -683,7 +682,7 @@ class ServerContextManagerCallbacks extends ContextManagerCallbacks {
         if (analysisServer._hasAnalysisServiceSubscription(
             AnalysisService.HIGHLIGHTS, path)) {
           _runDelayed(() {
-            notificationManager.recordHighlightRegions(
+            _notificationManager.recordHighlightRegions(
                 NotificationManager.serverId,
                 path,
                 _computeHighlightRegions(unit));
@@ -692,7 +691,7 @@ class ServerContextManagerCallbacks extends ContextManagerCallbacks {
         if (analysisServer._hasAnalysisServiceSubscription(
             AnalysisService.NAVIGATION, path)) {
           _runDelayed(() {
-            notificationManager.recordNavigationParams(
+            _notificationManager.recordNavigationParams(
                 NotificationManager.serverId,
                 path,
                 _computeNavigationParams(path, unit));
@@ -701,7 +700,7 @@ class ServerContextManagerCallbacks extends ContextManagerCallbacks {
         if (analysisServer._hasAnalysisServiceSubscription(
             AnalysisService.OCCURRENCES, path)) {
           _runDelayed(() {
-            notificationManager.recordOccurrences(
+            _notificationManager.recordOccurrences(
                 NotificationManager.serverId, path, _computeOccurrences(unit));
           });
         }
@@ -753,6 +752,13 @@ class ServerContextManagerCallbacks extends ContextManagerCallbacks {
     analysisDriver.priorityFiles = analysisServer.priorityFiles.toList();
   }
 
+  @override
+  void recordAnalysisErrors(String path, List<AnalysisError> errors) {
+    filesToFlush.add(path);
+    _notificationManager.recordAnalysisErrors(
+        NotificationManager.serverId, path, errors);
+  }
+
   List<HighlightRegion> _computeHighlightRegions(CompilationUnit unit) {
     return DartUnitHighlightsComputer(unit).compute();
   }

pkg/analysis_server/lib/src/context_manager.dart

@@ -6,7 +6,6 @@ import 'dart:async';
 import 'dart:collection';
 import 'dart:core';
 
-import 'package:analysis_server/src/plugin/notification_manager.dart';
 import 'package:analysis_server/src/services/correction/fix/data_driven/transform_set_parser.dart';
 import 'package:analyzer/error/listener.dart';
 import 'package:analyzer/file_system/file_system.dart';
@@ -117,9 +116,6 @@ abstract class ContextManager {
 /// operations return data structures describing how context state should be
 /// modified.
 abstract class ContextManagerCallbacks {
-  /// Return the notification manager associated with the server.
-  AbstractNotificationManager get notificationManager;
-
   /// Called after analysis contexts are created, usually when new analysis
   /// roots are set, or after detecting a change that required rebuilding
   /// the set of analysis contexts.
@@ -141,6 +137,9 @@ abstract class ContextManagerCallbacks {
   ///
   /// TODO(scheglov) Just pass results in here?
   void listenAnalysisDriver(AnalysisDriver driver);
+
+  /// Record error information for the file with the given [path].
+  void recordAnalysisErrors(String path, List<protocol.AnalysisError> errors);
 }
 
 /// Class that maintains a mapping from included/excluded paths to a set of
@@ -268,7 +267,7 @@ class ContextManagerImpl implements ContextManager {
   /// Use the given analysis [driver] to analyze the content of the analysis
   /// options file at the given [path].
   void _analyzeAnalysisOptionsFile(AnalysisDriver driver, String path) {
-    List<protocol.AnalysisError> convertedErrors;
+    var convertedErrors = const <protocol.AnalysisError>[];
     try {
       var content = _readFile(path);
       var lineInfo = _computeLineInfo(content);
@@ -283,16 +282,13 @@ class ContextManagerImpl implements ContextManager {
       // If the file cannot be analyzed, fall through to clear any previous
       // errors.
     }
-    callbacks.notificationManager.recordAnalysisErrors(
+    callbacks.recordAnalysisErrors(path, convertedErrors);
-        NotificationManager.serverId,
-        path,
-        convertedErrors ?? <protocol.AnalysisError>[]);
   }
 
   /// Use the given analysis [driver] to analyze the content of the
   /// data file at the given [path].
   void _analyzeDataFile(AnalysisDriver driver, String path) {
-    List<protocol.AnalysisError> convertedErrors;
+    var convertedErrors = const <protocol.AnalysisError>[];
     try {
       var file = resourceProvider.getFile(path);
       var packageName = file.parent2.parent2.shortName;
@@ -308,16 +304,13 @@ class ContextManagerImpl implements ContextManager {
       // If the file cannot be analyzed, fall through to clear any previous
       // errors.
     }
-    callbacks.notificationManager.recordAnalysisErrors(
+    callbacks.recordAnalysisErrors(path, convertedErrors);
-        NotificationManager.serverId,
-        path,
-        convertedErrors ?? const <protocol.AnalysisError>[]);
   }
 
   /// Use the given analysis [driver] to analyze the content of the
   /// AndroidManifest file at the given [path].
   void _analyzeManifestFile(AnalysisDriver driver, String path) {
-    List<protocol.AnalysisError> convertedErrors;
+    var convertedErrors = const <protocol.AnalysisError>[];
     try {
       var content = _readFile(path);
       var validator =
@@ -332,16 +325,13 @@ class ContextManagerImpl implements ContextManager {
       // If the file cannot be analyzed, fall through to clear any previous
       // errors.
     }
-    callbacks.notificationManager.recordAnalysisErrors(
+    callbacks.recordAnalysisErrors(path, convertedErrors);
-        NotificationManager.serverId,
-        path,
-        convertedErrors ?? <protocol.AnalysisError>[]);
   }
 
   /// Use the given analysis [driver] to analyze the content of the pubspec file
   /// at the given [path].
   void _analyzePubspecFile(AnalysisDriver driver, String path) {
-    List<protocol.AnalysisError> convertedErrors;
+    var convertedErrors = const <protocol.AnalysisError>[];
     try {
       var content = _readFile(path);
       var node = loadYamlNode(content);
@@ -391,10 +381,7 @@ class ContextManagerImpl implements ContextManager {
       // If the file cannot be analyzed, fall through to clear any previous
       // errors.
     }
-    callbacks.notificationManager.recordAnalysisErrors(
+    callbacks.recordAnalysisErrors(path, convertedErrors);
-        NotificationManager.serverId,
-        path,
-        convertedErrors ?? <protocol.AnalysisError>[]);
   }
 
   void _checkForDataFileUpdate(String path) {
@@ -602,20 +589,21 @@ class ContextManagerImpl implements ContextManager {
             break;
           case ChangeType.REMOVE:
             analysisContext.driver.removeFile(path);
-            // TODO(scheglov) Why not `isInAnalysisRoot()`?
-            // TODO(scheglov) Why not always?
-            var resource = resourceProvider.getResource(path);
-            if (resource is File &&
-                _shouldFileBeAnalyzed(resource, mustExist: false)) {
-              callbacks.applyFileRemoved(path);
-            }
             break;
         }
       }
     }
 
-    _checkForManifestUpdate(path);
+    switch (type) {
-    _checkForDataFileUpdate(path);
+      case ChangeType.ADD:
+      case ChangeType.MODIFY:
+        _checkForManifestUpdate(path);
+        _checkForDataFileUpdate(path);
+        break;
+      case ChangeType.REMOVE:
+        callbacks.applyFileRemoved(path);
+        break;
+    }
   }
 
   /// On windows, the directory watcher may overflow, and we must recover.
@@ -652,22 +640,6 @@ class ContextManagerImpl implements ContextManager {
     return resourceProvider.getFile(path).readAsStringSync();
   }
 
-  /// Return `true` if the given [file] should be analyzed.
-  bool _shouldFileBeAnalyzed(File file, {bool mustExist = true}) {
-    for (var glob in analyzedFilesGlobs) {
-      if (glob.matches(file.path)) {
-        // Emacs creates dummy links to track the fact that a file is open for
-        // editing and has unsaved changes (e.g. having unsaved changes to
-        // 'foo.dart' causes a link '.#foo.dart' to be created, which points to
-        // the non-existent file 'username@hostname.pid'. To avoid these dummy
-        // links causing the analyzer to thrash, just ignore links to
-        // non-existent files.
-        return !mustExist || file.exists;
-      }
-    }
-    return false;
-  }
-
   /// Listens to files generated by Bazel that were found or searched for.
   ///
   /// This is handled specially because the files are outside the package

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

@@ -26,6 +26,7 @@ import 'package:analysis_server/src/lsp/mapping.dart';
 import 'package:analysis_server/src/lsp/notification_manager.dart';
 import 'package:analysis_server/src/lsp/progress.dart';
 import 'package:analysis_server/src/lsp/server_capabilities_computer.dart';
+import 'package:analysis_server/src/plugin/notification_manager.dart';
 import 'package:analysis_server/src/plugin/plugin_manager.dart';
 import 'package:analysis_server/src/protocol_server.dart' as protocol;
 import 'package:analysis_server/src/server/crash_reporting_attachments.dart';
@@ -773,10 +774,6 @@ class LspServerContextManagerCallbacks extends ContextManagerCallbacks {
 
   LspServerContextManagerCallbacks(this.analysisServer, this.resourceProvider);
 
-  @override
-  LspNotificationManager get notificationManager =>
-      analysisServer.notificationManager;
-
   @override
   void afterContextsCreated() {
     analysisServer.addContextsToDeclarationsTracker();
@@ -863,6 +860,13 @@ class LspServerContextManagerCallbacks extends ContextManagerCallbacks {
     analysisDriver.priorityFiles = analysisServer.priorityFiles.toList();
   }
 
+  @override
+  void recordAnalysisErrors(String path, List<protocol.AnalysisError> errors) {
+    filesToFlush.add(path);
+    analysisServer.notificationManager
+        .recordAnalysisErrors(NotificationManager.serverId, path, errors);
+  }
+
   bool _shouldSendDiagnostic(AnalysisError error) =>
       error.errorCode.type != ErrorType.TODO ||
       analysisServer.clientConfiguration.showTodos;

pkg/analysis_server/test/domain_analysis_test.dart

@@ -342,6 +342,7 @@ class AnalysisDomainTest extends AbstractAnalysisTest {
     if (notification.event == ANALYSIS_NOTIFICATION_FLUSH_RESULTS) {
       var decoded = AnalysisFlushResultsParams.fromNotification(notification);
       flushResults.addAll(decoded.files);
+      decoded.files.forEach(filesErrors.remove);
     }
     if (notification.event == ANALYSIS_NOTIFICATION_ERRORS) {
       var decoded = AnalysisErrorsParams.fromNotification(notification);
@@ -397,7 +398,7 @@ analyzer:
       notAnalyzed: [options_path],
     );
 
-    // Add a non-Dart file that we know how to analyze.
+    // Add 'analysis_options.yaml' that has an error.
     newFile(options_path, content: '''
 analyzer:
   error:
@@ -645,6 +646,7 @@ dependencies: true
   }
 
   Future<void> test_fileSystem_changeFile_analysisOptions() async {
+    var options_path = '$testPackageRootPath/analysis_options.yaml';
     var a_path = '$testPackageLibPath/a.dart';
     var b_path = '$testPackageLibPath/b.dart';
     var c_path = '$testPackageLibPath/c.dart';
@@ -652,7 +654,7 @@ dependencies: true
     _createFilesWithErrors([a_path, b_path, c_path]);
 
     // Exclude b.dart from analysis.
-    newFile('$testPackageRootPath/analysis_options.yaml', content: r'''
+    newFile(options_path, content: r'''
 analyzer:
   exclude:
     - lib/b.dart
@@ -668,7 +670,7 @@ analyzer:
     );
 
     // Exclude c.dart from analysis.
-    newFile('$testPackageRootPath/analysis_options.yaml', content: r'''
+    newFile(options_path, content: r'''
 analyzer:
   exclude:
     - lib/c.dart
@@ -678,9 +680,10 @@ analyzer:
     await server.onAnalysisComplete;
 
     // Errors for all files were flushed, a.dart and b.dart analyzed.
-    _assertFlushedResults([a_path, c_path]);
+    _assertFlushedResults([options_path, a_path, c_path]);
     _assertAnalyzedFiles(
       hasErrors: [a_path, b_path],
+      noErrors: [options_path],
       notAnalyzed: [c_path],
     );
   }
@@ -710,7 +713,7 @@ analyzer:
     await pumpEventQueue();
     await server.onAnalysisComplete;
 
-    // An error was reports.
+    // An error was reported.
     assertHasErrors(path);
   }
 
@@ -896,13 +899,14 @@ void f(A a) {}
   }
 
   Future<void> test_fileSystem_deleteFile_analysisOptions() async {
+    var options_path = '$testPackageRootPath/analysis_options.yaml';
     var a_path = '$testPackageLibPath/a.dart';
     var b_path = '$testPackageLibPath/b.dart';
 
     _createFilesWithErrors([a_path, b_path]);
 
     // Exclude b.dart from analysis.
-    newFile('$testPackageRootPath/analysis_options.yaml', content: r'''
+    newFile(options_path, content: r'''
 analyzer:
   exclude:
     - lib/b.dart
@@ -918,19 +922,46 @@ analyzer:
     );
 
     // Delete the options file.
-    deleteFile('$testPackageRootPath/analysis_options.yaml');
+    deleteFile(options_path);
 
     await pumpEventQueue();
     await server.onAnalysisComplete;
 
     // Errors for a.dart were flushed, a.dart and b.dart analyzed.
-    _assertFlushedResults([a_path]);
+    _assertFlushedResults([options_path, a_path]);
     _assertAnalyzedFiles(
       hasErrors: [a_path, b_path],
-      notAnalyzed: [],
+      notAnalyzed: [options_path],
     );
   }
 
+  Future<void> test_fileSystem_deleteFile_androidManifestXml() async {
+    var path = '$testPackageRootPath/AndroidManifest.xml';
+
+    newFile('$testPackageLibPath/a.dart', content: '');
+
+    // Has an error - no touch screen.
+    newFile(path, content: '<manifest/>');
+
+    newFile('$testPackageRootPath/analysis_options.yaml', content: '''
+analyzer:
+  optional-checks:
+    chrome-os-manifest-checks: true
+''');
+
+    setRoots(included: [workspaceRootPath], excluded: []);
+
+    // An error was reported.
+    _assertAnalyzedFiles(hasErrors: [path], notAnalyzed: []);
+
+    // Delete the file.
+    deleteFile(path);
+    await pumpEventQueue();
+
+    // We received a flush notification.
+    _assertAnalyzedFiles(hasErrors: [], notAnalyzed: [path]);
+  }
+
   Future<void> test_fileSystem_deleteFile_dart() async {
     var a_path = '$testPackageLibPath/a.dart';
 
@@ -941,7 +972,6 @@ analyzer:
 
     // a.dart was analyzed
     assertHasErrors(a_path);
-    forgetReceivedErrors();
 
     deleteFile(a_path);
     await pumpEventQueue();
@@ -1025,6 +1055,33 @@ void f(A a) {}
     assertNoErrorsNotification(a_path);
   }
 
+  Future<void> test_fileSystem_deleteFile_fixDataYaml() async {
+    var path = '$testPackageLibPath/fix_data.yaml';
+
+    newFile('$testPackageLibPath/a.dart', content: '');
+
+    // Make sure that it is a package.
+    writePackageConfig(
+      '$testPackageRootPath/.dart_tool/package_config.json',
+      PackageConfigFileBuilder(),
+    );
+
+    // This file has an error.
+    newFile(path, content: '0: 1');
+
+    setRoots(included: [workspaceRootPath], excluded: []);
+
+    // The file was analyzed.
+    _assertAnalyzedFiles(hasErrors: [path], notAnalyzed: []);
+
+    // Delete the file.
+    deleteFile(path);
+    await pumpEventQueue();
+
+    // We received a flush notification.
+    _assertAnalyzedFiles(hasErrors: [], notAnalyzed: [path]);
+  }
+
   Future<void> test_fileSystem_deleteFile_packageConfigJsonFile() async {
     var aaaRootPath = '/packages/aaa';
     var a_path = '$aaaRootPath/lib/a.dart';
@@ -1295,12 +1352,17 @@ void f(A a) {}
 
   void _assertAnalyzedFiles({
     @required List<String> hasErrors,
+    List<String> noErrors = const [],
     @required List<String> notAnalyzed,
   }) {
     for (var path in hasErrors) {
       assertHasErrors(path);
     }
 
+    for (var path in noErrors) {
+      assertNoErrors(path);
+    }
+
     for (var path in notAnalyzed) {
       assertNoErrorsNotification(path);
     }