[PackageDescription] correct semantic version parsing and comparison #3486
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WowbaggersLiquidLunch
force-pushed
the
correct-semantic-version-parsing
branch
from
e8dd2e7
to
6e7aefa
Compare
WowbaggersLiquidLunch
changed the title
WowbaggersLiquidLunch
force-pushed
the
correct-semantic-version-parsing
branch
2 times, most recently
from
13d808f
to
411e395
Compare
WowbaggersLiquidLunch
requested review from
abertelrud,
friedbunny,
neonichu and
tomerd
as code owners
|
Thanks for your contribution. I'm a bit busy right now, so I will take a closer look in a few days. |
WowbaggersLiquidLunch
force-pushed
the
correct-semantic-version-parsing
branch
from
24c4c21
to
41c52dd
Compare
|
@neonichu will be ready to review this PR soon? |
WowbaggersLiquidLunch
force-pushed
the
correct-semantic-version-parsing
branch
2 times, most recently
from
aef1978
to
30c8fd7
Compare
neonichu
approved these changes
Jun 22, 2021
|
@swift-ci please smoke test |
|
@neonichu Should I cherrypick this to 5.4 and 5.5 branches? |
|
Don't think we would add it to 5.4 at this point and not sure about 5.5, either, unless there is a strong case for it. |
|
I think an argument for cherrypicking to 5.5 is that it's not introducing a new feature, but fixing the correctness of an existing feature. Since we have a chance to introduce the correction to 5.5 before it ships, maybe we should do it. I think this argument is in line with 5.5's release process, but I don't know if the risk and impact are low enough to pass the bar. |
if there are no known severe reports of bugs caused by this, I would de-risk from 5.5 (but merge into main) |
Sources/PackageDescription/Version+StringLiteralConvertible.swift
Outdated
Show resolved
Hide resolved
…t(_ version: Version)` `init(_ version: Version)` is removed because it doesn’t have any caller left.
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.
`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).
It already satisfies the requirement (`init?(_ versionString: String)`).
`VersionTests.testBasics` has been replaced by much more thorough test cases in the same file.
WowbaggersLiquidLunch
force-pushed
the
correct-semantic-version-parsing
branch
from
30c8fd7
to
01375ce
Compare
|
@swift-ci please smoke test |
|
@neonichu @WowbaggersLiquidLunch is this ready to be merged? |
|
@tomerd yes I think it's ready. There was some discussion on swiftlang/swift-tools-support-core#214 that basically says that initializers should fail or throw errors when the input is bad, instead of creating dummy versions. In my opinion those changes should come as separate PRs, because they're not closely related to this PR. |
WowbaggersLiquidLunch
added a commit
to WowbaggersLiquidLunch/swift-package-manager
that referenced
this pull request
Aug 18, 2021
There is also some minor formatting of previous entries for style consistency.
WowbaggersLiquidLunch
added a commit
to WowbaggersLiquidLunch/swift-package-manager
that referenced
this pull request
Aug 18, 2021
There is also some minor formatting of previous entries for style consistency.
WowbaggersLiquidLunch
added a commit
to WowbaggersLiquidLunch/swift-package-manager
that referenced
this pull request
Aug 18, 2021
There is also some minor formatting of previous entries for style consistency.
tomerd
pushed a commit
that referenced
this pull request
Aug 18, 2021
There is also some minor formatting of previous entries for style consistency.
WowbaggersLiquidLunch
added a commit
to WowbaggersLiquidLunch/swift
that referenced
this pull request
Sep 20, 2021
Currently `Comparable` inherits from `Equatable`, but does not provide a default implementation for `==`, so the compiler synthesizes one composed of [member-wise `==`s](https://github.com/apple/swift-evolution/blob/main/proposals/0185-synthesize-equatable-hashable.md#implementation-details). This leads to a problem where if a type's `<` is not composed of member-wise inequalities, then `<`, `>`, and `==` can all evaluate to `false` for some pairs of values, contradicting `Comparable`'s documentation: > Types with Comparable conformance implement the less-than operator (`<`) and the equal-to operator (`==`). These two operations impose a strict total order on the values of a type, in which exactly one of the following must be true for any two values `a` and `b`: > * `a == b` > * `a < b` > * `b < a` For example: ```swift struct Length: Comparable { enum Unit: Double, Comparable { case mm = 0.001 case m = 1 case banana = 0.178 } let magnitude: Double let unit: Unit static func < (lhs: Self, rhs: Self) -> Bool { lhs.magnitude * lhs.unit.rawValue < rhs.magnitude * rhs.unit.rawValue } } let aBanana = Length(magnitude: 1, unit: .banana) let oneBanana = Length(magnitude: 0.178, unit: .m) print(aBanana < oneBanana) // prints "false", because Length's < says so. print(aBanana > oneBanana) // prints "false", because Comparable's default implementation of >(a,b) is <(b,a). print(aBanana == oneBanana) // prints "false", because the 2 Length instances are not member-wise equal. ``` Relevant forums discussion: https://forums.swift.org/t/add-default-implementation-of-to-comparable/48832 This bug has previously resulted in incorrect semantic version comparison in SwiftPM (swiftlang/swift-package-manager#3486 and swiftlang/swift-tools-support-core#214)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
1. parsing
The semantic versioning specification 2.0.0 states that pre-release identifiers must be positioned after the version core, and build metadata identifiers after pre-release identifiers. It also states that both pre-release and build metadata identifiers can contain "-" (hyphens), while at the same time "-" is used to indicate where pre-release identifiers begin.
In the old (currently shipped) implementation, if a version core was appended with build metadata identifiers that contain "-", the first "-" would be mistaken as an indication of pre-release identifiers thereafter. Then, the position of the first "-" would be treated as where the version core ends, resulting in a false negative after it was found that the version core (plus a part of the build metadata identifiers) contained non-numeric characters.
For example: the semantic version
1.2.3+some-meta.datais a well-formed, with1.2.3being the version core andsome-meta.datathe build metadata identifiers. However, the old implementation ofVersion.init?(_ versionString: String)would incorrectly treat1.2.3+someas the version core andmeta.datathe 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.
2. comparison
Versionalready conforms toComparable, butComparabledoes not provide a default implementation for==, so the compiler synthesises one composed of member-wise comparisons. This leads to a falsefalsewhen 2 semantic versions differ by only their build metadata identifiers, contradicting SemVer 2.0.0's comparison rules.This PR adds an implementation of
==toVersionthat returnstrueiff one version is neither greater nor less than the other.