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

Fix ambiguity issues in use of Borrow<T> #509

Merged
merged 1 commit into from
Jun 15, 2018

Conversation

sicking
Copy link
Contributor

@sicking sicking commented Jun 15, 2018

There's a big problem with #506 which I missed during both authoring and testing. All of the following work fine:

rng.gen_range(5, 10);
rng.gen_range(&5, 10);
rng.gen_range(5, &10);

However the following does not

rng.gen_range(&5, &10);

It fails because &usize implement both Borrow<usize> and Borrow<&usize>. And so there are two solution to gen_range's bound where B1: Borrow<T>, B2: Borrow<T> resulting in a disambiguation error. I.e. rustc doesn't know if we want T=usize or T=&usize.

Even though gen_range also says where T: SampleUniform, which is only satisfied for T=usize, that apparently does not help.

The problem could be solved by writing gen_range::<usize, _, _>(&5, &10), but that's obviously not something we want to require.

This PR instead implements a SampleBorrow trait. Which is essentially an exact copy of Borrow, except that it's only implemented where T=SampleUniform. So &usize implements SampleBorrow<usize>, but not SampleBorrow<&usize>, resolving the ambiguity.

This adds no complexity at callsites. Implementations of SampleUniform will have to write SampleBorrow instead of Borrow, but otherwise will write exactly the same code.

It's a bummer that we can't use the standard Borrow, but I don't know of any way to solve that.

And SampleBorrow does give us more flexibility. For example we could enable passing &&5, or &Box::new(5) if we wanted (which can sometime occur with iterators). But that'd be a separate PR if so.

@dhardy
Copy link
Member

dhardy commented Jun 15, 2018

Thanks for sorting this.

Could we not just name this Borrow ourselves? It's not in the prelude and I don't think users need to import it for this to function so there shouldn't be name conflicts, except possibly people implementing SampleUniform etc.

Edit: sorry, didn't see how it worked. I guess the name SampleBorrow does make sense then.

@dhardy dhardy merged commit 757f8ee into rust-random:master Jun 15, 2018
@dhardy dhardy mentioned this pull request Jun 20, 2018
28 tasks
@sicking sicking deleted the borrow_fix branch June 21, 2018 08:23
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