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

Remove excessive ZeroFrom usage #6201

Closed
wants to merge 3 commits into from

Conversation

robertbastian
Copy link
Member

@robertbastian robertbastian commented Feb 26, 2025

We currently derive ZeroFrom for every single data struct. However, it is not required for a data struct to be ZeroFrom, it's only required to use DataPayload::map_project, DataPayload::try_map_project, and DataPayload::with_mut, which have a total of one usage in all of ICU4X. So it suffices to derive ZeroFrom for the one data struct, and remove all the other derives.

This also exposes the ZeroFrom trait (not derive macro) through zerovec::ule::ZeroFrom. This is because VarZeroVec fundamentally requires this trait, so it should be imported from there, not from icu_provider::prelude (where I've removed it, because it's not needed).

Manishearth
Manishearth previously approved these changes Feb 26, 2025
Copy link
Member

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Final commit looks good. We used to call this in every baked data but I think we don't anymore.

Are there any Cargo.toml deps that can be removed because of this?

@robertbastian
Copy link
Member Author

Are there any Cargo.toml deps that can be removed because of this?

icu_provider does not depend on zerofrom/derive anymore, just zerofrom

@robertbastian robertbastian marked this pull request as ready for review February 26, 2025 16:22
@robertbastian robertbastian requested review from Manishearth and removed request for a team, aethanyc, dminor, zbraniecki, hsivonen, echeran, makotokato, sffc, younies, pdogr and snktd February 26, 2025 16:22
Manishearth
Manishearth previously approved these changes Feb 26, 2025
Manishearth
Manishearth previously approved these changes Feb 26, 2025
@robertbastian
Copy link
Member Author

So this breaks this tutorial.

Do we want to guarantee that all data structs are ZeroFrom so that clients can do this kind of stuff? Then we should add it to the bound on DataMarker::DataStruct.

@robertbastian robertbastian marked this pull request as draft February 26, 2025 16:52
@Manishearth
Copy link
Member

I think the generic bound gets gnarly; I'd be fine with policy that every data struct has this bound without having it be enforced in the trait.

@sffc
Copy link
Member

sffc commented Feb 26, 2025

DataPayload::with_mut is important.

I kind of like the status quo of every data struct implementing all the traits it needs to implement, and the things that need the traits add them to impl bounds. I think it's hard to express them all as requirements on DataMarker::DataStruct, but I'm happy to be proven wrong.

It would be nice though to have a way to enforce these impls on data structs, because it's simply a bug if a core data struct doesn't implement one of these constellation traits such as ZeroFrom. One way to do that would be to add it on the ExportBox bound, even if the trait isn't used by that impl.

@Manishearth
Copy link
Member

because it's simply a bug if a core data struct doesn't implement one of these constellation traits such as ZeroFrom.

Yep, I'm fine with this status quo.

The idea is that ZeroFrom is functionality that is not necessary for the basic functionality of these types (so shouldn't go on the trait), but is an additional goodie that we provide for people doing mutation. If we forget to provide it, that's a bug, but we don't need to force ourselves to provide it with additional trait bounds. Perhaps we could "enforce" that internally with a registry test.

This is similar to how we try to provide PartialEq and Hash impls for types that make sense for them: if we forget, it's fine, we can add them as requested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants