Skip to content
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

[DNM] Comply SymbolGraph.SemanticVersion to SemVer 2.0.0 #36

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

WowbaggersLiquidLunch
Copy link
Contributor

@WowbaggersLiquidLunch WowbaggersLiquidLunch commented May 26, 2022

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

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.
@WowbaggersLiquidLunch
Copy link
Contributor Author

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
Copy link
Contributor Author

@swift-ci please test

Copy link
Contributor

@d-ronnqvist d-ronnqvist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Package.swift Outdated Show resolved Hide resolved
/// The patch version.
public let patch: UInt
/// Dot-separated pre-release identifiers.
public var prereleaseIdentifiers: [String] { prerelease.identifiers.map(\.description) }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is essentially a stylistic preference.

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

/// - 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 {
Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Contributor

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.

}

func testOversizedNumericValues() {
func sum(_ summand1: String, _ summand2: String) -> Substring {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// TODO: Add tests for the precedence of error-throwing.

func assertThrowingEmptyIdentifierError(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@testable import SymbolKit

final class PrecedenceTests: XCTestCase {
func assert(_ version1: SymbolGraph.SemanticVersion, precedes version2: SymbolGraph.SemanticVersion) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

final class PrecedenceTests: XCTestCase {
func assert(_ version1: SymbolGraph.SemanticVersion, precedes version2: SymbolGraph.SemanticVersion) {
XCTAssertLessThan(version1, version2)
XCTAssertLessThanOrEqual(version1, version2)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 and others added 2 commits May 27, 2022 10:59
It appears that whatever caused the type checking problems before has already been fixed.
…litting the prerelease into identifiers

Co-authored-by: David Rönnqvist <[email protected]>
WowbaggersLiquidLunch and others added 4 commits May 27, 2022 17:42
…ifier` when validating build metadata

Co-authored-by: David Rönnqvist <[email protected]>
It’s used for more than validating prerelease.
This initializer should be much more ergonomic to use in the common cases, and it's safe because it's guaranteed not to encounter any error.
Reference to [SR-14665](swiftlang/swift#57016) links to more explanation on why the manual `==` implemenatation is necessary.
@WowbaggersLiquidLunch
Copy link
Contributor Author

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
Copy link
Contributor Author

@swift-ci please test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants