Encapsulate cfg(feature = "track_location") in a type.

[chescock]

Objective

Eliminate the need to write cfg(feature = "track_location") every time one uses an API that may use location tracking. It's verbose, and a little intimidating. And it requires code outside of bevy_ecs that wants to use location tracking needs to either unconditionally enable the feature, or include conditional compilation of its own. It would be good for users to be able to log locations when they are available without needing to add feature flags to their own crates.

Reduce the number of cases where code compiles with the track_location feature enabled, but not with it disabled, or vice versa. It can be hard to remember to test it both ways!

Remove the need to store a None in HookContext when the track_location feature is disabled.

Solution

Create an TrackLocationOption<T> type that contains a T if the track_location feature is enabled, and is a ZST if it is not. The overall API is similar to Option, but whether the value is Some or None is set at compile time and is the same for all values.

Create a MaybeLocation alias for the common case of &'static Location<'static>.

Remove all cfg(feature = "track_location") blocks outside of the implementation of that type, and instead call methods on it.

When track_location is disabled, TrackLocationOption is a ZST and all methods are #[inline] and empty, so they should be entirely removed by the compiler. But the code will still be visible to the compiler and checked, so if it compiles with the feature disabled then it should also compile with it enabled, and vice versa.

Open Questions

The names TrackLocationOption<T> and MaybeLocation aren't great. Suggestions for better names are welcome!

Where should these types live? I put them in change_detection because that's where the existing MaybeLocation types were, but we now use these outside of change detection.

While I believe that the compiler should be able to remove all of these calls, I have not actually tested anything. If we want to take this approach, what testing is required to ensure it doesn't impact performance?

Migration Guide

Methods like Ref::changed_by() that return a &'static Location<'static> will now be available even when the track_location feature is disabled, but they will return a new MaybeLocation type. MaybeLocation wraps a &'static Location<'static> when the feature is enabled, and is a ZST when the feature is disabled.

Existing code that needs a &Location can call into_option().unwrap() to recover it. Many trait impls are forwarded, so if you only need Display then no changes will be necessary.

If that code was conditionally compiled, you may instead want to use the methods on TrackLocationOption to remove the need for conditional compilation.

[SpecificProtagonist]

Nice, we do want something like this.

I'll take a proper look later, but from a quick glance I can't see why we need TrackLocationOption<T>:
Shouldn't defining MaybeLocation as struct MaybeLocation { #[cfg(feature = "track_location")] caller: &'static Location<'static>} and using it in all the places where we previously had a cfg'd Location be enough? E.g. &'w mut MaybeLocation instead of TrackLocationOption<&'w mut &'static Location<'static>>.

Remove the need to store a None in HookContext when the track_location feature is disabled.

I was surprised that this didn't happen in the PR that introduced it (didn't get a chance to look at it before it got merged).

[chescock]

I'll take a proper look later, but from a quick glance I can't see why we need TrackLocationOption<T>:
Shouldn't defining MaybeLocation as struct MaybeLocation { #[cfg(feature = "track_location")] caller: &'static Location<'static>} and using it in all the places where we previously had a cfg'd Location be enough? E.g. &'w mut MaybeLocation instead of TrackLocationOption<&'w mut &'static Location<'static>>.

That would be simpler! I was trying to make sure that there was never any cost when the feature was disabled, though, and &mut ZST is pointer-sized instead of zero-sized. It would also mean things like set_spawned_or_despawned_by would have to actually do array indexing before assigning the ZST. I'm already a little worried that this PR will get rejected because we can't be certain that it's zero-cost :).

[Seldom-SE]

This would help avoid this pitfall I've seen a couple times cBournhonesque/lightyear#745 joseph-gio/bevy-trait-query#77

[eckz]

Just to avoid repetition in names like TrackLocationOption<Option<T>>, what about renaming TrackLocationOption to

pub struct MaybeLocation<T: ?Sized = &'static Location<'static>> { ... }

and removing the type alias

[chescock]

Just to avoid repetition in names like TrackLocationOption<Option<T>>, what about renaming TrackLocationOption to

pub struct MaybeLocation<T: ?Sized = &'static Location<'static>> { ... }

and removing the type alias

Hmm, that's tempting. I'm concerned that the default type there is a little complex, though; usually default types are simple things like () or Global. And the repetition isn't exactly wrong, as calling into_option() gives you an Option<Option<T>>.

I think I'll leave it alone for now, but if someone else chimes in with a preference to change it to that then I'm happy to do so!