bevy_reflect: enum_utility cleanup

[MrGVSV]

Objective

The enum_utility module contains the get_variant_constructors function, which is used to generate token streams for constructing enums. It's used for the FromReflect::from_reflect implementation and the Reflect::try_apply implementation.

Due to the complexity of enums, this function is understandably a little messy and difficult to extend.

Solution

Clean up the enum_utility module.

Now "clean" is a bit subjective. I believe my solution is "cleaner" in that the logic to generate the tokens are strictly coupled with the intended usage. Because of this, try_apply is also no longer strictly coupled with from_reflect.

This makes it easier to extend with new functionality, which is something I'm doing in a future unrelated PR that I have based off this one.

Testing

There shouldn't be any testing required other than ensuring that the project still builds and that CI passes.

[cBournhonesque]

I left some small comments; overall the new structure is clearer to me!

[cBournhonesque]

I'm not sure if this design with field_accessor, field_unwrapper, field_constructor is the clearest.

Maybe just having a BaseEnumVariantOutputDataBuilder
and then two different structs sEnumVariantOutputDataBuilderForFromReflect
and EnumVariantOutputDataBuilderForTryApply that share the underlying BaseEnum... but have different implementations for the unwrapper and constructor would be easier to understand?

I don't think it's a blocker, because you're the one primarily maintaining this code so it's already an improvement if it's clearer to you. I think the new structure is overall much clearer.

[MrGVSV]

Maybe just having a BaseEnumVariantOutputDataBuilder
and then two different structs sEnumVariantOutputDataBuilderForFromReflect
and EnumVariantOutputDataBuilderForTryApply that share the underlying BaseEnum... but have different implementations for the unwrapper and constructor would be easier to understand?

Hm, could you give an example? I'm probably misunderstanding, but this almost sounds like what I have now with the base builder type and the two pub(crate) generator functions.

[cBournhonesque]

Hm I guess you're right.
I was thinking of something with generics

trait EnumTokenBuilder {
   fn field_accessor(&Ident, VariantField) -> TokenStream;
   
   fn ...
}

just to avoid the Box<dyn Fn> and having to build the struct without those functions existing.
I guess I'm just not a fan of the builder pattern in this instance, especially if the fields are not optional. A new contributor might not understand why the new function doesn't just require those fields.
It's mostly a matter of taste, though

[MrGVSV]

Yeah tbh I don't love it either. Alternatively, I could scrap both new and the with_*** and just have it all defined like:

let builder = EnumVariantOutputDataBuilder {
  this,
  reflect_enum,
  field_accessor: Box::new(/* ... */),
  // ...
};

builder.build()

It just means that any fields with a potential default value will need to be defined each time.

The trait method you propose is nice too and is probably a bit Rustier. I might look into that actually. 🤔

[MrGVSV]

I've updated the code to use traits instead. I think it's a lot cleaner so thank you for the suggestion!

Let me know if you have any further thoughts on it (or if you even prefer the old version haha)!

[cBournhonesque]

I like it!
I also think it's cleaner with the traits rather than the boxed functions :)

[alice-i-cecile]

@MrGVSV once this has merge conflicts resolved I'll merge this.