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 multiple available paths #24

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

jsha
Copy link
Contributor

@jsha jsha commented Dec 7, 2023

In rustls/rustls#1662 I raised the issue that it's confusing when seemingly-identical types appear under different paths in different crates, because there's no way for a user to confirm they are in fact identical, compatible types (short of reading the source).

Here's one attempt to solve that: in pki-types, we can document both main paths that an item is available. That documentation line will show up in both pki-types and rustls docs, so there's a signpost no matter which way a reader came to the documentation. Note that I couldn't use intra-doc links because pki-types doesn't depend on rustls.

I've only updated on item so far. I'm happy to do them all (or write a derive macro), but I wanted to get feedback on the concept first.

@cpu
Copy link
Member

cpu commented Dec 7, 2023

Would the re-export in the webpki crate be worth considering here too? I'm thinking no simply because direct usage of webpki + pki_types feels pretty niche compared to rustls + pki_types

Looking at that with fresh eyes it's a little bit unfortunate we went with types for the webpki re-export but pki_types for the rustls one :-/

@jsha
Copy link
Contributor Author

jsha commented Dec 7, 2023

I think it's worth mentioning those too, because someone may see them in code they are reading.

Note that the pki-types items don't actually have their own pages in the rustls-webpki docs: https://docs.rs/rustls-webpki/latest/webpki/index.html. In other words, they are not inlined:

https://doc.rust-lang.org/rustdoc/write-documentation/re-exports.html#inlining-rules

I can't figure out why rustdoc inlines them in the rustls docs, but not in the webpki docs. I know there are a number of bugs in rustdoc with regards to cross-crate re-exports. Perhaps each crate is simply tickling those bugs in different ways?

@@ -274,6 +274,9 @@ impl<'a> From<Vec<u8>> for CertificateRevocationListDer<'a> {
/// Certificates are identified in PEM context as `CERTIFICATE` and when stored in a
/// file usually use a `.pem`, `.cer` or `.crt` extension. For more on PEM files, refer to the
/// crate documentation.
///
/// This type is available as either `rustls_pki_types::CertificateDer` or re-exported as
/// `rustls::pki_types::CertificateDer`.
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I'm onboard with this crate documenting things going on in others. Even though minor, it seems like a bit of a layering violation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not a layering violation because pki-types isn't a distinct layer from rustls. It's a workaround for a specific problem of semver versioning, but it is notionally a part of rustls. It is maintained by the same team and it's part of rustls' public API. It's okay for the documentation pages of rustls and pki_types (and other rustls ecosystem crates) to be aware of each other. More than okay: it's important for the documentation pages of rustls ecosystem crates to be aware of each other, so we can help users understand how the pieces fit together.

@djc
Copy link
Member

djc commented Dec 8, 2023

I can't figure out why rustdoc inlines them in the rustls docs, but not in the webpki docs. I know there are a number of bugs in rustdoc with regards to cross-crate re-exports. Perhaps each crate is simply tickling those bugs in different ways?

It looks like rustls-webpki has this:

use pki_types as types;

whereas rustls has this:

pub mod pki_types {
    pub use pki_types::*;
}

I'm guessing that might explain the difference? I think I agree with Joe that this feels like a layering violation. If we decide to also add a re-export in tokio-rustls, we'd have to add that here? Doesn't seem very maintainable.

(I still think this is a rustdoc shortcoming -- it probably has enough information to annotate types hailing from another crate as such and that would go a long way towards alleviating this problem.)

@djc
Copy link
Member

djc commented Dec 8, 2023

Looking at that with fresh eyes it's a little bit unfortunate we went with types for the webpki re-export but pki_types for the rustls one :-/

For 0.103 we should probably just get rid of the webpki re-export, doesn't feel like it makes as much sense there.

@jsha
Copy link
Contributor Author

jsha commented Dec 8, 2023

(I still think this is a rustdoc shortcoming -- it probably has enough information to annotate types hailing from another crate as such and that would go a long way towards alleviating this problem.)

This is probably true, and we should open a discussion on the rust repo if there's not one already. I know cross-crate re-exports are historically a hard problem and a source of many bugs for rustdoc, in part because rustdoc doesn't have access to the full crate source for crate B when it's building documentation for crate A. It would indeed be great to have rustdoc be more explicit about re-exports, but we shouldn't delay writing useful documentation for our users because we are waiting for a different tool that might do it automatically for us.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants