Skip to content

Conversation

@Manishearth
Copy link
Member

Fixes the issue found in #7374

This was an oversight when we did new properties.

@Manishearth Manishearth requested a review from a team as a code owner January 7, 2026 18:14
provider: &(impl icu_provider::buf::BufferProvider + ?Sized),
) -> Result<Self, DataError>
where
T: EnumeratedProperty + for<'a> serde::Deserialize<'a>,
Copy link
Member Author

Choose a reason for hiding this comment

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

the Deserialize impl shouldn't be necessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

Never mind; CPTs use T's serde impl in human readable mode.

Copy link
Member

Choose a reason for hiding this comment

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

Isn't there a macro that generates the whole impl for you?

Copy link
Member Author

Choose a reason for hiding this comment

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

We have generics here.

That's probably how we messed up in the first place, because we couldn't use the macro.

Copy link
Member

@robertbastian robertbastian left a comment

Choose a reason for hiding this comment

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

this was not an oversight, it was intentional. properties data markers are more stable than other crates, because each data struct is an enum that we can evolve by adding new variants. this means that we will never need fallback code in a buffer constructor, so we don't need buffer constructors.

@Manishearth
Copy link
Member Author

I don't see the relationship between buffer providers and data struct format fallback. Data struct format fallback is something we can do with buffer providers (and don't need to do with baked), but that's a property of custom providers in general, and it's not their raison d'être.

There are at least two use cases for buffer providers in properties. Firstly, a low-memory application may choose to load properties on demand. I think this is a pretty marginal use case since the deserialization code has its own cost, but ... possible.

The other one is tailoring/private use: the OP in #7374 cared about that use case in particular.

We should have a stable ctor that allows one to load properties off of a buffer provider. Currently you need to use try_new_unstable.

If we are 100% sure that the marker will never change then we could design try_new_from_provider instead (which has try_new_unstable's bounds) but this would be a deviation from how we do data elsewhere and would need to be appropriately documented with examples for use with a buffer provider (since it's nontrivial to figure out you need to use AsDeserializing).

@robertbastian
Copy link
Member

In any case this is a new API so let's discuss this in the TC.

@Manishearth Manishearth added the discuss-priority Discuss at the next ICU4X meeting label Jan 7, 2026
@Manishearth
Copy link
Member Author

Fair enough.

@sffc
Copy link
Member

sffc commented Jan 8, 2026

I favor adding with_buffer_provider here because of consistency and discoverability. It does not hurt to have the constructor, even if you subscribe to the line of reasoning presented by @robertbastian

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

discuss-priority Discuss at the next ICU4X meeting

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants