Skip to content

Upgrade the version solver's algorithm #1758

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jan 10, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
722 changes: 722 additions & 0 deletions doc/solver.md

Large diffs are not rendered by default.

33 changes: 27 additions & 6 deletions lib/src/package_name.dart
Original file line number Diff line number Diff line change
Expand Up @@ -48,12 +48,6 @@ abstract class PackageName {
description = null,
isMagic = true;

String toString() {
if (isRoot) return "$name (root)";
if (isMagic) return name;
return "$name from $source";
}

/// Returns a [PackageRef] with this one's [name], [source], and
/// [description].
PackageRef toRef() => isMagic
Expand Down Expand Up @@ -82,6 +76,9 @@ abstract class PackageName {
source.hashCode ^
source.hashDescription(description);
}

/// Like [toString], but leaves off some information for extra terseness.
String toTerseString();
}

/// A reference to a [Package], but not any particular version(s) of it.
Expand All @@ -98,6 +95,18 @@ class PackageRef extends PackageName {
/// Creates a reference to a magic package (see [isMagic]).
PackageRef.magic(String name) : super._magic(name);

String toString() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this still useful? Could we just make it do what toTerseString() does and only have the one method?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we do use PackageName.toString() in some places where it makes sense to provide full information about the package names.

if (isRoot) return "$name (root)";
if (isMagic) return name;
return "$name from $source";
}

String toTerseString() {
if (isMagic) return name;
if (isRoot || source.name != 'hosted') return "$name";
return "$name from $source";
}

bool operator ==(other) => other is PackageRef && samePackage(other);
}

Expand Down Expand Up @@ -146,6 +155,12 @@ class PackageId extends PackageName {
if (isMagic) return name;
return "$name $version from $source";
}

String toTerseString() {
if (isMagic) return name;
if (isRoot || source.name == 'hosted') return "$name $version";
return "$name $version from $source";
}
}

/// A reference to a constrained range of versions of one package.
Expand Down Expand Up @@ -214,6 +229,12 @@ class PackageRange extends PackageName {
return "$prefix ($description)";
}

String toTerseString() {
if (isMagic) return name;
if (isRoot || source.name == 'hosted') return "$name $constraint";
return "$name $constraint from $source";
}

/// Returns a new [PackageRange] with [features] merged with [this.features].
PackageRange withFeatures(Map<String, FeatureDependency> features) {
if (features.isEmpty) return this;
Expand Down
26 changes: 26 additions & 0 deletions lib/src/solver/assignment.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
// Copyright (c) 2017, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.

import '../package_name.dart';
import 'incompatibility.dart';
import 'term.dart';

/// A term in a [PartialSolution] that tracks some additional metadata.
class Assignment extends Term {
/// The number of decisions at or before this in the [PartialSolution] that
/// contains it.
final int decisionLevel;

/// The index of this assignment in [PartialSolution.assignments].
final int index;

/// The incompatibility that caused this assignment to be derived, or `null`
/// if the assignment isn't a derivation.
final Incompatibility cause;

Assignment(
PackageName package, bool isPositive, this.decisionLevel, this.index,
{this.cause})
: super(package, isPositive);
}
120 changes: 0 additions & 120 deletions lib/src/solver/cache.dart

This file was deleted.

97 changes: 97 additions & 0 deletions lib/src/solver/incompatibility.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
// Copyright (c) 2017, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.

import '../package_name.dart';
import 'term.dart';

/// A set of mutually-incompatible terms.
///
/// See https://github.com/dart-lang/pub/tree/master/doc/solver.md#incompatibility.
class Incompatibility {
/// The mutually-incompatibile terms.
final List<Term> terms;

/// Creates an incompatibility with [terms].
///
/// This normalizes [terms] so that each package has at most one term
/// referring to it.
factory Incompatibility(List<Term> terms) {
if (terms.length == 1 ||
// Short-circuit in the common case of a two-term incompatibility with
// two different packages (for example, a dependency).
(terms.length == 2 &&
terms.first.package.name != terms.last.package.name)) {
return new Incompatibility._(terms);
}

// Coalesce multiple terms about the same package if possible.
var byName = <String, Map<PackageRef, Term>>{};
for (var term in terms) {
var byRef = byName.putIfAbsent(term.package.name, () => {});
var ref = term.package.toRef();
if (byRef.containsKey(ref)) {
byRef[ref] = byRef[ref].intersect(term);

// If we have two terms that refer to the same package but have a null
// intersection, they're mutually exclusive, making this incompatibility
// irrelevant, since we already know that mutually exclusive version
// ranges are incompatible. We should never derive an irrelevant
// incompatibility.
assert(byRef[ref] != null);
} else {
byRef[ref] = term;
}
}

return new Incompatibility._(byName.values.expand((byRef) {
// If there are any positive terms for a given package, we can discard
// any negative terms.
var positiveTerms =
byRef.values.where((term) => term.isPositive).toList();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the toList() needed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not strictly, but it avoids re-doing work when we call isNotEmpty.

if (positiveTerms.isNotEmpty) return positiveTerms;

return byRef.values;
}).toList());
}

Incompatibility._(this.terms);

String toString() {
if (terms.length == 1) {
var term = terms.single;
return "${term.package.toTerseString()} is "
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice if toString() had the right output so you didn't need all of the explicit toTerseString() calls.

If we do want both, maybe move the old behavior to toVerboseString()?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes more sense to have the default behavior—which is more likely to come up during debugging and so on—use the more verbose output that provides full information about what's going on.

"${term.isPositive ? 'forbidden' : 'required'}";
}

if (terms.length == 2) {
var term1 = terms.first;
var term2 = terms.last;
if (term1.isPositive != term2.isPositive) {
var positive = (term1.isPositive ? term1 : term2).package;
var negative = (term1.isPositive ? term2 : term1).package;
return "if ${positive.toTerseString()} then ${negative.toTerseString()}";
} else if (term1.isPositive) {
return "${term1.package.toTerseString()} is incompatible with "
"${term2.package.toTerseString()}";
} else {
return "either ${term1.package.toTerseString()} or "
"${term2.package.toTerseString()}";
}
}

var positive = <String>[];
var negative = <String>[];
for (var term in terms) {
(term.isPositive ? positive : negative).add(term.package.toTerseString());
}

if (positive.isNotEmpty && negative.isNotEmpty) {
return "if ${positive.join(' and ')} then ${negative.join(' and ')}";
} else if (positive.isNotEmpty) {
return "one of ${positive.join(' or ')} must be false";
} else {
return "one of ${negative.join(' or ')} must be true";
}
}
}
Loading