Better TextStyle construct function parameters using Borrow<T>

[IDEDARY]

Objective

Make using parameter structs less annoying.

If you are making a function that takes in a struct with defining info like TextStyle, you will probably think that taking a borrow is better than ownership. But that depends on the use case. Sometimes the user will create the struct beforehand and will pass out references to it. Or sometimes he initiates the struct directly in the argument with something like ::new().

fn foo (style: &TextStyle) {
   style.do_something()
}

foo(&style);
foo(TextStyle::new()); // Doesnt compile, forced to add "&"

In the latter case, he is forced to write "&TextStyle::new()" instead of just "TextStyle::new()", which is bothersome. But the other case is taking an ownership, which if he used it like the first example, he would need to do .clone() which is even worse.

But what if there is a way to do both at once?

fn foo (style: impl Borrow<TextStyle>) {
   style.borrow().do_something()
}

foo(&style);
foo(TextStyle::new()); // Works just fine

It allows us to leave out the "&" where it is not needed. You might think it is an unnecessary change, but I think it's quite nice UX improvement without any sacrifice (Maybe it's less clear what impl Borrow<TextStyle> is but that's about it)

I have been using this in Bevy-Lunex for a while and it's quite pleasant.

[alice-i-cecile]

Hmm, I think I can see the appeal, but I'm a little nervous to merge this without a motivating in-engine use case or clearly-improved example. Is there a place in bevy_ui currently where this pattern would be nice?

[tim-blackbird]

You can use Borrow<T> instead which is already implemented for all T.

fn foo (style: impl Borrow<TextStyle>) {
   style.borrow().do_something()
}

foo(&TextStyle::new());
foo(TextStyle::new());

[IDEDARY]

You can use Borrow<T> instead which is already implemented for all T.

fn foo (style: impl Borrow<TextStyle>) {
   style.borrow().do_something()
}

foo(&TextStyle::new());
foo(TextStyle::new());

Haa, this makes this PR obsolete. I removed AsRef and AsMut and changed function parameters in text.rs to Borrow<>. And I updated the text2d.rs example to not use .clone() after each style usage. So now it's just an update to text example and 2 functions.

[alice-i-cecile]

I like this better! I'm on board now, but you need to update the PR description before this is mergeable.

[djeedai]

I've read about Borrow in the docs, and looking at that PR description, unless I'm mistaken I think it's misusing it, so I'm against this change.

Borrow is designed to borrow a type as another type, like borrow a String as a str. It's not designed to make value and reference be accepted for the same type.

What this change does is obfuscating the semantic of the function (we don't know if it wants a reference or a value), and doing so via a trait which will probably incur a virtual call. That's a lot to avoid a single & character.

[tim-blackbird]

Borrow is designed to borrow a type as another type, like borrow a String as a str. It's not designed to make value and reference be accepted for the same type.

If this was the case then std would not have a blanket impl of Borrow<T> for &T.

Quoting these docs:

Unlike AsRef, Borrow has a blanket impl for any T, and can be used to accept either a reference or a value.

It seems very much like the intended use-case.

doing so via a trait which will probably incur a virtual call.

There's definitely no runtime cost to using traits unless you're explicitly doing dynamic stuff.

[rparrett]

Looks like this was probably made irrelevant by #15591 and/or a related PR from the 0.15 cycle.