[TSCUtility] Correct semantic version parsing and comparison #214

WowbaggersLiquidLunch · 2021-05-13T20:41:17Z

This PR is similar to swiftlang/swift-package-manager#3486, and likely conflicts with #212.

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.data is a well-formed, with 1.2.3 being the version core and some-meta.data the build metadata identifiers. However, the old implementation of Version.init?(_ versionString: String) would incorrectly 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) which calls init(versionString: String) throws. 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.

comparison

Version already conforms to Comparable, but Comparable does not provide a default implementation for ==, so the compiler synthesises one composed of member-wise comparisons. This leads to a false false when 2 semantic versions differ by only their build metadata identifiers, contradicting SemVer 2.0.0's comparison rules.

This PR adds an implementation of == to Version that returns true iff one version is neither greater nor less than the other. One consequence, though, is that now two versions that differ by only their build metadata identifiers are not allowed in the same set, and one assertion in the tests is inverted accordingly.

Also, because Version declares conformance to Hashable, this PR adds a custom hash(into:) that aligns with the custom ==.

WowbaggersLiquidLunch · 2021-05-13T20:44:11Z

Tests/TSCUtilityTests/VersionTests.swift

-        XCTAssertNotEqual(Set([Version(1,2,3)]), Set([Version(1,2,3, buildMetadataIdentifiers: ["1011"])]))
+        XCTAssertEqual(Set([Version(1,2,3)]), Set([Version(1,2,3, buildMetadataIdentifiers: ["1011"])]))


Now that two version that differ only by their build metadata identifiers are considered equal, they can not exist in the same set, or as keys in the same dictionary, etc. Would this be considered a correct behaviour?

I'm not sure whether it would or not — could this cause problems for existing packages? I suppose in that case there would already have been an error about there being two versions of a single package in the same graph, when according to semver rules those should always have been treated the same. So if I understand correctly this would be safe because it's more permissive than it would have been in the past?

I'm not sure whether that really answers anything — I'm trying to understand whether there could be anything here that causes existing packages to break.

WowbaggersLiquidLunch · 2021-05-13T20:54:03Z

When given an invalid version string, most Version initialisers crash instead of creating a dummy version 0.0.0 like what they do in SwiftPM. How do I write tests to check that invalid strings do result in fatalerror without actually crashing the tests?

tomerd · 2021-05-13T22:03:36Z

When given an invalid version string, most Version initialisers crash instead of creating a dummy version 0.0.0 like what they do in SwiftPM. How do I write tests to check that invalid strings do result in fatalerror without actually crashing the tests?

the way to do this with Swift would be a throwing or failable initializer, fatalError is designed for more severe assertion like situation when the program must stop running due to risk of memory corruption etc

WowbaggersLiquidLunch · 2021-05-13T22:26:25Z

the way to do this with Swift would be a throwing or failable initializer, fatalError is designed for more severe assertion like situation when the program must stop running due to risk of memory corruption etc

I agree that fatalError doesn't seem right in Version's initializers. Instead of changing them to throwing or failable, which is going to affect their call sites in SwiftPM, what do you think of having them creating 0.0.0 dummy versions, like SwiftPM's Version initialisers do?

Also, should I change them as part of this PR, or in a follow-up PR?

tomerd · 2021-05-13T22:35:20Z

what do you think of having them creating 0.0.0 dummy versions, like SwiftPM's Version initialisers do?

IMO returning dummy versions is not ideal, and a throwable or optional feels safer. that said, I dont have the history on why SwiftPM does that. @abertelrud @neonichu wdyt?

tomerd · 2021-05-13T22:35:59Z

Also, should I change them as part of this PR, or in a follow-up PR?

personally, I always prefer smaller PRs where possible

abertelrud · 2021-05-17T19:15:32Z

what do you think of having them creating 0.0.0 dummy versions, like SwiftPM's Version initialisers do?

IMO returning dummy versions is not ideal, and a throwable or optional feels safer. that said, I dont have the history on why SwiftPM does that. @abertelrud @neonichu wdyt?

Creating dummy versions (or any kind of value, really) on error is a surprise to me, and seems wrong. So if SwiftPM is doing that it should probably change (in a separate PR to keep each one as focused as possible), instead throwing an error if there is a problem. The caller can then always decide to substitute a default value after reporting the error, if that's appropriate.

WowbaggersLiquidLunch · 2021-06-03T02:46:25Z

Sorry for the late response.

So if SwiftPM is doing that it should probably change (in a separate PR to keep each one as focused as possible), instead throwing an error if there is a problem.

Would a failable initializer be better than a throwing one? I don't remember where I read it (I can't find it in any documentation or the API design guidelines), but I remember reading that failable initializers should be used where the reason of the failure is clear. For example, init?(string:). And throwing initializers should be used where there are many reasons why it can fail.

neonichu · 2021-06-29T22:23:51Z

@swift-ci please test

WowbaggersLiquidLunch · 2021-07-08T04:14:33Z

Regarding the fatalError in one of TSC's Version.init and dummy version 0.0.0 in one of SwiftPM's, I just checked them again with fresh eyes.

Both of them happen in init(stringLiteral value: String), which is a requirement of ExpressibleByStringLiteral. Because the requirement is non-throwing and now-failable, neither throwing nor failable initializers can satisfy it. This leaves fatalError and dummy version as the only solutions.

I don't know if it was even correct to make Version be ExpressibleByStringLiteral to begin with, since it only works with specifically formatted strings. However, the correctness probably doesn't matter much now, because it doesn't seem possible to either remove or replace the conformance, because doing so is a big source break.

I also don't know which is the better choice between fatalError and dummy version. On one hand, it's (probably) better to crash than to create invalid output from invalid input; on the other hand, SwiftPM (probably), like the compiler, shouldn't crash even when the input is invalid. However, regardless of which one is better, I think TSC and SwiftPM should be consistent and use the same one.

neonichu · 2021-07-08T21:41:10Z

I don't know if it was even correct to make Version be ExpressibleByStringLiteral to begin with, since it only works with specifically formatted strings.

Not entirely sure, but I am assuming this was done to be able to use literal strings for versions in a package manifest which does seem desirable. That said, it should be possible to express that in a different way today, e.g. possibly an enum with a case for an invalid version?

neonichu · 2021-07-12T17:24:52Z

@WowbaggersLiquidLunch Apart from the question of how this API should potentially look like, I think for the purpose of this PR we can leave it as-is and you could write all your tests around the fallible initializer that also exists. What do you think?

Sources/TSCUtility/Version.swift

WowbaggersLiquidLunch · 2021-07-14T02:24:39Z

Sorry for the late response!

Not entirely sure, but I am assuming this was done to be able to use literal strings for versions in a package manifest which does seem desirable.

Yes I think this is exactly why SwiftPM's Version is ExpressibleByStringLiteral. However, I don't think TSC's Version is used in the manifest.

That said, it should be possible to express that in a different way today, e.g. possibly an enum with a case for an invalid version?

I haven't thought of it. It does seem like a possible solution though.

Apart from the question of how this API should potentially look like, I think for the purpose of this PR we can leave it as-is

I agree. What this API should look like should be its own PR. Would it be better if I move this discussion to the forums?

you could write all your tests around the fallible initializer that also exists.

This prompted me to check the tests again, and I think init?(_) is missing some tests for invalid inputs. I'm going to add those tests.

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)

Also rearranged the tests.

Sources/TSCUtility/Version.swift

abertelrud · 2021-08-02T20:11:23Z

@neonichu Do you think this can be merged at this point, given that it's blocking #212?

Sources/TSCUtility/Version.swift

abertelrud · 2021-08-02T21:47:00Z

@swift-ci please test

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.

WowbaggersLiquidLunch · 2021-08-03T21:11:04Z

Sorry for the new force-push. Just fixed a few more typos and squashed it with one of the commits. No more changes to this PR unless requested.

tomerd · 2021-08-03T21:13:23Z

@swift-ci please test

tomerd · 2021-08-04T18:22:23Z

thank you @WowbaggersLiquidLunch!

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)

WowbaggersLiquidLunch requested review from abertelrud, aciidgh, friedbunny, neonichu and tomerd as code owners May 13, 2021 20:41

WowbaggersLiquidLunch commented May 13, 2021

View reviewed changes

tomerd assigned abertelrud May 13, 2021

tomerd assigned neonichu and unassigned abertelrud May 13, 2021

WowbaggersLiquidLunch force-pushed the correct-semantic-version-parsing-and-comparison branch 2 times, most recently from 0e96e65 to a95194d Compare June 12, 2021 04:03

WowbaggersLiquidLunch mentioned this pull request Jun 26, 2021

[PackageDescription] correct semantic version parsing and comparison swiftlang/swift-package-manager#3486

Merged

neonichu reviewed Jul 14, 2021

View reviewed changes

Sources/TSCUtility/Version.swift Show resolved Hide resolved

WowbaggersLiquidLunch added 5 commits July 28, 2021 14:08

add additional tests for initialising Version

298ce36

Also rearranged the tests.

[garderning] fix typo "ranage" → "range"

f717de4

WowbaggersLiquidLunch force-pushed the correct-semantic-version-parsing-and-comparison branch from a95194d to 4c4689c Compare July 29, 2021 18:04

WowbaggersLiquidLunch commented Jul 29, 2021

View reviewed changes

Sources/TSCUtility/Version.swift Show resolved Hide resolved

WowbaggersLiquidLunch commented Jul 29, 2021

View reviewed changes

Sources/TSCUtility/Version.swift Outdated Show resolved Hide resolved

WowbaggersLiquidLunch mentioned this pull request Jul 29, 2021

Add lenient mode to Version string parsing that doesn't require patch version #212

Merged

WowbaggersLiquidLunch force-pushed the correct-semantic-version-parsing-and-comparison branch from 4bfd1ce to 9feba0d Compare August 2, 2021 21:17

WowbaggersLiquidLunch commented Aug 2, 2021

View reviewed changes

Sources/TSCUtility/Version.swift Show resolved Hide resolved

abertelrud approved these changes Aug 2, 2021

View reviewed changes

neonichu approved these changes Aug 3, 2021

View reviewed changes

WowbaggersLiquidLunch added 2 commits August 3, 2021 16:48

[gardening] remove horizontal whitespace from whitespace-only lines

7336c0c

WowbaggersLiquidLunch force-pushed the correct-semantic-version-parsing-and-comparison branch from 9feba0d to 7336c0c Compare August 3, 2021 21:10

tomerd merged commit df0b2ea into swiftlang:main Aug 4, 2021

WowbaggersLiquidLunch mentioned this pull request Aug 25, 2021

[WIP][SR-14665] Warn of potentially incorrectly synthesized Equatable conformance for custom Comparable conformance swiftlang/swift#39047

Draft

WowbaggersLiquidLunch mentioned this pull request Mar 16, 2023

[SR-5693] SemVer variant incompatible with SwiftPM swiftlang/swift-package-manager#4966

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TSCUtility] Correct semantic version parsing and comparison #214

[TSCUtility] Correct semantic version parsing and comparison #214

WowbaggersLiquidLunch commented May 13, 2021 •

edited

Loading

WowbaggersLiquidLunch May 13, 2021

abertelrud May 17, 2021

abertelrud May 17, 2021

WowbaggersLiquidLunch commented May 13, 2021

tomerd commented May 13, 2021 •

edited

Loading

WowbaggersLiquidLunch commented May 13, 2021

tomerd commented May 13, 2021

tomerd commented May 13, 2021

abertelrud commented May 17, 2021

WowbaggersLiquidLunch commented Jun 3, 2021

neonichu commented Jun 29, 2021

WowbaggersLiquidLunch commented Jul 8, 2021

neonichu commented Jul 8, 2021

neonichu commented Jul 12, 2021

WowbaggersLiquidLunch commented Jul 14, 2021

abertelrud commented Aug 2, 2021

abertelrud commented Aug 2, 2021

WowbaggersLiquidLunch commented Aug 3, 2021

tomerd commented Aug 3, 2021

tomerd commented Aug 4, 2021

		XCTAssertNotEqual(Set([Version(1,2,3)]), Set([Version(1,2,3, buildMetadataIdentifiers: ["1011"])]))
		XCTAssertEqual(Set([Version(1,2,3)]), Set([Version(1,2,3, buildMetadataIdentifiers: ["1011"])]))

[TSCUtility] Correct semantic version parsing and comparison #214

[TSCUtility] Correct semantic version parsing and comparison #214

Conversation

WowbaggersLiquidLunch commented May 13, 2021 • edited Loading

parsing

comparison

WowbaggersLiquidLunch May 13, 2021

Choose a reason for hiding this comment

abertelrud May 17, 2021

Choose a reason for hiding this comment

abertelrud May 17, 2021

Choose a reason for hiding this comment

WowbaggersLiquidLunch commented May 13, 2021

tomerd commented May 13, 2021 • edited Loading

WowbaggersLiquidLunch commented May 13, 2021

tomerd commented May 13, 2021

tomerd commented May 13, 2021

abertelrud commented May 17, 2021

WowbaggersLiquidLunch commented Jun 3, 2021

neonichu commented Jun 29, 2021

WowbaggersLiquidLunch commented Jul 8, 2021

neonichu commented Jul 8, 2021

neonichu commented Jul 12, 2021

WowbaggersLiquidLunch commented Jul 14, 2021

abertelrud commented Aug 2, 2021

abertelrud commented Aug 2, 2021

WowbaggersLiquidLunch commented Aug 3, 2021

tomerd commented Aug 3, 2021

tomerd commented Aug 4, 2021

WowbaggersLiquidLunch commented May 13, 2021 •

edited

Loading

tomerd commented May 13, 2021 •

edited

Loading