Skip to content

rustdoc: pub use for macros is inlined too eagerly #50647

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
petrochenkov opened this issue May 11, 2018 · 11 comments
Closed

rustdoc: pub use for macros is inlined too eagerly #50647

petrochenkov opened this issue May 11, 2018 · 11 comments
Assignees
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Comments

@petrochenkov
Copy link
Contributor

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.

@petrochenkov petrochenkov added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. labels May 11, 2018
@alexcrichton alexcrichton added this to the 1.28 milestone May 11, 2018
@alexcrichton alexcrichton added regression-from-stable-to-beta Performance or correctness regression from stable to beta. and removed regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. labels May 11, 2018
@alexcrichton alexcrichton modified the milestones: 1.28, 1.27 May 11, 2018
@alexcrichton
Copy link
Member

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

@pietroalbini
Copy link
Member

cc @rust-lang/rustdoc

@GuillaumeGomez
Copy link
Member

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

@GuillaumeGomez GuillaumeGomez self-assigned this May 15, 2018
@GuillaumeGomez GuillaumeGomez added the A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools label May 15, 2018
@jkordish jkordish added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label May 17, 2018
@pietroalbini
Copy link
Member

@GuillaumeGomez what's the status on this?

@GuillaumeGomez
Copy link
Member

Still on my todo list.

@QuietMisdreavus
Copy link
Member

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

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:

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
Copy link
Member

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
Copy link
Member

ollie27 commented May 23, 2018

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

// 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
Copy link
Member

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
Copy link
Member

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.

kennytm added a commit to kennytm/rust that referenced this issue May 24, 2018
… r=ollie27

 rustdoc: hide macro export statements from docs

As mentioned in rust-lang#50647, rustdoc now prints both the import statement and the macro itself when re-exporting macros. This is a stopgap solution to clean up the std docs and get something small backported into beta.

What this does: When rustdoc finds an export statement for a macro, instead of printing the export and bailing, now it will instead hide the export and bail. Until we can solve rust-lang#34843 or have a better way to find the attributes on an export statement when inlining macros, this will at least match the current behavior and clean up the re-export statements from the docs.
@steveklabnik steveklabnik removed the A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools label May 28, 2018
@QuietMisdreavus QuietMisdreavus self-assigned this May 29, 2018
@QuietMisdreavus
Copy link
Member

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.

@pietroalbini pietroalbini added regression-from-stable-to-stable Performance or correctness regression from one stable version to another. and removed regression-from-stable-to-beta Performance or correctness regression from stable to beta. labels Jun 20, 2018
@pietroalbini pietroalbini removed this from the 1.27 milestone Jun 20, 2018
bors added a commit that referenced this issue Jul 4, 2018
rustdoc: import cross-crate macros alongside everything else

The thrilling conclusion of the cross-crate macro saga in rustdoc! After #51425 made sure we saw all the namespaces of an import (and prevented us from losing the `vec!` macro in std's documentation), here is the PR to handle cross-crate macro re-exports at the same time as everything else. This way, attributes like `#[doc(hidden)]` and `#[doc(no_inline)]` can be used to control how the documentation for these macros is seen, rather than rustdoc inlining every macro every time.

Fixes #50647
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

8 participants