rustdoc: render problems for re-exported implementors

[jsha]

On both stable and nightly, the "Implementors" section of a trait page incorrectly renders re-exported types. For instance, on https://doc.rust-lang.org/std/marker/trait.Sync.html, there are two entries for LinkedList.

std::collections::LinkedList

alloc::collections::LinkedList

These are different in the way they are displayed: for instance, the where clause is displayed differently even though these are the same type. Also, the alloc version is missing its [src] link.

Looking at the HTML, the alloc version looks like this (I included some of the HTML of the previous, correct element for comparison):

Some issues here:

• There's no need for a table layout
• This should have id=impl-Sync-51
• There's a <code> tag inside another <code> tag, which is unnecessary.

[jyn514]

@jsha does this also happen for re-exports in the same crate? or only across crates?

[jsha]

I'm not sure, sorry!

[jsha]

It turns out the implementors that are rendered differently are inserted by JavaScript here:

rust/src/librustdoc/html/static/main.js

Lines 661 to 725 in 3f9646d

	window.register_implementors = function(imp) {
	var implementors = document.getElementById("implementors-list");
	var synthetic_implementors = document.getElementById("synthetic-implementors-list");
	if (synthetic_implementors) {
	// This `inlined_types` variable is used to avoid having the same implementation
	// showing up twice. For example "String" in the "Sync" doc page.
	//
	// By the way, this is only used by and useful for traits implemented automatically
	// (like "Send" and "Sync").
	var inlined_types = new Set();
	onEachLazy(synthetic_implementors.getElementsByClassName("impl"), function(el) {
	var aliases = el.getAttribute("data-aliases");
	if (!aliases) {
	return;
	}
	aliases.split(",").forEach(function(alias) {
	inlined_types.add(alias);
	});
	});
	}
	var libs = Object.getOwnPropertyNames(imp);
	for (var i = 0, llength = libs.length; i < llength; ++i) {
	if (libs[i] === window.currentCrate) { continue; }
	var structs = imp[libs[i]];
	struct_loop:
	for (var j = 0, slength = structs.length; j < slength; ++j) {
	var struct = structs[j];
	var list = struct.synthetic ? synthetic_implementors : implementors;
	if (struct.synthetic) {
	for (var k = 0, stlength = struct.types.length; k < stlength; k++) {
	if (inlined_types.has(struct.types[k])) {
	continue struct_loop;
	}
	inlined_types.add(struct.types[k]);
	}
	}
	var code = document.createElement("code");
	code.innerHTML = struct.text;
	onEachLazy(code.getElementsByTagName("a"), function(elem) {
	var href = elem.getAttribute("href");
	if (href && href.indexOf("http") !== 0) {
	elem.setAttribute("href", window.rootPath + href);
	}
	});
	var display = document.createElement("h3");
	addClass(display, "impl");
	display.innerHTML = "<span class=\"in-band\"><table class=\"table-display\">" +
	"<tbody><tr><td><code>" + code.outerHTML + "</code></td><td></td></tr>" +
	"</tbody></table></span>";
	list.appendChild(display);
	}
	}
	};
	if (window.pending_implementors) {
	window.register_implementors(window.pending_implementors);
	}

That pulls in a list of implementors from a .js file, which is generated by the Rust code here:

rust/src/librustdoc/html/render/write_shared.rs

Lines 541 to 552 in 3f9646d

	let mut v = String::from("(function() {var implementors = {};\n");
	for implementor in &all_implementors {
	writeln!(v, "{}", *implementor).unwrap();
	}
	v.push_str(
	"if (window.register_implementors) {\
	window.register_implementors(implementors);\
	} else {\
	window.pending_implementors = implementors;\
	}",
	);
	v.push_str("})()");

I haven't figured out what triggers the HTML page to load that extra JS.

[jsha]

Here's the code that adds the script tag to load the extra JS:

rust/src/librustdoc/html/render/print_item.rs

Lines 755 to 769 in 3f9646d

	write!(
	w,
	"<script type=\"text/javascript\" \
	src=\"{root_path}/implementors/{path}/{ty}.{name}.js\" async>\
	</script>",
	root_path = vec![".."; cx.current.len()].join("/"),
	path = if it.def_id.is_local() {
	cx.current.join("/")
	} else {
	let (ref path, _) = cx.cache.external_paths[&it.def_id.expect_real()];
	path[..path.len() - 1].join("/")
	},
	ty = it.type_(),
	name = *it.name.as_ref().unwrap()
	);

[jsha]

So, a summary of the problem here:

For in-crate implementors, rustdoc renders HTML directly. For out-of-crate implementors, rustdoc renders HTML into a string in a JS file. That means we have two different code paths putting the implementors into the HTML page so they're likely to drift apart.

This also means that out-of-crate implementors can't be sorted alongside the in-crate implementors.

The ideal would be for the out-of-crate implementors to be rendered directly into the HTML (and sorted along with all the in-crate implementors). I think that's not really practical, because as I understand it, the out-of-crate implementors can be generated by separate invocations of rustdoc.

One hack to improve the incorrect sorting would be to have a separate heading for out-of-crate implementors. But that doesn't address the "likely to drift apart" problem.

[jsha]

Relatedly, there is sometimes a section for "Implementations on Foreign Types" (e.g. https://doc.rust-lang.org/nightly/std/cmp/trait.Eq.html#foreign-impls). It's not clear to me what causes an impl to wind up in "Implementations on Foreign Types" vs appended to the Implementors section by way of JS. Maybe it makes sense for the JS-provided impls to wind up under that section?

Also we should probably define "Foreign Types." AFAICT by searching Google and The Book, this is not defined in commonly used Rust materials.

[jyn514]

"foreign types" is common in the compiler, but maybe not in the documentation. It means types from another crate; I see it used a lot in explanations of the orphan rules.

Yes, I agree putting everything in "Foreign Types" instead of JS is a better approach. I don't know enough about the code to help, sorry.

[jsha]

Unfortunately, this isn't an alternative to loading the implementations from JS, it's just an informational-organization thing.

For a trait in crate foo, there are two types of implementations on foreign types (I figured this out since my last post):

• implementations inside crate foo (usually in the same file as the trait is defined). These are currently rendered into "Implementations on Foreign Types" during the pass that renders foo.
• implementations in the foreign crates themselves. These are generated into JS when those crates are rendered. Currently, main.js will insert them under the general "Implementations" heading, but they should probably go under "Implementations on Foreign Types" for consistency.

[jyn514]

I don't think those are the same case. "Implementation on Foreign Types" is an implementation by the trait author on types from other crates". "Implementations" is an implementation on a new downstream crate in the same crate that the type is defined. "Foreign" here is foreign from the perspective of the impl, not the trait - it matters for coherence.

[jsha]

Right, they're not the same case from the trait author's perspective (or the struct/enum author's perspective). But from the perspective of a user reading the docs for trait Foo, is it different? As a concrete example, say trait colors::Green has a foreign implementation on fruits::Apple, and also vegetables::Celery has a local implementation of Green. If I'm on Green's page, looking for things that implement it, how does the difference between Apple and Celery matter to me?

[jsha]

That said, the problem of sorting would actually be much easier to solve by further subdividing things, so we'd have:

Implementations in this Crate on Types in this Crate
Implementations in this Crate on Types in Other Crates
Implementations in Other Crates

Then each section could be sorted individually. The "Implementations in Other Crates" section would come purely from JS (/JSON) and could be sorted in JS.

[jsha]

Related: #86632