gen_range and Uniform doesn't work great for types that don't implement Copy

[sicking]

gen_range and Uniform is intentionally written to be extensible to other types defined by users of rand.

However those APIs require that ownership of high and low is transferred to the called API. This works fine for types that are Copy, however can be a problem for types that aren't.

For example, an implementation of Uniform<BigUint> would require that the types are transferred, or explicitly cloned at the call site. This is probably ok when calling the Uniform ctor, but less good when calling gen_range in a loop with changing endpoints.

Ideal would be if you could call gen_range passing &BigUint as type for high and low, and get back a BigUint, as demonstrated by the RandBigInt trait.

I can see several ways of addressing this.

Option 1: Do nothing

We can decide that things are fine as-is. Types that really want to avoid transferring ownership to gen_range can create their own APIs, like RandBigInt does.

Option 2: Use Borrow

We can change Rng::gen_range and Uniform::new, UniformSampler::new, etc to take a Borrow<Self::X> rather than a Self::X. This adds a bit of complexity in all implementations of UniformSampler, but seems otherwise pretty narrowly targetted.

I think it also does add complexity to APIs which wrap Uniform, like gen_range and choose_weighted. Mainly in the form of more complex where clauses in the function signatures.

Option 3: Add a UniformSampler::Output associated type

This would enable implementing Uniform<&BigUint>, but set the associated Output type to be BigUint. This was the solution I used in my experiment API.

This provides a lot of flexibility, but also gets pretty complicated pretty fast. APIs like choose_weighted has to deal with two types and it gets messy with regards to which type which argument should use. For example the total_weight type ended up having to be the output type, which kind'a defeated the purpose since that means passing the total weight as a BigUint rather than a &BigUint.

---

Ultimately I think Option 2 is the way to go. It definitely feels like a bummer if the Uniform extension hook doesn't even work BigUint, which seems like one of the most obvious candidates for using this hook.

[dhardy]

Can you make a rough/partial implementation of option 2? It sounds at least worth taking a closer look at that option.

[sicking]

I put a partial implementation here.

I had to remove the default implementation of sample_single only because this is a partial implementation. Once we use Borrow in new and new_inclusive the default implementation can be put back.

It's a fair amount of boiler plate in each implementation of UniformSampler as well as in every API which wants to forward to Uniform (like gen_range and choose_weighted).

Using impl trait in argument position would work great and reduce a bunch of the boiler plate. At least in UniformSampler implementations and gen_range, but not something we can do until we upgrade compiler requirements.

I didn't see any performance differences here, which is expected. (In fact I saw a slight performance improvement, but I'm pretty sure that's just noise).

[sicking]

So, does this look like something we want to move forward with?

[dhardy]

Okay, I had a look at your implementation. Looks like it increases flexibility with only mild increases to code complexity and compile time cost, so I don't see any good reason we shouldn't do this, although I don't like increasing complexity without good motivation.

@pitdicker what do you think?

[pitdicker]

The changes look very reasonable, and if this increases the usefulness of Uniform, I am all for it. Just unfortunate we have to change it so soon after 0.5.

[dhardy]

Okay, then I think we can go ahead with this.

There's no point lamenting having passed up the 0.5 release. Probably this could even be included in 0.5.2 if we wish, but I hope 0.6 has a significantly shorter development cycle than 0.5 😄

[sicking]

Yeah, sorry for filing this just after 0.5 is out the door. Is there any place I can look at to see what the release schedule/plans look like? If so I can definitely try to have all the API changes I wanted to look in to filed in time for 0.6.

[dhardy]

There isn't any release schedule. I made a tracker issue for the 0.5 release but haven't so far compiled a list of issues for 0.6 — though it'd be nice to get some work done on distributions/high-precision sampling code and either migrate PRNGs out of the main Rand lib or at least add some more small PRNGs (#431).