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

Document traits as either sealed or implementable #4338

Open
sffc opened this issue Nov 20, 2023 · 6 comments
Open

Document traits as either sealed or implementable #4338

sffc opened this issue Nov 20, 2023 · 6 comments
Labels
2.0-breaking Changes that are breaking API changes C-meta Component: Relating to ICU4X as a whole

Comments

@sffc
Copy link
Member

sffc commented Nov 20, 2023

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 sffc added C-meta Component: Relating to ICU4X as a whole discuss Discuss at a future ICU4X-SC meeting 2.0-breaking Changes that are breaking API changes labels Nov 20, 2023
@sffc sffc added this to the ICU4X 2.0 milestone Nov 20, 2023
@sffc sffc added needs-approval One or more stakeholders need to approve proposal and removed discuss Discuss at a future ICU4X-SC meeting labels Dec 7, 2023
@sffc
Copy link
Member Author

sffc commented Dec 7, 2023

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
Copy link
Member

Manishearth commented Dec 7, 2023

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
Copy link
Member Author

sffc commented Dec 7, 2023

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
Copy link
Member

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.

@sffc sffc removed the needs-approval One or more stakeholders need to approve proposal label Mar 14, 2024
@Manishearth
Copy link
Member

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

@sffc
Copy link
Member Author

sffc commented Mar 29, 2024

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 Manishearth removed their assignment Mar 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.0-breaking Changes that are breaking API changes C-meta Component: Relating to ICU4X as a whole
Projects
Status: Blocked on other issue
Development

No branches or pull requests

2 participants