icu4x icon indicating copy to clipboard operation
icu4x copied to clipboard

#[derive(ULE, VarULE)]` could be more useful for simple wrapper types

Open Manishearth opened this issue 3 years ago • 5 comments

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

Manishearth avatar Aug 02 '22 17:08 Manishearth

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_varule was 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 using make_varule with 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.

sffc avatar Feb 16 '23 07:02 sffc

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

Manishearth avatar Feb 16 '23 16:02 Manishearth

#[make_varule(Self)] works for me. And #[make_ule(Self)] can generate the reflexive AsULE.

sffc avatar Feb 16 '23 17:02 sffc

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 u8 or 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.

Manishearth avatar Jun 26 '24 00:06 Manishearth

Moving that discussion to https://github.com/unicode-org/icu4x/issues/5127

Manishearth avatar Jun 26 '24 00:06 Manishearth