Code review changes

Validate package names in CS.Input.  Fix comments.  Remove dead code.
Use an equality requirement at the end of logic-solver's minMax.

packages/constraint-solver/constraint-solver-input.js

@@ -38,6 +38,18 @@ CS.Input = function (dependencies, constraints, catalogCache, options) {
   check(self.previousSolution, Match.OneOf(Object, null));
 
   self.catalogCache = catalogCache;
+  check(self.catalogCache, CS.CatalogCache);
+  // The catalog presumably has valid package names in it, but make sure
+  // there aren't any characters in there somehow that will trip us up
+  // with creating valid variable strings.
+  self.catalogCache.eachPackage(function (packageName) {
+    validatePackageName(packageName);
+  });
+  self.catalogCache.eachPackageVersion(function (packageName, depsMap) {
+    _.each(depsMap, function (deps, depPackageName) {
+      validatePackageName(depPackageName);
+    });
+  });
 
   _.each(self.dependencies, validatePackageName);
   _.each(self.upgrade, validatePackageName);
@@ -59,7 +71,7 @@ CS.Input = function (dependencies, constraints, catalogCache, options) {
   });
 };
 
-var validatePackageName = function (name) {
+validatePackageName = function (name) {
   PV.validatePackageName(name);
   // we have some hard constraints of our own, so check them
   // in case validatePackageName isn't.
@@ -67,6 +79,9 @@ var validatePackageName = function (name) {
     throw new Error("First character of package name cannot be: " +
                     name.charAt(0));
   }
+  if (/ /.test(name)) {
+    throw new Error("No space allowed in package name");
+  }
 };
 
 CS.Input.prototype.isKnownPackage = function (p) {

packages/constraint-solver/solver.js

@@ -71,15 +71,19 @@ CS.Solver.prototype.analyze = function () {
   analysis.allowedVersions = {};
   analysis.packagesWithNoAllowedVersions = {}; // package -> [constraints]
 
-  // process top-level constraints, applying them right now by limiting
-  // what package versions we even consider (e.g. create variables for).
-  // we won't even consider versions that don't match them.
-  // this speeds up solving, especially when we have equality
-  // constraints.  we can't throw any errors yet, because
-  // `input.constraints` doesn't establish any dependencies (so we
-  // don't know if it's a problem that some package has no legal
-  // versions), but we can track such packages in packagesWithNoAllowedVersions
-  // so that we throw a good error later.
+  // Process top-level constraints, applying them right now by
+  // limiting what package versions we even consider.  This speeds up
+  // solving, especially given the equality constraints on core
+  // packages.  For versions we don't allow, we get to avoid generating
+  // Constraint objects for their constraints, which saves us both
+  // clause generation time and solver work up through the point where we
+  // determine there are no conflicts between constraints.
+  //
+  // we can't throw any errors yet, because `input.constraints`
+  // doesn't establish any dependencies (so we don't know if it's a
+  // problem that some package has no legal versions), but we can
+  // track such packages in packagesWithNoAllowedVersions so that we
+  // throw a good error later.
   _.each(_.groupBy(input.constraints, 'package'), function (cs, p) {
     var versions = cache.getPackageVersions(p);
     if (! versions.length) {
@@ -127,9 +131,9 @@ CS.Solver.prototype.analyze = function () {
   ////////// ANALYZE REACHABILITY
 
   // A "reachable" package is one that is either a root dependency or
-  // a strong dependency of any version of a reachable package.
+  // a strong dependency of any "allowed" version of a reachable package.
   // In other words, we walk all strong dependencies starting
-  // with the root dependencies, and visiting all versions of each
+  // with the root dependencies, and visiting all allowed versions of each
   // package.
   //
   // This analysis is mainly done for performance, because if there are
@@ -471,7 +475,7 @@ CS.Solver.prototype.getSolution = function (options) {
 
   // generate package version variables for known, reachable packages
   _.each(_.keys(analysis.reachablePackages), function (p) {
-    if (! analysis.packagesWithNoAllowedVersions[p]) {
+    if (! _.has(analysis.packagesWithNoAllowedVersions, p)) {
       var versionVars = _.map(self.getVersions(p),
                               function (v) {
                                 return pvVar(p, v);
@@ -853,15 +857,6 @@ CS.Solver.prototype.getPathsToPackageVersion = function (packageAndVersion) {
   var solution = self.solution;
 
   var versionMap = self.currentVersionMap();
-  // Return list of package names of strong dependencies of `package`
-  var getDeps = function (package) {
-    var deps = cache.getDependencyMap(package, versionMap[package]);
-    return _.map(_.filter(deps, function (dep) {
-      return ! dep.isWeak;
-    }), function (dep) {
-      return dep.packageConstraint.package;
-    });
-  };
   var hasDep = function (p1, p2) {
     // Include weak dependencies, because their constraints matter.
     return _.has(cache.getDependencyMap(p1, versionMap[p1]), p2);

packages/logic-solver/optimize.js

@@ -88,8 +88,7 @@ var minMax = function (solver, solution, costTerms, costWeights, options, isMin)
     }
     solver.forbid(nonZeroTerms);
   } else {
-    solver.require((isMin ? Logic.lessThanOrEqual : Logic.greaterThanOrEqual)(
-      weightedSum, Logic.constantBits(curCost)));
+    solver.require(Logic.equalBits(weightedSum, Logic.constantBits(curCost)));
   }
 
   if (progress) {