Support for pubspec based contexts in subfolders.

pkg/analysis_server/lib/src/context_manager.dart

Index: pkg/analysis_server/lib/src/context_manager.dart
diff --git a/pkg/analysis_server/lib/src/context_manager.dart b/pkg/analysis_server/lib/src/context_manager.dart
index 5e759893b94a5e10f4f7a97674103c5e426b811e..58f9ee38a27ee649b909dc5d2164ebfa28eb4365 100644
--- a/pkg/analysis_server/lib/src/context_manager.dart
+++ b/pkg/analysis_server/lib/src/context_manager.dart
@@ -11,24 +11,26 @@ import 'package:analysis_server/src/package_map_provider.dart';
 import 'package:analyzer/file_system/file_system.dart';
 import 'package:analyzer/src/generated/engine.dart';
 import 'package:analyzer/src/generated/source.dart';
+import 'package:path/path.dart' as pathos;
 import 'package:watcher/watcher.dart';
 
+
+/**
+ * File name of pubspec files.
+ */
+const String PUBSPEC_NAME = 'pubspec.yaml';
+
+
 /**
  * Class that maintains a mapping from included/excluded paths to a set of
  * folders that should correspond to analysis contexts.
  */
 abstract class ContextManager {
   /**
-   * File name of pubspec files.
-   */
-  static const String PUBSPEC_NAME = 'pubspec.yaml';
-
-  /**
    * [_ContextInfo] object for each included directory in the most
    * recent successful call to [setRoots].
    */
-  Map<Folder, _ContextInfo> _currentDirectoryInfo =
-      new HashMap<Folder, _ContextInfo>();
+  Map<Folder, _ContextInfo> _contexts = new HashMap<Folder, _ContextInfo>();
 
   /**
    * The [ResourceProvider] using which paths are converted into [Resource]s.
@@ -36,12 +38,20 @@ abstract class ContextManager {
   final ResourceProvider resourceProvider;
 
   /**
+   * The [path.Context] for this manager.

[Brian Wilkerson, 2014/08/08 14:00:02]

I assume "path" was suppose to be "pathos".

As worded, this comment really doesn't tell me anything. It would be better if
you described what it's used for, such as

The context used to ...

[scheglov, 2014/08/08 15:54:33]

Done.

+   */
+  pathos.Context pathContext;
+
+
+  /**
    * Provider which is used to determine the mapping from package name to
    * package folder.
    */
   final PackageMapProvider packageMapProvider;
 
-  ContextManager(this.resourceProvider, this.packageMapProvider);
+  ContextManager(this.resourceProvider, this.packageMapProvider) {
+    pathContext = resourceProvider.pathContext;
+  }
 
   /**
    * Called when a new context needs to be created.
@@ -61,7 +71,7 @@ abstract class ContextManager {
    */
   bool isInAnalysisRoot(String path) {
     // TODO(scheglov) check for excluded paths
-    for (Folder root in _currentDirectoryInfo.keys) {
+    for (Folder root in _contexts.keys) {
       if (root.contains(path)) {
         return true;
       }
@@ -100,16 +110,32 @@ abstract class ContextManager {
     }
     Set<Folder> excludedFolders = new HashSet<Folder>();
     // diff
-    Set<Folder> currentFolders = _currentDirectoryInfo.keys.toSet();
-    Set<Folder> newFolders = includedFolders.difference(currentFolders);
-    Set<Folder> oldFolders = currentFolders.difference(includedFolders);
+    Set<Folder> currentFolders = _contexts.keys.toSet();

[Brian Wilkerson, 2014/08/08 14:00:02]

Are we making a copy to avoid modifying the keys while we're iterating over
them? I can't see where that would happen, but I might be missing something.

[scheglov, 2014/08/08 15:54:33]

We use it twice in the code below.

[Paul Berry, 2014/08/08 16:07:10]

Agreed, but if we go with my suggestions below, then the keys will be modified
while we're iterating over them, so I'd say let's keep this line as is.

One nit, though: the name "currentFolders" isn't very meaningful to me--I keep
forgetting that it is the set of folders associated with current *contexts*. 
How about if we call it "contextFolders"?

[scheglov, 2014/08/08 16:34:23]

Done.

+    Set<Folder> newFolders = new HashSet<Folder>();
+    Set<Folder> oldFolders = new HashSet<Folder>();
+    for (Folder currentFolder in currentFolders) {
+      bool isIncluded = includedFolders.any((folder) {
+        return folder.contains(currentFolder.path);
+      });
+      if (!isIncluded) {
+        oldFolders.add(currentFolder);

[Paul Berry, 2014/08/08 16:07:10]

I think it would be clearer to just do:

  _destroyContext(currentFolder);

immediately, and drop the "oldFolders" set.

[scheglov, 2014/08/08 16:34:23]

Done.

+      }
+    }
+    for (Folder includedFolder in includedFolders) {
+      bool wasIncluded = currentFolders.any((folder) {
+        return folder.contains(includedFolder.path);
+      });
+      if (!wasIncluded) {
+        newFolders.add(includedFolder);

[Paul Berry, 2014/08/08 16:07:10]

Similarly, I think it would be clearer to just to:

  _createContexts(includedFolder, false);

immediately, and drop the "newFolders" set.

[scheglov, 2014/08/08 16:34:23]

Done.

+      }
+    }
     // destroy old contexts
     for (Folder folder in oldFolders) {
       _destroyContext(folder);
     }
     // create new contexts
     for (Folder folder in newFolders) {
-      _createContext(folder);
+      _createContexts(folder, false);
     }
   }
 
@@ -120,44 +146,135 @@ abstract class ContextManager {
       List<Folder>> packageMap);
 
   /**
-   * Create a new context associated with the given folder.
+   * Create a new empty context associated with [folder].
    */
-  void _createContext(Folder folder) {
-    _ContextInfo info = new _ContextInfo();
-    _currentDirectoryInfo[folder] = info;
+  _ContextInfo _createContext(Folder folder, List<_ContextInfo> children) {
+    _ContextInfo info = new _ContextInfo(folder, children);
+    _contexts[folder] = info;
     info.changeSubscription = folder.changes.listen((WatchEvent event) {
       _handleWatchEvent(folder, info, event);
     });
-    File pubspecFile = folder.getChild(PUBSPEC_NAME);
     PackageMapInfo packageMapInfo =
         packageMapProvider.computePackageMap(folder);
     info.packageMapDependencies = packageMapInfo.dependencies;
     // TODO(paulberry): if any of the dependencies is outside of [folder],
     // we'll need to watch their parent folders as well.
     addContext(folder, packageMapInfo.packageMap);
+    return info;
+  }
+
+  /**
+   * Create a new context associated with [folder] and fills its with sources.
+   */
+  _ContextInfo _createContextWithSources(Folder folder,
+      List<_ContextInfo> children) {
+    _ContextInfo info = _createContext(folder, children);
     ChangeSet changeSet = new ChangeSet();
     _addSourceFiles(changeSet, folder, info);
     applyChangesToContext(folder, changeSet);
+    return info;
+  }
+
+  /**
+   * Creates a new context associated with [folder].
+   *
+   * If there are subfolders with 'pubspec.yaml' files, separate contexts
+   * are created for them, and excluded from the context associated with
+   * [folder].
+   *
+   * If [folder] itself contains a 'pubspec.yaml' file, subfolders are ignored.
+   *
+   * Returns create pubspec-based contexts.
+   */

[Paul Berry, 2014/08/08 16:07:10]

Can you document the meaning of "withPubspecOnly" here?

[scheglov, 2014/08/08 16:34:23]

Done.

+  List<_ContextInfo> _createContexts(Folder folder, bool withPubspecOnly) {
+    // check if there is a pubspec in the folder
+    {
+      File pubspecFile = folder.getChild(PUBSPEC_NAME);
+      if (pubspecFile.exists) {
+        _ContextInfo info = _createContextWithSources(folder, <_ContextInfo>[]);
+        return [info];
+      }
+    }
+    // try to find subfolders with pubspec files
+    List<_ContextInfo> children = <_ContextInfo>[];
+    for (Resource child in folder.getChildren()) {
+      if (child is Folder) {
+        List<_ContextInfo> childContexts = _createContexts(child, true);
+        children.addAll(childContexts);
+      }
+    }
+    // no pubspec, done
+    if (withPubspecOnly) {
+      return children;
+    }
+    // OK, create a context without a pubspec
+    _createContextWithSources(folder, children);
+    return children;
   }
 
   /**
    * Clean up and destroy the context associated with the given folder.
    */
   void _destroyContext(Folder folder) {
-    _currentDirectoryInfo[folder].changeSubscription.cancel();
-    _currentDirectoryInfo.remove(folder);
+    _contexts[folder].changeSubscription.cancel();
+    _contexts.remove(folder);
     removeContext(folder);
   }
 
+  /**
+   * Extract a new [pubspecFile]-based context from [oldInfo].
+   */
+  void _extractContext(_ContextInfo oldInfo, File pubspecFile) {
+    Folder newFolder = pubspecFile.parent;
+    _ContextInfo newInfo = _createContext(newFolder, []);
+    newInfo.parent = oldInfo;
+    // prepare sources to extract
+    Map<String, Source> extractSources = new HashMap<String, Source>();

[Paul Berry, 2014/08/08 16:07:10]

Rename to "extractedSources"

[scheglov, 2014/08/08 16:34:23]

Done.

+    oldInfo.sources.forEach((path, source) {
+      if (newFolder.contains(path)) {
+        extractSources[path] = source;
+      }
+    });
+    // update new context
+    {
+      ChangeSet changeSet = new ChangeSet();
+      extractSources.forEach((path, source) {
+        newInfo.sources[path] = source;
+        changeSet.addedSource(source);
+      });
+      applyChangesToContext(newFolder, changeSet);
+    }
+    // update old context
+    {
+      ChangeSet changeSet = new ChangeSet();
+      extractSources.forEach((path, source) {
+        oldInfo.sources.remove(path);
+        changeSet.removedSource(source);
+      });
+      applyChangesToContext(oldInfo.folder, changeSet);
+    }
+  }
+
   void _handleWatchEvent(Folder folder, _ContextInfo info, WatchEvent event) {
+    String path = event.path;
+    // maybe excluded, so other context will handle it
+    if (info.excludes(path)) {
+      return;
+    }
+    // handle the change
     switch (event.type) {
       case ChangeType.ADD:
-        if (_isInPackagesDir(event.path, folder)) {
+        if (_isInPackagesDir(path, folder)) {
           // TODO(paulberry): perhaps we should only skip packages dirs if
           // there is a pubspec.yaml?
           break;
         }
-        Resource resource = resourceProvider.getResource(event.path);
+        Resource resource = resourceProvider.getResource(path);
+        // pubspec was added, extract a new context
+        if (_isPubspec(path)) {
+          _extractContext(info, resource);
+          return;
+        }
         // If the file went away and was replaced by a folder before we
         // had a chance to process the event, resource might be a Folder.  In
         // that case don't add it.
@@ -168,21 +285,26 @@ abstract class ContextManager {
             Source source = file.createSource();
             changeSet.addedSource(source);
             applyChangesToContext(folder, changeSet);
-            info.sources[event.path] = source;
+            info.sources[path] = source;
           }
         }
         break;
       case ChangeType.REMOVE:
-        Source source = info.sources[event.path];
+        // pubspec was removed, merge the context into its parent
+        if (info.isPubspec(path)) {
+          _mergeContext(info);
+          return;
+        }
+        Source source = info.sources[path];
         if (source != null) {
           ChangeSet changeSet = new ChangeSet();
           changeSet.removedSource(source);
           applyChangesToContext(folder, changeSet);
-          info.sources.remove(event.path);
+          info.sources.remove(path);
         }
         break;
       case ChangeType.MODIFY:
-        Source source = info.sources[event.path];
+        Source source = info.sources[path];
         if (source != null) {
           ChangeSet changeSet = new ChangeSet();
           changeSet.changedSource(source);
@@ -191,7 +313,7 @@ abstract class ContextManager {
         break;
     }
 
-    if (info.packageMapDependencies.contains(event.path)) {
+    if (info.packageMapDependencies.contains(path)) {
       // TODO(paulberry): when computePackageMap is changed into an
       // asynchronous API call, we'll want to suspend analysis for this context
       // while we're rerunning "pub list", since any analysis we complete while
@@ -208,9 +330,8 @@ abstract class ContextManager {
    * directory.
    */
   bool _isInPackagesDir(String path, Folder folder) {
-    String relativePath =
-        resourceProvider.pathContext.relative(path, from: folder.path);
-    List<String> pathParts = resourceProvider.pathContext.split(relativePath);
+    String relativePath = pathContext.relative(path, from: folder.path);
+    List<String> pathParts = pathContext.split(relativePath);
     for (int i = 0; i < pathParts.length - 1; i++) {
       if (pathParts[i] == 'packages') {
         return true;
@@ -220,10 +341,36 @@ abstract class ContextManager {
   }
 
   /**
+   * Returns `true` if the given absolute [path] is a pubspec file.
+   */
+  bool _isPubspec(String path) {
+    return pathContext.basename(path) == PUBSPEC_NAME;
+  }
+
+  /**
+   * Merges [info] context into its parent.
+   */
+  void _mergeContext(_ContextInfo info) {
+    // destroy the context
+    _destroyContext(info.folder);
+    // add files to the parent context
+    _ContextInfo parentInfo = info.parent;
+    if (parentInfo != null) {
+      parentInfo.children.remove(info);
+      ChangeSet changeSet = new ChangeSet();
+      _addSourceFiles(changeSet, info.folder, parentInfo);

[Paul Berry, 2014/08/08 16:07:10]

It looks like this will re-read the directory contents.  Would it be better to
just extract the sources from the old context and merge them into the parent, as
we do in _extractContext?

[scheglov, 2014/08/08 16:34:23]

Done.

+      applyChangesToContext(parentInfo.folder, changeSet);
+    }
+  }
+
+  /**
    * Resursively adds all Dart and HTML files to the [changeSet].
    */
   static void _addSourceFiles(ChangeSet changeSet, Folder folder,
       _ContextInfo info) {
+    if (info.excludesResource(folder)) {
+      return;
+    }
     List<Resource> children = folder.getChildren();
     for (Resource child in children) {
       if (child is File) {
@@ -262,6 +409,26 @@ abstract class ContextManager {
  */
 class _ContextInfo {
   /**
+   * The [Folder] for which this information object is created.
+   */
+  final Folder folder;
+
+  /**
+   * The enclosed pubspec-based contexts.
+   */
+  final List<_ContextInfo> children;
+
+  /**
+   * The [_ContextInfo] that encloses this one.
+   */
+  _ContextInfo parent;
+
+  /**
+   * The `pubspec.yaml` file path for this context.
+   */
+  String pubspecPath;
+
+  /**
    * Stream subscription we are using to watch the context's directory for
    * changes.
    */
@@ -278,4 +445,34 @@ class _ContextInfo {
    * If any of these files changes, the package map needs to be recomputed.
    */
   Set<String> packageMapDependencies;
+
+  _ContextInfo(this.folder, this.children) {
+    pubspecPath = folder.getChild(PUBSPEC_NAME).path;
+    for (_ContextInfo child in children) {
+      child.parent = this;
+    }
+  }
+
+  /**
+   * Returns `true` if [path] is excluded, as it is in one of the children.
+   */
+  bool excludes(String path) {
+    return children.any((child) {
+      return child.folder.contains(path);
+    });
+  }
+
+  /**
+   * Returns `true` if [resource] is excldued, as it is in one of the children.
+   */
+  bool excludesResource(Resource resource) {
+    return excludes(resource.path);
+  }
+
+  /**
+   * Returns `true` if [path] is the pubspec file of this context.
+   */
+  bool isPubspec(String path) {
+    return path == pubspecPath;
+  }
 }