[Merged by Bors] - bevy_derive: Add derives for `Deref` and `DerefMut` #4328

MrGVSV · 2022-03-25T02:42:47Z

Objective

A common pattern in Rust is the newtype. This is an especially useful pattern in Bevy as it allows us to give common/foreign types different semantics (such as allowing it to implement Component or FromWorld) or to simply treat them as a "new type" (clever). For example, it allows us to wrap a common Vec<String> and do things like:

#[derive(Component)]
struct Items(Vec<String>);

fn give_sword(query: Query<&mut Items>) { 
  query.single_mut().0.push(String::from("Flaming Poisoning Raging Sword of Doom"));
}

We could then define another struct that wraps Vec<String> without anything clashing in the query.

However, one of the worst parts of this pattern is the ugly .0 we have to write in order to access the type we actually care about. This is why people often implement Deref and DerefMut in order to get around this.

Since it's such a common pattern, especially for Bevy, it makes sense to add a derive macro to automatically add those implementations.

Solution

Added a derive macro for Deref and another for DerefMut (both exported into the prelude). This works on all structs (including tuple structs) as long as they only contain a single field:

#[derive(Deref)]
struct Foo(String);

#[derive(Deref, DerefMut)]
struct Bar {
  name: String,
}

This allows us to then remove that pesky .0:

#[derive(Component, Deref, DerefMut)]
struct Items(Vec<String>);

fn give_sword(query: Query<&mut Items>) { 
  query.single_mut().push(String::from("Flaming Poisoning Raging Sword of Doom"));
}

Alternatives

There are other alternatives to this such as by using the derive_more crate. However, it doesn't seem like we need an entire crate just yet since we only need Deref and DerefMut (for now).

Considerations

One thing to consider is that the Rust std library recommends not using Deref and DerefMut for things like this: "Deref should only be implemented for smart pointers to avoid confusion" (reference). Personally, I believe it makes sense to use it in the way described above, but others may disagree.

Additional Context

Discord: https://discord.com/channels/691052431525675048/692572690833473578/956648422163746827 (controversiality discussed here)

Changelog

Add Deref derive macro (exported to prelude)
Add DerefMut derive macro (exported to prelude)
Updated most newtypes in examples to use one or both derives

alice-i-cecile · 2022-03-25T02:45:56Z

I have a fresh case that needs this in the breakout example; can you add this to the Velocity struct there?

alice-i-cecile · 2022-03-25T03:05:20Z

Wow, that change log is a great example of the clarity benefits. I love it.

alice-i-cecile · 2022-03-25T03:15:07Z

Actually, one more place that I think we should change this immediately @MrGVSV: the Timer examples. This one tripped me up really badly when I first started using Bevy, and it's a very common idiom.

MrGVSV · 2022-03-25T03:25:15Z

Actually, one more place that I think we should change this immediately @MrGVSV: the Timer examples. This one tripped me up really badly when I first started using Bevy, and it's a very common idiom.

Should I do this for all Timer newtypes in all examples? Or just the timers.rs example? (Or for all newtypes in all examples? 😅)

alice-i-cecile · 2022-03-25T03:32:38Z

Hmm. I think that for consistency it should probably be all newtypes in all examples: .0 is awful, and consistency is good.

I don't think it'll make the PR too bad to review: the actual meat of the changes are still nicely concentrated (and the technical complexity is very low).

crates/bevy_derive/src/derefs.rs

MrGVSV · 2022-03-25T03:55:17Z

I think that for consistency it should probably be all newtypes in all examples

I left some newtypes as is, especially ones that just access the inner type itself (println!("{}", foo.0)) and ones that would require a double-dereference (**foo.clone()).

alice-i-cecile · 2022-03-25T04:27:45Z

I left some newtypes as is, especially ones that just access the inner type itself (println!("{}", foo.0)) and ones that would require a double-dereference (**foo.clone()).

Sounds good to me :) We can revise more as it comes up, but this goes a long way.

alice-i-cecile

LGTM now, and I'm in favor of this change. rustc can provide a better solution if they don't like people using Deref.

Wording change on the compiler errors is just a suggestion; feel free to take it or leave it.

Co-authored-by: Alice Cecile <[email protected]>

cart

This looks good to me! Nice work!

I've gone on the record a number of times already saying that I dont care that std says Deref is for smart pointers. But for posterity: until std (or rust) gives us a better tool, Deref is the best tool we have for the job. some_public_api.0 in our public apis (or our users' public apis) is way worse than breaking some arbitrary std suggestion. Deref as implemented by std is a workable solution and "the cat is already out of the bag".

cart · 2022-03-29T02:09:49Z

bors r+

# Objective A common pattern in Rust is the [newtype](https://doc.rust-lang.org/rust-by-example/generics/new_types.html). This is an especially useful pattern in Bevy as it allows us to give common/foreign types different semantics (such as allowing it to implement `Component` or `FromWorld`) or to simply treat them as a "new type" (clever). For example, it allows us to wrap a common `Vec<String>` and do things like: ```rust #[derive(Component)] struct Items(Vec<String>); fn give_sword(query: Query<&mut Items>) { query.single_mut().0.push(String::from("Flaming Poisoning Raging Sword of Doom")); } ``` > We could then define another struct that wraps `Vec<String>` without anything clashing in the query. However, one of the worst parts of this pattern is the ugly `.0` we have to write in order to access the type we actually care about. This is why people often implement `Deref` and `DerefMut` in order to get around this. Since it's such a common pattern, especially for Bevy, it makes sense to add a derive macro to automatically add those implementations. ## Solution Added a derive macro for `Deref` and another for `DerefMut` (both exported into the prelude). This works on all structs (including tuple structs) as long as they only contain a single field: ```rust #[derive(Deref)] struct Foo(String); #[derive(Deref, DerefMut)] struct Bar { name: String, } ``` This allows us to then remove that pesky `.0`: ```rust #[derive(Component, Deref, DerefMut)] struct Items(Vec<String>); fn give_sword(query: Query<&mut Items>) { query.single_mut().push(String::from("Flaming Poisoning Raging Sword of Doom")); } ``` ### Alternatives There are other alternatives to this such as by using the [`derive_more`](https://crates.io/crates/derive_more) crate. However, it doesn't seem like we need an entire crate just yet since we only need `Deref` and `DerefMut` (for now). ### Considerations One thing to consider is that the Rust std library recommends _not_ using `Deref` and `DerefMut` for things like this: "`Deref` should only be implemented for smart pointers to avoid confusion" ([reference](https://doc.rust-lang.org/std/ops/trait.Deref.html)). Personally, I believe it makes sense to use it in the way described above, but others may disagree. ### Additional Context Discord: https://discord.com/channels/691052431525675048/692572690833473578/956648422163746827 (controversiality discussed [here](https://discord.com/channels/691052431525675048/692572690833473578/956711911481835630)) --- ## Changelog - Add `Deref` derive macro (exported to prelude) - Add `DerefMut` derive macro (exported to prelude) - Updated most newtypes in examples to use one or both derives Co-authored-by: MrGVSV <[email protected]>

bors · 2022-03-29T02:30:48Z

Pull request successfully merged into main.

Build succeeded:

# Objective A common pattern in Rust is the [newtype](https://doc.rust-lang.org/rust-by-example/generics/new_types.html). This is an especially useful pattern in Bevy as it allows us to give common/foreign types different semantics (such as allowing it to implement `Component` or `FromWorld`) or to simply treat them as a "new type" (clever). For example, it allows us to wrap a common `Vec<String>` and do things like: ```rust #[derive(Component)] struct Items(Vec<String>); fn give_sword(query: Query<&mut Items>) { query.single_mut().0.push(String::from("Flaming Poisoning Raging Sword of Doom")); } ``` > We could then define another struct that wraps `Vec<String>` without anything clashing in the query. However, one of the worst parts of this pattern is the ugly `.0` we have to write in order to access the type we actually care about. This is why people often implement `Deref` and `DerefMut` in order to get around this. Since it's such a common pattern, especially for Bevy, it makes sense to add a derive macro to automatically add those implementations. ## Solution Added a derive macro for `Deref` and another for `DerefMut` (both exported into the prelude). This works on all structs (including tuple structs) as long as they only contain a single field: ```rust #[derive(Deref)] struct Foo(String); #[derive(Deref, DerefMut)] struct Bar { name: String, } ``` This allows us to then remove that pesky `.0`: ```rust #[derive(Component, Deref, DerefMut)] struct Items(Vec<String>); fn give_sword(query: Query<&mut Items>) { query.single_mut().push(String::from("Flaming Poisoning Raging Sword of Doom")); } ``` ### Alternatives There are other alternatives to this such as by using the [`derive_more`](https://crates.io/crates/derive_more) crate. However, it doesn't seem like we need an entire crate just yet since we only need `Deref` and `DerefMut` (for now). ### Considerations One thing to consider is that the Rust std library recommends _not_ using `Deref` and `DerefMut` for things like this: "`Deref` should only be implemented for smart pointers to avoid confusion" ([reference](https://doc.rust-lang.org/std/ops/trait.Deref.html)). Personally, I believe it makes sense to use it in the way described above, but others may disagree. ### Additional Context Discord: https://discord.com/channels/691052431525675048/692572690833473578/956648422163746827 (controversiality discussed [here](https://discord.com/channels/691052431525675048/692572690833473578/956711911481835630)) --- ## Changelog - Add `Deref` derive macro (exported to prelude) - Add `DerefMut` derive macro (exported to prelude) - Updated most newtypes in examples to use one or both derives Co-authored-by: MrGVSV <[email protected]>

Add derives for Deref and DerefMut

1384047

github-actions bot added the S-Needs-Triage This issue needs to be labelled label Mar 25, 2022

Add Deref/DerefMut derives to breakout example

1b6c397

alice-i-cecile added C-Usability A targeted quality-of-life change that makes Bevy easier to use and removed S-Needs-Triage This issue needs to be labelled labels Mar 25, 2022

alice-i-cecile added the C-Examples An addition or correction to our examples label Mar 25, 2022

alice-i-cecile reviewed Mar 25, 2022

View reviewed changes

crates/bevy_derive/src/derefs.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed Mar 25, 2022

View reviewed changes

crates/bevy_derive/src/derefs.rs Outdated Show resolved Hide resolved

Add Deref/DerefMut derives to newtypes in examples

1f9537d

alice-i-cecile approved these changes Mar 25, 2022

View reviewed changes

Apply suggestions from code review

904937c

Co-authored-by: Alice Cecile <[email protected]>

cart approved these changes Mar 29, 2022

View reviewed changes

bors bot changed the title ~~bevy_derive: Add derives for Deref and DerefMut~~ [Merged by Bors] - bevy_derive: Add derives for Deref and DerefMut Mar 29, 2022

bors bot closed this Mar 29, 2022

MrGVSV deleted the derive-derefs branch March 29, 2022 02:31

alice-i-cecile mentioned this pull request Apr 4, 2022

Round out Mut<T> api using lenses + auto-unwrapping #4413

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - bevy_derive: Add derives for `Deref` and `DerefMut` #4328

[Merged by Bors] - bevy_derive: Add derives for `Deref` and `DerefMut` #4328

MrGVSV commented Mar 25, 2022 •

edited

Loading

alice-i-cecile commented Mar 25, 2022

alice-i-cecile commented Mar 25, 2022

alice-i-cecile commented Mar 25, 2022

MrGVSV commented Mar 25, 2022 •

edited

Loading

alice-i-cecile commented Mar 25, 2022 •

edited

Loading

MrGVSV commented Mar 25, 2022

alice-i-cecile commented Mar 25, 2022

alice-i-cecile left a comment

cart left a comment •

edited

Loading

cart commented Mar 29, 2022

bors bot commented Mar 29, 2022

[Merged by Bors] - bevy_derive: Add derives for Deref and DerefMut #4328

[Merged by Bors] - bevy_derive: Add derives for Deref and DerefMut #4328

Conversation

MrGVSV commented Mar 25, 2022 • edited Loading

Objective

Solution

Alternatives

Considerations

Additional Context

Changelog

alice-i-cecile commented Mar 25, 2022

alice-i-cecile commented Mar 25, 2022

alice-i-cecile commented Mar 25, 2022

MrGVSV commented Mar 25, 2022 • edited Loading

alice-i-cecile commented Mar 25, 2022 • edited Loading

MrGVSV commented Mar 25, 2022

alice-i-cecile commented Mar 25, 2022

alice-i-cecile left a comment

Choose a reason for hiding this comment

cart left a comment • edited Loading

Choose a reason for hiding this comment

cart commented Mar 29, 2022

bors bot commented Mar 29, 2022

[Merged by Bors] - bevy_derive: Add derives for `Deref` and `DerefMut` #4328

[Merged by Bors] - bevy_derive: Add derives for `Deref` and `DerefMut` #4328

MrGVSV commented Mar 25, 2022 •

edited

Loading

MrGVSV commented Mar 25, 2022 •

edited

Loading

alice-i-cecile commented Mar 25, 2022 •

edited

Loading

cart left a comment •

edited

Loading