Documentation fails to build

[jonhoo]

Hey there!

Just a heads up that amadeus-parquet 0.1.7, and older versions of some other amadeus- crates (like amadeus-postgres 0.1.5) are affected by https://github.com/rust-lang/rust/issues/65863, and their documentation do not build with cargo doc. See for example https://docs.rs/crate/amadeus-parquet/0.1.7. Not much to do about it at the moment (except by trying to fix the compiler), just trying to bring attention to the problem :)

[alecmocatta]

Hey @jonhoo, thanks for the issue!

I worked around this where it wasn't too inconvient by doing this: https://github.com/constellation-rs/amadeus/blob/master/amadeus-postgres/src/lib.rs#L171-L174 I don't know if that's an option for noria (if you haven't already worked around it)? I haven't done it for amadeus-parquet as there are so many usages of type_alias_impl_trait and to be honest I'd rather hoped someone with more insight into rustdoc than myself would have fixed it in the meantime!

[jonhoo]

Hehe, no, unfortunately noria is in the same position as amadeus-parquet. Or rather, it's not that there are so many impls, but rather than they'd be very difficult (if not impossible) to actually name :'(

[alecmocatta]

The workaround doesn't actually necessitate naming the concrete type. It just involves creating a dummy struct that implements the desired trait, and naming that instead iff building documentation.

Like:

https://github.com/constellation-rs/amadeus/blob/790c1d105e26f3adb2135b953041533821fd582f/amadeus-postgres/src/lib.rs#L171-L174

https://github.com/constellation-rs/amadeus/blob/790c1d105e26f3adb2135b953041533821fd582f/amadeus-core/src/util.rs#L65-L84

[jonhoo]

Ohh, I see, interesting.. Yeah, that could maybe work. Thanks for the pointer!

[jonhoo]

Hmm, unfortunately this trick only really works one crate deep. If I have two crates, both with type = impl Trait, then the lower crate is built without the feature (or, in my case, I use #[cfg(doc)]), and it ends up hitting this assertion and crashing the compiler. But I'll see if there's a way for me to patch my way around that.

[jonhoo]

I've filed that as https://github.com/rust-lang/rust/issues/73061 — fun times on nightly :sweat_smile:

[alecmocatta]

Are you seeing #[cfg(doc)] being passed to dependencies? I think I tested and found it wasn't but that could have changed.

I think if you control (or can fork) the dependencies then you can use #[cfg(feature = "doc")] and do this to ensure it's passed to the dependency: https://github.com/constellation-rs/amadeus/blob/790c1d105e26f3adb2135b953041533821fd582f/Cargo.toml#L34

[jonhoo]

No, they are not passed to dependencies, which is exactly why it breaks. It's true that you could work around this with features, but I'd rather not add a feature just for building docs, because it becomes infectious — every downstream crate would need to also enable that feature for their docs, all the way down. This shouldn't be necessary.

It's interesting, because there are two different issues at play here. The first is that cargo doc tries to be "smart" when it compiles a given crate's documentation by replacing that crate's function bodys with empty loops. That's what causes the original problem. But that only applies to the crate that documentation is being generated for. If b depends on a, and documentation is being generated for b, then a will not have that problematic replacement applied to it, so everything should be fine as long as b handles cfg(doc) correctly on its own. That's where https://github.com/rust-lang/rust/issues/73061 comes in. It makes it so that cargo doc cannot handle impl Trait even when the body is known. Once that is fixed, it should be sufficient to just use cfg(doc) even though it doesn't apply to dependencies (again, because dependencies don't have the problematic substitution applied to them).

[alecmocatta]

every downstream crate would need to also enable that feature for their docs, all the way down. This shouldn't be necessary.

That's a good point and I agree. Thanks for surfacing the problem, hopefully it'll attract some attention and failing that I can have a go at fixing it later in the year.

[jonhoo]

Looks like https://github.com/rust-lang/rust/pull/72080 is the PR to watch. It won't fix the issue that requires cfg(doc) in the first place, but it will fix the dependency problem, making you only need cfg(doc).

[jonhoo]

Heads up that https://github.com/rust-lang/rust/issues/73061 just landed, so starting in the next nightly you should be all good using just cfg(doc) to work around this on a crate-by-crate basis.

[jonhoo]

Now that https://github.com/rust-lang/rust/pull/73566 has landed, even the cfg(doc) workaround shouldn't be needed any more on newer nightlies!

[alecmocatta]

Thanks for the heads up! Always feels good to remove a long-standing workaround :) Will be closed by #97