Uuid 2.0 RFC

[rrichardson]

RFC proposal

Rendered: https://github.com/rrichardson/uuid/blob/rfc-app-2/docs/rfcs/uuidgen-refactor.md

Description

This is the core of my proposal for a refactor of the Uuid crate. Fundamentally, it doesn't change how the existing APIs are used a whole lot, it introduces a TimeSource and changes the ClockSequence to SequenceSource (where a sequence can either be monotonic or random).

The biggest changes are:

1. Removal of features for versions. Features for 3rd party functionality (rng, std, etc) remain.
2. The addition of (and, IMO a focus on) the UuidGen and corresponding Builder struct.

Related Issue(s)

#612

[KodrAus]

Thanks for writing this @rrichardson! I'll spend some proper time reading through the RFC, but wanted to set the expectation early that since we committed to a stable uuid API that other libraries could depend on publicly we'd need a really compelling reason to make any breaking changes. That reason might be within the text already.

[joshtriplett]

Rather than making this part of SequenceSource, what about requiring that TimeSource::Output implements Eq, and then deciding inside the UUID library whether to check for duplicates or not?

I'm also wondering to what extent it makes sense for TimeSource::Output to be parameterized, without having some notion of precision with which to turn it into the component needed for a UUID? Or is the expectation that the library will nonetheless rely on specific output types rather than being generic?

[rrichardson]

I definitely like the idea of removing any awareness of TimeSource from SequenceSource. However, whether or not the SequenceSource checks the time for duplicates isn't really a version-specific thing, it's a preference of the implementation. If we could pass in some notion of "there was a conflict" into SequenceSource::next() that would be fine by me. Maybe an Option<bool>

Regarding Output precision: My assumption was that the precision required would be version-specific, so the Output would always be parameterized by each UuidV# Implementation. The only exception to this, though, would be UuidV8, which violates this rule.

TBH, this GenBuilder API was inspired by the idea that one could use a builder to make a highly tuned UuidV8 implementation by choosing how many bits they wanted for seconds, frac-seconds, random and sequence. However, there might be better/safer ways to accommodate that level of flexibility in the API design. (I was going to add the precision/size check at runtime in the Builder construction)

[joshtriplett]

@rrichardson That does seem like perhaps more flexibility than necessary.

And yeah, passing in "there was a conflict" seems like an improvement, at least.

[joshtriplett]

👍 for the proposal to have feature flags only for dependencies. As someone with crates that indirectly depend on uuid, I would appreciate being able to prune dependencies. (Please consider having all of those feature flags not part of default.) But eliminating APIs without eliminating dependencies doesn't seem sufficiently high-value to be worth the maintenance costs.

[rrichardson]

Thanks for writing this @rrichardson! I'll spend some proper time reading through the RFC, but wanted to set the expectation early that since we committed to a stable uuid API that other libraries could depend on publicly we'd need a really compelling reason to make any breaking changes. That reason might be within the text already.

Much of this proposal could be tacked on without changing the existing API.
However, there are a couple of "bugs" in the existing API that have become obvious when implementing v6-8. All of these issues I bring up have workarounds, but at the expense of making the API more convoluted.

Firstly, one can't create a Timestamp without creating an impl ClockSequence. IMO, this is an unnecessary conflation of APIs. It specifically makes no sense for Uuidv7, which doesn't leverage any clock sequence, it only uses unix-time + random.

Secondly, the Traits and timestamps assume too much. They are fixed at u64, u32, and u16. This, again, makes certain version implementations a bit awkward and they don't compose well. We should be able to use a SequenceGenerator to generate 128 bit random numbers to serve the v7 and v8 use cases.

Thirdly, things like Uuid::get_timestamp() and get_version() do a version switch or throw a runtime error. This pattern, IMO, is screaming that it should be part of a trait, and simply be version-specific impls, so there is no way to get it wrong.

Fourthly, (and less important) the Uuid struct makes it too easy to use accidentally use Uuid versions interchangeably, and this again, is enforced with a runtime check, when it could easily be enforced by the compiler.

Lastly, I think we need to refactor how features are done, because, IMO, it is a bit of a mess. If we do this, we do need to make sure it's done in a way that is backwards compatible, or at least define an evolutionary path.

[rrichardson]

Since this is a large API change, even for a v2.0. What are your thoughts on putting the new implementation under a new module (called next or whatever. This way people could evolve towards the 2.0 approach.. Then the next step would be to make it part of the main implementation, and put the v1 API under the old module.
We could build a compatibility layer with the old approach.

[kinggoesgaming]

Since this is a large API change, even for a v2.0. What are your thoughts on putting the new implementation under a new module (called next or whatever. This way people could evolve towards the 2.0 approach.. Then the next step would be to make it part of the main implementation, and put the v1 API under the old module.

We could build a compatibility layer with the old approach.

I support this approach. Allows experimentation without affecting current users.

[KodrAus]

Thanks for writing this up @rrichardson. At first, this seems like a lot of machinery to generate UUIDs, and I'm not sure that splitting UUIDs into separate types by version is necessarily the best user experience. It seems like the main problem we've got is that the responsibilities of our Builder and Uuid types are effectively reversed. On Uuid, we have high-level methods like now_v7() that depend on crate features being enabled, and on Builder we have low-level methods like from_unix_timestamp_millis that don't depend on any crate features or non-libcore APIs. If the roles were reversed and Uuid was just low-level construction and formatting it would make considering different strategies for constructing them easier. I somewhat regret that I didn't see the opportunity to pull APIs off Uuid and onto the Builder when first introducing it, because it would also make it possible to release major versions of the library that all re-exported the same Uuid.

I'd welcome any experimentation to build a better API as an external library, but I think the bar for heavy refactoring at this point is high. The end-user pay-off really needs to justify shifting things around, especially when the basic APIs have remained the same for many years.

[KodrAus]

One major version I would be keen to do now was one that simply re-exported the entire current API, but made the v1 and timestamp modules private.