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

doc: restructure project to enable cross-platform rustdoc #3768

Closed
wants to merge 7 commits into from

Conversation

udoprog
Copy link
Contributor

@udoprog udoprog commented May 9, 2021

Experimental first stab at improving the state of #3275

Motivation

Currently windows-only documentation does not show up by default on https://docs.rs/tokio. This patch tries to make that mostly happen while keeping the status quo unchanged. E.g. unix specific documentation is left unchanged since that's what docs.rs uses. But windows-specific modules will also be visible with a few std APIs stubbed and documented locally (through tokio::io).

Solution

Each module using platform-specific imports that are picked up by rustdoc adds something like this to the top of it:

use crate::os::unix::io::{AsRawFd, RawFd};
use crate::os::unix::net;

// helps rustdoc on non-supported platforms.
doc_prelude! {
    mod mock {
        pub(super) mod mio_net {
            pub type UnixListener = ();
        }
    }

    mod unix {
        pub(super) use mio::net as mio_net;
        // note: these are only used in the module, so we don't need mocks for them.
        pub(super) use std::os::unix::io::{FromRawFd, IntoRawFd};
    }
}

It generates something like this:

mod doc {
    #[cfg(any(not(doc), unix))]
    mod doc {
        // platform things.
    }

    #[cfg(all(doc, not(unix)))]
    mod doc {
        // mocked things.
    }

    use self::doc::*;
}

The approach is elaborated from the rustdoc advanced features guide, and the upshot is that rustdoc mostly only looks at function signatures. The content of the function is largely ignored, with a few exceptions:

  • function-local use statements, despite being said in the advanced guide to be ignored sometimes aren't!
  • rustdoc only seems to ingore use statements in os-specific modules if they're gated with a cfg flag like #[cfg(unix)], and are not composite (e.g. #[cfg(all(doc, unix))], or if the module is private. It's a bit of a hit and miss.

Note that as rustdoc improves, it is expected that these documentation preludes becomes smaller and smaller. E.g. it would make sense that rustdoc at some point also filters out non-public items which would remove the need to mock for example net::UnixListener above.

This does have a few downsides:

  • Anything outlined in the mock section won't correctly link to the correct upstream crate for types used unless it's built for that platform.
  • Trait implementations that mentions types in the mock section won't be visible in generated documentation unless it's built for that platform.

To actually get documentation generated for these types, I've provided the beforementioned platform-neutral tokio::os stubs, which includes the minimum level of std documentation that is referenced by Tokio.

@udoprog udoprog added T-docs Topic: documentation A-tokio Area: The main tokio crate labels May 9, 2021
@udoprog
Copy link
Contributor Author

udoprog commented May 9, 2021

Taking this for a spin in CI to get an idea of the things going wrong. And the first issue is that platform-specific doctest are still enabled.

I'd want to try and find a way to run doctests appropriately per-platform, since that's part of what's currently failing in #3760. And it might need to involve refactoring the doctest somehow which is a pretty big change, so I'm not sure if it's worth it.

Such a macro could look like this:

/// Helper for writing a unix-specific doctest.
#[macro_export]
#[doc(hidden)]
macro_rules! doctest_unix {
    ($($body:tt)*) => {
        #[cfg(unix)]
        #[tokio::main]
        async fn main() -> Result<(), Box<dyn ::std::error::Error>> { $($body)* }

        #[cfg(not(unix))]
        fn main() {}
    }
}

And used like so:

/// ```
/// # tokio::doctest_unix! {
/// use tokio::net::UnixDatagram;
///
/// // Create the pair of sockets
/// let (sock1, sock2) = UnixDatagram::pair()?;
///
/// // Since the sockets are paired, the paired send/recv
/// // functions can be used
/// let bytes = b"hello world";
/// sock1.send(bytes).await?;
///
/// let mut buff = vec![0u8; 24];
/// let size = sock2.recv(&mut buff).await?;
///
/// let dgram = &buff[..size];
/// assert_eq!(dgram, bytes);
/// # Ok(()) }
/// ```

@Darksonn
Copy link
Contributor

Darksonn commented May 9, 2021

See also #3275.

@udoprog
Copy link
Contributor Author

udoprog commented May 9, 2021

Assuming the build passes after 2904d74, this should now basically work.

It would be preferable if the doctest could somehow be selectively ignored, but I don't think that's possible without changes to rustdoc, so this now relies on the introduced doctest_unix! and doctest_windows! macros.

Meaning doctests that are only supposed to compile actual test code on a given platform needs to be written like this:

# tokio::doctest_unix! {
let tokio_listener = tokio::net::UnixListener::bind("127.0.0.1:0")?;
let std_listener = tokio_listener.into_std()?;
std_listener.set_nonblocking(false)?;
# Ok(()) }

On other platforms the macro still generates a stub fn main() {}, so it will be reported as passed even though it effectively does nothing.

$($items:tt)*
})*
) => {
#[cfg(all(doc, not(any($($($meta)*),*))))]
Copy link
Contributor

Choose a reason for hiding this comment

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

We might want to consider using the docsrs condition here instead, since it would make it work cross-crate. For example, the tokio-stream crate has something that depends on tokio::signal::windows, and it would be nice to have Tokio contain those as real items when building documentation for tokio-stream.

Comment on lines 2 to 12
#[macro_export]
macro_rules! doctest_unix {
($($body:tt)*) => {
#[cfg(unix)]
#[tokio::main]
async fn main() -> Result<(), Box<dyn ::std::error::Error>> { $($body)* }

#[cfg(not(unix))]
fn main() {}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this public? If so, can we gate it behind the docsrs thing?

Copy link
Contributor Author

@udoprog udoprog May 9, 2021

Choose a reason for hiding this comment

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

It is public, but it was supposed to be #[doc(hidden)] (will fix).

I'll mention gating behind --cfg docsrs briefly here: The unfortunate side effect of doing that would be that running any doc tests would require specifying docsrs (if I understand your proposal correctly). Cause this macro is used in all platform-specific doctests.

Copy link
Contributor Author

@udoprog udoprog May 9, 2021

Choose a reason for hiding this comment

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

Ah. I see what you mean now from looking at what you did in #3770. Hide the module completely unless docsrs is specified or it's the correct platform. If docsrs is enabled, do the stubbing.

This fixes doc testing, because it doesn't enable #[cfg(doc)].

I've incorporated that approach here now. So the macros are no longer needed.

Comment on lines 111 to 113
/// [`std::os::unix::net::UnixListener`]: std::os::unix::net::UnixListener
/// [`set_nonblocking`]: fn@std::os::unix::net::UnixListener::set_nonblocking
pub fn into_std(self) -> io::Result<std::os::unix::net::UnixListener> {
/// [`std::os::unix::net::UnixListener`]: crate::os::unix::net::UnixListener
/// [`set_nonblocking`]: fn@crate::os::unix::net::UnixListener::set_nonblocking
pub fn into_std(self) -> io::Result<net::UnixListener> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do these links work?

Copy link
Contributor Author

@udoprog udoprog May 9, 2021

Choose a reason for hiding this comment

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

Locally - yes. All though I really need to test this stuff on docs.rs to be 100% sure.

The only one which doesn't work is the one which references the signal_hook_registry crate directly, since that's a unix-conditional dependency:
image

Copy link
Contributor Author

@udoprog udoprog May 9, 2021

Choose a reason for hiding this comment

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

It is worth noting that if you build the documentation on windows where std::os::unix::net (etc...) aren't available, these would instead take you to the stub documentation.

Which is kinda... weird. But the alternative is to either live with broken links for the platform which the documentation is not being built on, or rewrite all of them to directly reference std docs through http links instead.

@Darksonn
Copy link
Contributor

Darksonn commented May 9, 2021

Can you fix tokio_stream::wrappers too to verify that this works cross-crate?

@udoprog
Copy link
Contributor Author

udoprog commented May 11, 2021

FWIW, this is ready as far as I can tell.

I've tried the changes across crates, and APIs do not build through rustdoc w/ docsrs enabled*, so the stream wrappers would have to be done separately from the tokio crate as far as I can tell. I'd be happy to do it in a separate PR since it's a bit more involved. It would require something like the setup I did in #3760 to reference types in the tokio crate.

*: You get things like this, which is not picked up if you build it only for the tokio crate (i.e. it doesn't look past the signature). All though it might be possible if we take a more extensive wrapping approach to documentation really hiding every platform internal detail:
image

@Darksonn
Copy link
Contributor

It if doesn't build with docsrs set, that's not great. Downstream crates may set it when building their documentation.

@udoprog
Copy link
Contributor Author

udoprog commented May 14, 2021

It if doesn't build with docsrs set, that's not great. Downstream crates may set it when building their documentation.

This should still be fine - because it's only building documentation for things which are imported. And imported things should already be platform gated on a downstream crate because it wouldn't compile otherwise. You get the above by naively changing cfg(windows) to something like cfg(any(windows, docsrs)) in tokio-stream hoping that the docsrs stubbing works transitively.

This means that if tokio-stream does add its own handling of docsrs like the tokio crate, it has to stub out its local imports. Alternatively we try a much more aggressive hiding strategy in tokio, but such a change would be much more intrusive (and I'm not even entirely sure it works) and can in my view safely wait until a future PR.

@Darksonn
Copy link
Contributor

I'm pretty sure that the approach in #3770 would work if used with tokio-stream. At least, a normal build compiles when using docsrs.

@udoprog
Copy link
Contributor Author

udoprog commented May 14, 2021

I'm pretty sure that the approach in #3770 would work if used with tokio-stream. At least, a normal build compiles when using docsrs.

It probably would and is what I'm hinting at. But it's also a fairly intrusive change for current and future windows-specific APIs. Everything needs to be stubbed through docsrs-specific modules.

Is that the route we want to take going forward?

@udoprog
Copy link
Contributor Author

udoprog commented May 14, 2021

Closed in favor of #3770

@udoprog udoprog closed this May 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate T-docs Topic: documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants