Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
Manishearth opened this issue Aug 2, 2022 · 5 comments
Open
Assignees
Labels
C-zerovec Component: Yoke, ZeroVec, DataBake S-medium Size: Less than a week (larger bug fix or enhancement) T-enhancement Type: Nice-to-have but not required
Milestone

Comments

@Manishearth
Copy link
Member

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 Manishearth added T-enhancement Type: Nice-to-have but not required S-medium Size: Less than a week (larger bug fix or enhancement) labels Aug 2, 2022
@Manishearth Manishearth added this to the Utilities 1.0 milestone Aug 2, 2022
@sffc sffc added the C-zerovec Component: Yoke, ZeroVec, DataBake label Dec 22, 2022
@sffc
Copy link
Member

sffc commented Feb 16, 2023

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.

@Manishearth
Copy link
Member Author

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

@sffc
Copy link
Member

sffc commented Feb 16, 2023

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

@Manishearth
Copy link
Member Author

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
Copy link
Member Author

Moving that discussion to #5127

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-zerovec Component: Yoke, ZeroVec, DataBake S-medium Size: Less than a week (larger bug fix or enhancement) T-enhancement Type: Nice-to-have but not required
Projects
None yet
Development

No branches or pull requests

2 participants