Add support for package cycles linking, more tests.

pkg/analyzer/lib/src/summary/pub_summary.dart

Index: pkg/analyzer/lib/src/summary/pub_summary.dart
diff --git a/pkg/analyzer/lib/src/summary/pub_summary.dart b/pkg/analyzer/lib/src/summary/pub_summary.dart
index 21c074db34ed7751bb38b4cd0f54b8aeffefbc55..6f9da57ba975d17dbfa285772a3e7efa129f1628 100644
--- a/pkg/analyzer/lib/src/summary/pub_summary.dart
+++ b/pkg/analyzer/lib/src/summary/pub_summary.dart
@@ -28,6 +28,20 @@ import 'package:analyzer/src/util/fast_uri.dart';
 import 'package:path/path.dart' as pathos;
 
 /**
+ * Unlinked and linked information about a [PubPackage].
+ */
+class LinkedPubPackage {
+  final PubPackage package;
+  final PackageBundle unlinked;
+  final PackageBundle linked;
+
+  LinkedPubPackage(this.package, this.unlinked, this.linked);
+
+  @override
+  String toString() => package.toString();
+}
+
+/**
  * A package in the pub cache.
  */
 class PubPackage {
@@ -51,16 +65,6 @@ class PubPackage {
 }
 
 /**
- * Unlinked and linked information about a [PubPackage].
- */
-class LinkedPubPackage {
-  final PubPackage package;
-  final PackageBundle unlinked;
-  final PackageBundle linked;
-  LinkedPubPackage(this.package, this.unlinked, this.linked);
-}
-
-/**
  * Class that manages summaries for pub packages.
  *
  * The client should call [getLinkedBundles] after creating a new
@@ -140,13 +144,11 @@ class PubSummaryManager {
 
     // Create graph nodes for packages.
     List<_LinkedNode> nodes = <_LinkedNode>[];
-    Map<String, _LinkedNode> uriToNode = <String, _LinkedNode>{};
+    Map<String, _LinkedNode> packageToNode = <String, _LinkedNode>{};
     unlinkedBundles.forEach((package, unlinked) {
-      _LinkedNode node = new _LinkedNode(package, unlinked, uriToNode);
+      _LinkedNode node = new _LinkedNode(package, unlinked, packageToNode);
       nodes.add(node);
-      for (String uri in unlinked.unlinkedUnitUris) {
-        uriToNode[uri] = node;
-      }
+      packageToNode[package.name] = node;
     });
 
     // Fill the store with unlinked bundles.
@@ -175,6 +177,7 @@ class PubSummaryManager {
     }
 
     // TODO(scheglov) compute dependency hashes and write linked bundles.
+    // TODO(scheglov) don't forget to include the SDK API signature.
 
     // Done.
     return linkedPackages;
@@ -356,6 +359,23 @@ class PubSummaryManager {
   }
 
   /**
+   * If the given [uri] has the `package` scheme, return the names of the

[Paul Berry, 2016/08/10 11:58:28]

s/names/name/

[scheglov, 2016/08/10 16:08:18]

Done.

+   * package that contains the referenced resource.  Otherwise return `null`.
+   *
+   * For example `package:foo/bar.dart` => `foo`.
+   */
+  static String getPackageName(String uri) {
+    const String PACKAGE_SCHEME = 'package:';
+    if (uri.startsWith(PACKAGE_SCHEME)) {
+      int index = uri.indexOf('/');
+      if (index != -1) {
+        return uri.substring(PACKAGE_SCHEME.length, index);
+      }
+    }
+    return null;
+  }
+
+  /**
    * Return `true` if the given absolute [path] is in the pub cache.
    */
   static bool isPathInPubCache(pathos.Context pathContext, String path) {
@@ -378,34 +398,43 @@ class PubSummaryManager {
 class _LinkedNode extends Node<_LinkedNode> {
   final PubPackage package;
   final PackageBundle unlinked;
-  final Map<String, _LinkedNode> uriToNode;
+  final Map<String, _LinkedNode> packageToNode;
 
   PackageBundleBuilder linkedBuilder;
   bool failed = false;
 
-  _LinkedNode(this.package, this.unlinked, this.uriToNode);
+  _LinkedNode(this.package, this.unlinked, this.packageToNode);
 
   @override
   bool get isEvaluated => linkedBuilder != null || failed;
 
   @override
   List<_LinkedNode> computeDependencies() {
-    Set<String> referencedUris = new Set<String>();
+    Set<_LinkedNode> dependencies = new Set<_LinkedNode>();
     for (UnlinkedUnit unit in unlinked.unlinkedUnits) {
       for (UnlinkedImport import in unit.imports) {
-        String uri = import.isImplicit ? 'dart:core' : import.uri;
-        if (uri.startsWith('dart:')) {
-          // Ignore SDK imports.
-        } else if (uri.startsWith('package:')) {
-          referencedUris.add(uri);
+        String uriStr = import.isImplicit ? 'dart:core' : import.uri;
+        Uri uri = FastUri.parse(uriStr);
+        if (!uri.hasScheme) {
+          // A relative path in this package, skip it.
+        } else if (uri.scheme == 'dart') {
+          // SDK is always available.

[Paul Berry, 2016/08/10 11:58:27]

It looks like you're not going with my suggestion from the previous CL review
(consider the SDK to be a regular summary just like the others, so we don't have
to special-case "dart:" imports).  I see how your approach could work for
ordinary SDK's (not modified by _embedder.yaml and _sdkext).  But I don't see
how this is going to work for Flutter.  How do you plan to address the Flutter
use case?

[scheglov, 2016/08/10 16:08:18]

getLinkedBundles() accepts "PackageBundle sdkBundle". As I understand after
discussion with Brian, the decision about Flutter was that the Flutter package
will come with its own full SDK summary bundle. So, we will use its API
signatures and so ensure non-sharing of linked summaries for Flutter and
non-Flutter users.

[Paul Berry, 2016/08/10 21:39:37]

Ok, based on our discussions, I'm happy landing this as is and improving it in
follow-up commits.  Consider adding a TODO comment.

+          // It's API signature is always mixed in.

[Paul Berry, 2016/08/10 11:58:27]

s/It's/Its/

[scheglov, 2016/08/10 16:08:18]

Done.

+        } else if (uriStr.startsWith('package:')) {
+          String package = PubSummaryManager.getPackageName(uriStr);
+          _LinkedNode packageNode = packageToNode[package];
+          if (packageNode == null) {
+            failed = true;
+            return const <_LinkedNode>[];
+          }
+          dependencies.add(packageNode);
         } else {
           failed = true;
           return const <_LinkedNode>[];
         }
       }
     }
-    // TODO(scheglov) fail if no corresponding node
-    return referencedUris.map((uri) => uriToNode[uri]).toSet().toList();
+    return dependencies.toList();
   }
 
   @override
@@ -421,36 +450,72 @@ class _LinkedWalker extends DependencyWalker<_LinkedNode> {
   _LinkedWalker(this.store);
 
   @override
-  void evaluate(_LinkedNode v) {
-    Set<String> libraryUris = v.unlinked.unlinkedUnitUris.toSet();
-    Map<String, LinkedLibraryBuilder> map = link(libraryUris, (String absUri) {
-      LinkedLibrary dependencyLibrary = store.linkedMap[absUri];
+  void evaluate(_LinkedNode node) {

[Paul Berry, 2016/08/10 11:58:28]

There's a lot of commonality between evaluate() and evaluateScc() now.  I think
it would be better to eliminate code dupliation by replacing evaluate() with:

void evaluate(_LinkedNode node) {
  evaluateScc([node]);
}

(This has the disadvantage that we'll be doing slightly more bookkeeping during
each evaluate() call, since we'll be using the more general algorithm that
supports a cycle among packages.  But I think that's outweighed by the fact that
the code will be shorter overall, and the more complex logic in evaluateScc()
will actually get exercised in real-world cases).

[scheglov, 2016/08/10 16:08:18]

Done.

+    Set<String> libraryUris = node.unlinked.unlinkedUnitUris.toSet();
+    Map<String, LinkedLibraryBuilder> linkedLibraries =
+        link(libraryUris, (String absoluteUri) {
+      LinkedLibrary dependencyLibrary = store.linkedMap[absoluteUri];
       if (dependencyLibrary == null) {
-        // TODO(scheglov) add test
-        v.failed = true;
+        node.failed = true;
       }
       return dependencyLibrary;
-    }, (String absUri) {
-      UnlinkedUnit unlinkedUnit = store.unlinkedMap[absUri];
+    }, (String absoluteUri) {
+      UnlinkedUnit unlinkedUnit = store.unlinkedMap[absoluteUri];
       if (unlinkedUnit == null) {
-        // TODO(scheglov) add test
-        v.failed = true;
+        node.failed = true;
       }
       return unlinkedUnit;
     }, false);
-    if (!v.failed) {
+    // Assemble the linked bundle and put it into the store.
+    if (!node.failed) {
       PackageBundleAssembler assembler = new PackageBundleAssembler();
-      map.forEach((uri, linkedLibrary) {
+      linkedLibraries.forEach((uri, linkedLibrary) {
         assembler.addLinkedLibrary(uri, linkedLibrary);
       });
-      v.linkedBuilder = assembler.assemble();
-      store.addBundle(null, v.linkedBuilder);
+      node.linkedBuilder = assembler.assemble();
+      store.addBundle(null, node.linkedBuilder);
     }
   }
 
   @override
   void evaluateScc(List<_LinkedNode> scc) {
-    print('evaluateScc: $scc');
-    // TODO(scheglov): implement evaluateScc
+    Map<String, _LinkedNode> uriToNode = <String, _LinkedNode>{};
+    for (_LinkedNode node in scc) {
+      for (String uri in node.unlinked.unlinkedUnitUris) {
+        uriToNode[uri] = node;
+      }
+    }
+    Set<String> libraryUris = uriToNode.keys.toSet();
+    // Perform linking.
+    bool failed = false;
+    Map<String, LinkedLibraryBuilder> linkedLibraries =
+        link(libraryUris, (String absoluteUri) {
+      LinkedLibrary dependencyLibrary = store.linkedMap[absoluteUri];
+      if (dependencyLibrary == null) {
+        failed = true;
+      }
+      return dependencyLibrary;
+    }, (String absoluteUri) {
+      UnlinkedUnit unlinkedUnit = store.unlinkedMap[absoluteUri];
+      if (unlinkedUnit == null) {
+        failed = true;
+      }
+      return unlinkedUnit;
+    }, false);
+    // Assemble linked bundles and put them into the store.
+    if (!failed) {
+      for (_LinkedNode node in scc) {
+        PackageBundleAssembler assembler = new PackageBundleAssembler();
+        linkedLibraries.forEach((uri, linkedLibrary) {
+          if (identical(uriToNode[uri], node)) {
+            assembler.addLinkedLibrary(uri, linkedLibrary);
+          }
+        });
+        node.linkedBuilder = assembler.assemble();
+        store.addBundle(null, node.linkedBuilder);
+      }
+    } else {
+      scc.forEach((node) => node.failed = true);
+    }
   }
 }