icu4x
icu4x copied to clipboard
#[derive(ULE, VarULE)]` could be more useful for simple wrapper types
In general we nudge people towards #[make_ule] and #[make_varule], but there is a small use case for the direct derives: when you're making a simple wrapper type around an existing ULE or VarULE type.
It would be nice if #[derive(ULE)] could be asked to generate AsULE and ZeroMapKV implementations, and #[derive(VarULE)] could be asked to generate Serialize, Ord (etc), and ZeroMapKV
In general we nudge people towards
#[make_ule]and#[make_varule], but there is a small use case for the direct derives: when you're making a simple wrapper type around an existing ULE or VarULE type.
Just to offer some data here. Here's a full inventory of the VarULE types in ICU4X, excluding tests and benches:
- locid_transform::provider::StrStrPairVarULE. This is a struct with two strings. Seems like a good use of
make_varule. - plurals::rules::runtime::ast::RelationULE. This is the use case for which
make_varulewas invented. - zerovec::ZeroSlice and zerovec::VarZeroSlice. These are custom impls and probably will remain custom impls.
- provider_blob::blob_schema::Index32U8. This should be
#[derive(VarULE)]but it is instead usingmake_varulewith a comment that we should migrate at some point. I think that may have been the case that prompted this issue to be open. - compactdecimal::provider::PatternULE. Another model case for
make_varule. - zerovec::ule::UnvalidatedStr. This has a bunch of custom impls and should be
#[derive(VarULE)]. - The new
NormalizedPropertyName, in the same bucket as UnvalidatedStr.
Therefore, we have 3 cases that are good make_varule, and 3 cases that are good #[derive(VarULE)].
Both buckets span generalizable zerovec types (StrStrPairVarULE, UnvalidatedStr) and custom client types (PatternULE, NormalizedPropertyName).
Therefore, I think it's not a correct characterization to describe the second bucket as "a small use case". I think it's a use case that is just as valid and practical.
The second bucket are simply types for which we don't need the non-ULE type, because the ULE type does the whole job. This means simply any type that is already unaligned. We shouldn't make such types second-class; they're the ones that work best with ZeroVec's architecture. They may be more efficient, too, because we don't need to calculate lengths and pointers to fill in Cow<str>s and stuff.
Yeah I think when I filed this issue it felt small, but for VarULE specifically I don't think it is anymore. (For ULE it's almost always the case that having the AsULE is good, but I think there's room for support for reflexive AsULEs)
One thing we could do is make this #[make_varule(Self)]. Mostly because it's not a custom derive (it's not implementing one trait), and then it can take all of the make_varule flags. But I'm also open do having it be something like #[derive(VarULE] #[zerovec::implement_other_varule_traits] or something
#[make_varule(Self)] works for me. And #[make_ule(Self)] can generate the reflexive AsULE.
In #5124 we're also discussing finding a way to avoid these derives and using macros instead.
Looking at the codebase, I see a bunch of similar but distinct use cases:
- A wrapper ULE type that has a custom AsULE impl pointing to it but otherwise transparently wraps a
u8or something. e.g.SkeletonDataIndexULE. We need a way to generate passthrough ULE impls for this. - A wrapper VarULE type that uses ref-cast like impls, e.g.
NormalizedPropertyNameStr. We need a way to generate passtrhoguh ULE impls and ref-cast-like methods for this. - A wrapper AsULE type that is transparent for unclear reasons: e.g. every property type. Do these actually need to be transparent? Can these just be AsULE? Either way, this can be handled with a non-wrapping macro that just does
impl_passthrough_asule!(ULEType, AsULEType, AsULEWrappedType)or something.
Moving that discussion to https://github.com/unicode-org/icu4x/issues/5127