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.

Sign up for GitHub

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

Jump to bottom

[Merged by Bors] - Derive default for enums where possible #5158

Closed
wants to merge 2 commits into from
Closed

[Merged by Bors] - Derive default for enums where possible #5158

wants to merge 2 commits into from

Conversation

rparrett
Copy link
Contributor

Objective

Fixes #5153

Solution

Search for all enums and manually check if they have default impls that can use this new derive.

By my reckoning:

enum num
total 159
has default impl 29
default is unit variant 23

@rparrett rparrett added the C-Code-Quality A section of code that is hard to understand or change label Jun 30, 2022
hymm
hymm approved these changes Jun 30, 2022
View reviewed changes
Copy link
Contributor

@hymm hymm left a comment

Choose a reason for hiding this comment

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

Changes in the PR look good. I didn't double check if there were any enums that were missed.

@tim-blackbird
Copy link
Contributor

tim-blackbird commented Jun 30, 2022 •
edited
Loading

Found a couple that were missed:
https://github.com/rparrett/bevy/blob/c27a3cff6d593e1b1220886673018a508aaa785d/crates/bevy_reflect/bevy_reflect_derive/src/container_attributes.rs#L39
https://github.com/rparrett/bevy/blob/fb813a3a64cdafdd75f7590a461364bcdf34bf2e/crates/bevy_reflect/bevy_reflect_derive/src/field_attributes.rs#L37

james7132
james7132 approved these changes Jun 30, 2022
View reviewed changes
Copy link
Member

@james7132 james7132 left a comment

Choose a reason for hiding this comment

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

Changes look good. Less code is better code.

@james7132 james7132 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 Jun 30, 2022
@rparrett
Copy link
Contributor Author

rparrett commented Jul 1, 2022

Found a couple that were missed: https://github.com/rparrett/bevy/blob/c27a3cff6d593e1b1220886673018a508aaa785d/crates/bevy_reflect/bevy_reflect_derive/src/container_attributes.rs#L39 https://github.com/rparrett/bevy/blob/fb813a3a64cdafdd75f7590a461364bcdf34bf2e/crates/bevy_reflect/bevy_reflect_derive/src/field_attributes.rs#L37

Thanks. Looks like I missed pub(crate) enums. Will do a sweep for those.

@rparrett
Copy link
Contributor Author

rparrett commented Jul 1, 2022

Of the 10 pub(crate) enums, only those two were relevant.

@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Jul 1, 2022
@rparrett
Derive default for enums where possible (#5158)
5e17569 
# Objective

Fixes #5153

## Solution

Search for all enums and manually check if they have default impls that can use this new derive.

By my reckoning:

| enum | num |
|-|-|
| total | 159 |
| has default impl | 29 |
| default is unit variant | 23 |
@bors
Copy link
Contributor

bors bot commented Jul 1, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Derive default for enums where possible [Merged by Bors] - Derive default for enums where possible Jul 1, 2022
@bors bors bot closed this Jul 1, 2022
@adsick
Copy link
Contributor

adsick commented Jul 20, 2022

examples on main are not compiling for me because of this, what should I do?

bevy/examples/2d  main ✔                                                                    5h38m
▶ cargo run --example sprite
   Compiling gltf-derive v1.0.0
   Compiling hash32-derive v0.1.1
   Compiling serde v1.0.137
   Compiling thiserror v1.0.31
   Compiling bytemuck v1.9.1
   Compiling bevy_reflect_derive v0.8.0-dev (/Users/anymacstore/rust/bevy/crates/bevy_reflect/bevy_reflect_derive)
   Compiling bevy_derive v0.8.0-dev (/Users/anymacstore/rust/bevy/crates/bevy_derive)
   Compiling bevy_render_macros v0.8.0-dev (/Users/anymacstore/rust/bevy/crates/bevy_render/macros)
error[E0658]: deriving `Default` on enums is experimental
  --> crates/bevy_reflect/bevy_reflect_derive/src/container_attributes.rs:27:17
   |
27 | #[derive(Clone, Default)]
   |                 ^^^^^^^
   |
   = note: see issue #86985 <https://github.com/rust-lang/rust/issues/86985> for more information
   = note: this error originates in the derive macro `Default` (in Nightly builds, run with -Z macro-backtrace for more info)

@alice-i-cecile
Copy link
Member

Update your Rust version using rustup update :)

@adsick
Copy link
Contributor

adsick commented Jul 20, 2022

I've already done it:

bevy/examples/2d  main ✔                                                                   5h39m  ⍉
▶ rustup update
info: syncing channel updates for 'stable-x86_64-apple-darwin'
info: syncing channel updates for 'nightly-x86_64-apple-darwin'
info: checking for self-updates

   stable-x86_64-apple-darwin unchanged - rustc 1.62.1 (e092d0b6b 2022-07-16)
  nightly-x86_64-apple-darwin unchanged - rustc 1.64.0-nightly (9a7b7d5e5 2022-07-19)

info: cleaning up downloads & tmp directories

bevy/examples/2d  main ✔

@alice-i-cecile
Copy link
Member

That's very surprising. I would attempt cargo clean + deleting Cargo.lock + cargo update.

If that still fails, I would attempt a minimal reproduction and file a bug with the Rustlang team.

@adsick
Copy link
Contributor

adsick commented Jul 20, 2022

done all these steps and still(

@alice-i-cecile
Copy link
Member

Hmm. Perhaps you're running Bevy with an old Rust version via some project specific configuration? I would try a fresh example project, where you just define a toy enum and use this feature.

Either way, I would delete your Bevy folder and clone it again from scratch.

@adsick
Copy link
Contributor

adsick commented Jul 20, 2022

sounds OK, I'll try, thanks

@mockersf
Copy link
Member

a few commands to check your rust versions:

  • rustup show shows everything
  • rustup toolchain list shows all toolchains installed
  • rustup overrides list shows if there are overrides from rustup
  • cargo rustc -- --version shows rustc version used by cargo, but I think it needs to compile to work

@adsick
Copy link
Contributor

adsick commented Jul 20, 2022

after some experiments in a minimal example I've realized that there are two versions of Rust on my mac - one is installed in a 'classic' manner and the other via brew package manager. for some reason when I run rustup/cargo --version command it is referring to classic which is brand new, but when I try to compile something it uses that brew version, meh.

solution:

brew uninstall rust

@harudagondi harudagondi mentioned this pull request Aug 7, 2022
Closed
inodentry pushed a commit to IyesGames/bevy that referenced this pull request Aug 8, 2022
@rparrett @inodentry
Derive default for enums where possible (bevyengine#5158)
2c6c0eb 
# Objective

Fixes bevyengine#5153

## Solution

Search for all enums and manually check if they have default impls that can use this new derive.

By my reckoning:

| enum | num |
|-|-|
| total | 159 |
| has default impl | 29 |
| default is unit variant | 23 |
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
@rparrett @james7132
Derive default for enums where possible (bevyengine#5158)
5cccd1a 
# Objective

Fixes bevyengine#5153

## Solution

Search for all enums and manually check if they have default impls that can use this new derive.

By my reckoning:

| enum | num |
|-|-|
| total | 159 |
| has default impl | 29 |
| default is unit variant | 23 |
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
@rparrett @ItsDoot
Derive default for enums where possible (bevyengine#5158)
a34003e 
# Objective

Fixes bevyengine#5153

## Solution

Search for all enums and manually check if they have default impls that can use this new derive.

By my reckoning:

| enum | num |
|-|-|
| total | 159 |
| has default impl | 29 |
| default is unit variant | 23 |
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tim-blackbird tim-blackbird tim-blackbird approved these changes

@james7132 james7132 james7132 approved these changes

@hymm hymm hymm approved these changes

Assignees
No one assigned
Labels
C-Code-Quality A section of code that is hard to understand or change 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use new enum default method throughout code base
7 participants
@rparrett @tim-blackbird @alice-i-cecile @adsick @mockersf @hymm @james7132