[TSCUtility] Correct semantic version parsing and comparison (#214)

The semantic versioning specification 2.0.0 [states](https://semver.org/#spec-item-9) that pre-release identifiers must be positioned after the version core, and build metadata identifiers after pre-release identifiers.

In the old implementation, if a version core was appended with metadata identifiers that contain hyphens ("-"), the first hyphen would be mistaken as an indication of pre-release identifiers thereafter. Then, the position of the first hyphen would be treated as where the version core ends, resulting in a false negative after it was found that the "version core" contained non-numeric characters.

For example: the semantic version `1.2.3+some-meta.data` is a well-formed, with `1.2.3` being the version core and `some-meta.data` the metadata identifiers. However, the old implementation of `Version.init?(_ versionString: String)` would falsely treat `1.2.3+some` as the version core and `meta.data` the pre-release identifiers.

The new implementation fixes this problem by restricting the search area for "-" to the substring before the first "+".

The initialiser wherein the parsing takes place has been renamed from `init?(string: String)` to `init?(_ versionString: String)`. The old initialiser is not removed but marked as deprecated for source compatibility with SwiftPM. With the new initialiser name, `Version` now conforms to `LosslessStringConvertible`.

In addition, the logic for breaking up the version core into numeric identifiers has been rewritten to be more understandable.

`Comparable` does not provide a default implementation for `==`, so the compiler synthesises one composed of [member-wise comparisons](https://github.com/apple/swift-evolution/blob/main/proposals/0185-synthesize-equatable-hashable.md#implementation-details). This leads to a false `false` when 2 semantic versions differ by only their build metadata identifiers, contradicting to SemVer 2.0.0's [comparison rules](https://semver.org/#spec-item-10).

This commit adds a manual implementation of `==` for `Version`, along with appropriate tests. One consequence, though, is that now two versions that differ by only their build metadata identifiers are not allowed in the same set.

Because we have a non-synthesised `Equatable` conformance, the synthesised `Hashable` conformance composed of member-wise hashes is incorrect. `buildMetadataIdentifiers` does not participate in `Version`'s `Equatable` conformance, so it shouldn't participate in `Version`'s `Hashable` conformance either.

Relevant: [SR-11588](https://bugs.swift.org/browse/SR-11588)

other changes:
* add additional tests for initialising `Version`
*  rearranged the tests.
* [garderning] fix typo "ranage" → "range"
* add a throwing initialiser for creating a version from a string
This new initialiser throws a `VersionError` instance when initialisation fails. This gives the user more information and control over error handling. `Version`'s conformance to `LosslessStringConvertible` is preserved by having `init?(_ versionString: String)` call this new initialiser, and return `nil` when an error is thrown.
* [gardening] remove horizontal whitespace from whitespace-only lines

Author: WowbaggersLiquidLunch

Sources/TSCUtility/Version.swift

@@ -1,7 +1,7 @@
 /*
  This source file is part of the Swift.org open source project
 
- Copyright (c) 2014 - 2017 Apple Inc. and the Swift project authors
+ Copyright (c) 2014 - 2021 Apple Inc. and the Swift project authors
  Licensed under Apache License v2.0 with Runtime Library Exception
 
  See http://swift.org/LICENSE.txt for license information
@@ -11,7 +11,7 @@
 import TSCBasic
 
 /// A struct representing a semver version.
-public struct Version: Hashable {
+public struct Version {
 
     /// The major version.
     public let major: Int
@@ -28,7 +28,7 @@ public struct Version: Hashable {
     /// The build metadata.
     public let buildMetadataIdentifiers: [String]
 
-    /// Create a version object.
+    /// Creates a version object.
     public init(
         _ major: Int,
         _ minor: Int,
@@ -45,12 +45,122 @@ public struct Version: Hashable {
     }
 }
 
-extension Version: Comparable {
+/// An error that occurs during the creation of a version.
+public enum VersionError: Error, CustomStringConvertible {
+    /// The version string contains non-ASCII characters.
+    case nonASCIIVersionString(_ versionString: String)
+    /// The version core contains an invalid number of Identifiers.
+    case invalidVersionCoreIdentifiersCount(_ identifiers: [String])
+    /// Some or all of the version core identifiers contain non-numerical characters or are empty.
+    case nonNumericalOrEmptyVersionCoreIdentifiers(_ identifiers: [String])
+    /// Some or all of the pre-release identifiers contain characters other than alpha-numerics and hyphens.
+    case nonAlphaNumerHyphenalPrereleaseIdentifiers(_ identifiers: [String])
+    /// Some or all of the build metadata identifiers contain characters other than alpha-numerics and hyphens.
+    case nonAlphaNumerHyphenalBuildMetadataIdentifiers(_ identifiers: [String])
+
+    public var description: String {
+        switch self {
+        case let .nonASCIIVersionString(versionString):
+            return "non-ASCII characters in version string '\(versionString)'"
+        case let .invalidVersionCoreIdentifiersCount(identifiers):
+            return "\(identifiers.count < 3 ? "fewer" : "more") than 3 identifiers in version core '\(identifiers.joined(separator: "."))'"
+        case let .nonNumericalOrEmptyVersionCoreIdentifiers(identifiers):
+            if !identifiers.allSatisfy( { !$0.isEmpty } ) {
+                return "empty identifiers in version core '\(identifiers.joined(separator: "."))'"
+            } else {
+                // Not checking for `.isASCII` here because non-ASCII characters should've already been caught before this.
+                let nonNumericalIdentifiers = identifiers.filter { !$0.allSatisfy(\.isNumber) }
+                return "non-numerical characters in version core identifier\(nonNumericalIdentifiers.count > 1 ? "s" : "") \(nonNumericalIdentifiers.map { "'\($0)'" } .joined(separator: ", "))"
+            }
+        case let .nonAlphaNumerHyphenalPrereleaseIdentifiers(identifiers):
+            // Not checking for `.isASCII` here because non-ASCII characters should've already been caught before this.
+            let nonAlphaNumericalIdentifiers = identifiers.filter { !$0.allSatisfy { $0.isLetter || $0.isNumber || $0 == "-" } }
+            return "characters other than alpha-numerics and hyphens in pre-release identifier\(nonAlphaNumericalIdentifiers.count > 1 ? "s" : "") \(nonAlphaNumericalIdentifiers.map { "'\($0)'" } .joined(separator: ", "))"
+        case let .nonAlphaNumerHyphenalBuildMetadataIdentifiers(identifiers):
+            // Not checking for `.isASCII` here because non-ASCII characters should've already been caught before this.
+            let nonAlphaNumericalIdentifiers = identifiers.filter { !$0.allSatisfy { $0.isLetter || $0.isNumber || $0 == "-" } }
+            return "characters other than alpha-numerics and hyphens in build metadata identifier\(nonAlphaNumericalIdentifiers.count > 1 ? "s" : "") \(nonAlphaNumericalIdentifiers.map { "'\($0)'" } .joined(separator: ", "))"
+        }
+    }
+}
+
+extension Version {
+    // TODO: Rename this function to `init(string: String) throws`, after `init?(string: String)` is removed.
+    // TODO: Find a better error-checking order.
+    // Currently, if a version string is "forty-two", this initializer throws an error that says "forty" is only 1 version core identifier, which is not enough.
+    // But this is misleading the user to consider "forty" as a valid version core identifier.
+    // We should find a way to check for (or throw) "wrong characters used" errors first, but without overly-complicating the logic.
+    /// Creates a version from the given string.
+    /// - Parameter versionString: The string to create the version from.
+    /// - Throws: A `VersionError` instance if the `versionString` doesn't follow [SemVer 2.0.0](https://semver.org).
+    public init(versionString: String) throws {
+        // SemVer 2.0.0 allows only ASCII alphanumerical characters and "-" in the version string, except for "." and "+" as delimiters. ("-" is used as a delimiter between the version core and pre-release identifiers, but it's allowed within pre-release and metadata identifiers as well.)
+        // Alphanumerics check will come later, after each identifier is split out (i.e. after the delimiters are removed).
+        guard versionString.allSatisfy(\.isASCII) else {
+            throw VersionError.nonASCIIVersionString(versionString)
+        }
+
+        let metadataDelimiterIndex = versionString.firstIndex(of: "+")
+        // SemVer 2.0.0 requires that pre-release identifiers come before build metadata identifiers
+        let prereleaseDelimiterIndex = versionString[..<(metadataDelimiterIndex ?? versionString.endIndex)].firstIndex(of: "-")
+
+        let versionCore = versionString[..<(prereleaseDelimiterIndex ?? metadataDelimiterIndex ?? versionString.endIndex)]
+        let versionCoreIdentifiers = versionCore.split(separator: ".", omittingEmptySubsequences: false)
+
+        guard versionCoreIdentifiers.count == 3 else {
+            throw VersionError.invalidVersionCoreIdentifiersCount(versionCoreIdentifiers.map { String($0) })
+        }
+
+        guard
+            // Major, minor, and patch versions must be ASCII numbers, according to the semantic versioning standard.
+            // Converting each identifier from a substring to an integer doubles as checking if the identifiers have non-numeric characters.
+            let major = Int(versionCoreIdentifiers[0]),
+            let minor = Int(versionCoreIdentifiers[1]),
+            let patch = Int(versionCoreIdentifiers[2])
+        else {
+            throw VersionError.nonNumericalOrEmptyVersionCoreIdentifiers(versionCoreIdentifiers.map { String($0) })
+        }
+
+        self.major = major
+        self.minor = minor
+        self.patch = patch
+
+        if let prereleaseDelimiterIndex = prereleaseDelimiterIndex {
+            let prereleaseStartIndex = versionString.index(after: prereleaseDelimiterIndex)
+            let prereleaseIdentifiers = versionString[prereleaseStartIndex..<(metadataDelimiterIndex ?? versionString.endIndex)].split(separator: ".", omittingEmptySubsequences: false)
+            guard prereleaseIdentifiers.allSatisfy( { $0.allSatisfy { $0.isLetter || $0.isNumber || $0 == "-" } } ) else {
+                throw VersionError.nonAlphaNumerHyphenalPrereleaseIdentifiers(prereleaseIdentifiers.map { String($0) })
+            }
+            self.prereleaseIdentifiers = prereleaseIdentifiers.map { String($0) }
+        } else {
+            self.prereleaseIdentifiers = []
+        }
+
+        if let metadataDelimiterIndex = metadataDelimiterIndex {
+            let metadataStartIndex = versionString.index(after: metadataDelimiterIndex)
+            let buildMetadataIdentifiers = versionString[metadataStartIndex...].split(separator: ".", omittingEmptySubsequences: false)
+            guard buildMetadataIdentifiers.allSatisfy( { $0.allSatisfy { $0.isLetter || $0.isNumber || $0 == "-" } } ) else {
+                throw VersionError.nonAlphaNumerHyphenalBuildMetadataIdentifiers(buildMetadataIdentifiers.map { String($0) })
+            }
+            self.buildMetadataIdentifiers = buildMetadataIdentifiers.map { String($0) }
+        } else {
+            self.buildMetadataIdentifiers = []
+        }
+    }
+}
+
+extension Version: Comparable, Hashable {
 
     func isEqualWithoutPrerelease(_ other: Version) -> Bool {
         return major == other.major && minor == other.minor && patch == other.patch
     }
 
+    // Although `Comparable` inherits from `Equatable`, it does not provide a new default implementation of `==`, but instead uses `Equatable`'s default synthesised implementation. The compiler-synthesised `==`` is composed of [member-wise comparisons](https://github.com/apple/swift-evolution/blob/main/proposals/0185-synthesize-equatable-hashable.md#implementation-details), which leads to a false `false` when 2 semantic versions differ by only their build metadata identifiers, contradicting SemVer 2.0.0's [comparison rules](https://semver.org/#spec-item-10).
+    @inlinable
+    public static func == (lhs: Version, rhs: Version) -> Bool {
+        !(lhs < rhs) && !(lhs > rhs)
+    }
+
     public static func < (lhs: Version, rhs: Version) -> Bool {
         let lhsComparators = [lhs.major, lhs.minor, lhs.patch]
         let rhsComparators = [rhs.major, rhs.minor, rhs.patch]
@@ -64,7 +174,7 @@ extension Version: Comparable {
         }
 
         guard rhs.prereleaseIdentifiers.count > 0 else {
-            return true // Prerelease lhs < non-prerelease rhs 
+            return true // Prerelease lhs < non-prerelease rhs
         }
 
         let zippedIdentifiers = zip(lhs.prereleaseIdentifiers, rhs.prereleaseIdentifiers)
@@ -88,6 +198,15 @@ extension Version: Comparable {
 
         return lhs.prereleaseIdentifiers.count < rhs.prereleaseIdentifiers.count
     }
+
+    // Custom `Equatable` conformance leads to custom `Hashable` conformance.
+    // [SR-11588](https://bugs.swift.org/browse/SR-11588)
+    public func hash(into hasher: inout Hasher) {
+        hasher.combine(major)
+        hasher.combine(minor)
+        hasher.combine(patch)
+        hasher.combine(prereleaseIdentifiers)
+    }
 }
 
 extension Version: CustomStringConvertible {
@@ -103,47 +222,28 @@ extension Version: CustomStringConvertible {
     }
 }
 
-public extension Version {
+extension Version: LosslessStringConvertible {
+    /// Initializes a version struct with the provided version string.
+    /// - Parameter version: A version string to use for creating a new version struct.
+    public init?(_ versionString: String) {
+        try? self.init(versionString: versionString)
+    }
+}
 
+extension Version {
+    // This initialiser is no longer necessary, but kept around for source compatibility with SwiftPM.
     /// Create a version object from string.
-    ///
-    /// - Parameters:
-    ///   - string: The string to parse.
-    init?(string: String) {
-        let prereleaseStartIndex = string.firstIndex(of: "-")
-        let metadataStartIndex = string.firstIndex(of: "+")
-
-        let requiredEndIndex = prereleaseStartIndex ?? metadataStartIndex ?? string.endIndex
-        let requiredCharacters = string.prefix(upTo: requiredEndIndex)
-        let requiredComponents = requiredCharacters
-            .split(separator: ".", maxSplits: 2, omittingEmptySubsequences: false)
-            .map(String.init).compactMap({ Int($0) }).filter({ $0 >= 0 })
-
-        guard requiredComponents.count == 3 else { return nil }
-
-        self.major = requiredComponents[0]
-        self.minor = requiredComponents[1]
-        self.patch = requiredComponents[2]
-
-        func identifiers(start: String.Index?, end: String.Index) -> [String] {
-            guard let start = start else { return [] }
-            let identifiers = string[string.index(after: start)..<end]
-            return identifiers.split(separator: ".").map(String.init)
-        }
-
-        self.prereleaseIdentifiers = identifiers(
-            start: prereleaseStartIndex,
-            end: metadataStartIndex ?? string.endIndex)
-        self.buildMetadataIdentifiers = identifiers(
-            start: metadataStartIndex,
-            end: string.endIndex)
+    /// - Parameter  string: The string to parse.
+    @available(*, deprecated, renamed: "init(_:)")
+    public init?(string: String) {
+        self.init(string)
     }
 }
 
 extension Version: ExpressibleByStringLiteral {
 
     public init(stringLiteral value: String) {
-        guard let version = Version(string: value) else {
+        guard let version = Version(value) else {
             fatalError("\(value) is not a valid version")
         }
         self = version
@@ -163,7 +263,7 @@ extension Version: JSONMappable, JSONSerializable {
         guard case .string(let string) = json else {
             throw JSON.MapError.custom(key: nil, message: "expected string, got \(json)")
         }
-        guard let version = Version(string: string) else {
+        guard let version = Version(string) else {
             throw JSON.MapError.custom(key: nil, message: "Invalid version string \(string)")
         }
         self.init(version)
@@ -192,7 +292,7 @@ extension Version: Codable {
         let container = try decoder.singleValueContainer()
         let string = try container.decode(String.self)
 
-        guard let version = Version(string: string) else {
+        guard let version = Version(string) else {
             throw DecodingError.dataCorrupted(.init(
                 codingPath: decoder.codingPath,
                 debugDescription: "Invalid version string \(string)"))
@@ -232,11 +332,10 @@ extension Range where Bound == Version {
 }
 
 extension Range where Bound == Version {
-
     public func contains(version: Version) -> Bool {
         // Special cases if version contains prerelease identifiers.
         if !version.prereleaseIdentifiers.isEmpty {
-            // If the ranage does not contain prerelease identifiers, return false.
+            // If the range does not contain prerelease identifiers, return false.
             if lowerBound.prereleaseIdentifiers.isEmpty && upperBound.prereleaseIdentifiers.isEmpty {
                 return false
             }