Make version ordering consistent with equality

README.md

@@ -61,7 +61,21 @@ The coursier documentation [details](https://get-coursier.io/docs/other-version-
 how versions are compared.
 
 `Version` implements `Ordered[Version]`, so that `Version` instances can be compared together,
-and a sequence of `Version`s can be sorted.
+and a sequence of `Version`s can be sorted. That order is total, and consistent with `equals`:
+`a.compare(b)` is `0` if and only if `a == b`. Versions that mean the same thing but are spelled
+differently, like `1.0` and `1.0.0`, or `1.2+foo` and `1.2+bar`, are ordered by their
+representation, so that sorted collections and hash-based ones agree on which versions are
+distinct.
+
+Use `compareSemantic` to compare versions up to that equivalence - it returns `0` for versions
+that only differ by padding, separators, or build metadata. This is the order that decides
+whether a version sits in an interval, or whether two constraints can be reconciled:
+```scala
+Version("1.0").compare(Version("1.0.0"))
+// res: Int = -2
+Version("1.0").compareSemantic(Version("1.0.0"))
+// res: Int = 0
+```
 
 ## `VersionConstraint`
 

README.template.md

@@ -54,7 +54,21 @@ The coursier documentation [details](https://get-coursier.io/docs/other-version-
 how versions are compared.
 
 `Version` implements `Ordered[Version]`, so that `Version` instances can be compared together,
-and a sequence of `Version`s can be sorted.
+and a sequence of `Version`s can be sorted. That order is total, and consistent with `equals`:
+`a.compare(b)` is `0` if and only if `a == b`. Versions that mean the same thing but are spelled
+differently, like `1.0` and `1.0.0`, or `1.2+foo` and `1.2+bar`, are ordered by their
+representation, so that sorted collections and hash-based ones agree on which versions are
+distinct.
+
+Use `compareSemantic` to compare versions up to that equivalence - it returns `0` for versions
+that only differ by padding, separators, or build metadata. This is the order that decides
+whether a version sits in an interval, or whether two constraints can be reconciled:
+```scala
+Version("1.0").compare(Version("1.0.0"))
+// res: Int = -2
+Version("1.0").compareSemantic(Version("1.0.0"))
+// res: Int = 0
+```
 
 ## `VersionConstraint`
 

versions/shared/src/coursier/version/Version.scala

@@ -18,10 +18,41 @@ case class Version(repr: String) extends Ordered[Version] {
       items0 = Version.items(repr)
     items0
   }
-  def compare(other: Version) = {
+  /**
+   * Total order on versions, consistent with `equals` and `hashCode`.
+   *
+   * `a.compare(b) == 0` if and only if `a == b`. Versions that are semantically
+   * equivalent but spelled differently (`1.0` and `1.0.0`, `1.2` and `1.2+foo`,
+   * `1.2+bar` and `1.2+foo`, …) are ordered by their representation, so that
+   * sorted collections and hash-based ones agree on which versions are distinct.
+   *
+   * Use [[compareSemantic]] to compare versions up to that equivalence.
+   */
+  def compare(other: Version): Int = {
     if (repr == other.repr) 0 // fast path
-    else Version.listCompare(items, other.items)
+    else {
+      val cmp = Version.listCompare(items, other.items)
+      // Break ties so that the order is total: versions that only differ by
+      // padding, separators, or build metadata are ordered by representation.
+      if (cmp == 0) repr.compareTo(other.repr)
+      else cmp
+    }
   }
+
+  /**
+   * Compares versions up to the equivalence induced by version parsing.
+   *
+   * Returns `0` for versions that have the same meaning but different
+   * representations, like `1.0` and `1.0.0`, or `1.2+foo` and `1.2+bar`
+   * (Semver § 10: build metadata doesn't take part in precedence).
+   *
+   * This is *not* consistent with `equals` - it is the order to use to decide
+   * whether a version sits in an interval, or whether two versions can be
+   * reconciled. Use [[compare]] for sorting or for sorted collections.
+   */
+  def compareSemantic(other: Version): Int =
+    if (repr == other.repr) 0 // fast path
+    else Version.listCompare(items, other.items)
   def isEmpty = items.forall(_.isEmpty)
 
   def withRepr(repr: String): Version =

versions/shared/src/coursier/version/VersionCompatibility.scala

@@ -101,7 +101,7 @@ object VersionCompatibility {
         .filter(_.forall(_.isNumber))
         .map(_.collect { case n: Version.Numeric => n })
         .map(items => items.map(_.repr).mkString("."))
-        .filter(s => Version(s).compareTo(v) <= 0)
+        .filter(s => Version(s).compareSemantic(v) <= 0)
       candidateOpt.getOrElse(version)
     }
   }
@@ -134,7 +134,7 @@ object VersionCompatibility {
         .filter(items => items.nonEmpty && items.forall(_.isNumber) && items.forall(!_.isEmpty))
         .map(_.collect { case n: Version.Numeric => n })
         .map(items => items.map(_.repr).mkString("."))
-        .filter(s => Version(s).compareTo(v) <= 0)
+        .filter(s => Version(s).compareSemantic(v) <= 0)
       candidateOpt.getOrElse(version)
     }
   }
@@ -155,7 +155,7 @@ object VersionCompatibility {
         .filter(_.forall(_.isNumber))
         .map(_.collect { case n: Version.Numeric => n })
         .map(items => items.map(_.repr).mkString("."))
-        .filter(s => Version(s).compareTo(v) <= 0)
+        .filter(s => Version(s).compareSemantic(v) <= 0)
       candidateOpt.getOrElse(version)
     }
   }

versions/shared/src/coursier/version/VersionConstraint.scala

@@ -18,13 +18,13 @@ sealed abstract class VersionConstraint extends Product with Serializable with O
 
   private lazy val compareKey = preferred.headOption.orElse(interval.from).getOrElse(Version.zero)
   def compare(other: VersionConstraint): Int =
-    compareKey.compare(other.compareKey)
+    compareKey.compareSemantic(other.compareKey)
 
   def isValid: Boolean =
     interval.isValid && preferred.forall { v =>
       interval.contains(v) ||
         interval.to.forall { to =>
-          val cmp = v.compare(to)
+          val cmp = v.compareSemantic(to)
           cmp < 0 || (cmp == 0 && interval.toIncluded)
         }
     }
@@ -87,7 +87,7 @@ object VersionConstraint {
           interval.from match {
             case Some(from) =>
               allPreferred.filter { v =>
-                val cmp = from.compare(v)
+                val cmp = from.compareSemantic(v)
                 cmp < 0 || (cmp == 0 && interval.fromIncluded)
               }
             case None =>

versions/shared/src/coursier/version/VersionInterval.scala

@@ -12,7 +12,7 @@ case class VersionInterval(
       for {
         f <- from
         t <- to
-        cmd = f.compare(t)
+        cmd = f.compareSemantic(t)
       } yield cmd < 0 || (cmd == 0 && fromIncluded && toIncluded)
 
     fromToOrder.forall(x => x) && (from.nonEmpty || !fromIncluded) && (to.nonEmpty || !toIncluded)
@@ -21,12 +21,12 @@ case class VersionInterval(
   def contains(version: Version): Boolean = {
     val fromCond =
       from.forall { from0 =>
-        val cmp = from0.compare(version)
+        val cmp = from0.compareSemantic(version)
         cmp < 0 || cmp == 0 && fromIncluded
       }
     lazy val toCond =
       to.forall { to0 =>
-        val cmp = version.compare(to0)
+        val cmp = version.compareSemantic(to0)
         cmp < 0 || cmp == 0 && toIncluded
       }
 
@@ -49,7 +49,7 @@ case class VersionInterval(
     val (newFrom, newFromIncluded) =
       (from, other.from) match {
         case (Some(a), Some(b)) =>
-          val cmp = a.compare(b)
+          val cmp = a.compareSemantic(b)
           if (cmp < 0) (Some(b), other.fromIncluded)
           else if (cmp > 0) (Some(a), fromIncluded)
           else (Some(a), fromIncluded && other.fromIncluded)
@@ -62,7 +62,7 @@ case class VersionInterval(
     val (newTo, newToIncluded) =
       (to, other.to) match {
         case (Some(a), Some(b)) =>
-          val cmp = a.compare(b)
+          val cmp = a.compareSemantic(b)
           if (cmp < 0) (Some(a), toIncluded)
           else if (cmp > 0) (Some(b), other.toIncluded)
           else (Some(a), toIncluded && other.toIncluded)

versions/shared/test/src/coursier/version/VersionTests.scala

@@ -6,6 +6,9 @@ import utest._
 object VersionTests extends TestSuite {
 
   def compare(first: String, second: String) =
+    Version(first).compareSemantic(Version(second))
+
+  def compareTotal(first: String, second: String) =
     Version(first).compare(Version(second))
 
   def increasing(versions: String*): Boolean =
@@ -52,6 +55,20 @@ object VersionTests extends TestSuite {
         assert(compare("1.2+bar.1", "1.2+bar.2") == 0)
       }
 
+      test("total") {
+        // build metadata doesn't take part in precedence, but it still tells versions
+        // apart, so that the order stays total (see #13)
+        assert(compareTotal("1.2", "1.2+foo") < 0)
+        assert(compareTotal("2.0", "2.0+20130313144700") < 0)
+        assert(compareTotal("2.0+20130313144700", "2.0.2") < 0)
+
+        assert(compareTotal("1.2+bar", "1.2+foo") < 0)
+        assert(compareTotal("1.2+bar.1", "1.2+bar.2") < 0)
+
+        // scalajs-scalalib is published as <scalaVersion>+<scalaJsVersion>
+        assert(compareTotal("2.13.16+1.18.2", "2.13.16+1.19.0") < 0)
+      }
+
       test("shouldNotParseMetadata") {
         test {
           val items = Version("1.2+bar.2").items
@@ -429,6 +446,76 @@ object VersionTests extends TestSuite {
       assert(items == expectedItems)
     }
 
+    test("totalOrder") {
+      // https://github.com/coursier/versions/issues/13
+      val equivalent = Seq(
+        Seq("1", "1.0", "1.0.0", "1.0.0.0", "1-ga", "1-final", "1.0000000000000", "01"),
+        Seq("1.2", "1.2+foo", "1.2+bar", "1.02"),
+        Seq("1.0-alpha", "1.0.0-alpha", "1.0alpha", "1.0.ALPHA", "1.0-a")
+      )
+
+      test("consistentWithEquals") {
+        for {
+          group <- equivalent
+          first <- group
+          second <- group
+        } {
+          val a = Version(first)
+          val b = Version(second)
+          // compare is only 0 for versions that are equal, unlike compareSemantic
+          assert((a.compare(b) == 0) == (a == b))
+          assert(a.compareSemantic(b) == 0)
+        }
+      }
+
+      test("antisymmetric") {
+        for {
+          group <- equivalent
+          first <- group
+          second <- group
+        } {
+          val a = Version(first)
+          val b = Version(second)
+          assert(a.compare(b).signum == -b.compare(a).signum)
+        }
+      }
+
+      test("sortedAndHashedAgree") {
+        for (group <- equivalent) {
+          val versions = group.map(Version(_))
+          val hashed = versions.toSet
+          val sorted = scala.collection.immutable.TreeSet.empty[Version] ++ versions
+          assert(hashed.size == group.distinct.length)
+          assert(sorted.size == hashed.size)
+          assert(sorted == hashed)
+        }
+      }
+
+      test("hashCodeConsistent") {
+        for {
+          group <- equivalent
+          first <- group
+          second <- group
+          if Version(first) == Version(second)
+        } assert(Version(first).hashCode == Version(second).hashCode)
+      }
+
+      test("pvp") {
+        // PVP orders versions by the lexicographic ordering of their components,
+        // so extra trailing components make a version greater
+        assert(compareTotal("1.0.0", "1.0.0.0") < 0)
+        assert(compareTotal("1", "1.0") < 0)
+        assert(compareTotal("2.0.1", "1.3.2") > 0)
+      }
+
+      test("semanticOrderStillWins") {
+        // the tie-break only kicks in for versions that compareSemantic considers equal
+        assert(compareTotal("1.9", "1.10") < 0)
+        assert(compareTotal("1.0-SNAPSHOT", "1.0") < 0)
+        assert(compareTotal("1.0.0.0.0.1", "1.0.1") < 0)
+      }
+    }
+
     test("isStable") {
       assert(Version("1.2.3").isStable)
       assert(Version("1.2.3-3").isStable)