rustdoc: pub use for macros is inlined too eagerly

[petrochenkov]

pub use for macros is "inlined" (in rustdoc sense) too eagerly and may show duplicated documentation (both inlined and non-inlined) and ignore #[doc(no_inline)] and apparently #[doc(hidden)] (#35896 (comment)) as well.

This can now be observed on nightly docs for the standard library - https://doc.rust-lang.org/nightly/std/.
The "Re-exports" section wasn't there previously and everything was in inlined-only form.

[alexcrichton]

Looks like this affects beta as well - https://doc.rust-lang.org/beta/std/#reexports

[pietroalbini]

cc @rust-lang/rustdoc

[GuillaumeGomez]

Ah, indeed. I'll take a look as soon as possible.

[pietroalbini]

@GuillaumeGomez what's the status on this?

[GuillaumeGomez]

Still on my todo list.

[QuietMisdreavus]

Some initial poking:

Re-exported macros are currently pulled in during RustdocVisitor::visit_mod_contents, and they currently don't handle doc(no_inline) or doc(hidden):

rust/src/librustdoc/visit_ast.rs

Lines 219 to 252 in 1962a70

	if let Some(exports) = self.cx.tcx.module_exports(def_id) {
	for export in exports.iter().filter(|e| e.vis == Visibility::Public) {
	if let Def::Macro(def_id, ..) = export.def {
	if def_id.krate == LOCAL_CRATE {
	continue // These are `krate.exported_macros`, handled in `self.visit()`.
	}
	let imported_from = self.cx.tcx.original_crate_name(def_id.krate);
	let def = match self.cstore.load_macro_untracked(def_id, self.cx.sess()) {
	LoadedMacro::MacroDef(macro_def) => macro_def,
	// FIXME(jseyfried): document proc macro re-exports
	LoadedMacro::ProcMacro(..) => continue,
	};
	let matchers = if let ast::ItemKind::MacroDef(ref def) = def.node {
	let tts: Vec<_> = def.stream().into_trees().collect();
	tts.chunks(4).map(|arm| arm[0].span()).collect()
	} else {
	unreachable!()
	};
	om.macros.push(Macro {
	def_id,
	attrs: def.attrs.clone().into(),
	name: def.ident.name,
	whence: def.span,
	matchers,
	stab: self.stability(def.id),
	depr: self.deprecation(def.id),
	imported_from: Some(imported_from),
	})
	}
	}
	}

The actual export statement is handled in visit_item, which processes through maybe_inline_local and adds the export statement to the module for processing:

rust/src/librustdoc/visit_ast.rs

Lines 409 to 443 in 1962a70

	hir::ItemUse(ref path, kind) => {
	let is_glob = kind == hir::UseKind::Glob;
	// If there was a private module in the current path then don't bother inlining
	// anything as it will probably be stripped anyway.
	if item.vis == hir::Public && self.inside_public_path {
	let please_inline = item.attrs.iter().any(|item| {
	match item.meta_item_list() {
	Some(ref list) if item.check_name("doc") => {
	list.iter().any(|i| i.check_name("inline"))
	}
	_ => false,
	}
	});
	let name = if is_glob { None } else { Some(name) };
	if self.maybe_inline_local(item.id,
	path.def,
	name,
	is_glob,
	om,
	please_inline) {
	return;
	}
	}
	om.imports.push(Import {
	name,
	id: item.id,
	vis: item.vis.clone(),
	attrs: item.attrs.clone(),
	path: (**path).clone(),
	glob: is_glob,
	whence: item.span,
	});
	}

Notably, this means that any doc(hidden) on the export statement will hide this export rather than the macro itself.

A "quick fix" for this would be to check the import statement for doc(no_inline) or doc(hidden) where the import is being processed, but i'm not convinced that this is the best place to process these imports anyway. The doc comment (and name) on maybe_inline_local indicates that only local re-exports are meant to go through that function, but i haven't yet found where cross-crate inlining properly occurs. (Or even whether this statement is incorrect! That comment was added 2 years ago, according to git blame.) The "proper" fix would be to move this processing to that location so we can re-use the "should we inline/expose this item" logic there.

[QuietMisdreavus]

While finishing that comment, i realized that cross-crate inlining happens in clean/inline.rs. A clause would need to be added to try_inline to catch macros that fall through there, and the import process can be moved from visit_ast::RustdocVisitor::visit_mod_contents to a new function clean::inline::build_macro or the like.

[ollie27]

The problem with moving the macro handling code to try_inline is that is doesn't handle re-exports from multiple namespaces (#34843).

rust/src/librustc_resolve/resolve_imports.rs

Lines 916 to 921 in 1962a70

	// Record what this import resolves to for later uses in documentation,
	// this may resolve to either a value or a type, but for documentation
	// purposes it's good enough to just favor one over the other.
	self.per_ns(|this, ns| if let Some(binding) = result[ns].get().ok() {
	this.def_map.entry(directive.id).or_insert(PathResolution::new(binding.def()));
	});

If it wasn't for std::vec we could get away with it for now.

[QuietMisdreavus]

I'm confused. Does tcx.module_exports (called in visit_mod_contents to grab the macro exports) do something different then? If it can find the vec! macro that way, but still finds the module through the regular alloc::vec module export from its export statement (when creating the module item in the std docs), how does that come into play? If tcx.module_exports can get all the namespaces of an import and create distinct Defs for them all, why aren't we using that?

[QuietMisdreavus]

Based on a suggestion from @ollie27 on IRC, i've opened #51011 which hides the re-export statements for macros. It doesn't do anything for the root issue, but it's a small change and cleans up the docs.

[QuietMisdreavus]

Now that #51425 is merged, that paves the way for #51611 to import macros alongside all other cross-crate re-exports, and finally close this issue.