Correctly validate characters in semantic version identifiers #254
Conversation
Semantic Versioning 2.0.0 allows only _ASCII_ alpha-numeric characters and "-" in identifiers.
WowbaggersLiquidLunch
requested review from
abertelrud,
aciidgh,
friedbunny,
neonichu and
tomerd
as code owners
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
|
@elsh please help review this one |
tomerd
requested review from
elsh
and removed request for
friedbunny and
aciidgh
| $0.allSatisfy { $0.isASCII && ($0.isLetter || $0.isNumber || $0 == "-") } | ||
| }, | ||
| #"Build metadata identifiers can contain only ASCII alpha-numeric characters and "-"."# | ||
| ) |
There was a problem hiding this comment.
I am concerned about the use of preconditions here for what could be a runtime issue. should this be throwing instead?
There was a problem hiding this comment.
This is a good point/suggestion. Making it throwing (or failable) also makes it possible to write tests against it.
Also, I found a few more bugs that I'm trying to fix before the 5.6 branch is cut: empty identifiers, leading zeros in numeric identifiers, and negative identifiers should not be allowed according to semver 2.0.0. With so many things to validate, it makes sense to throw errors for them instead of more lines of preconditions.
One slight problem though is that the call sites of this initializer in SwiftPM (and possibly llbuild and swift-driver too) need to be updated when this initializer becomes throwing or failable, so there might need some deprecation dances.
|
Changed this PR to a draft, because I'm going to do a new PR using throwing initializer as @tomerd suggested. I'm leaving this draft open until the new PR is opened to gather additional reviews/suggestions. |
Semantic Versioning 2.0.0 allows only ASCII alpha-numeric characters and "-" in identifiers.
This change makes version initialization more restrictive, so it might be source breaking.
Same as swiftlang/swift-package-manager#3839, but for TSC's
Version.