-
Notifications
You must be signed in to change notification settings - Fork 91
Interface version canonicalization #536
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
base: main
Are you sure you want to change the base?
Conversation
7b6bd7d
to
2f8eda8
Compare
For the binary encoding the most straightforward option from a quick review would seem to be adding variants of
I suppose if we wanted to optimize the binary a bit this extra field could contain just the part of the original version that got lopped off by canonicalization. On this field width:
https://semver.org/#does-semver-have-a-size-limit-on-the-version-string
🤷 |
@lann Thanks for starting this! For the binary encoding question: yes, taking over the
Is there a simplicity argument to be made that requiring the concatenation of the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! A few drive-by comments:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(oops, meant to "comment" not approve before it's even ready to review 🙃 )
For the binary encoding, here's another possible encoding:
maybe with affordances for rc/etc unsure. The basic idea though is that the actual import name would always be
For this I'd recommend using |
@alexcrichton Good idea; that cleanly answers some of the questions above. My only light concern is that tools might just treat the |
I agree yeah there's risk since the name in the binary format is "so simple", but yeah that's also where I'd hope that tests could weed things out. It'd be pretty simple in parser libraries I'd imagine to avoid exposing the full name as the import name if the discriminant was present. My thinking though was that the name always has a full and valid semver, as defined by semver itself. That way the discriminant says what the "real" import name is (e.g. chopping off other stuff) for linking/semantic purposes. Although I may be misunderstanding what you're thinking about how to drop the discriminant? |
I think @lukewagner is suggesting that the differences between 1/2/3 can be derived from the string itself. The algo would be something like:
|
Ah I see! So something like (as a transition to the future):
where in the future we'd drop 0x00 entirely (and possibly rename 0x01 to 0x00). The |
I think of the "semver-aware" options I prefer @lukewagner's 1 (extra) discriminant option; if you are parsing semver anyway then the logic is only marginally more complex than the 3 discriminant option. I'm more ambivalent on whether the parser should be semver-aware. I like the conceptual simplicity of "the name is the name" but this is a binary encoding and if we're going to require validation of semver then we're probably already committing to most of that code complexity anyway. |
We discussed this in a meeting today and decided to simplify a bit:
|
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
@@ -399,7 +402,10 @@ Notes: | |||
`(result (own $R))`, where `$R` is the resource labeled `r`. | |||
* Validation of `[method]` names requires the first parameter of the function | |||
to be `(param "self" (borrow $R))`, where `$R` is the resource labeled `r`. | |||
* `<valid semver>` is as defined by [https://semver.org](https://semver.org/) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<valid semver>
wasn't referenced in this file.
28b07b3
to
531cb92
Compare
namespace ::= <words> ':' | ||
words ::= <word> | ||
| <words> '-' <word> | ||
projection ::= '/' <label> | ||
version ::= '@' <valid semver> | ||
# FIXME: surrounding alignment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO after final review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks! Just a few small comments:
canonversion ::= [1-9] [0-9]* | ||
| '0.' [1-9] [0-9]* | ||
| '0.0.' [1-9] [0-9]* | ||
semversuffix ::= [0-9A-Za-z.+-]* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This just checks for valid semver characters. The "real" validation is covered in prose below, i.e. <canonversion><semversuffix>
must match valid semver
.
I had a version of this as [.+-][0-9A-Za-z.+-]+
which was less ambiguous but also implied that versionsuffix
and the binary 0x01
variant couldn't be used with an empty suffix like you'd get with full version 0.0.1
. Given that we're saying we'll remove non-canonical interface versions I think allowing the empty suffix will ultimately be simpler.
See #534