Don't enable icu_provider/alloc from component crates

[robertbastian]

This makes the DataPayload bigger, and not Send+Sync without the sync feature. We shouldn't enable icu_provider/alloc just to get zerofrom/alloc and yoke/alloc.

[Manishearth]

I'm very wary of similar problems recurring similar to the last release.

I think the main problem that could happen here is if some component relied on a dependency crate to enable icu_provider/alloc. I think it's fine since otherwise you would have had to add an icu_provider/alloc somewhere here to pass CI. But I would prefer this be double-checked somehow.

Perhaps update all cfg(feature = "alloc") in icu_provider to alloc2 or something and ensure that the only crates which break are the ones with explicit icu_provider + alloc deps?

[Manishearth]

I don't want to have to tie this change to this PR: can this be done separately?

I'm opposed to this being in a new util crate. I'd rather it be inlined in experimental, and if it is not to be inlined, we should probably follow the "copy around utils files" model we agreed upon in #4467 (comment) for shared macro code.

I don't think this two-line crate meets the bar of a utils crate. I know it's fixing what some of us view as a deficiency in the serde ecosystem, but as it stands I do not see the ecosystem picking this up, so it becomes yet another ICU4X specific util that, to me, doesn't seem to pull its weight.

[sffc]

I don't want them inlined in icu_experimental because we want these available to other ICU4X component crates. These helpers exist to work around a bug in Serde (serde-rs/serde#2072). They've previously been used by others, and I guess at the moment icu_experimental is the only one using them, but when we start graduating things from icu_experimental, then we need them in both places again.

I agree that these helpers are probably better suited for icu4x_shared.rs. However, moving these out of icu_provider breaks our internal stability policy.

[Manishearth]

However, moving these out of icu_provider breaks our internal stability policy.

Only if it is used by released icu non-experimental crates, right? (I was meaning to check this but I wanted this split out either way so as not to bog this issue down)

But I just checked, datetime in 2.0 uses this code. So yes, we cannot remove this in the 2.x stream.

[sffc]

The internal stability policy is about ease of integration, not promises made to clients, so even if this was used in only icu_experimental, I think we would still want to stick to it. Obviously this is an internal policy and we can always make exceptions to it, but exceptions should have justification.

[Manishearth]

The internal stability policy is about ease of integration, not promises made to clients

Yes, I know, I was the one who pushed for the policy.

I didn't intend for icu_experimental to be a part of that since icu_experimental breaks ease of client integration anyway. I don't consider this an exception. But it's moot anyway.

[sffc]

The 2.0.x component crates I released rely on icu_provider/alloc implying writeable/alloc and zerovec/alloc. If we do this, then we need to patch-release 2.0.x as well as 2.1.x crates, or else we repeat the same sort of problem as last time.

[sffc]

Can you clarify why the alloc feature increases the size of DataPayload?

I wonder if we could instead split the alloc feature in two, where alloc enables the new APIs like serde_borrow_de_utils and bubbles down to sub-crates, and alloc_bikeshed does things that change DataPayload.

[robertbastian]

I guess it doesn't actually increase the size unless the compiler can optimize &'static ():

/// The actual cart type (private typedef).
#[cfg(feature = "alloc")]
pub(crate) type CartInner = SelectedRc<Box<[u8]>>;
#[cfg(not(feature = "alloc"))]
pub(crate) type CartInner = &'static ();

It does however makes the struct not Send+Sync when the alloc feature is enabled but the sync feature isn't: #7208

[sffc]

Issue: This PR breaks the meaning of the alloc feature in the 2.0.x and 2.1.x component crates. Changing no-default-features behavior is not forbidden, but given that this is a low-level crate, we should do such a change only if it is better than feasible alternatives.

I think splitting the alloc feature into two pieces, as suggested before, would be an example of a feasible alternative. There might be others.

[Manishearth]

@robertbastian Figured it out: Assuming the serde stuff is undone, the issue is not between component crates; it is for users of ICU4X

If I am a user of icu_casemap and relied on it enabling icu_provider/alloc indirectly (without realizing it), I am broken by this change when I update icu_casemap.

This is probably a minor issue. Questions:

• Does it even make sense to "rely" indirectly on icu_provider/alloc? I think that's the type of thing only cared about by component crates
• Is it easy to accidentally rely on it? I think no.
• Can we consider this a user bug? I think yes, but we should be careful.

I think we lucked out by versioning policy here: all crates which depend on icu_provider are ones where we use ~ deps. So this is probably all fine.

[sffc]

WG notes:

• @sffc The core issue is icu_provider 2.0.0 -> 2.1.0 has breaking changes (DataPayload is not Send+Sync by default anymore) #7208
• @robertbastian The cause is that component crates enable icu_provider/alloc which they didn't used to do.
• @hsivonen Seems like a pain point that people depend on Send/Sync being dependent on features.
• @robertbastian We could require the sync feature for Send/Sync in all cases, but that's also a breaking change.
• @Manishearth There are multiple buckets of problems. There are problems for existing users, problems for future users. We need to be careful about moving problems around between buckets.
• @robertbastian Can you explain why the PR breaks things?
• @sffc 2.0 and 2.1 crates rely on icu_provider/alloc in order to get the ZeroVec/Writeable alloc
• @robertbastian But that will still work; icu_provider/alloc still does that.
• @Manishearth There could be external crates relying on that behavior, too. It's an issue for other crates; maybe it isn't between component crates any more.

[sffc]

As long as this change doesn't break 2.0.x and 2.1.x crates, I approve the change to the Cargo.toml files.

We still must figure out what to do with the serde utils.

[sffc]

Thought/Suggestion: I don't find it objectionable for icu_experimental/serde to enable icu_provider/alloc. I think it's probably better than duplicating the serde util code.