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

bevy_derive: Add #[deref] attribute #8552

Merged
merged 5 commits into from May 16, 2023

Conversation

MrGVSV
Copy link
Member

@MrGVSV MrGVSV commented May 5, 2023

Objective

Bevy code tends to make heavy use of the newtype pattern, which is why we have a dedicated derive for Deref and DerefMut. This derive works for any struct with a single field:

#[derive(Component, Deref, DerefMut)]
struct MyNewtype(usize);

One reason for the single-field limitation is to prevent confusion and footguns related that would arise from allowing multi-field structs:

Similar structs, different derefs
#[derive(Deref, DerefMut)]
struct MyStruct {
  foo: usize, // <- Derefs usize
  bar: String,
}
#[derive(Deref, DerefMut)]
struct MyStruct {
  bar: String, // <- Derefs String
  foo: usize,
}
Why `.1`?
#[derive(Deref, DerefMut)]
struct MyStruct(Vec<usize>, Vec<f32>);

let mut foo = MyStruct(vec![123], vec![1.23]);

// Why can we skip the `.0` here?
foo.push(456);
// But not here?
foo.1.push(4.56);

However, there are certainly cases where it's useful to allow for structs with multiple fields. Such as for structs with one "real" field and one PhantomData to allow for generics:

#[derive(Deref, DerefMut)]
struct MyStruct<T>(
  // We want use this field for the `Deref`/`DerefMut` impls
  String,
  // But we need this field so that we can make this struct generic
  PhantomData<T>
);

// ERROR: Deref can only be derived for structs with a single field
// ERROR: DerefMut can only be derived for structs with a single field

Additionally, the possible confusion and footguns are mainly an issue for newer Rust/Bevy users. Those familiar with Deref and DerefMut understand what adding the derive really means and can anticipate its behavior.

Solution

Allow users to opt into multi-field Deref/DerefMut derives using a #[deref] attribute:

#[derive(Deref, DerefMut)]
struct MyStruct<T>(
  // Use this field for the `Deref`/`DerefMut` impls
  #[deref] String,
  // We can freely include any other field without a compile error
  PhantomData<T>
);

This prevents the footgun pointed out in the first issue described in the previous section, but it still leaves the possible confusion surrounding .0-vs-.#. However, the idea is that by making this behavior explicit with an attribute, users will be more aware of it and can adapt appropriately.


Changelog

  • Added #[deref] attribute to Deref and DerefMut derives

@MrGVSV MrGVSV added the C-Usability A simple quality-of-life change that makes Bevy easier to use label May 5, 2023
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this, and I like that the attribute is shared.

Once the examples are beefed up this LGTM.

If we're going to include this feature I think it makes sense to extend the macro in uncontroversial ways.

@MrGVSV
Copy link
Member Author

MrGVSV commented May 5, 2023

Updated macro logic to give a proper error for field-less structs.

Copy link
Member

@JoJoJet JoJoJet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems useful, and the implementation looks correct.

Have you considered making the #[deref] attribute unnecessary if all but one field name starts with an underscore?

@JoJoJet JoJoJet added the C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide label May 10, 2023
@JoJoJet
Copy link
Member

JoJoJet commented May 10, 2023

As far as I can tell this is a breaking change. Might be good to have a compile_fail doctest with a struct that has multiple fields and no #[deref] attribute.

@JoJoJet JoJoJet removed the C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide label May 10, 2023
@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label May 10, 2023
@alice-i-cecile
Copy link
Member

If we're adding compile fail tests I think that one that fails on multiple deref attributes would be nice too :)

@mockersf
Copy link
Member

I'm very opposed to this! At least before we merge #8573 or another PR updating syn... it was a pain to write and I would greatly appreciate not resolving conflicts 🙏

@MrGVSV
Copy link
Member Author

MrGVSV commented May 10, 2023

Have you considered making the #[deref] attribute unnecessary if all but one field name starts with an underscore?

Could be an interesting way of achieving this, but I worry it's a little too implicit and might trip up some users (though, I suppose the error message would help guide them). Doing this would also add a difference between tuple structs and standard structs which may not be ideal.

For now maybe we just keep the attribute and consider underscores in the future?


Might be good to have a compile_fail doctest with a struct that has multiple fields and no #[deref] attribute.

Good idea! Should those go in bevy_ecs_compile_fail_tests or should I make a separate crate for utility macros (i.e. bevy_derive, bevy_utils_proc_macros, etc.)?

I originally tried doing this as a test module in the same file but I think parse_quote or syn::Error was having some trouble.


I'm very opposed to this! At least before we merge #8573 or another PR updating syn... it was a pain to write and I would greatly appreciate not resolving conflicts 🙏

Makes sense, I don't mind waiting and rebasing. That gives me some time to add the compile fail tests 😄


Tasks

@MrGVSV MrGVSV added the S-Blocked This cannot more forward until something else changes label May 10, 2023
@soqb
Copy link
Contributor

soqb commented May 12, 2023

one possible way to resolve most of the .0 vs .1 issue would be to require all non-dereferenced fields be private.

@MrGVSV
Copy link
Member Author

MrGVSV commented May 12, 2023

one possible way to resolve most of the .0 vs .1 issue would be to require all non-dereferenced fields be private.

That could help in some cases, but unfortunately it doesn't really help out when the type is in the same module as the access.

And this is especially likely for new Rust/Bevy users who may just copy+paste code from an example or tutorial to start with and suddenly become confused with the field access. In such cases, I feel an explicit #[deref] in their type would help lead them on the right path.

@alice-i-cecile alice-i-cecile removed the S-Blocked This cannot more forward until something else changes label May 16, 2023
@alice-i-cecile
Copy link
Member

#8573 is in the merge queue for you :)

@MrGVSV
Copy link
Member Author

MrGVSV commented May 16, 2023

@alice-i-cecile Should be ready to go!

Thanks @mockersf for suggesting waiting on #8573, since this actually ended up needing a fix for syn2 despite there being no conflicts.

Also added two more compile tests for both derives that I forgot about:

  • invalid_attribute.fail.rs - The attribute should not take any arguments
  • single_field.pass.rs - No attribute needed for single-field structs (already handled in doc tests but figured I'd include one here as well)

@alice-i-cecile alice-i-cecile added this pull request to the merge queue May 16, 2023
Merged via the queue into bevyengine:main with commit 56686a8 May 16, 2023
23 of 24 checks passed
@MrGVSV MrGVSV deleted the deref-attribute branch May 16, 2023 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-Usability A simple quality-of-life change that makes Bevy easier to use S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants