bevy_reflect: Split up the std module

[MrGVSV]

Important

To maintainers: we should wait to merge this one in until after #18944 lands so that cherry-picking the latter for 0.16.1 is simpler.

Objective

The std module is where we implement the reflection traits for types that are exported from std (including core and alloc). Over time, this file has grown increasingly large, making it difficult to navigate and a pain point for merge conflicts.

The goal of this PR is to break up the module into smaller chunks.

Solution

The std module has been split into many submodules:

• alloc
• bevy_platform
• core
• std

Each of these new modules is comprised of submodules that closely resemble the actual module. For example, the impls for ::alloc::vec::Vec have been moved to bevy_reflect::impls::alloc::vec::Vec.

Some liberties were taken. For example, Cow<'static, Path> was kept in bevy_reflect::impls::std::path rather than bevy_reflect::impls::alloc::borrow.

You may ask: Isn't this a little overkill? Why does the one-line impl for TypeId need its own file?

And yes, it is partly overkill. But the benefit with this approach is that where an std-related type should live is mostly unambiguous. If we wanted to reflect ::core::net::Ipv4Addr, it's very clear that it should be done in bevy_reflect::impls::core::net.

We can discuss better ways of breaking this up if people have other ideas or opinions, but I think this is a pretty straightforward way of doing it.

Note to Reviewers

The code is pretty much copy-paste from the mega module to the new submodules. It's probably best to focus efforts on reviewing the general module structure, as well as maybe which impls are included where.

You can review the code contained within each impl, but I promise you the only thing I touched were the paths in the macros so they could be more hygienic :)

Testing

You can just check that everything compiles still:

cargo check -p bevy_reflect --tests

[hukasu]

The files that have only one item could be moved to the modules root, though

[MrGVSV]

The files that have only one item could be moved to the modules root, though

Possibly. However that risks growing in size too unless we remember to break things up once it gets too large. It also struggles with discoverability (i.e. it's slightly harder to find an impl based on the folder structure alone).

I’m also just of the mindset that mod.rs files should simply act as a directory and a place for module-related tests. So maybe I’m a little biased there haha

[bushrat011899]

I like this, but I have one suggestion: split into core, alloc, and std. This has a secondary benefit in that the std feature gates could all be combined together into a single:

#[cfg(feature ="std")]
mod std;

Otherwise, totally agree. I hate large files like that, and git diffs generally do too. Even single-line files are nice IMO, as long as the structure of the modules makes sense (which it does, since it mirrors a particular existing structure).

[MrGVSV]

I like this, but I have one suggestion: split into core, alloc, and std. This has a secondary benefit in that the std feature gates could all be combined together into a single:

#[cfg(feature ="std")]
mod std;

Otherwise, totally agree. I hate large files like that, and git diffs generally do too. Even single-line files are nice IMO, as long as the structure of the modules makes sense (which it does, since it mirrors a particular existing structure).

You know what, that's a really good idea. I'll try doing that!

[MrGVSV]

Alright, I split it up even further into alloc, core, and bevy_platform!

[bushrat011899]

Just some minor comments, nothing blocking. LGTM!

[bushrat011899]

Nit: Maybe move these gates up a level in the module tree:

#[cfg(any(unix, windows))]
mod ffi;

[MrGVSV]

Oh actually we can't do this since the other gate is the inverse (i.e. cfg(not(any(unix, windows)))]).

[bushrat011899]

Future PR: It'd be nice to move these to the bevy_platform implementation module, since we can remove these target_has_atomic feature gates.

[MrGVSV]

Oh? How so?

[bushrat011899]

bevy_platform has a sync::atomic module which includes all those target_has_atomic flags already, and for unsupported types, it uses portable-atomic as the fallback.

[MrGVSV]

Oh okay gotcha. Yeah that might be better served as a followup. I'll try to remember to make an issue or PR once this one lands

[alice-i-cecile]

@MrGVSV once merge conflicts are fixed I'll get this in so it doesn't accumulate more.