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 rust-lang/rust#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:

amadeus/amadeus-postgres/src/lib.rs

Lines 171 to 174 in 790c1d1

	#[cfg(not(feature = "doc"))]
	type DistIter = impl DistributedIterator<Item = Result<Self::Item, Self::Error>>;
	#[cfg(feature = "doc")]
	type DistIter = amadeus_core::util::ImplDistributedIterator<Result<Self::Item, Self::Error>>;

amadeus/amadeus-core/src/util.rs

Lines 65 to 84 in 790c1d1

	pub struct ImplDistributedIterator<T>(PhantomData<fn(T)>);
	impl<T> ImplDistributedIterator<T> {
	pub fn new<U>(_drop: U) -> Self
	where
	U: DistributedIterator<Item = T>,
	{
	Self(PhantomData)
	}
	}
	impl<T: 'static> DistributedIterator for ImplDistributedIterator<T> {
	type Item = T;
	type Task = ImplConsumer<T>;
	fn size_hint(&self) -> (usize, Option<usize>) {
	unreachable!()
	}
	fn next_task(&mut self) -> Option<Self::Task> {
	unreachable!()
	}
	}

[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 rust-lang/rust#73061 — fun times on nightly 😅

[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:

amadeus/Cargo.toml

Line 34 in 790c1d1

doc = ["amadeus-aws/doc", "amadeus-commoncrawl/doc", "amadeus-postgres/doc", "amadeus-serde/doc"] # "amadeus-parquet/doc" when https://github.com/rust-lang/cargo/issues/3494 is resolved

[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 rust-lang/rust#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 rust-lang/rust#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 rust-lang/rust#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 rust-lang/rust#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