[PackageDescription] correct semantic version parsing and comparison (#…

…3486)

* [gardening] rename `PackageDescription4Tests` to `PackageDescriptionTests`

* assign directly to `self` the parsed version, instead of calling `init(_ version: Version)`

`init(_ version: Version)` is removed because it doesn’t have any caller left.

* correct semantic version parsing

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 "+".

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

* add tests for all sorts of `Version.init`

* correct semantic version comparison

`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).

* conform `Version` to `LosslessStringConvertible`

It already satisfies the requirement (`init?(_ versionString: String)`).

* [gardening] improve markdown formatting in documentation comments

* remove outdated test function

`VersionTests.testBasics` has been replaced by much more thorough test cases in the same file.

Package.swift

@@ -270,7 +270,7 @@ let package = Package(
             name: "FunctionalPerformanceTests",
             dependencies: ["swift-build", "swift-package", "swift-test", "SPMTestSupport"]),
         .testTarget(
-            name: "PackageDescription4Tests",
+            name: "PackageDescriptionTests",
             dependencies: ["PackageDescription"]),
         .testTarget(
             name: "SPMBuildCoreTests",

Sources/PackageDescription/Version+StringLiteralConvertible.swift

@@ -1,22 +1,19 @@
 /*
  This source file is part of the Swift.org open source project
 
- Copyright (c) 2018 Apple Inc. and the Swift project authors
+ Copyright (c) 2018 - 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
  See http://swift.org/CONTRIBUTORS.txt for Swift project authors
 */
 
 extension Version: ExpressibleByStringLiteral {
-
     /// Initializes a version struct with the provided string literal.
-    ///
-    /// - Parameters:
-    ///     - version: A string literal to use for creating a new version struct.
+    /// - Parameter version: A string literal to use for creating a new version struct.
     public init(stringLiteral value: String) {
         if let version = Version(value) {
-            self.init(version)
+            self = version
         } else {
             // If version can't be initialized using the string literal, report
             // the error and initialize with a dummy value. This is done to
@@ -44,51 +41,50 @@ extension Version: ExpressibleByStringLiteral {
     }
 }
 
-extension Version {
-
-    /// Initializes a version struct with the provided version.
-    ///
-    /// - Parameters:
-    ///     - version: A version object to use for creating a new version struct.
-    public init(_ version: Version) {
-        major = version.major
-        minor = version.minor
-        patch = version.patch
-        prereleaseIdentifiers = version.prereleaseIdentifiers
-        buildMetadataIdentifiers = version.buildMetadataIdentifiers
-    }
-
+extension Version: LosslessStringConvertible {
     /// Initializes a version struct with the provided version string.
-    ///
-    /// - Parameters:
-    ///     - version: A version string to use for creating a new version struct.
+    /// - Parameter version: A version string to use for creating a new version struct.
     public init?(_ versionString: String) {
-        let prereleaseStartIndex = versionString.firstIndex(of: "-")
-        let metadataStartIndex = versionString.firstIndex(of: "+")
-
-        let requiredEndIndex = prereleaseStartIndex ?? metadataStartIndex ?? versionString.endIndex
-        let requiredCharacters = versionString.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 = versionString[versionString.index(after: start)..<end]
-            return identifiers.split(separator: ".").map(String.init)
+        // 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 { return nil }
+
+        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,
+            // 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 { return nil }
+
+        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 { return nil }
+            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 { return nil }
+            self.buildMetadataIdentifiers = buildMetadataIdentifiers.map { String($0) }
+        } else {
+            self.buildMetadataIdentifiers = []
         }
-
-        self.prereleaseIdentifiers = identifiers(
-            start: prereleaseStartIndex,
-            end: metadataStartIndex ?? versionString.endIndex)
-        self.buildMetadataIdentifiers = identifiers(start: metadataStartIndex, end: versionString.endIndex)
     }
 }

Sources/PackageDescription/Version.swift

@@ -1,7 +1,7 @@
 /*
  This source file is part of the Swift.org open source project
 
- Copyright (c) 2018 Apple Inc. and the Swift project authors
+ Copyright (c) 2018 - 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
@@ -55,11 +55,11 @@ public struct Version {
     /// Initializes a version struct with the provided components of a semantic version.
     ///
     /// - Parameters:
-    ///     - major: The major version number.
-    ///     - minor: The minor version number.
-    ///     - patch: The patch version number.
-    ///     - prereleaseIdentifiers: The pre-release identifier.
-    ///     - buildMetaDataIdentifiers: Build metadata that identifies a build.
+    ///   - major: The major version number.
+    ///   - minor: The minor version number.
+    ///   - patch: The patch version number.
+    ///   - prereleaseIdentifiers: The pre-release identifier.
+    ///   - buildMetaDataIdentifiers: Build metadata that identifies a build.
     public init(
         _ major: Int,
         _ minor: Int,
@@ -77,6 +77,12 @@ public struct Version {
 }
 
 extension Version: Comparable {
+    // 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]