Document traits as either sealed or implementable

[sffc]

Just like we document structs and enums as either exhaustive or non-exhaustive, we should document traits as either sealed or implementable.

For sealed traits, we could add a __sealed() function, and add a slab like this to the docs:

/// <div class="stab unstable">
/// 🚧 This trait is considered unstable; it may change at any time, in breaking or non-breaking ways,
/// including in SemVer minor releases. Do not implement this trait in userland.
/// </div>

Unclear if there is a Clippy lint to enforce this.

[sffc]

We need to decide both for CldrCalendar specifically and the language/code for the more general case.

Assigned to @Manishearth for the purpose of making a proposal.

[Manishearth]

For sealing traits I would suggest that we just use the typical : Sealed pattern. We don't need any documentation string because it is impossible to implement outside of our code.

If we need a trait to be implementable across crate boundaries, the Sealed trait can get a doc(hidden) reexport. In such a case we can document using the string above.

For CldrCalendar specifically I think we should seal it and mark every method as doc(hidden) so that nobody calls it; this way it is a pure bound. We should use the unstable doc string above for this trait.

Approval:

• @sffc
• @robertbastian

[sffc]

I like the overall shape of this.

Question: when you say "use the typical : Sealed pattern", what exactly is Sealed? Is it a per-trait type that is defined alongside the trait being sealed? Do we define one per crate in the lib.rs file? And to be clear, it is a trait that is just not exported, which the type system lets us do?

I remain concerned with the proliferation of #[doc(hidden)] for things that are internal/experimental APIs. I apologize for the digression but:

1. Sometimes #[doc(hidden)] means that we want to hide the docs (e.g. for a re-export) and other times #[doc(hidden)] means that we are poking a hole to expose an internal API. I don't really like conflating these two things
2. #[doc(hidden)] doesn't generate any warnings. I wonder if we should be using #[deprecated] instead? Should we be doing that for experimental code, too?
3. I don't have a problem with people using internal APIs so long as they don't come kicking and screaming that we deleted or changed them. Opt-in is nice, and #[doc(hidden)] doesn't require opt-in. Should we be putting them in a feature #[cfg(feature = "internal")] or something?
4. For people whom we trust, it's nice to see the internal APIs in docs. I wonder if we should show them on dev docs and hide them on docs.rs.

For the question in this thread, I am LGTM to make cross-crate-boundary-implementable sealed traits be "marked as internal using a convention which we decide elsewhere", not specifically #[doc(hidden)].

[Manishearth]

Question: when you say "use the typical : Sealed pattern", what exactly is Sealed? Is it a per-trait type that is defined alongside the trait being sealed? Do we define one per crate in the lib.rs file? And to be clear, it is a trait that is just not exported, which the type system lets us do?

One per crate, does not matter where it lives because it is private.

And yes, it is just not exported.

I don't have a problem with people using internal APIs so long as they don't come kicking and screaming that we deleted or changed them. Opt-in is nice, and #[doc(hidden)] doesn't require opt-in. Should we be putting them in a feature #[cfg(feature = "internal")] or something?

Yeah I think so. Though we have a lot of internal APIs we need to unconditionally make public like provider APIs.

For people whom we trust, it's nice to see the internal APIs in docs. I wonder if we should show them on dev docs and hide them on docs.rs.

Worth considering using document-private-items.

For the question in this thread, I am LGTM to make cross-crate-boundary-implementable sealed traits be "marked as internal using a convention which we decide elsewhere", not specifically #[doc(hidden)].

To be clear, I'm only suggestion doc(hidden) for cases like CldrCalendar where we may want other people to still be able to opt in to it. But I think for now marking CldrCalendar as fully sealed and reopening (hah!) this discussion later when we wish to allow others to reimplement, is a good call.

[Manishearth]

@sffc quick ping on approving this or moving the discussion forward

[sffc]

Yeah I approve and have already been doing this for example in the icu_pattern code. OK to make the sealed trait be internal according to the decision in #4467

[Manishearth]

For sealed traits, we could add a __sealed() function, and add a slab like this to the docs:

/// <div class="stab unstable">
/// 🚧 This trait is considered unstable; it may change at any time, in breaking or non-breaking ways,
/// including in SemVer minor releases. Do not implement this trait in userland.
/// </div>

@sffc thinking about this more: I don't think we should actually do this: these traits aren't implementable anyway, and we shouldn't scare people off from using APIs that use this traits.

Rather, I recommend having text of the form ":no_entry_sign: This trait is sealed and cannot be implemented in userland code"

[sffc]

What I ended up doing in datetime is export a trait called UnstableSealed. So the traits can still be implemented in userland, but they need to also implement the scary trait UnstableSealed, which also can have all the needed docs on it.

[sffc]

icu_datetime::scaffold::UnstableSealed

https://unicode-org.github.io/icu4x/rustdoc/icu_datetime/scaffold/trait.UnstableSealed.html

That particular name might be reasonable to use as a standard name elsewhere, too. scaffold::UnstableSealed.

[Manishearth]

@sffc yeah, but in this issue we agreed to just seal it, so we don't need UnstableSealed anymore

[sffc]

I'm scrolling and I'm not sure what the agreement is?

I said in #4338 (comment) that I agreed to the proposal in #4338 (comment) conditional on the decision in #4467, because I do not want a proliferation of doc(hidden).

Since I said that, there has not been much movement on doc(hidden) solutions, and we separately agreed on the new scaffold module convention.

[sffc]

In most cases I've seen, there's no reason to seal traits other than us reserving the right to change them. However, in many cases, even in datetime, I want to retain the ability to build custom implementations of certain traits in third-party code; if someone wants to do something that ICU4X can't do by itself, I want to have as many knobs available as possible. UnstableSealed solves this cleanly.

Only when there are actual safety invariants involved do I see a reason for the traits to actually be private-sealed.

I don't see a use case behind doc-hidden sealed.

[sffc]

I agree on doc(hidden) of trait members only because there isn't a way to mark the methods as unstable, except perhaps naming the methods unstable, which I quite like.

Or we just say that the scaffold module is unstable.

[Manishearth]

@sffc

For CldrCalendar specifically I think we should seal it and mark every method as doc(hidden) so that nobody calls it; this way it is a pure bound. We should use the unstable doc string above for this trait.

That's the agreement^

I think I know what happened here: CldrCalendar uses UnstableSealed, but so does everything else in the crate. I mistakenly took the agreement above to mean that datetime should use Sealed everywhere, but it's just CldrCalendar that does that. Everything else in the crate should use UnstableSealed. I'll redo my PR.

I don't think we should proliferate the UnstableSealed pattern unless there's a strong reason. The "slab of docs" you proposed makes sense for UnstableSealed, not regular Sealed.

I agree on doc(hidden) of trait members only

yes, this is what I mean by "doc hidden sealed"

there has not been much movement on doc(hidden) solutions

Is the main problem just ICU4X dev discovery? We can publish docs with --document-hidden-items, I'm not sure what further solutions are desired here. I'd rather not block API decisions on us implementing tooling that we know is implementable.

[Manishearth]

Anyway, here's what I plan to do tomorrow:

• UnstableSealed is left alone. It already has the right disclaimer on it. I may try to improve the docs slightly
• CldrCalendar moves to : Sealed
• All : Sealed traits get a header talking about the sealedness
• All : Sealed traits get doc(hidden) on their methods, unless we explicitly desire these methods to be callable.
• All other traits get a non-doc comment saying that they are implementable, or that they are implicitly sealed through blanket impls