Show a better error message on no version errors.

(Also includes a little solver optimization I stumbled onto.)

BUG=https://code.google.com/p/dart/issues/detail?id=18300
R=nweiz@google.com

Review URL: https://codereview.chromium.org//242373006

git-svn-id: https://dart.googlecode.com/svn/branches/bleeding_edge/dart@35230 260f80e4-7a28-3924-810f-c04153c831b5

sdk/lib/_internal/pub/lib/src/solver/backtracking_solver.dart

@@ -144,14 +144,7 @@ class BacktrackingSolver {
       // Gather some solving metrics.
       var buffer = new StringBuffer();
       buffer.writeln('${runtimeType} took ${stopwatch.elapsed} seconds.');
-      buffer.writeln(
-          '- Requested ${cache.versionCacheMisses} version lists');
-      buffer.writeln(
-          '- Looked up ${cache.versionCacheHits} cached version lists');
-      buffer.writeln(
-          '- Requested ${cache.pubspecCacheMisses} pubspecs');
-      buffer.writeln(
-          '- Looked up ${cache.pubspecCacheHits} cached pubspecs');
+      buffer.writeln(cache.describeResults());
       log.solver(buffer);
     });
   }
@@ -491,11 +484,11 @@ class Traverser {
       for (var dep in deps) {
         if (!dep.isRoot && !_solver.sources.contains(dep.source)) {
           throw new UnknownSourceException(id.name,
-              [new Dependency(id.name, dep)]);
+              [new Dependency(id.name, id.version, dep)]);
         }
       }
 
-      return _traverseDeps(id.name, new DependencyQueue(_solver, deps));
+      return _traverseDeps(id, new DependencyQueue(_solver, deps));
     });
   }
 
@@ -504,7 +497,8 @@ class Traverser {
   /// Desctructively modifies [deps]. Completes to a list of packages if the
   /// traversal is complete. Completes it to an error if a failure occurred.
   /// Otherwise, recurses.
-  Future<List<PackageId>> _traverseDeps(String depender, DependencyQueue deps) {
+  Future<List<PackageId>> _traverseDeps(PackageId depender,
+      DependencyQueue deps) {
     // Move onto the next package if we've traversed all of these references.
     if (deps.isEmpty) return _traversePackage();
 
@@ -514,7 +508,7 @@ class Traverser {
 
         // Add the dependency.
         var dependencies = _getDependencies(dep.name);
-        dependencies.add(new Dependency(depender, dep));
+        dependencies.add(new Dependency(depender.name, depender.version, dep));
 
         // If the package is barback, pub has an implicit version constraint on
         // it since pub itself uses barback too. Note that we don't check for
@@ -534,7 +528,7 @@ class Traverser {
           // find barback.
           var barbackDep = new PackageDep(dep.name, dep.source,
               barback.supportedVersions, dep.description);
-          dependencies.add(new Dependency("pub itself", barbackDep));
+          dependencies.add(new Dependency("pub itself", null, barbackDep));
         }
 
         var constraint = _getConstraint(dep.name);
@@ -578,7 +572,7 @@ class Traverser {
 
       if (allowed.isEmpty) {
         _solver.logSolve('no versions for ${dep.name} match $constraint');
-        throw new NoVersionException(dep.name, constraint,
+        throw new NoVersionException(dep.name, null, constraint,
             _getDependencies(dep.name));
       }
 
@@ -608,7 +602,7 @@ class Traverser {
   /// other dependencies on the same package. Throws a [SolveFailure]
   /// exception if not. Only validates sources and descriptions, not the
   /// version.
-  void _validateDependency(PackageDep dep, String depender) {
+  void _validateDependency(PackageDep dep, PackageId depender) {
     // Make sure the dependencies agree on source and description.
     var required = _getRequired(dep.name);
     if (required == null) return;
@@ -618,7 +612,7 @@ class Traverser {
       _solver.logSolve('source mismatch on ${dep.name}: ${required.dep.source} '
                        '!= ${dep.source}');
       throw new SourceMismatchException(dep.name,
-          [required, new Dependency(depender, dep)]);
+          [required, new Dependency(depender.name, depender.version, dep)]);
     }
 
     // Make sure all of the existing descriptions match the new reference.
@@ -627,7 +621,7 @@ class Traverser {
       _solver.logSolve('description mismatch on ${dep.name}: '
                        '${required.dep.description} != ${dep.description}');
       throw new DescriptionMismatchException(dep.name,
-          [required, new Dependency(depender, dep)]);
+          [required, new Dependency(depender.name, depender.version, dep)]);
     }
   }
 
@@ -643,7 +637,7 @@ class Traverser {
     // Make sure it meets the constraint.
     if (!dep.constraint.allows(selected.version)) {
       _solver.logSolve('selection $selected does not match $constraint');
-      throw new NoVersionException(dep.name, constraint,
+      throw new NoVersionException(dep.name, selected.version, constraint,
                                    _getDependencies(dep.name));
     }
 

sdk/lib/_internal/pub/lib/src/solver/dependency_queue.dart

@@ -130,6 +130,18 @@ class DependencyQueue {
     }
 
     return _solver.cache.getVersions(dep.toRef()).then((versions) {
+      // If the root package depends on this one, ignore versions that don't
+      // match that constraint. Since the root package's dependency constraints
+      // won't change during solving, we can safely filter out packages that
+      // don't meet it.
+      for (var rootDep in _solver.root.immediateDependencies) {
+        if (rootDep.name == dep.name) {
+          versions = versions.where(
+              (id) => rootDep.constraint.allows(id.version));
+          break;
+        }
+      }
+
       return versions.length;
     }).catchError((error, trace) {
       // If it fails for any reason, just treat that as no versions. This

sdk/lib/_internal/pub/lib/src/solver/version_solver.dart

@@ -116,19 +116,19 @@ class PubspecCache {
 
   /// The number of times a version list was requested and it wasn't cached and
   /// had to be requested from the source.
-  int versionCacheMisses = 0;
+  int _versionCacheMisses = 0;
 
   /// The number of times a version list was requested and the cached version
   /// was returned.
-  int versionCacheHits = 0;
+  int _versionCacheHits = 0;
 
   /// The number of times a pubspec was requested and it wasn't cached and had
   /// to be requested from the source.
-  int pubspecCacheMisses = 0;
+  int _pubspecCacheMisses = 0;
 
   /// The number of times a pubspec was requested and the cached version was
   /// returned.
-  int pubspecCacheHits = 0;
+  int _pubspecCacheHits = 0;
 
   PubspecCache(this._sources);
 
@@ -141,11 +141,11 @@ class PubspecCache {
   Future<Pubspec> getPubspec(PackageId id) {
     // Complete immediately if it's already cached.
     if (_pubspecs.containsKey(id)) {
-      pubspecCacheHits++;
+      _pubspecCacheHits++;
       return new Future<Pubspec>.value(_pubspecs[id]);
     }
 
-    pubspecCacheMisses++;
+    _pubspecCacheMisses++;
 
     var source = _sources[id.source];
     return source.describe(id).then((pubspec) {
@@ -172,11 +172,10 @@ class PubspecCache {
     // See if we have it cached.
     var versions = _versions[package];
     if (versions != null) {
-      versionCacheHits++;
+      _versionCacheHits++;
       return new Future.value(versions);
     }
-
-    versionCacheMisses++;
+    _versionCacheMisses++;
 
     var source = _sources[package.source];
     return source.getVersions(package.name, package.description)
@@ -194,6 +193,49 @@ class PubspecCache {
   /// Returns the previously cached list of versions for the package identified
   /// by [package] or returns `null` if not in the cache.
   List<PackageId> getCachedVersions(PackageRef package) => _versions[package];
+
+  /// Returns a user-friendly output string describing metrics of the solve.
+  String describeResults() {
+    var results = '''- Requested $_versionCacheMisses version lists
+- Looked up $_versionCacheHits cached version lists
+- Requested $_pubspecCacheMisses pubspecs
+- Looked up $_pubspecCacheHits cached pubspecs
+''';
+
+    // Uncomment this to dump the visited package graph to JSON.
+    //results += _debugWritePackageGraph();
+
+    return results;
+  }
+
+  /// This dumps the set of packages that were looked at by the solver to a
+  /// JSON map whose format matches the map passed to [testResolve] in the
+  /// version solver unit tests.
+  ///
+  /// If a real-world version solve is failing, this can be used to mirror that
+  /// data to build a regression test using mock packages.
+  String _debugDescribePackageGraph() {
+    var packages = {};
+    _pubspecs.forEach((id, pubspec) {
+      var deps = {};
+      packages["${id.name} ${id.version}"] = deps;
+
+      for (var dep in pubspec.dependencies) {
+        deps[dep.name] = dep.constraint.toString();
+      }
+    });
+
+    // Add in the packages that we know of but didn't need their pubspecs.
+    _versions.forEach((ref, versions) {
+      for (var id in versions) {
+        packages.putIfAbsent("${id.name} ${id.version}", () => {});
+      }
+    });
+
+    // TODO(rnystrom): Include dev dependencies and dependency overrides.
+
+    return JSON.encode(packages);
+  }
 }
 
 /// A reference from a depending package to a package that it depends on.
@@ -201,12 +243,17 @@ class Dependency {
   /// The name of the package that has this dependency.
   final String depender;
 
+  /// The version of the depender that has this dependency.
+  ///
+  /// This will be `null` when [depender] is the magic "pub itself" dependency.
+  final Version dependerVersion;
+
   /// The package being depended on.
   final PackageDep dep;
 
-  Dependency(this.depender, this.dep);
+  Dependency(this.depender, this.dependerVersion, this.dep);
 
-  String toString() => '$depender -> $dep';
+  String toString() => '$depender $dependerVersion -> $dep';
 }
 
 /// Base class for all failures that can occur while trying to resolve versions.
@@ -238,16 +285,16 @@ abstract class SolveFailure implements ApplicationException {
     var buffer = new StringBuffer();
     buffer.write("$_message:");
 
-    var map = {};
-    for (var dep in dependencies) {
-      map[dep.depender] = dep.dep;
-    }
+    var sorted = dependencies.toList();
+    sorted.sort((a, b) => a.depender.compareTo(b.depender));
 
-    var names = ordered(map.keys);
-
-    for (var name in names) {
+    for (var dep in sorted) {
       buffer.writeln();
-      buffer.write("- $name ${_describeDependency(map[name])}");
+      buffer.write("- ${log.bold(dep.depender)}");
+      if (dep.dependerVersion != null) {
+        buffer.write(" ${dep.dependerVersion}");
+      }
+      buffer.write(" ${_describeDependency(dep.dep)}");
     }
 
     return buffer.toString();
@@ -275,12 +322,27 @@ class BadSdkVersionException extends SolveFailure {
 class NoVersionException extends SolveFailure {
   final VersionConstraint constraint;
 
-  NoVersionException(String package, this.constraint,
+  /// The last selected version of the package that failed to meet the new
+  /// constraint.
+  ///
+  /// This will be `null` when the failure occurred because there are no
+  /// versions of the package *at all* that match the constraint. It will be
+  /// non-`null` when a version was selected, but then the solver tightened a
+  /// constraint such that that version was no longer allowed.
+  final Version version;
+
+  NoVersionException(String package, this.version, this.constraint,
       Iterable<Dependency> dependencies)
       : super(package, dependencies);
 
-  String get _message => "Package $package has no versions that match "
-      "$constraint derived from";
+  String get _message {
+    if (version == null) {
+      return "Package $package has no versions that match $constraint derived "
+          "from";
+    }
+
+    return "Package $package $version does not match $constraint derived from";
+  }
 }
 
 // TODO(rnystrom): Report the list of depending packages and their constraints.

sdk/lib/_internal/pub/test/get/path/nonexistent_dir_test.dart

@@ -20,6 +20,6 @@ main() {
 
     pubGet(error: """Could not find package foo at "$badPath".
 Depended on by:
-- myapp""");
+- myapp 0.0.0""");
   });
 }

sdk/lib/_internal/pub/test/hosted/offline_test.dart

@@ -61,7 +61,7 @@ main() {
 
       pubCommand(command, args: ['--offline'], error:
           "Package foo has no versions that match >2.0.0 derived from:\n"
-          "- myapp depends on version >2.0.0");
+          "- myapp 0.0.0 depends on version >2.0.0");
     });
   });
 }

sdk/lib/_internal/pub/test/implicit_barback_dependency_test.dart

@@ -115,7 +115,7 @@ main() {
 
     pubGet(error: """
 Package barback has no versions that match >=$current <$max derived from:
-- myapp depends on version any
+- myapp 0.0.0 depends on version any
 - pub itself depends on version >=$current <$max""");
   });
 
@@ -129,7 +129,7 @@ Package barback has no versions that match >=$current <$max derived from:
 
     pubGet(error: """
 Incompatible version constraints on barback:
-- myapp depends on version $previous
+- myapp 0.0.0 depends on version $previous
 - pub itself depends on version >=$current <$max""");
   });
 }

sdk/lib/_internal/pub/test/test_pub.dart

@@ -611,7 +611,7 @@ void ensureGit() {
 /// [hosted] is a list of package names to version strings for dependencies on
 /// hosted packages.
 void createLockFile(String package, {Iterable<String> sandbox,
-    Iterable<String> pkg, Map<String, Version> hosted}) {
+    Iterable<String> pkg, Map<String, String> hosted}) {
   var dependencies = {};
 
   if (sandbox != null) {

sdk/lib/_internal/pub/test/version_solver_test.dart

@@ -413,6 +413,32 @@ unsolvable() {
     'a 1.0.0': {},
     'b 1.0.0': {}
   }, error: noVersion(['myapp', 'b']), maxTries: 1);
+
+
+  // This is a regression test for #18300.
+  testResolve('...', {
+    "myapp 0.0.0": {
+      "angular": "any",
+      "collection": "any"
+    },
+    "analyzer 0.12.2": {},
+    "angular 0.10.0": {
+      "di": ">=0.0.32 <0.1.0",
+      "collection": ">=0.9.1 <1.0.0"
+    },
+    "angular 0.9.11": {
+      "di": ">=0.0.32 <0.1.0",
+      "collection": ">=0.9.1 <1.0.0"
+    },
+    "angular 0.9.10": {
+      "di": ">=0.0.32 <0.1.0",
+      "collection": ">=0.9.1 <1.0.0"
+    },
+    "collection 0.9.0": {},
+    "collection 0.9.1": {},
+    "di 0.0.37": {"analyzer": ">=0.13.0 <0.14.0"},
+    "di 0.0.36": {"analyzer": ">=0.13.0 <0.14.0"}
+  }, error: noVersion(['myapp', 'angular', 'collection']), maxTries: 9);
 }
 
 badSource() {
@@ -681,6 +707,27 @@ backtracking() {
     'c': '2.0.0'
   }, maxTries: 2);
 
+  // This is similar to the above test. When getting the number of versions of
+  // a package to determine which to traverse first, versions that are
+  // disallowed by the root package's constraints should not be considered.
+  // Here, foo has more versions of bar in total (4), but fewer that meet
+  // myapp's constraints (only 2). There is no solution, but we will do less
+  // backtracking if foo is tested first.
+  testResolve('take root package constraints into counting versions', {
+    "myapp 0.0.0": {
+      "foo": ">2.0.0",
+      "bar": "any"
+    },
+    "foo 1.0.0": {"none": "2.0.0"},
+    "foo 2.0.0": {"none": "2.0.0"},
+    "foo 3.0.0": {"none": "2.0.0"},
+    "foo 4.0.0": {"none": "2.0.0"},
+    "bar 1.0.0": {},
+    "bar 2.0.0": {},
+    "bar 3.0.0": {},
+    "none 1.0.0": {}
+  }, error: noVersion(["foo", "none"]), maxTries: 2);
+
   // This sets up a hundred versions of foo and bar, 0.0.0 through 9.9.0. Each
   // version of foo depends on a baz with the same major version. Each version
   // of bar depends on a baz with the same minor version. There is only one
@@ -1372,7 +1419,7 @@ Package mockPackage(PackageId id, Map dependencyStrings, Map overrides) {
 /// The "from mock" optional suffix is the name of a source for the package.
 /// If omitted, it defaults to "mock1".
 PackageId parseSpec(String text, [String version]) {
-  var pattern = new RegExp(r"(([a-z]*)(-[a-z]+)?)( ([^ ]+))?( from (.*))?$");
+  var pattern = new RegExp(r"(([a-z_]*)(-[a-z_]+)?)( ([^ ]+))?( from (.*))?$");
   var match = pattern.firstMatch(text);
   if (match == null) {
     throw new FormatException("Could not parse spec '$text'.");