Allow changing a file from a part to a library.

lib/src/dependency_graph.dart

Index: lib/src/dependency_graph.dart
diff --git a/lib/src/dependency_graph.dart b/lib/src/dependency_graph.dart
index 1092d6255dafc3a747ec7432206600851581eb7c..a6a4fea4541944806a19bf00e19834fcac869e64 100644
--- a/lib/src/dependency_graph.dart
+++ b/lib/src/dependency_graph.dart
@@ -43,26 +43,15 @@ class SourceGraph {
   SourceGraph(this._context, this._options);
 
   /// Node associated with a resolved [uri].
-  SourceNode nodeFromUri(Uri uri, [bool isPart = false]) {
+  SourceNode nodeFromUri(Uri uri) {
     var uriString = Uri.encodeFull('$uri');
-    var kind = uriString.endsWith('.html')
-        ? SourceKind.HTML
-        : isPart ? SourceKind.PART : SourceKind.LIBRARY;
-    return nodeFor(uri, _context.sourceFactory.forUri(uriString), kind);
-  }
-
-  /// Construct the node of the given [kind] with the given [uri] and [source].
-  SourceNode nodeFor(Uri uri, Source source, SourceKind kind) {
-    // TODO(sigmund): validate canonicalization?
-    // TODO(sigmund): add support for changing a file from one kind to another
-    // (e.g.  converting a file from a part to a library).
     return nodes.putIfAbsent(uri, () {
-      if (kind == SourceKind.HTML) {
+      var source = _context.sourceFactory.forUri(uriString);
+      var extension = path.extension(uriString);
+      if (extension == '.html') {
         return new HtmlSourceNode(uri, source);
-      } else if (kind == SourceKind.LIBRARY) {
-        return new LibrarySourceNode(uri, source);
-      } else if (kind == SourceKind.PART) {
-        return new PartSourceNode(uri, source);
+      } else if (extension == '.dart') {
+        return new DartSourceNode(uri, source);
       } else {
         assert(false); // unreachable
       }
@@ -89,9 +78,14 @@ abstract class SourceNode {
   /// exports, or parts) changed after we reparsed its contents.
   bool structureChanged = false;
 
-  /// Direct dependencies (script tags for HtmlSourceNodes; imports, exports and
-  /// parts for LibrarySourceNodes).
-  Iterable<SourceNode> get directDeps;
+  /// Direct dependencies in the [SourceGraph]. These include script tags for
+  /// [HtmlSourceNode]s; and imports, exports and parts for [DartSourceNode]s.
+  Iterable<SourceNode> get allDeps;
+
+  /// Like [allDeps] but excludes parts for [DartSourceNode]s. For many
+  /// operations we mainly care about dependencies at the library level, so
+  /// parts are excluded from this list.
+  Iterable<SourceNode> get depsWithoutParts;
 
   SourceNode(this.uri, this.source);
 
@@ -117,7 +111,10 @@ class HtmlSourceNode extends SourceNode {
   Set<LibrarySourceNode> scripts = new Set<LibrarySourceNode>();
 
   @override
-  Iterable<SourceNode> get directDeps => scripts;
+  Iterable<SourceNode> get allDeps => scripts;
+
+  @override
+  Iterable<SourceNode> get depsWithoutParts => scripts;
 
   /// Parsed document, updated whenever [update] is invoked.
   Document document;
@@ -156,41 +153,46 @@ class HtmlSourceNode extends SourceNode {
   }
 }
 
-/// A node representing a Dart part.
-class PartSourceNode extends SourceNode {
-  final Iterable<SourceNode> directDeps = const [];
-  PartSourceNode(uri, source) : super(uri, source);
-}
+/// A node representing a Dart library or part.
+class DartSourceNode extends SourceNode {
+  /// Set of imported libraries (empty for part files).
+  Set<DartSourceNode> imports = new Set<DartSourceNode>();
+
+  /// Set of exported libraries (empty for part files).
+  Set<DartSourceNode> exports = new Set<DartSourceNode>();
 
-/// A node representing a Dart library.
-class LibrarySourceNode extends SourceNode {
-  LibrarySourceNode(uri, source) : super(uri, source);
+  /// Parts of this library (empty for part files).
+  Set<DartSourceNode> parts = new Set<DartSourceNode>();
 
-  Set<LibrarySourceNode> imports = new Set<LibrarySourceNode>();
-  Set<LibrarySourceNode> exports = new Set<LibrarySourceNode>();
-  Set<PartSourceNode> parts = new Set<PartSourceNode>();
+  DartSourceNode(uri, source) : super(uri, source);
 
-  Iterable<SourceNode> get directDeps =>
+  @override
+  Iterable<SourceNode> get allDeps => 
       [imports, exports, parts].expand((e) => e);
 
+  @override
+  Iterable<SourceNode> get depsWithoutParts =>
+      [imports, exports].expand((e) => e);
+
   LibraryInfo info;
 
   void update(SourceGraph graph) {
     super.update(graph);
+
     if (needsRebuild && source.contents.data != null) {
       // If the defining compilation-unit changed, the structure might have
       // changed.
       var unit = parseDirectives(source.contents.data, name: source.fullName);
-      var newImports = new Set<LibrarySourceNode>();
-      var newExports = new Set<LibrarySourceNode>();
-      var newParts = new Set<PartSourceNode>();
+      var newImports = new Set<DartSourceNode>();
+      var newExports = new Set<DartSourceNode>();
+      var newParts = new Set<DartSourceNode>();
       for (var d in unit.directives) {
         if (d is LibraryDirective) continue;
         var target =
             ParseDartTask.resolveDirective(graph._context, source, d, null);
         var uri = target.uri;
-        var node = graph.nodeFor(uri, target,
-            d is PartDirective ? SourceKind.PART : SourceKind.LIBRARY);
+        var node = graph.nodes.putIfAbsent(uri,
+            () => new DartSourceNode(uri, target));
         if (!node.source.exists()) {
           _log.severe(spanForNode(unit, source, d).message(
               'File $uri not found',
@@ -225,10 +227,21 @@ class LibrarySourceNode extends SourceNode {
     // The library should be marked as needing rebuild if a part changed
     // internally:
     for (var p in parts) {
-      p.update(graph);
+      p._updateAsPart(graph);
       if (p.needsRebuild) needsRebuild = true;
     }
   }
+
+  void _updateAsPart(SourceGraph graph) {
+    // For parts we don't need to look at the contents, we don't expect any
+    // structural changes, we simply clear dependencies in case this part was
+    // previously a library file, and look just for file changes.

[Jennifer Messerly, 2015/03/05 01:37:01]

just curious: how does the other direction work? part -> library?

[Siggi Cherem (dart-lang), 2015/03/06 18:52:06]

Good question - initially I was leaving the imports/exports/parts empty and
assume that the user would have to modify the file to get some imports in the
library in place.

Turns out that had a potential problem, it was sensitive to the order of the
operations. In particular, if you first edit the part and make it a library, and
then edit the library that included the part and switch there to include an
import, then we would have had an inconsistent graph.

After thinking more about it, I went ahead and changed the implementation.
Instead of clearing the imports here I'm tracking dependencies for parts, even
though we don't traverse them. That way, when we first switch over the directive
to an import, we'll notice it and can enqueue the work.

I added a bunch of tests now to cover all these transitions.

+    imports.clear();
+    exports.clear();
+    parts.clear();
+    super.update(graph);
+  }
+
 }
 
 /// Updates the structure and `needsRebuild` marks in nodes of [graph] reachable
@@ -243,13 +256,14 @@ class LibrarySourceNode extends SourceNode {
 /// changes (e.g. when the API of a dependency changed) are handled later in
 /// [rebuild].
 void refreshStructureAndMarks(SourceNode start, SourceGraph graph) {
-  visitInPreOrder(start, (n) => n.update(graph));
+  visitInPreOrder(start, (n) => n.update(graph), includeParts: false);
 }
 
 /// Clears all the `needsRebuild` and `structureChanged` marks in nodes
 /// reachable from [start].
 void clearMarks(SourceNode start) {
-  visitInPreOrder(start, (n) => n.needsRebuild = n.structureChanged = false);
+  visitInPreOrder(start, (n) => n.needsRebuild = n.structureChanged = false,
+      includeParts: true);
 }
 
 /// Traverses from [start] with the purpose of building any source that needs to
@@ -259,11 +273,11 @@ void clearMarks(SourceNode start) {
 /// reachable nodes. There are four rules used to decide when to rebuild a node
 /// (call [build] on a node):
 ///
-///   * Only rebuild Dart libraries ([LibrarySourceNode]) or HTML files
-///     ([HtmlSourceNode]), but never part files ([PartSourceNode]). That is
-///     because those are built as part of some library.
+///   * Only rebuild Dart libraries ([DartSourceNode]) or HTML files
+///     ([HtmlSourceNode]), but skip part files. That is because those are
+///     built as part of some library.
 ///
-///   * Always rebuild [LibrarySourceNode]s and [HtmlSourceNode]s with local
+///   * Always rebuild [DartSourceNode]s and [HtmlSourceNode]s with local
 ///     changes or changes in a part of the library. Internally this function
 ///     calls [refreshStructureAndMarks] to ensure that the graph structure is
 ///     up-to-date and that these nodes with local changes contain the
@@ -273,7 +287,7 @@ void clearMarks(SourceNode start) {
 ///     down its reachable subgraph. This is done because HTML files embed the
 ///     transitive closure of the import graph in their output.
 ///
-///   * Rebuild [LibrarySourceNode]s that depend on other [LibrarySourceNode]s
+///   * Rebuild [DartSourceNode]s that depend on other [DartSourceNode]s
 ///     whose API may have changed. The result of [build] is used to determine
 ///     whether other nodes need to be rebuilt. The function [build] is expected
 ///     to return `true` on a node `n` if it detemines other nodes that import
@@ -291,10 +305,9 @@ rebuild(SourceNode start, SourceGraph graph, bool build(SourceNode node)) {
   bool structureHasChanged = false;
 
   bool shouldBuildNode(SourceNode n) {
-    if (n is PartSourceNode) return false;
     if (n.needsRebuild) return true;
     if (n is HtmlSourceNode) return structureHasChanged;
-    return (n as LibrarySourceNode).imports
+    return (n as DartSourceNode).imports
         .any((i) => apiChangeDetected.contains(i));
   }
 
@@ -302,31 +315,42 @@ rebuild(SourceNode start, SourceGraph graph, bool build(SourceNode node)) {
     if (n.structureChanged) structureHasChanged = true;
     if (shouldBuildNode(n)) {
       if (build(n)) apiChangeDetected.add(n);
-    } else if (n is LibrarySourceNode &&
+    } else if (n is DartSourceNode &&
         n.exports.any((e) => apiChangeDetected.contains(e))) {
       apiChangeDetected.add(n);
     }
     n.needsRebuild = false;
     n.structureChanged = false;
-  });
+    if (n is DartSourceNode) {
+      n.parts.forEach((p) => p.needsRebuild = false);
+    }
+  }, includeParts: false);
 }
 
-/// Helper that runs [action] on nodes reachable from [node] in pre-order.
-visitInPreOrder(SourceNode node, void action(SourceNode node),
-    {Set<SourceNode> seen}) {
-  if (seen == null) seen = new HashSet<SourceNode>();
-  if (!seen.add(node)) return;
-  action(node);
-  node.directDeps.forEach((d) => visitInPreOrder(d, action, seen: seen));
+/// Helper that runs [action] on nodes reachable from [start] in pre-order.
+visitInPreOrder(SourceNode start, void action(SourceNode node),
+    {bool includeParts: false}) {
+  var seen = new HashSet<SourceNode>();
+  helper(SourceNode node) {
+    if (!seen.add(node)) return;
+    action(node);
+    var deps = includeParts ? node.allDeps : node.depsWithoutParts;
+    deps.forEach(helper);
+  }
+  helper(start);
 }
 
-/// Helper that runs [action] on nodes reachable from [node] in post-order.
-visitInPostOrder(SourceNode node, void action(SourceNode node),
-    {Set<SourceNode> seen}) {
-  if (seen == null) seen = new HashSet<SourceNode>();
-  if (!seen.add(node)) return;
-  node.directDeps.forEach((d) => visitInPostOrder(d, action, seen: seen));
-  action(node);
+/// Helper that runs [action] on nodes reachable from [start] in post-order.
+visitInPostOrder(SourceNode start, void action(SourceNode node),
+    {bool includeParts: false}) {
+  var seen = new HashSet<SourceNode>();
+  helper(SourceNode node) {
+    if (!seen.add(node)) return;
+    var deps = includeParts ? node.allDeps : node.depsWithoutParts;
+    deps.forEach(helper);
+    action(node);
+  }
+  helper(start);
 }
 
 bool _same(Set a, Set b) => a.length == b.length && a.containsAll(b);