Create a "PackageRef" class that defines a versionless package reference.

sdk/lib/_internal/pub/lib/src/package.dart

Index: sdk/lib/_internal/pub/lib/src/package.dart
diff --git a/sdk/lib/_internal/pub/lib/src/package.dart b/sdk/lib/_internal/pub/lib/src/package.dart
index ce1ad35d7a370699e156466a02c7f3e1cb5fc538..30d0770eeb355366772a6908dd0e63a6fe7731f7 100644
--- a/sdk/lib/_internal/pub/lib/src/package.dart
+++ b/sdk/lib/_internal/pub/lib/src/package.dart
@@ -36,7 +36,7 @@ class Package {
 
   /// The ids of the packages that this package depends on. This is what is
   /// specified in the pubspec when this package depends on another.
-  List<PackageRef> get dependencies => pubspec.dependencies;
+  List<PackageDep> get dependencies => pubspec.dependencies;
 
   /// Returns the path to the README file at the root of the entrypoint, or null
   /// if no README file is found. If multiple READMEs are found, this uses the
@@ -77,14 +77,14 @@ class Package {
   String toString() => '$name $version ($dir)';
 }
 
-/// An unambiguous resolved reference to a package. A package ID contains enough
-/// information to correctly install the package.
-///
-/// Note that it's possible for multiple distinct package IDs to point to
-/// different directories that happen to contain identical packages. For
-/// example, the same package may be available from multiple sources. As far as
-/// Pub is concerned, those packages are different.
-class PackageId implements Comparable<PackageId> {
+/// A reference to a [Package]. This is the common base class of [PackageId]
+/// which precisely identifies a single concrete package version in the world

[nweiz, 2013/04/25 21:07:19]

I think "in the world of packages" is more confusing than it's worth here.

[Bob Nystrom, 2013/04/26 22:17:49]

Done.

+/// of packages, and [PackageDep] which identifies one package's dependency on

[nweiz, 2013/04/25 21:07:19]

The name "PackageDep" is fine and all, but I think we should document it in
terms of its structure rather than its use. So for example, instead of saying
"which identifies one package's dependency on another", say "which identifies a
range of versions of a package."

[Bob Nystrom, 2013/04/26 22:17:49]

Done.

+/// another. This class represents a reference to a conceptually identified
+/// package.

[nweiz, 2013/04/25 21:07:19]

"Conceptually identified" is a confusing term that isn't defined anywhere. I'd
rather be more explicit and say "a reference to a package, but not any specific
version(s) of that package".

[Bob Nystrom, 2013/04/26 22:17:49]

Done.

+class PackageRef {
+  PackageRef(this.name, this.source, this.description);
+
   /// The name of the package being identified.
   final String name;
 
@@ -92,38 +92,89 @@ class PackageId implements Comparable<PackageId> {
   /// this is a root package ID, this will be `null`.
   final Source source;
 
-  /// The package's version.
-  final Version version;
-
   /// The metadata used by the package's [source] to identify and locate it. It
   /// contains whatever [Source]-specific data it needs to be able to install
   /// the package. For example, the description of a git sourced package might
   /// by the URL "git://github.com/dart/uilib.git".
   final description;
 
-  PackageId(this.name, this.source, this.version, this.description);
-
-  /// Creates an ID for the given root package.
-  PackageId.root(Package package)
-      : name = package.name,
-        source = null,
-        version = package.version,
-        description = package.name;
+  int get hashCode => name.hashCode ^ source.hashCode;
 
   /// Whether this ID identifies the root package.
   bool get isRoot => source == null;
 
-  int get hashCode => name.hashCode ^ source.hashCode ^ version.hashCode;
-
   /// Gets the directory where this package is or would be found in the
   /// [SystemCache].
   Future<String> get systemCacheDirectory => source.systemCacheDirectory(this);
 
   bool operator ==(other) {
-    if (other is! PackageId) return false;
-    // TODO(rnystrom): We're assuming here the name/version/source tuple is
-    // enough to uniquely identify the package and that we don't need to delve
-    // into the description.
+    // Can only be equal to other PackageRefs and not even subclasses.
+    if (other.runtimeType != PackageRef) return false;
+
+    // We assume here the name/source tuple is enough to identify the reference
+    // and that we don't need to delve into the description.

[nweiz, 2013/04/25 21:07:19]

I think this should still be a TODO. "git://github.com/nex3/foo.git" shouldn't
be == to "git://github.com/munificent/foo.git".

[Bob Nystrom, 2013/04/26 22:17:49]

Done.

+    return other.name == name && other.source == source;
+  }
+
+  String toString() {
+    if (isRoot) return "$name (root)";
+    if (source.isDefault) return name;
+    return "$name from $source";
+  }
+
+  /// Returns a [PackageRef] with this one's [name], [source], and
+  /// [description]. Useful for getting a plain PackageRef from an instance of
+  /// a subclass.
+  PackageRef toRef() => new PackageRef(name, source, description);

[nweiz, 2013/04/25 21:07:19]

This method seems weird, since every subclass should behave as a PackageRef
anyway. But they won't behave that way with == or hashCode, so I guess it's
fine.

Basically I just don't like the intersection of equality and inheritance.

[Bob Nystrom, 2013/04/26 22:17:49]

This is less weird now that PackageRef isn't the base class of PackageID and
PackageDep.

+
+  /// Returns a [PackageId] generated from this [PackageRef] with the given
+  /// concrete version.
+  PackageId atVersion(Version version) =>
+    new PackageId(name, source, version, description);

[nweiz, 2013/04/25 21:07:19]

Should we have one of these to create a PackageDep, for good measure?

[Bob Nystrom, 2013/04/26 22:17:49]

We don't have a use case for that yet, so in the absence of that, I'd rather not
add dead code. It's easy to add once we need it.

+
+  /// Returns `true` if this ref's description matches [other]'s.
+  bool descriptionEquals(PackageRef other) {
+    return source.descriptionsEqual(description, other.description);
+  }
+
+  /// Gets the list of ids of all versions of the package that are described by
+  /// this reference.

[nweiz, 2013/04/25 21:07:19]

Is this useful for anything other than PackageDep?

If so, explain the semantics of this for each class in their respective
docstrings.

[Bob Nystrom, 2013/04/26 22:17:49]

It's actually only called right now given an instance of PackageRef, not
PackageDep. This method is a little strange in general. Because PackageRef has a
reference to the actual Source object, it has enough state to be able to
implement this, so I moved it here. It seemed less strange then doing:

someRef.source.getVersions(someRef);

It's weird to call a method on a field of an object and pass that selfsame
object back in as a paramater.

Part of this strangeness is that PackageRef (and its subclasses) are
conceptually lightweight identifiers but by having the actual Source object,
they can actually do real work.

+  Future<List<PackageId>> getVersions() {
+    if (isRoot) {
+      throw new StateError("Cannot get versions for the root package.");
+    }
+
+    return source.getVersions(name, description).then((versions) {
+      return versions.map((version) => atVersion(version)).toList();
+    });
+  }
+}
+
+/// An unambiguous resolved reference to a package. A package ID contains enough

[nweiz, 2013/04/25 21:07:19]

I'd describe this as "A reference to a specific version of a package."

[Bob Nystrom, 2013/04/26 22:17:49]

Done.

+/// information to correctly install the package.
+///
+/// Note that it's possible for multiple distinct package IDs to point to
+/// different directories that happen to contain identical packages. For

[nweiz, 2013/04/25 21:07:19]

"Directories" is a little inaccurate here; I would say "different packages that
have identical contents".

[Bob Nystrom, 2013/04/26 22:17:49]

Done.

+/// example, the same package may be available from multiple sources. As far as
+/// Pub is concerned, those packages are different.
+class PackageId extends PackageRef {

[nweiz, 2013/04/25 21:07:19]

The inheritance structure of these classes is weird. Why is PackageId a
PackageRef, but not a PackageDep? Do we get any value from being able to treat
PackageId and PackageDep as a PackageRef (other than making the implementations
more concise)? Does this value outweigh the risk of passing a PackageId to a
method that's annotated as PackageRef but expects a concrete PackageRef?

My preference would be to remove all inheritance relationships here (or add a
private base class so they're invisible). I think it confuses the semantics more
than it adds value. We can add methods like toRef/atVersion to convert between
them explicitly.

[Bob Nystrom, 2013/04/26 22:17:49]

No, it's pure code reuse.
Done.

+  /// The package's version.
+  final Version version;
+
+  PackageId(String name, Source source, this.version, description)
+      : super(name, source, description);
+
+  /// Creates an ID for the given root package.
+  PackageId.root(Package package)
+      : version = package.version,
+        super(package.name, null, package.name);
+
+  int get hashCode => name.hashCode ^ source.hashCode ^ version.hashCode;
+
+  bool operator ==(other) {
+    // Can only be equal to other PackageIds and not even subclasses.
+    if (other.runtimeType != PackageId) return false;
+
     return other.name == name &&

[nweiz, 2013/04/25 21:07:19]

Add the above TODO about description checks here as well.

[Bob Nystrom, 2013/04/26 22:17:49]

Done.

            other.source == source &&
            other.version == version;
@@ -135,78 +186,41 @@ class PackageId implements Comparable<PackageId> {
     return "$name $version from $source";
   }
 
-  int compareTo(PackageId other) {

[nweiz, 2013/04/25 21:07:19]

What were we using this for?

[Bob Nystrom, 2013/04/26 22:17:49]

I think the old solver may have used it? I'm not really sure. Nothing broke when
I took it out, so I think it was just dead code.

-    var sourceComp = source.name.compareTo(other.source.name);
-    if (sourceComp != 0) return sourceComp;
-
-    var nameComp = name.compareTo(other.name);
-    if (nameComp != 0) return nameComp;
-
-    return version.compareTo(other.version);
-  }
-
   /// Returns the pubspec for this package.
   Future<Pubspec> describe() => source.systemCache.describe(this);
 
-  /// Returns a future that completes to the resovled [PackageId] for this id.
+  /// Returns a future that completes to the resolved [PackageId] for this id.
   Future<PackageId> get resolved => source.resolveId(this);
 
-  /// Returns a [PackageRef] that references this package and constrains its
+  /// Returns a [PackageDep] that references this package and constrains its
   /// version to exactly match [version].
-  PackageRef toRef() {
-    return new PackageRef(name, source, version, description);
-  }
-
-  /// Returns `true` if this id's description matches [other]'s.
-  bool descriptionEquals(PackageRef other) {
-    return source.descriptionsEqual(description, other.description);
+  PackageDep toRef() {
+    return new PackageDep(name, source, version, description);

[nweiz, 2013/04/25 21:07:19]

Woah, this is very confusing. I expected toRef to always return a PackageRef,
not a subclass. I thought that was the whole point. Now for PackageId it returns
a PackageDep instead? What's going on here?

[Bob Nystrom, 2013/04/26 22:17:49]

Haha, yes! Very confusing!

This is an old dead method whose name toRef() referred to the *old* name before
I renamed PackageRef -> PackageDep. Removed. Oops!

   }
 }

[nweiz, 2013/04/25 21:07:19]

If you keep getVersions in PackageRef, this should override it.

[Bob Nystrom, 2013/04/26 22:17:49]

It's in PackageRef, but PackageRef is no longer a base class for PackageDep.

 
-/// A reference to a package. Unlike a [PackageId], a PackageRef may not
+/// A dependency on a package. Unlike a [PackageId], a PackageDep may not

[nweiz, 2013/04/25 21:07:19]

See above about using a structure- rather than usage-based description.

[Bob Nystrom, 2013/04/26 22:17:49]

Done.

 /// unambiguously refer to a single package. It may describe a range of allowed
 /// packages.
-class PackageRef {
-  /// The name of the package being identified.
-  final String name;
-
-  /// The [Source] used to look up the package. If this refers to a root
-  /// package, this will be `null`.
-  final Source source;
-
+class PackageDep extends PackageRef {
   /// The allowed package versions.
   final VersionConstraint constraint;
 
-  /// The metadata used to identify the package being referenced. The
-  /// interpretation of this will vary based on the [source].
-  final description;
-
-  PackageRef(this.name, this.source, this.constraint, this.description);
-
-  // TODO(rnystrom): Remove this if the old version solver is removed.
-  /// Creates a reference to the given root package.
-  PackageRef.root(Package package)
-      : name = package.name,
-        source = null,
-        constraint = package.version,
-        description = package.name;
-
-  /// Whether this refers to the root package.
-  bool get isRoot => source == null;
+  PackageDep(String name, Source source, this.constraint, description)
+      : super(name, source, description);
 
   String toString() {
     if (isRoot) return "$name $constraint (root)";
     return "$name $constraint from $source ($description)";
   }
 
-  /// Returns a [PackageId] generated from this [PackageRef] with the given
-  /// concrete version.
-  PackageId atVersion(Version version) =>
-    new PackageId(name, source, version, description);
+  bool operator ==(other) {
+    // Can only be equal to other PackageDeps and not even subclasses.
+    if (other.runtimeType != PackageDep) return false;
 
-  /// Returns `true` if this reference's description matches [other]'s.
-  bool descriptionEquals(PackageRef other) {
-    return source.descriptionsEqual(description, other.description);
+    return other.name == name &&

[nweiz, 2013/04/25 21:07:19]

Add the TODO message here too.

[Bob Nystrom, 2013/04/26 22:17:49]

Done.

+           other.source == source &&
+           other.constraint == constraint;
   }
 }

[nweiz, 2013/04/25 21:07:19]

This should definitely override getVersions.

[Bob Nystrom, 2013/04/26 22:17:49]

This no longer inherits from a type that defines that.