rust-lang / libs-team

The home of the library team
Apache License 2.0
116 stars 18 forks source link

Add Borrow<u32> for NonZeroU32 #216

Closed SoniEx2 closed 1 year ago

SoniEx2 commented 1 year ago

Proposal

Problem statement

Sometimes it can be really verbose to use NonZeroU32. It would be nice if we could reduce some of that.

Motivation, use-cases

For example, with HashSet today:

let mut x: HashSet<NonZeroU32> = Default::default();
x.insert(NonZeroU32::new(...).unwrap());
NonZeroU32::new(v).and_then(|v| x.get(&v));

With this proposal:

let mut x: HashSet<NonZeroU32> = Default::default();
x.insert(NonZeroU32::new(...).unwrap());
x.get(&v); // way simpler

Solution sketches

"Just" impl Borrow?

Links and related work

N/A

What happens now?

This issue is part of the libs-api team API change proposal process. Once this issue is filed the libs-api team will review open proposals in its weekly meeting. You should receive feedback within a week or two.

pitaj commented 1 year ago

What happens if v is zero? You can't have an &NonZeroU32 that's zero.

cuviper commented 1 year ago

The Borrow goes the other way -- any NonZeroU32 can produce a &u32 to compare against a query key. If the query key is &0, then it will never be a match.

pitaj commented 1 year ago

If that's the case, am I crazy or does their example not make any sense?

Nevermind, I get it

cuviper commented 1 year ago

The method for HashSet<T> looks like:

pub fn get<Q>(&self, value: &Q) -> Option<&T>
where
    T: Borrow<Q>,
    Q: Hash + Eq + ?Sized,

In this scenario, T = NonZeroU32 and Q = u32. So get will hash the value, and for each T item that matches the hash, it will see if item.borrow() == value, comparing as &Q. But without this proposed Borrow<u32> for NonZeroU32, they can only use Q = NonZeroU32, so they have to convert their plain u32 first.

pitaj commented 1 year ago

Ah my understanding of the direction of borrow for HashMap::get was backwards. My mistake.

scottmcm commented 1 year ago

Would it be worth starting with AsRef instead, as a lighter-weight promise?

Borrow is a very strict trait, though I guess for this case it probably won't have the downsides it can have in other cases (like arrays).

SoniEx2 commented 1 year ago

honestly we would even argue for making it coercible but that's t-types :p

(as an example of coercions that don't use Deref: consider pointer whose methods can't be called on a reference, and Any: String::new().downcast_ref::<String>() doesn't compile.)

SoniEx2 commented 1 year ago

(btw we use it/its pronouns)