[DNM] Comply SymbolGraph.SemanticVersion to SemVer 2.0.0

[WowbaggersLiquidLunch]

This is a correct but somewhat over-engineered reinplementation of SymbolGraph.SemanticVersion.

The most important (and also most invasive) changes are the handling of pre-release and build metadata. These include their validation and (their contribution to version-) comparison.

Some less important changes include the following:

• Version core identifiers are changed to be backed by UInt, because they’re not allowed be less than 0. This might appear to be a departure from the convention of using Int for even always-positive integers (e.g. Array.Index). However, in this specific case, the use of version coure identifiers is mostly contained in the domain of constructing semantic versions, and does not have any apparent need to participate in numeric operations outside of the domain other than comparing individual identifiers. And considering Swift’s current lack of build-time evaluation and error handling, using UInt looks like the right trade off between ergnomics and “safety”.

• Deprecated the initializer for a new one that throws error, so that erros can be handled instead of runtime trapping.

• Added some facilities for inspecting and working with versions’ semantics in general.

• Updated the tools version specifier in the main package manifest to “5.6”, because Swift 5.2 has some type-checking bugs that affect the new implementation.

• Added a lot of tests, which should cover a large area of (edge) cases.

Checklist

• Added tests
• Ran the ./bin/test script and it succeeded (Tests pass using the 2020-05-23 trunk toolchain on Xcode, but ./bin/test fails with "_$s14SymbolKitTests05ErrorC0C26testOversizedNumericValuesyyF3sumL_ySsSS_SStF10zeroPaddedL__8toLengthSaySJGSS_SitF", referenced from: _$s14SymbolKitTests05ErrorC0C26testOversizedNumericValuesyyF3sumL_ySsSS_SStF in ErrorTests.swift.o"
• Updated documentation if necessary

[WowbaggersLiquidLunch]

This PR is ready for review, but please DO NOT MERGE for now. The callsites in swift-docc need to be adjusted before this can be merged.

[WowbaggersLiquidLunch]

@swift-ci please test

[d-ronnqvist]

Hi @WowbaggersLiquidLunch,

Thanks for opening this PR. The changes are fairly big so I've left some feedback inline but I'll repeat some of the key feedback below:

The most important (and also most invasive) changes are the handling of pre-release and build metadata. These include their validation and (their contribution to version-) comparison.

Note that this is a source breaking change since these public variables are no longer strings. This can be resolved by keeping the string properties as computed descriptions of the validated values but marking them as deprecated.

I think that these two would be better modeled as optional information instead of empty arrays of information.

Deprecated the initializer for a new one that throws error, so that errors can be handled instead of runtime trapping.

I feel like there should be an initializer with only major, minor, and patch information that doesn't throw so that developers don't need to know the implementation to know when it's safe to use try!.

Added some facilities for inspecting and working with versions’ semantics in general.

I have some concerns that developers could expect member-wise behaviors for the == and < operators.

Some of the named functions for operating on or comparing could be shortened or written less formally. For example

- func nextMajorReleaseVersion(from version: Self) -> Self 
+ func nextMajor(from version: Self) -> Self 

- static var initialStableReleaseVersion: Self
+ static var initialStable: Self

- var denotesStableRelease: Bool
+ var isStable: Bool

- func denotesSourceBreakingRelease(fromThatWidth other: Self) -> Bool
+ func isSourceBreakingUpdate(from other: Self) -> Bool

Updated the tools version specifier in the main package manifest to “5.6”, because Swift 5.2 has some type-checking bugs that affect the new implementation.

Can you add explicit type annotations in the code to so that this change isn't needed?

[d-ronnqvist]

I feel that it would be more correct model prerelease information ad build metadata with optionals.

[WowbaggersLiquidLunch]

Could you elaborate a bit on the reasoning for preferring optionals here?

The reason I have chosen arrays is because by my understanding the semantic version string encodes arrays of identifiers at the pre-release and build metadata positions.

[d-ronnqvist]

To clarify: I meant that I feel that it would be more correct to model prerelease information and build metadata as optional array of strings ([String]?). That way, if the version didn't have any prerelease information it would be nil but if it had more than one piece of prerelease information than those would be values in the array.

That change would mean that Prerelease doesn't need to support initializing with a nil value and places that currently do isEmpty checks get replaced with nil checks.

[WowbaggersLiquidLunch]

Thanks for the clarification.

That change would mean that Prerelease doesn't need to support initializing with a nil value

This combined with the other comment, do you mean to avoid taking an optional argument by initializing like this:

prereleaseStorage = prereleaseSubstring.map { Prerelease(String($0)) }

If this is the case, then I think it makes more sense to me now. Is there any advantage for doing it this way (other than for stylistic preferences)? So far it feels to me like an additional layer of expressing emptiness that an empty array is already capable of.

[d-ronnqvist]

It is essentially a stylistic preference.

The only difference would be at the call site where an optional value needs to be handled explicitly.

[d-ronnqvist]

If the prerelease information on semantic was optional, then this could accept a non-optional String argument.

[WowbaggersLiquidLunch]

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 a String? by SymbolKit. The only way to make it non-optional imo is moving the optional checking outside to right before the call site.

[d-ronnqvist]

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.

[d-ronnqvist]

I feel like this test would be simpler if it hardcoded too large string values and used comments to indicate that what values are larger than the max value.

[WowbaggersLiquidLunch]

I don't think it's a good idea to limit the capability of SemanticVersion just so it's easier to test.

[d-ronnqvist]

It would not limit the capability of SemanticVersion to use hardcoded strings in the test.

For example, knowing that UInt.max is 18446744073709551615 (2^64-1) on a 64-bit system, it's possible to replace String(sum("\(UInt.max)", "1") with "18446744073709551616" // UInt.max + 1.

That way someone who reads the tests doesn't need to know what the sum function does and doesn't have wonder if the sum function is API.

If looks like the assertions in testOversizedNumericValues only use a few values so perhaps the cleanest solution would be to define local variables for those at the start of the function

let largestValidNumber = "18446744073709551615" // UInt.max
let tooLargeNumber     = "18446744073709551616" // UInt.max + 1

[WowbaggersLiquidLunch]

Ah I see. Sorry, I misunderstood your previous comment as adding hardcoded limits to the implementation. Thanks for the clarification!

[d-ronnqvist]

Minor: These test assertions could be simplified if they asserted that the raised error had the expected error message.

[WowbaggersLiquidLunch]

True, but I think it's better if we test that the error messages are as expected as well. Because minor mistakes such as typos do happen, and a bit of more test can help catch these errors.

[d-ronnqvist]

I think maybe we're saying the same thing here. I'm saying that what really matters to test with the errors are their error messages.

These tests do verify the error messages, which is what's important. My minor feedback was just that there are 5 very similar test helpers to assert the error messages of the 5 different errors.

[WowbaggersLiquidLunch]

there are 5 very similar test helpers to assert the error messages of the 5 different errors

This is a very good point. I'll try to combine them.

[d-ronnqvist]

I find these assertions harder to read because I need to skim the line to find the precedes and equals argument labels to know which assertion it is.

[WowbaggersLiquidLunch]

I think you're right. Originally I stayed away from naming them like assertEqual on purpose in order to distinguish them more from XCTest's assertion functions. Do you have suggestions on better naming?

[d-ronnqvist]

These assertions aren't necessary. This is verifying language behavior of types that conform to Comparable.

[WowbaggersLiquidLunch]

I think they're actually necessary. Although Comparable requires that all conformances satisfy certain binary relations, it has no vehicle for enforcement. So, it's completely reliant on the conformances themselves being tested on satisfaction of these relations. In fact, as shown in SR-14665, in cases where comparison is not member-wise, the synthesized implementations can break the relations by themselves.

[d-ronnqvist]

I see. That would be helpful to information to add in a comment in these test helpers so that other developers who read the code know why it's making all these seemingly redundant assertions.

[WowbaggersLiquidLunch]

I added a bit of hints in 98a49de, but I think I can elaborate more in there.

[d-ronnqvist]

That said, I'm torn on the Comparable conformance (and to a smaller extent the Equatable conformance). I'm not against it but I'm not for it either.

I definitely see the benefits of using these protocols when the developer's understanding of the behavior matches the actual behavior but if developer's expect two versions with different build metadata to not be equal or expect different builds in the same pre-release to be sorted by build number then that incorrect assumption could result in other bugs.

It becomes a tradeoff between making it easy to do the right thing and making it hard to do the wrong thing. Some factors to consider would be how likely the "wrong thing" is, how bad its impact is, and how hard it's to debug.

It may just be my history with writing Objective-C code but I don't feel like function such as
func precedes(_ other: SemanticVersion) -> Bool or func isEarlierRelease(comparedTo other: SemanticVersion) -> Bool are too bad.

I wonder what would be a good terminology to use for two versions with the same major, minor, patch, and pre-release components but different build metadata. For example 1.0.0-alpha+001 and 1.0.0-alpha+002.

Would it be correct to say that they are the same "release" or would that term not include the pre-release component? If the pre-release component is considered part of the "release" then that terminology could be used in function names such as

func isSameRelease(as other: SemanticVersion) -> Bool { ... }
func isEarlierRelease(comparedTo other: SemanticVersion) -> Bool { ... }

[WowbaggersLiquidLunch]

HI @d-ronnqvist, thanks for the review, and sorry for the time it took before I responded! I adopted some of your suggestions and left comments/reasoning on those that I didn't adopt. Could you give it another look?

[WowbaggersLiquidLunch]

@swift-ci please test