add `MutUntyped::map_unchanged`

[jakobhellermann]

Objective

MutUntyped is the untyped variant of Mut<T> that stores a PtrMut instead of a &mut T. Working with a MutUntyped is a bit annoying, because as soon you want to use the ptr e.g. as a &mut dyn Reflect you cannot use a type like Mut<dyn Reflect> but instead need to carry around a &mut dyn Reflect and a impl FnMut() to mark the value as changed.

Solution

• Provide a method map_unchanged to turn a MutUntyped into a Mut<T> by mapping the PtrMut<'a> to a &'a mut T This can be used like this:

// SAFETY: ptr is of type `u8`
let val: Mut<u8> = mut_untyped.map_unchanged(|ptr| unsafe { ptr.deref_mut::<u8>() });

// SAFETY: from the context it is known that `ReflectFromPtr` was made for the type of the `MutUntyped`
let val: Mut<dyn Reflect> = mut_untyped.map_unchanged(|ptr| unsafe { reflect_from_ptr.as_reflect_ptr_mut(ptr) });

Note that nothing prevents you from doing

mut_untyped.map_unchanged(|ptr| &mut ());

or using any other mutable reference you can get, but IMO that is fine since that will only result in a Mut that will dereference to that value and mark the original value as changed. The lifetimes here prevent anything bad from happening.

Alternatives

1. Make Ticks public and provide a method to get construct a Mut from Ticks and &mut T. More powerful and more easy to misuse.
2. Do nothing. People can still do everything they want, but they need to pass (&mut dyn Reflect, impl FnMut() + '_) around instead of Mut<dyn Reflect>

Changelog

• add MutUntyped::map_unchanged to turn a MutUntyped into its typed counterpart

[alice-i-cecile]

Definitely opposed to alternate solution 1: it exposes far more internals of change detection than I'd like. I trust you that solution 2 is too annoying.

[alice-i-cecile]

I'd like to see PR description and title updated + the nit addressed before we merge.

[jakobhellermann]

I'd like to see PR description and title updated + the nit addressed before we merge.

Done

[alice-i-cecile]

@jakobhellermann once CI is passing I'm comfortable merging this.

[jakobhellermann]

CI is green

[alice-i-cecile]

@jakobhellermann if you could remove that unsafe here in the next day or two that would be great. I'll be sure to get this in regardless though; we can make a quick follow-up PR.

[alice-i-cecile]

bors r+

[bors[bot]]

Build failed (retrying...):

• check-compiles

[bors[bot]]

Build failed:

• build (macos-latest)

[alice-i-cecile]

Ah, the suggestion left a dead import around.

[alice-i-cecile]

bors r-

[alice-i-cecile]

@jakobhellermann can you fix this up? I'd love to merge this for the release.

[BoxyUwU]

The PR description seems very out of date to me, the motivation is basically invalidated by the existence of MutUntyped::with_type which would let you write code as untyped.with_type::<T>().map_unchanged(|data| ...) instead of untyped.map_unchanged::<T>(|data| ...).

I would expect MutUntyped::map_unchanged to not involve giving a type to the pointer at all, i.e. fn map_unchanged(self, f: impl FnOnce(PtrMut<'w>) -> PtrMut<'w>) -> Self

[joseph-gio]

I would expect MutUntyped::map_unchanged to not involve giving a type to the pointer at all, i.e. fn map_unchanged(self, f: impl FnOnce(PtrMut<'w>) -> PtrMut<'w>) -> Self

What would a completely untyped map operation like that be useful for?

[BoxyUwU]

you are working with dynamic components and want to map the Mut to go to a field instead of the whole struct maybe

[jakobhellermann]

The PR description seems very out of date to me, the motivation is basically invalidated by the existence of MutUntyped::with_type which would let you write code as untyped.with_type::<T>().map_unchanged(|data| ...) instead of untyped.map_unchanged::<T>(|data| ...).

Your suggestion only works if you know the type T, not if you have another way of going from Ptr to what you want like

let val: Mut<dyn Reflect> = mut_untyped.map_unchanged(|ptr| unsafe { reflect_from_ptr.as_reflect_ptr_mut(ptr) });

I would expect MutUntyped::map_unchanged to not involve giving a type to the pointer at all, i.e. fn map_unchanged(self, f: impl FnOnce(PtrMut<'w>) -> PtrMut<'w>) -> Self

Do you have a name suggestion for what this PR provides? Because it maps the Ptr to anything else. It just doesn't map the pointer to any pointer.

[BoxyUwU]

Your method also only works if you a type T to use?

[jakobhellermann]

If I have a ReflectFromPtr instance I can use that to map the MutUntyped to a Mut<dyn Reflect>. Theoretically, not without this PR.

No ::<T> involved.

[BoxyUwU]

ah I see what you mean, with this you can do with_type like stuff that requires more than just .deref/.deref_mut

[jakobhellermann]

rebased. Are there any requested changes left? I think they're all addressed

[joseph-gio]

Looks like CI is failing due to #7905, which renamed the last_change_tick and change_tick fields to last_run and this_run. Also, I don't think the Complex tag is necessary since the only thing this function does is call a closure.

[alice-i-cecile]

@jakobhellermann once this is passing CI I'll merge it in :)

[alice-i-cecile]

Adding Adopt-Me on this since it's been a week <3 Shouldn't be hard to clean up at all.

[joseph-gio]

Closing in favor of #9194, which is an adoption of this PR :)