Fix comparison issues

chriseplettsonos · chriseplettsonos · commit a67dd6dc5ee9 · 2023-09-25T09:11:02.000-04:00
As implemented, there were a couple of problems where the library was not performing comparisons between semvers correctly.

The first issue is that the presence of buildmetadata was causing the library to treat the version as unstable, which also meant that, given the comparison implementation, the buildmetadata was being considered when comparing two semvers. This is a violation of Section 10 of the semver.org spec, which specifies that “Build metadata MUST be ignored when determining version precedence”. Additionally, it is not specified by semver.org that that the presence of buildmetadata should be involved in determining the stability of a semver.

The second problem is that the library uses simple ASCII lexical ordering to compare the pre-release string of two semvers, which does not match the rules defined in Section 11.4 of the semver specification.

This change addresses these problems with the following changes
1) Remove the build.isEmpty check from the implementation of the isStable property
2) Implement a PreReleaseIdentifier enum and add Comparable conformance to an array of these types, per the rules laid out in Section 11.4 of the semver specification
3) Add a computed property that returns an array of PreReleaseIndentifiers by parsing the preRelease string
4) Update the less than operator implementation to apply the above changes appropriately
5) Add additional test cases to verify the new behavior

Testing:
1) Existing unit tests pass
2) New unit tests pass
diff --git a/Sources/SemanticVersion/SemanticVersion.swift b/Sources/SemanticVersion/SemanticVersion.swift
@@ -43,6 +43,11 @@ public struct SemanticVersion: Codable, Equatable, Hashable {
         self.preRelease = preRelease
         self.build = build
     }
+
+    public enum PreReleaseIdentifier: Equatable {
+        case alphanumeric(String)
+        case numeric(Int)
+    }
 }
 
 
@@ -74,26 +79,81 @@ extension SemanticVersion: Comparable {
         if lhs.major != rhs.major { return lhs.major < rhs.major }
         if lhs.minor != rhs.minor { return lhs.minor < rhs.minor }
         if lhs.patch != rhs.patch { return lhs.patch < rhs.patch }
-        if lhs.preRelease != rhs.preRelease {
-            // Ensure stable versions sort after their betas ...
-            if lhs.isStable { return false }
-            if rhs.isStable { return true }
-            // ... otherwise sort by preRelease
-            return lhs.preRelease < rhs.preRelease
-        }
-        // ... and build
-        return lhs.build < rhs.build
+
+        // A stable release takes precedence over a pre-release
+        if lhs.isStable != rhs.isStable { return rhs.isStable }
+
+        // Otherwise compare the pre-releases per section 11.4
+        // See: https://semver.org/#spec-item-11
+        return lhs.preReleaseIdentifiers < rhs.preReleaseIdentifiers
+
+        // Note that per section 10, buildmetadata MUST not be
+        // considered when determining precedence
+        // See: https://semver.org/#spec-item-10
     }
 }
 
 
 extension SemanticVersion {
-    public var isStable: Bool { return preRelease.isEmpty && build.isEmpty }
+    public var isStable: Bool { return preRelease.isEmpty }
     public var isPreRelease: Bool { return !isStable }
     public var isMajorRelease: Bool { return isStable && (major > 0 && minor == 0 && patch == 0) }
     public var isMinorRelease: Bool { return isStable && (minor > 0 && patch == 0) }
     public var isPatchRelease: Bool { return isStable && patch > 0 }
     public var isInitialRelease: Bool { return self == .init(0, 0, 0) }
+
+    public var preReleaseIdentifiers: [PreReleaseIdentifier] {
+        return preRelease
+            .split(separator: ".")
+            .map { PreReleaseIdentifier(String($0)) }
+    }
+}
+
+extension SemanticVersion.PreReleaseIdentifier {
+    init(_ rawValue: String) {
+        if let number = Int(rawValue) {
+            self = .numeric(number)
+        } else {
+            self = .alphanumeric(rawValue)
+        }
+    }
+}
+
+extension SemanticVersion.PreReleaseIdentifier: Comparable {
+    public static func < (lhs: Self, rhs: Self) -> Bool {
+        // These rules are laid out in section 11.4 of the semver spec
+        // See: https://semver.org/#spec-item-11
+        switch (lhs, rhs) {
+        case (.numeric, .alphanumeric):
+            // 11.4.3 - Numeric identifiers always have lower precedence than non-numeric identifiers
+            return true
+        case (.alphanumeric, .numeric):
+            // 11.4.3 - Numeric identifiers always have lower precedence than non-numeric identifiers
+            return false
+        case (.numeric(let lhInt), .numeric(let rhInt)):
+            // 11.4.1 - Identifiers consisting of only digits are compared numerically
+            return lhInt < rhInt
+        case (.alphanumeric(let lhString), .alphanumeric(let rhString)):
+            // 11.4.2 - Identifiers with letters or hyphens are compared lexically in ASCII sort order
+            return lhString < rhString
+        }
+    }
+}
+
+
+extension Array: Comparable where Element == SemanticVersion.PreReleaseIdentifier {
+    public static func < (lhs: Self, rhs: Self) -> Bool {
+        // Per section 11.4 of the semver spec, compare left to right until a
+        // difference is found.
+        // See: https://semver.org/#spec-item-11
+        for (lhIdentifier, rhIdentifier) in zip(lhs, rhs) {
+            if lhIdentifier != rhIdentifier { return lhIdentifier < rhIdentifier }
+        }
+
+        // 11.4.4 - A larger set of identifiers will have a higher precendence
+        // than a smaller set, if all the preceding identifiers are equal.
+        return lhs.count < rhs.count
+    }
 }
 
 #if swift(>=5.5)
diff --git a/Tests/SemanticVersionTests/SemanticVersionTests.swift b/Tests/SemanticVersionTests/SemanticVersionTests.swift
@@ -124,7 +124,8 @@ final class SemanticVersionTests: XCTestCase {
         XCTAssert(SemanticVersion(1, 0, 0) < SemanticVersion(1, 1, 0))
         XCTAssert(SemanticVersion(1, 0, 0) < SemanticVersion(1, 0, 1))
         XCTAssert(SemanticVersion(1, 0, 0, "a") < SemanticVersion(1, 0, 0, "b"))
-        XCTAssert(SemanticVersion(1, 0, 0, "a", "a") < SemanticVersion(1, 0, 0, "a", "b"))
+        XCTAssertLessThan(SemanticVersion(1, 0, 0, "alpha.2"), SemanticVersion(1, 0, 0, "alpha.11"))
+        XCTAssertLessThan(SemanticVersion(1, 0, 0, "alpha.2"), SemanticVersion(1, 0, 0, "alpha.2.1"))
 
         // ensure betas come before releases
         XCTAssert(SemanticVersion(1, 0, 0, "b1") < SemanticVersion(1, 0, 0))
@@ -133,14 +134,20 @@ final class SemanticVersionTests: XCTestCase {
         XCTAssert(SemanticVersion(1, 0, 0) < SemanticVersion(1, 0, 1, "b1"))
         // once the patch bumps up to the beta level again, it sorts higher
         XCTAssert(SemanticVersion(1, 0, 1) > SemanticVersion(1, 0, 1, "b1"))
+
+        // Ensure metadata is not considered
+        XCTAssertFalse(SemanticVersion(1, 0, 0, "a", "a") < SemanticVersion(1, 0, 0, "a", "b"))
+        XCTAssertFalse(SemanticVersion(1, 0, 0, "a", "a") > SemanticVersion(1, 0, 0, "a", "b"))
+        XCTAssertLessThan(SemanticVersion(1, 0, 0, "alpha", "build1"), SemanticVersion(1, 0, 0, "", "build1"))
     }
 
     func test_isStable() throws {
         XCTAssert(SemanticVersion(1, 0, 0).isStable)
         XCTAssert(SemanticVersion(1, 0, 0, "").isStable)
         XCTAssert(SemanticVersion(1, 0, 0, "", "").isStable)
         XCTAssertFalse(SemanticVersion(1, 0, 0, "a").isStable)
-        XCTAssertFalse(SemanticVersion(1, 0, 0, "", "a").isStable)
+        XCTAssertTrue(SemanticVersion(1, 0, 0, "", "a").isStable)
+        XCTAssertFalse(SemanticVersion(1, 0, 0, "a", "b").isStable)
     }
 
     func test_isMajorRelease() throws {
