Consistent design for unstable APIs (including provider modules)

[Manishearth]

See also: #4193, #6630, #6629

@robertbastian came up with this plan earlier. The idea is that every ICU4X crate has an unstable feature, for exposing unstable/experimental APIs.

Provider modules (and try_new_unstable) of course must always be publicly available, but we mark them as cfg(not(feature = "unstable"), doc(hidden)).

This means:

• docs.rs still shows them (we run that with all-features). There will be stability banners (Use nightly feature documentation #6630)
• Local docs builds will not show them for most people
• cargo-semver-checks will ignore these APIs.

Crates may still choose to have doc(hidden) things that are not exposed with the feature: the line is whether we expect users to use this API at all. For example, the properties open enum newtype field should remain unconditionally doc(hidden); we do not wish for ICU4X users to see that field. We continue to be conservative about using doc(hidden).

cc @sffc @robertbastian

[Manishearth]

I think we mostly have agreement on this plan, @sffc noted that he couldn't find notes on it, filing an issue to document things and to get formal approval.

[Manishearth]

A related question is: how do we make cargo-semver-checks happy for when we make such a transition? It does let one override the baseline, but I'm not sure if it works the way we want: @robertbastian can you experiment with that?

An explicit ignorelist would be a nice feature addition.

[sffc]

Another related question is whether we want to continue maintaining two different classes of unstable and internal APIs, or whether we should consolidate this down to just one class? Manish and I discussed some pros and cons.

[sffc]

State of doc(hidden) in ICU4X:

$ find . -name '*.rs' -exec grep -F '#[doc(hidden)]' {} ';' | awk '{$1=$1};1' | sort | uniq -c
     92 #[doc(hidden)]
      1 #[doc(hidden)] // #2417
      2 #[doc(hidden)] // appears in public bound
      1 #[doc(hidden)] // canonical location in super
      1 #[doc(hidden)] // compiled constructors look for the baked provider here
      3 #[doc(hidden)] // databake
      7 #[doc(hidden)] // databake internal
      2 #[doc(hidden)] // draft API in ICU4C
      4 #[doc(hidden)] // for databake, but there are no invariants
      1 #[doc(hidden)] // for testing and macros
      1 #[doc(hidden)] // for testing only
      1 #[doc(hidden)] // for testing purposes only: checks if it is using the Bigger format
      4 #[doc(hidden)] // internal
     13 #[doc(hidden)] // macro
      3 #[doc(hidden)] // macro use
      2 #[doc(hidden)] // `ScriptWithExt` not intended as public-facing but for `ScriptWithExtensionsProperty` constructor
      3 #[doc(hidden)] // sealed trait
      2 #[doc(hidden)] // singletons might be used cross-crate
      1 #[doc(hidden)] // subject to change
      1 #[doc(hidden)] // testing
      1 #[doc(hidden)] // TODO(#4467): establish this as an internal API
      7 #[doc(hidden)] // TODO(#4467): Should be internal
     10 #[doc(hidden)] // unstable
      4 #[doc(hidden)] // used by FFI code
      1 #[doc(hidden)] // used in collator

[Manishearth]

Another related question is whether we want to continue maintaining two different classes of unstable and internal APIs, or whether we should consolidate this down to just one class? Manish and I discussed some pros and cons.

My position is that these are two different things: we have "unstable but exposed to power users" and we have "internal to be used cross-crate/in macros/for testing". I am very strongly against some of our internal-to-be-used-cross-crate-or-macros-or-testing stuff showing up on our public docs. However, I do not want unstable-but-exposed-to-power-users to require power users to sift through --doc-hidden-items. I don't really see an upside of consolidating these two aside from conceptual simplicity, but it's largely internal conceptual simplicity (it's simpler for us as maintainers, but doesn't really simplify things for users).

[Manishearth]

• @robertbastian: cargo-semver-checks treats any API or feature starting with unstable as unstable.
• @Manishearth: does cargo-semver-checks figure out that it needs to explicitly disable the unstable feature?
• @robertbastian: I think so.
• @Manishearth another benefit: we can now temporarily unstableize new APIs that we still want to iterate on, with ease, because the feature exists
• @robertbastian: metacrate should expose icu_experimental behind the unstable feature
• @sffc So does this mean that we will have an unstable feature but not an experimental feature?
• @Manishearth CSC treats unstable as a prefix. We could have unstable_experimental.
• @robertbastian what's the diff bw unstable and hidden
• @Manishearth My position is that these are two different things: we have "unstable but exposed to experimentors and power users" and we have "internal to be used cross-crate/in macros/for testing".
• @robertbastian: I don't see a distinction between "unstable" and "experimental"
• @Manishearth I don't really either. Bit of a distinction between perma-unstable and experimental.
• @Manishearth We can use banners if we want to talk about this. Exp APIs should be marked "experimental and unstable" in docs.
• @nekevss Has the experimental keyword been brought up to CSC before
• @Manishearth no, we could.
• @nekevss Can see many libraries needing that
• @Manishearth would be cool if CSC had configs for this
• @robertbastian: unstable features are also pretty widespread so it's just good

Conclusion 1: All component crates get an unstable feature (other crates as needed).

Conclusion 2: All of our non-provider experimental/unstable APIs that are intended to be used by power users or experimentors should be cfg(feature = "unstable"). We may make a distinction between experimental and perma-unstable in doc banners.

Conclusion 3: Provider code gets marked cfg(not(feature = "unstable"), doc(hidden)). The feature continues to be enabled on docs.rs. We continue to have stability banners.

Addendum to 3: also doc(cfg(feature = "unstable"))

More discussion:

• @sffc Objection to conclusion 2 is that we already have an experimental feature (plurals, datetime). Don't want to break it.
• @robertbastian It's unstable , we can break it
• @Manishearth We can leave the feature around empty. But overall I don't think we have
• @robertbastian We have it on capi, Moritz was using it for something
• @sffc We can have two features: unstable_provider and experimental.
• @robertbastian: it should be unstable_experimental (not a fan)
• @Manishearth the feature name needs to change for CSC anyway, unless we decide to permanently stick with the pinned baseline model
• @robertbastian Using these feature names would be better for the ecosystem.
• @Manishearth: Shane, do you care more about there being two feature names (I don't see a strong need for it) or changing the "experimental" feature name?
• @sffc Don't care abotu two feature names. Care about changing it. Would rather not be driven by the tool conventions.
• @Manishearth it's not just a tool convention thing, it makes some sense from a user perspective to be explicit
• @zbraniecki: What's wrong with "experimental", why can't CSC treat it specially
• @Manishearth: chance it could be used for non-unstable things. we could ask for an explicit opt out
• @Manishearth options on the table:
  1. We have one unstable feature for everything
  2. We continue to have an experimental feature, and we also have unstable_provider.
     a. We teach CSC about opt outs
     b. We use the scheme proposed in Split semver CI job #6663 to allow for acknowledged CI breakages. (Downside: that's a bit of a custom workflow and is slow)
  3. We have unstable and experimental. New things get added to the unstable feature and we phase out the experimental feature.
• @Manishearth Why do we care about the experimental feature continuing to work
• @sffc We do have code that uses the manual plural operands. And now we have the serde builder in datetime. This is a human timescale thing as opposed to a code timescale. The plurals experimental code we don't want to bless as stable, but it's basically stable.
• @robertbastian: Sounds like provider modules, "more unstable than experimental"
• @Manishearth Overall the main reason I'd be hesitant about a "breakage" in unstable code is a change you could make that would be hard to mitigate. I don't think that's the case here; it's easy to notice.
• @robertbastian: the compiler just tells you to use the new feature
• @robertbastian re: human timescales; we had the same discussion about providers.
• @sffc Was okay with changing the collator struct thing but it had only been out by two weeks
• @sffc convinced by rustc being helpful about this.
• @robertbastian We should keep the cargo feature around and have it do nothing so we can have rustc being helpful about it.
• @sffc Think it's cleaner if we're going to replace the feature for people to get the error "the feature is missing", they edit Cargo.toml, and then rustc tells them what to use instead
• @sffc We should discuss the degree to which we make things pub/crate and doc(hidden) in provider.
• @robertbastian We need a single cfg in the pub mod provider. Don't see why we need the markers to be stable.
• @robertbastian datagen can enable the unstable feature
• @Manishearth would rather not have datagen use the unstable feature unless it is using non-provider code. Provider code is hidden-but-pub anyway.

Conclusion 1: All component crates get an unstable feature (other crates as needed).

Agreed: @hsivonen @Manishearth @nekevss @sffc @robertbastian @zbraniecki

Conclusion 2: All of our non-provider experimental/unstable APIs that are intended to be used by power users or experimentors should be cfg(feature = "unstable") (or something with pub reexports if it is needed internally, but not doc hidden). We may make a distinction between experimental and perma-unstable in doc banners.

Agreed: @Manishearth @hsivonen @nekevss @sffc @robertbastian @zbraniecki

Conclusion 3: Provider code gets marked cfg(not(feature = "unstable"), doc(hidden)). The feature continues to be enabled on docs.rs. We continue to have stability banners (or doc(cfg(feature = "unstable")) when we have it). We don't necessarily consider this the long term solution and may refine it, for example, to expose markers without the unstable feature, or make parts of the provider module be pub(crate).

Agreed: @Manishearth @robertbastian @sffc @zbraniecki (@nekevss @hsivonen abstain)

[Manishearth]

All of our non-provider experimental/unstable APIs that are intended to be used by power users or experimentors should be cfg(feature = "unstable") (or something with pub reexports if it is needed internally, but not doc hidden). We may make a distinction between experimental and perma-unstable in doc banners.

We should have a caveat on this that we can use doc(hidden) instead of completely disappearing the trait if the trait needs to be used in other code.

This came up in #7092