-
Notifications
You must be signed in to change notification settings - Fork 40
[DNM] Comply SymbolGraph.SemanticVersion
to SemVer 2.0.0
#36
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
2dd562b
ff3e40f
4e2b309
b5f7003
ecbede7
1790ae6
4d9783d
c27b2bf
98a49de
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
/* | ||
This source file is part of the Swift.org open source project | ||
|
||
Copyright (c) 2022 Apple Inc. and the Swift project authors | ||
Licensed under Apache License v2.0 with Runtime Library Exception | ||
|
||
See https://swift.org/LICENSE.txt for license information | ||
See https://swift.org/CONTRIBUTORS.txt for Swift project authors | ||
*/ | ||
|
||
extension Character { | ||
/// A Boolean value indicating whether this character is allowed in a semantic version's identifier. | ||
internal var isAllowedInSemanticVersionIdentifier: Bool { | ||
isASCII && (isLetter || isNumber || self == "-") | ||
} | ||
|
||
/// A Boolean value indicating whether this character is allowed in a semantic version's numeric identifier. | ||
internal var isAllowedInSemanticVersionNumericIdentifier: Bool { | ||
isASCII && isNumber | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,144 @@ | ||
/* | ||
This source file is part of the Swift.org open source project | ||
|
||
Copyright (c) 2022 Apple Inc. and the Swift project authors | ||
Licensed under Apache License v2.0 with Runtime Library Exception | ||
|
||
See https://swift.org/LICENSE.txt for license information | ||
See https://swift.org/CONTRIBUTORS.txt for Swift project authors | ||
*/ | ||
|
||
extension SymbolGraph.SemanticVersion { | ||
/// A storage for pre-release identifiers. | ||
internal struct Prerelease { | ||
/// The identifiers. | ||
internal let identifiers: [Identifier] | ||
/// A pre-release identifier. | ||
internal enum Identifier { | ||
/// A numeric pre-release identifier. | ||
/// - Parameter identifier: The identifier. | ||
case numeric(_ identifier: UInt) | ||
/// An alphanumeric pre-release identifier. | ||
/// - Parameter identifier: The identifier. | ||
case alphanumeric(_ identifier: String) | ||
} | ||
} | ||
} | ||
|
||
// MARK: - Initializers | ||
|
||
extension SymbolGraph.SemanticVersion.Prerelease { | ||
/// Creates a semantic version pre-release from the given pre-release string. | ||
/// - Note: Empty string translates to an empty pre-release identifier, which is invalid. | ||
/// - Parameter dotSeparatedIdentifiers: The given pre-release string to create a semantic version pre-release from. | ||
/// - Throws: A `SymbolGraph.SemanticVersionError` instance if `dotSeparatedIdentifiers` is not a valid pre-release string. | ||
internal init(_ dotSeparatedIdentifiers: String?) throws { | ||
guard let dotSeparatedIdentifiers = dotSeparatedIdentifiers else { | ||
// FIXME: initialize 'identifiers' directly here after [SR-15670](https://bugs.swift.org/projects/SR/issues/SR-15670?filter=allopenissues) is resolved | ||
// currently 'identifiers' cannot be initialized directly because initializer delegation is flow-insensitive | ||
// self.identifiers = [] | ||
self.init(identifiers: []) | ||
return | ||
} | ||
let identifiers = dotSeparatedIdentifiers.split( | ||
separator: ".", | ||
omittingEmptySubsequences: false // Preserve empty sequences to be able to raise validation errors about empty prerelease identifiers. | ||
) | ||
try self.init(identifiers) | ||
} | ||
|
||
/// Creates a semantic version pre-release from the given pre-release identifier strings. | ||
/// - Parameter identifiers: The given pre-release identifier strings to create a semantic version pre-release from. | ||
/// - Throws: A `SymbolGraph.SemanticVersionError` instance if any element of `identifiers` is not a valid pre-release identifier string. | ||
internal init<C: Collection, S: StringProtocol>(_ identifiers: C) throws where C.Element == S, S.SubSequence == Substring { | ||
self.identifiers = try identifiers.map { | ||
try Identifier($0) | ||
} | ||
} | ||
} | ||
|
||
extension SymbolGraph.SemanticVersion.Prerelease.Identifier { | ||
/// Creates a semantic version pre-release identifier from the given pre-release identifier string. | ||
/// - Parameter identifierString: The given pre-release identifier string to create a semantic version pre-release identifier from. | ||
/// - Throws: A `SymbolGraph.SemanticVersionError` instance if `identifierString` is not a valid pre-release identifier string. | ||
internal init<S: StringProtocol>(_ identifierString: S) throws where S.SubSequence == Substring { | ||
guard !identifierString.isEmpty else { | ||
throw SymbolGraph.SemanticVersionError.emptyIdentifier(position: .prerelease) | ||
} | ||
guard identifierString.allSatisfy(\.isAllowedInSemanticVersionIdentifier) else { | ||
throw SymbolGraph.SemanticVersionError.invalidCharacterInIdentifier(String(identifierString), position: .prerelease) | ||
} | ||
if identifierString.allSatisfy(\.isNumber) { | ||
// diagnose the identifier as a numeric identifier, if all characters are ASCII digits | ||
guard identifierString.first != "0" || identifierString == "0" else { | ||
throw SymbolGraph.SemanticVersionError.invalidNumericIdentifier( | ||
String(identifierString), | ||
position: .prerelease, | ||
errorKind: .leadingZeros | ||
) | ||
} | ||
guard let numericIdentifier = UInt(identifierString) else { | ||
if identifierString.isEmpty { | ||
throw SymbolGraph.SemanticVersionError.emptyIdentifier(position: .prerelease) | ||
} else { | ||
throw SymbolGraph.SemanticVersionError.invalidNumericIdentifier( | ||
String(identifierString), | ||
position: .prerelease, | ||
errorKind: .oversizedValue | ||
) | ||
} | ||
} | ||
self = .numeric(numericIdentifier) | ||
} else { | ||
self = .alphanumeric(String(identifierString)) | ||
} | ||
} | ||
} | ||
|
||
// MARK: - Comparable Conformances | ||
|
||
// Compiler synthesised `Equatable`-conformance is correct here. | ||
extension SymbolGraph.SemanticVersion.Prerelease: Comparable { | ||
internal static func <(lhs: Self, rhs: Self) -> Bool { | ||
guard !lhs.identifiers.isEmpty else { return false } // non-pre-release lhs >= potentially pre-release rhs | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If prerelease was an optional value on SemanticVersion then these empty checks wouldn't be necessary. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. True, but then we'd be trading an empty check for an optional check. We always have to check whether there is pre-release information regardless. Additionally, it will necessitate re-parsing (and maybe validating) the pre-release string every time on comparison. |
||
guard !rhs.identifiers.isEmpty else { return true } // pre-release lhs < non-pre-release rhs | ||
return lhs.identifiers.lexicographicallyPrecedes(rhs.identifiers) | ||
} | ||
} | ||
|
||
// Compiler synthesised `Equatable`-conformance is correct here. | ||
extension SymbolGraph.SemanticVersion.Prerelease.Identifier: Comparable { | ||
internal static func <(lhs: Self, rhs: Self) -> Bool { | ||
switch (lhs, rhs) { | ||
case let (.numeric(lhs), .numeric(rhs)): | ||
return lhs < rhs | ||
case let(.alphanumeric(lhs), .alphanumeric(rhs)): | ||
return lhs < rhs | ||
case (.numeric, .alphanumeric): | ||
return true | ||
case (.alphanumeric, .numeric): | ||
return false | ||
} | ||
} | ||
} | ||
|
||
// MARK: CustomStringConvertible Conformances | ||
|
||
extension SymbolGraph.SemanticVersion.Prerelease: CustomStringConvertible { | ||
/// A textual description of the pre-release. | ||
internal var description: String { | ||
identifiers.map(\.description).joined(separator: ".") | ||
} | ||
} | ||
|
||
extension SymbolGraph.SemanticVersion.Prerelease.Identifier: CustomStringConvertible { | ||
/// A textual description of the identifier. | ||
internal var description: String { | ||
switch self { | ||
case .numeric(let identifier): | ||
return identifier.description | ||
case .alphanumeric(let identifier): | ||
return identifier.description | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
/* | ||
This source file is part of the Swift.org open source project | ||
|
||
Copyright (c) 2022 Apple Inc. and the Swift project authors | ||
Licensed under Apache License v2.0 with Runtime Library Exception | ||
|
||
See https://swift.org/LICENSE.txt for license information | ||
See https://swift.org/CONTRIBUTORS.txt for Swift project authors | ||
*/ | ||
|
||
extension SymbolGraph.SemanticVersion: 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). | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm torn about about using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What are some possible misuses you have in mind? As far as I can tell this is exactly what There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If a developer thinks that two versions with different build metadata should be considered different but they are considered equal and that developer writes code based on that assumption that has bugs because the assumption is wrong I would consider that an unintended misuse of this equality behavior. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see. Maybe a note in the documentation could help, maybe similar to how There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
In the Some of those considerations apply here as well. For example: let versions = try! ["1.2.3-alpha+002", "1.2.3-alpha+001"].map { SemanticVersion(...) }
versions.sorted() // order didn't change because build metadata isn't considered in comparison
versions.contains("1.2.3-alpha+003") // true because build metadata isn't considered in equality check Documentation helps but I'm still thinking about if this would be better addressed at the API level by not using Equatable/Comparable for these behaviors. See this comment. In the FloatingPoint case it's clearly worth it but I feel that the semantic versions aren't as frequently compared as floating point numbers are and that semantic versions with build metadata would be more common than not-a-number floating point values. So that's why I'm a bit hesitant on this. |
||
// [SR-14665](https://github.com/apple/swift/issues/57016) | ||
/// Returns a Boolean value indicating whether two semantic versions are equal. | ||
/// - Parameters: | ||
/// - lhs: A semantic version to compare. | ||
/// - rhs: Another semantic version to compare. | ||
/// - Returns: `true` if `lhs` and `rhs` are equal; `false` otherwise. | ||
@inlinable | ||
public static func == (lhs: Self, rhs: Self) -> Bool { | ||
!(lhs < rhs) && !(lhs > rhs) | ||
} | ||
|
||
/// Returns a Boolean value indicating whether the first semantic version precedes the second semantic version. | ||
/// - Parameters: | ||
/// - lhs: A semantic version to compare. | ||
/// - rhs: Another semantic version to compare. | ||
/// - Returns: `true` if `lhs` precedes `rhs`; `false` otherwise. | ||
public static func < (lhs: Self, rhs: Self) -> Bool { | ||
let lhsVersionCore = [lhs.major, lhs.minor, lhs.patch] | ||
let rhsVersionCore = [rhs.major, rhs.minor, rhs.patch] | ||
|
||
guard lhsVersionCore == rhsVersionCore else { | ||
return lhsVersionCore.lexicographicallyPrecedes(rhsVersionCore) | ||
} | ||
|
||
return lhs.prerelease < rhs.prerelease // not lexicographically compared | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
/* | ||
This source file is part of the Swift.org open source project | ||
|
||
Copyright (c) 2022 Apple Inc. and the Swift project authors | ||
Licensed under Apache License v2.0 with Runtime Library Exception | ||
|
||
See https://swift.org/LICENSE.txt for license information | ||
See https://swift.org/CONTRIBUTORS.txt for Swift project authors | ||
*/ | ||
|
||
extension SymbolGraph.SemanticVersion: CustomStringConvertible { | ||
/// A textual description of the semantic version. | ||
public var description: String { | ||
var versionString = "\(major).\(minor).\(patch)" | ||
if !prerelease.identifiers.isEmpty { | ||
versionString += "-\(prerelease)" | ||
} | ||
if !buildMetadataIdentifiers.isEmpty { | ||
versionString += "+" + buildMetadataIdentifiers.joined(separator: ".") | ||
} | ||
return versionString | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the prerelease information on semantic was optional, then this could accept a non-optional String argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the internal representation of the pre-release information has much to do with this initializer signature. Rather, the reason that
dotSeparatedIdentifiers
is optional is because the pre-release string it's an optional information in the SymbolKit's OpenAPI documentation, and decoded as aString?
by SymbolKit. The only way to make it non-optional imo is moving the optional checking outside to right before the call site.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See this comment: if the prerelease information on semantic was optional array of string then this initializer wouldn't need to check for nil values. Instead, if the prerelease string was
nil
then no Prerelease would be initialized. This could for example be done with optional map at the call site.