From 6f8bec93990434b5da5060f05d1e9fa7116bc743 Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Wed, 13 May 2020 00:21:54 -0400 Subject: [PATCH 1/3] Warn if linking to a private item --- src/librustdoc/lib.rs | 1 + .../passes/collect_intra_doc_links.rs | 30 +++++++++++++++++-- 2 files changed, 28 insertions(+), 3 deletions(-) diff --git a/src/librustdoc/lib.rs b/src/librustdoc/lib.rs index de6fa3dbd4a89..4961dc2d4bcea 100644 --- a/src/librustdoc/lib.rs +++ b/src/librustdoc/lib.rs @@ -34,6 +34,7 @@ extern crate rustc_metadata; extern crate rustc_middle; extern crate rustc_mir; extern crate rustc_parse; +extern crate rustc_privacy; extern crate rustc_resolve; extern crate rustc_session; extern crate rustc_span as rustc_span; diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index f5b2f1bb5b178..a19d3afc2e3b1 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -178,6 +178,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { let result = cx.enter_resolver(|resolver| { resolver.resolve_str_path_error(DUMMY_SP, &path_str, ns, module_id) }); + debug!("{} resolved to {:?} in namespace {:?}", path_str, ns, result); let result = match result { Ok((_, Res::Err)) => Err(ErrorKind::ResolutionFailure), _ => result.map_err(|_| ErrorKind::ResolutionFailure), @@ -202,7 +203,11 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { } return Ok((res, Some(path_str.to_owned()))); } - _ => return Ok((res, extra_fragment.clone())), + other => { + debug!("failed to resolve {} in namespace {:?} (got {:?})", path_str, ns, other); + debug!("extra_fragment is {:?}", extra_fragment); + return Ok((res, extra_fragment.clone())); + } }; if value != (ns == ValueNS) { @@ -555,9 +560,11 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> { } else { (parts[0].to_owned(), None) }; + let resolved_self; + let mut path_str; let (res, fragment) = { let mut kind = None; - let mut path_str = if let Some(prefix) = + path_str = if let Some(prefix) = ["struct@", "enum@", "type@", "trait@", "union@"] .iter() .find(|p| link.starts_with(**p)) @@ -614,7 +621,6 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> { let base_node = if item.is_mod() && item.attrs.inner_docs { None } else { parent_node }; - let resolved_self; // replace `Self` with suitable item's parent name if path_str.starts_with("Self::") { if let Some(ref name) = parent_name { @@ -760,6 +766,24 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> { if let Res::PrimTy(_) = res { item.attrs.links.push((ori_link, None, fragment)); } else { + // ~~WRONG: TODO: I think this happens too late and we need to instead put this in `self.resolve`~~ + debug!("item {:?} resolved to {:?}", item, res); + if let Some(local) = res.opt_def_id().and_then(|def_id| def_id.as_local()) { + let hir_id = self.cx.tcx.hir().as_local_hir_id(local); + if !self.cx.tcx.privacy_access_levels(rustc_hir::def_id::LOCAL_CRATE).is_exported(hir_id) { + let item_name = item.name.as_deref().unwrap_or(""); + build_diagnostic( + cx, + &item, + path_str, + &dox, + link_range, + &format!("public documentation for `{}` links to a private item", item_name), + "this item is private", + None, + ); + } + } let id = register_res(cx, res); item.attrs.links.push((ori_link, Some(id), fragment)); } From 20552c811a99a561f23b5918de9ffd95a06f815c Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Sat, 30 May 2020 11:35:35 -0400 Subject: [PATCH 2/3] Generate docs for links to private items when passed --document-private - Pass around document_private a lot more - Add tests + Add tests for intra-doc links to private items + Add ignored tests for warnings in reference links --- src/librustdoc/config.rs | 14 ++++------ src/librustdoc/core.rs | 15 +++++----- src/librustdoc/html/format.rs | 2 +- src/librustdoc/html/render.rs | 3 +- src/librustdoc/html/render/cache.rs | 6 ++++ src/librustdoc/lib.rs | 1 - .../passes/collect_intra_doc_links.rs | 28 +++++++++++++------ .../intra-links-private.public.stderr | 10 +++++++ src/test/rustdoc-ui/intra-links-private.rs | 10 +++++++ .../reference-link-has-one-warning.rs | 6 ++++ .../reference-link-has-one-warning.stderr | 10 +++++++ 11 files changed, 78 insertions(+), 27 deletions(-) create mode 100644 src/test/rustdoc-ui/intra-links-private.public.stderr create mode 100644 src/test/rustdoc-ui/intra-links-private.rs create mode 100644 src/test/rustdoc-ui/reference-link-has-one-warning.rs create mode 100644 src/test/rustdoc-ui/reference-link-has-one-warning.stderr diff --git a/src/librustdoc/config.rs b/src/librustdoc/config.rs index 5dbcc5c9ec8b9..35b15cf717cee 100644 --- a/src/librustdoc/config.rs +++ b/src/librustdoc/config.rs @@ -123,10 +123,6 @@ pub struct Options { /// /// Be aware: This option can come both from the CLI and from crate attributes! pub default_passes: DefaultPassOption, - /// Document items that have lower than `pub` visibility. - pub document_private: bool, - /// Document items that have `doc(hidden)`. - pub document_hidden: bool, /// Any passes manually selected by the user. /// /// Be aware: This option can come both from the CLI and from crate attributes! @@ -177,8 +173,6 @@ impl fmt::Debug for Options { .field("test_args", &self.test_args) .field("persist_doctests", &self.persist_doctests) .field("default_passes", &self.default_passes) - .field("document_private", &self.document_private) - .field("document_hidden", &self.document_hidden) .field("manual_passes", &self.manual_passes) .field("display_warnings", &self.display_warnings) .field("show_coverage", &self.show_coverage) @@ -250,6 +244,10 @@ pub struct RenderOptions { pub generate_search_filter: bool, /// Option (disabled by default) to generate files used by RLS and some other tools. pub generate_redirect_pages: bool, + /// Document items that have lower than `pub` visibility. + pub document_private: bool, + /// Document items that have `doc(hidden)`. + pub document_hidden: bool, } impl Options { @@ -567,8 +565,6 @@ impl Options { should_test, test_args, default_passes, - document_private, - document_hidden, manual_passes, display_warnings, show_coverage, @@ -597,6 +593,8 @@ impl Options { markdown_playground_url, generate_search_filter, generate_redirect_pages, + document_private, + document_hidden, }, output_format, }) diff --git a/src/librustdoc/core.rs b/src/librustdoc/core.rs index 1690b946bb625..8ab6c74289d17 100644 --- a/src/librustdoc/core.rs +++ b/src/librustdoc/core.rs @@ -62,6 +62,8 @@ pub struct DocContext<'tcx> { // FIXME(eddyb) make this a `ty::TraitRef<'tcx>` set. pub generated_synthetics: RefCell, DefId)>>, pub auto_traits: Vec, + /// The options given to rustdoc that could be relevant to a pass. + pub render_options: RenderOptions, } impl<'tcx> DocContext<'tcx> { @@ -281,8 +283,6 @@ pub fn run_core(options: RustdocOptions) -> (clean::Crate, RenderInfo, RenderOpt describe_lints, lint_cap, mut default_passes, - mut document_private, - document_hidden, mut manual_passes, display_warnings, render_options, @@ -448,6 +448,7 @@ pub fn run_core(options: RustdocOptions) -> (clean::Crate, RenderInfo, RenderOpt .cloned() .filter(|trait_def_id| tcx.trait_is_auto(*trait_def_id)) .collect(), + render_options, }; debug!("crate: {:?}", tcx.hir().krate()); @@ -524,7 +525,7 @@ pub fn run_core(options: RustdocOptions) -> (clean::Crate, RenderInfo, RenderOpt } if attr.is_word() && name == sym::document_private_items { - document_private = true; + ctxt.render_options.document_private = true; } } @@ -544,9 +545,9 @@ pub fn run_core(options: RustdocOptions) -> (clean::Crate, RenderInfo, RenderOpt for p in passes { let run = match p.condition { Always => true, - WhenDocumentPrivate => document_private, - WhenNotDocumentPrivate => !document_private, - WhenNotDocumentHidden => !document_hidden, + WhenDocumentPrivate => ctxt.render_options.document_private, + WhenNotDocumentPrivate => !ctxt.render_options.document_private, + WhenNotDocumentHidden => !ctxt.render_options.document_hidden, }; if run { debug!("running pass {}", p.pass.name); @@ -556,7 +557,7 @@ pub fn run_core(options: RustdocOptions) -> (clean::Crate, RenderInfo, RenderOpt ctxt.sess().abort_if_errors(); - (krate, ctxt.renderinfo.into_inner(), render_options) + (krate, ctxt.renderinfo.into_inner(), ctxt.render_options) }) }) }) diff --git a/src/librustdoc/html/format.rs b/src/librustdoc/html/format.rs index e60ff37fd279a..a453a8b3dcb2a 100644 --- a/src/librustdoc/html/format.rs +++ b/src/librustdoc/html/format.rs @@ -468,7 +468,7 @@ impl clean::Path { pub fn href(did: DefId) -> Option<(String, ItemType, Vec)> { let cache = cache(); - if !did.is_local() && !cache.access_levels.is_public(did) { + if !did.is_local() && !cache.access_levels.is_public(did) && !cache.document_private { return None; } diff --git a/src/librustdoc/html/render.rs b/src/librustdoc/html/render.rs index 1681b73d0c257..04c4685213b2e 100644 --- a/src/librustdoc/html/render.rs +++ b/src/librustdoc/html/render.rs @@ -469,6 +469,7 @@ pub fn run( static_root_path, generate_search_filter, generate_redirect_pages, + document_private, .. } = options; @@ -546,7 +547,7 @@ pub fn run( scx.ensure_dir(&dst)?; krate = sources::render(&dst, &mut scx, krate)?; let (new_crate, index, cache) = - Cache::from_krate(renderinfo, &extern_html_root_urls, &dst, krate); + Cache::from_krate(renderinfo, document_private, &extern_html_root_urls, &dst, krate); krate = new_crate; let cache = Arc::new(cache); let mut cx = Context { diff --git a/src/librustdoc/html/render/cache.rs b/src/librustdoc/html/render/cache.rs index 225940773413e..1b5c8a9378e41 100644 --- a/src/librustdoc/html/render/cache.rs +++ b/src/librustdoc/html/render/cache.rs @@ -91,6 +91,10 @@ crate struct Cache { /// The version of the crate being documented, if given from the `--crate-version` flag. pub crate_version: Option, + /// Whether to document private items. + /// This is stored in `Cache` so it doesn't need to be passed through all rustdoc functions. + pub document_private: bool, + // Private fields only used when initially crawling a crate to build a cache stack: Vec, parent_stack: Vec, @@ -126,6 +130,7 @@ crate struct Cache { impl Cache { pub fn from_krate( renderinfo: RenderInfo, + document_private: bool, extern_html_root_urls: &BTreeMap, dst: &Path, mut krate: clean::Crate, @@ -160,6 +165,7 @@ impl Cache { stripped_mod: false, access_levels, crate_version: krate.version.take(), + document_private, orphan_impl_items: Vec::new(), orphan_trait_impls: Vec::new(), traits: krate.external_traits.replace(Default::default()), diff --git a/src/librustdoc/lib.rs b/src/librustdoc/lib.rs index 4961dc2d4bcea..de6fa3dbd4a89 100644 --- a/src/librustdoc/lib.rs +++ b/src/librustdoc/lib.rs @@ -34,7 +34,6 @@ extern crate rustc_metadata; extern crate rustc_middle; extern crate rustc_mir; extern crate rustc_parse; -extern crate rustc_privacy; extern crate rustc_resolve; extern crate rustc_session; extern crate rustc_span as rustc_span; diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index a19d3afc2e3b1..e4fe8996a233f 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -204,7 +204,10 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { return Ok((res, Some(path_str.to_owned()))); } other => { - debug!("failed to resolve {} in namespace {:?} (got {:?})", path_str, ns, other); + debug!( + "failed to resolve {} in namespace {:?} (got {:?})", + path_str, ns, other + ); debug!("extra_fragment is {:?}", extra_fragment); return Ok((res, extra_fragment.clone())); } @@ -564,10 +567,9 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> { let mut path_str; let (res, fragment) = { let mut kind = None; - path_str = if let Some(prefix) = - ["struct@", "enum@", "type@", "trait@", "union@"] - .iter() - .find(|p| link.starts_with(**p)) + path_str = if let Some(prefix) = ["struct@", "enum@", "type@", "trait@", "union@"] + .iter() + .find(|p| link.starts_with(**p)) { kind = Some(TypeNS); link.trim_start_matches(prefix) @@ -766,22 +768,30 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> { if let Res::PrimTy(_) = res { item.attrs.links.push((ori_link, None, fragment)); } else { - // ~~WRONG: TODO: I think this happens too late and we need to instead put this in `self.resolve`~~ - debug!("item {:?} resolved to {:?}", item, res); + debug!("linked item {} resolved to {:?}", path_str, res); if let Some(local) = res.opt_def_id().and_then(|def_id| def_id.as_local()) { + use rustc_hir::def_id::LOCAL_CRATE; + let hir_id = self.cx.tcx.hir().as_local_hir_id(local); - if !self.cx.tcx.privacy_access_levels(rustc_hir::def_id::LOCAL_CRATE).is_exported(hir_id) { + if !self.cx.tcx.privacy_access_levels(LOCAL_CRATE).is_exported(hir_id) + && !self.cx.render_options.document_private + { let item_name = item.name.as_deref().unwrap_or(""); + let err_msg = format!( + "public documentation for `{}` links to a private item", + item_name + ); build_diagnostic( cx, &item, path_str, &dox, link_range, - &format!("public documentation for `{}` links to a private item", item_name), + &err_msg, "this item is private", None, ); + continue; } } let id = register_res(cx, res); diff --git a/src/test/rustdoc-ui/intra-links-private.public.stderr b/src/test/rustdoc-ui/intra-links-private.public.stderr new file mode 100644 index 0000000000000..0a8dafdaf9466 --- /dev/null +++ b/src/test/rustdoc-ui/intra-links-private.public.stderr @@ -0,0 +1,10 @@ +warning: `[DontDocMe]` public documentation for `DocMe` links to a private item + --> $DIR/intra-links-private.rs:6:11 + | +LL | /// docs [DontDocMe] + | ^^^^^^^^^ this item is private + | + = note: `#[warn(intra_doc_link_resolution_failure)]` on by default + +warning: 1 warning emitted + diff --git a/src/test/rustdoc-ui/intra-links-private.rs b/src/test/rustdoc-ui/intra-links-private.rs new file mode 100644 index 0000000000000..b7906aba5b1a9 --- /dev/null +++ b/src/test/rustdoc-ui/intra-links-private.rs @@ -0,0 +1,10 @@ +// check-pass +// revisions: public private +// [private]compile-flags: --document-private-items +#![cfg_attr(private, deny(intra_doc_resolution_failure))] + +/// docs [DontDocMe] +//[public]~^ WARNING `[DontDocMe]` public documentation for `DocMe` links to a private item +// FIXME: for [private] we should also make sure the link was actually generated +pub struct DocMe; +struct DontDocMe; diff --git a/src/test/rustdoc-ui/reference-link-has-one-warning.rs b/src/test/rustdoc-ui/reference-link-has-one-warning.rs new file mode 100644 index 0000000000000..21cb7eb9040bd --- /dev/null +++ b/src/test/rustdoc-ui/reference-link-has-one-warning.rs @@ -0,0 +1,6 @@ +// ignore-test +// check-pass + +/// docs [label][with#anchor#error] +//~^ WARNING has an issue with the link anchor +pub struct S; diff --git a/src/test/rustdoc-ui/reference-link-has-one-warning.stderr b/src/test/rustdoc-ui/reference-link-has-one-warning.stderr new file mode 100644 index 0000000000000..5bbc62b76dd04 --- /dev/null +++ b/src/test/rustdoc-ui/reference-link-has-one-warning.stderr @@ -0,0 +1,10 @@ +warning: `[with#anchor#error]` has an issue with the link anchor. + --> $DIR/reference-link-has-one-warning.rs:3:18 + | +LL | /// docs [label][with#anchor#error] + | ^^^^^^^^^^^^^^^^^ only one `#` is allowed in a link + | + = note: `#[warn(intra_doc_link_resolution_failure)]` on by default + +warning: 1 warning emitted + From 67423821aa0dd705720c9e183d04d4b7a55b723f Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Fri, 26 Jun 2020 08:24:45 -0400 Subject: [PATCH 3/3] Fix debug messages --- src/librustdoc/passes/collect_intra_doc_links.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index e4fe8996a233f..8da74f375d9ce 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -178,7 +178,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { let result = cx.enter_resolver(|resolver| { resolver.resolve_str_path_error(DUMMY_SP, &path_str, ns, module_id) }); - debug!("{} resolved to {:?} in namespace {:?}", path_str, ns, result); + debug!("{} resolved to {:?} in namespace {:?}", path_str, result, ns); let result = match result { Ok((_, Res::Err)) => Err(ErrorKind::ResolutionFailure), _ => result.map_err(|_| ErrorKind::ResolutionFailure), @@ -208,7 +208,6 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { "failed to resolve {} in namespace {:?} (got {:?})", path_str, ns, other ); - debug!("extra_fragment is {:?}", extra_fragment); return Ok((res, extra_fragment.clone())); } }; @@ -768,7 +767,7 @@ impl<'a, 'tcx> DocFolder for LinkCollector<'a, 'tcx> { if let Res::PrimTy(_) = res { item.attrs.links.push((ori_link, None, fragment)); } else { - debug!("linked item {} resolved to {:?}", path_str, res); + debug!("intra-doc link to {} resolved to {:?}", path_str, res); if let Some(local) = res.opt_def_id().and_then(|def_id| def_id.as_local()) { use rustc_hir::def_id::LOCAL_CRATE;