From 805d5f91cb41a3178e77c9508bb1441b9ee7beaf Mon Sep 17 00:00:00 2001 From: QuietMisdreavus Date: Thu, 20 Sep 2018 21:21:18 -0500 Subject: [PATCH 1/5] don't run doctests on private items --- src/librustdoc/lib.rs | 4 ++- src/librustdoc/test.rs | 75 ++++++++++++++++++++++++++++++++++++------ 2 files changed, 68 insertions(+), 11 deletions(-) diff --git a/src/librustdoc/lib.rs b/src/librustdoc/lib.rs index 5607c97a49689..9fefaf16efc65 100644 --- a/src/librustdoc/lib.rs +++ b/src/librustdoc/lib.rs @@ -532,6 +532,7 @@ fn main_args(args: &[String]) -> isize { let sort_modules_alphabetically = !matches.opt_present("sort-modules-by-appearance"); let resource_suffix = matches.opt_str("resource-suffix"); let enable_minification = !matches.opt_present("disable-minification"); + let document_private_items = matches.opt_present("document-private-items"); let edition = matches.opt_str("edition").unwrap_or("2015".to_string()); let edition = match edition.parse() { @@ -551,7 +552,8 @@ fn main_args(args: &[String]) -> isize { } (true, false) => { return test::run(Path::new(input), cfgs, libs, externs, test_args, crate_name, - maybe_sysroot, display_warnings, linker, edition, cg) + maybe_sysroot, display_warnings, linker, edition, cg, + document_private_items) } (false, true) => return markdown::render(Path::new(input), output.unwrap_or(PathBuf::from("doc")), diff --git a/src/librustdoc/test.rs b/src/librustdoc/test.rs index 3b07a2ccdde09..9efe9161d5744 100644 --- a/src/librustdoc/test.rs +++ b/src/librustdoc/test.rs @@ -12,6 +12,7 @@ use std::env; use std::ffi::OsString; use std::io::prelude::*; use std::io; +use std::mem; use std::path::{Path, PathBuf}; use std::panic::{self, AssertUnwindSafe}; use std::process::Command; @@ -41,7 +42,7 @@ use syntax_pos::{BytePos, DUMMY_SP, Pos, Span, FileName}; use errors; use errors::emitter::ColorConfig; -use clean::Attributes; +use clean::{Attributes, AttributesExt, NestedAttributesExt}; use html::markdown::{self, ErrorCodes, LangString}; #[derive(Clone, Default)] @@ -51,6 +52,8 @@ pub struct TestOptions { /// Whether to emit compilation warnings when compiling doctests. Setting this will suppress /// the default `#![allow(unused)]`. pub display_warnings: bool, + /// Whether to run doctests on private items. + pub document_private_items: bool, /// Additional crate-level attributes to add to doctests. pub attrs: Vec, } @@ -65,7 +68,8 @@ pub fn run(input_path: &Path, display_warnings: bool, linker: Option, edition: Edition, - cg: CodegenOptions) + cg: CodegenOptions, + document_private_items: bool) -> isize { let input = config::Input::File(input_path.to_owned()); @@ -124,6 +128,7 @@ pub fn run(input_path: &Path, }); let mut opts = scrape_test_config(hir_forest.krate()); opts.display_warnings |= display_warnings; + opts.document_private_items |= document_private_items; let mut collector = Collector::new( crate_name, cfgs, @@ -147,8 +152,9 @@ pub fn run(input_path: &Path, collector: &mut collector, map: &map, codes: ErrorCodes::from(sess.opts.unstable_features.is_nightly_build()), + current_vis: true, }; - hir_collector.visit_testable("".to_string(), &krate.attrs, |this| { + hir_collector.visit_testable("".to_string(), &krate.attrs, true, |this| { intravisit::walk_crate(this, krate); }); } @@ -169,9 +175,12 @@ fn scrape_test_config(krate: &::rustc::hir::Crate) -> TestOptions { let mut opts = TestOptions { no_crate_inject: false, display_warnings: false, + document_private_items: false, attrs: Vec::new(), }; + opts.document_private_items = krate.attrs.lists("doc").has_word("document_private_items"); + let test_attrs: Vec<_> = krate.attrs.iter() .filter(|a| a.check_name("doc")) .flat_map(|a| a.meta_item_list().unwrap_or_else(Vec::new)) @@ -664,12 +673,14 @@ struct HirCollector<'a, 'hir: 'a> { collector: &'a mut Collector, map: &'a hir::map::Map<'hir>, codes: ErrorCodes, + current_vis: bool, } impl<'a, 'hir> HirCollector<'a, 'hir> { fn visit_testable(&mut self, name: String, attrs: &[ast::Attribute], + item_is_pub: bool, nested: F) { let mut attrs = Attributes::from_ast(self.sess.diagnostic(), attrs); if let Some(ref cfg) = attrs.cfg { @@ -678,6 +689,21 @@ impl<'a, 'hir> HirCollector<'a, 'hir> { } } + let old_vis = if attrs.has_doc_flag("hidden") { + Some(mem::replace(&mut self.current_vis, false)) + } else { + None + }; + + if !self.collector.opts.document_private_items { + if !(self.current_vis && item_is_pub) { + if let Some(old_vis) = old_vis { + self.current_vis = old_vis; + } + return; + } + } + let has_name = !name.is_empty(); if has_name { self.collector.names.push(name); @@ -698,6 +724,10 @@ impl<'a, 'hir> HirCollector<'a, 'hir> { nested(self); + if let Some(old_vis) = old_vis { + self.current_vis = old_vis; + } + if has_name { self.collector.names.pop(); } @@ -710,31 +740,50 @@ impl<'a, 'hir> intravisit::Visitor<'hir> for HirCollector<'a, 'hir> { } fn visit_item(&mut self, item: &'hir hir::Item) { + let is_public = item.vis.node.is_pub(); + let name = if let hir::ItemKind::Impl(.., ref ty, _) = item.node { self.map.node_to_pretty_string(ty.id) } else { item.name.to_string() }; - self.visit_testable(name, &item.attrs, |this| { + let old_vis = if let hir::ItemKind::Mod(..) = item.node { + let old_vis = self.current_vis; + self.current_vis &= is_public; + Some(old_vis) + } else { + None + }; + + self.visit_testable(name, &item.attrs, is_public, |this| { intravisit::walk_item(this, item); }); + + if let Some(old_vis) = old_vis { + self.current_vis = old_vis; + } } fn visit_trait_item(&mut self, item: &'hir hir::TraitItem) { - self.visit_testable(item.ident.to_string(), &item.attrs, |this| { + self.visit_testable(item.ident.to_string(), &item.attrs, true, |this| { intravisit::walk_trait_item(this, item); }); } fn visit_impl_item(&mut self, item: &'hir hir::ImplItem) { - self.visit_testable(item.ident.to_string(), &item.attrs, |this| { + let is_public = item.vis.node.is_pub(); + self.visit_testable(item.ident.to_string(), &item.attrs, is_public, |this| { intravisit::walk_impl_item(this, item); }); } fn visit_foreign_item(&mut self, item: &'hir hir::ForeignItem) { - self.visit_testable(item.name.to_string(), &item.attrs, |this| { + let is_public = item.vis.node.is_pub(); + self.visit_testable(item.name.to_string(), + &item.attrs, + is_public, + |this| { intravisit::walk_foreign_item(this, item); }); } @@ -743,19 +792,25 @@ impl<'a, 'hir> intravisit::Visitor<'hir> for HirCollector<'a, 'hir> { v: &'hir hir::Variant, g: &'hir hir::Generics, item_id: ast::NodeId) { - self.visit_testable(v.node.name.to_string(), &v.node.attrs, |this| { + self.visit_testable(v.node.name.to_string(), &v.node.attrs, true, |this| { intravisit::walk_variant(this, v, g, item_id); }); } fn visit_struct_field(&mut self, f: &'hir hir::StructField) { - self.visit_testable(f.ident.to_string(), &f.attrs, |this| { + let is_public = f.vis.node.is_pub(); + self.visit_testable(f.ident.to_string(), + &f.attrs, + is_public, + |this| { intravisit::walk_struct_field(this, f); }); } fn visit_macro_def(&mut self, macro_def: &'hir hir::MacroDef) { - self.visit_testable(macro_def.name.to_string(), ¯o_def.attrs, |_| ()); + // FIXME(misdreavus): does macro export status surface to us? is it in AccessLevels, does + // its #[macro_export] attribute show up here? + self.visit_testable(macro_def.name.to_string(), ¯o_def.attrs, true, |_| ()); } } From 4c6bf4913d229d9ef5ae301cb53d3527fa279d6f Mon Sep 17 00:00:00 2001 From: QuietMisdreavus Date: Thu, 20 Sep 2018 21:21:38 -0500 Subject: [PATCH 2/5] run doctests on items if they're re-exported --- src/librustdoc/test.rs | 51 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 50 insertions(+), 1 deletion(-) diff --git a/src/librustdoc/test.rs b/src/librustdoc/test.rs index 9efe9161d5744..05f38378a410e 100644 --- a/src/librustdoc/test.rs +++ b/src/librustdoc/test.rs @@ -739,8 +739,57 @@ impl<'a, 'hir> intravisit::Visitor<'hir> for HirCollector<'a, 'hir> { intravisit::NestedVisitorMap::All(&self.map) } - fn visit_item(&mut self, item: &'hir hir::Item) { + fn visit_item(&mut self, mut item: &'hir hir::Item) { let is_public = item.vis.node.is_pub(); + if let hir::ItemKind::Use(ref path, hir::UseKind::Single) = item.node { + if is_public { + if let Some(node) = self.map.get_if_local(path.def.def_id()) { + // load doctests from the actual item, not the use statement + // TODO(misdreavus): this will run some doctests multiple times if an item is + // re-exported multiple times (say, into a prelude) + match node { + // if we've re-exported an item, continue with the new item, using the + // exported visibility + hir::Node::Item(new_item) => item = new_item, + // forward anything else we handle specially to our visitor + hir::Node::TraitItem(item) => { + self.visit_trait_item(item); + return; + } + hir::Node::ImplItem(item) => { + self.visit_impl_item(item); + return; + } + hir::Node::ForeignItem(item) => { + self.visit_foreign_item(item); + return; + } + hir::Node::Field(f) => { + self.visit_struct_field(f); + return; + } + hir::Node::MacroDef(def) => { + self.visit_macro_def(def); + return; + } + hir::Node::Variant(v) => { + // we need the Generics and NodeId of the parent enum + let variant_node_id = + self.map.local_def_id_to_node_id(path.def.def_id().to_local()); + let enum_node_id = self.map.get_parent(variant_node_id); + let enum_def_id = self.map.local_def_id(enum_node_id); + let generics = self.map.get_generics(enum_def_id) + .expect("enums always have a Generics struct"); + + self.visit_variant(v, generics, enum_node_id); + return; + } + // we don't care about any other kind of node, so bail early + _ => return, + } + } + } + } let name = if let hir::ItemKind::Impl(.., ref ty, _) = item.node { self.map.node_to_pretty_string(ty.id) From 9a2aeff51b6c82e21f9da7fa0b3f590577975a86 Mon Sep 17 00:00:00 2001 From: QuietMisdreavus Date: Fri, 21 Sep 2018 10:12:17 -0500 Subject: [PATCH 3/5] don't run doctests on the same item twice --- src/librustdoc/test.rs | 38 ++++++++++++++++++++++++++++++++++++-- 1 file changed, 36 insertions(+), 2 deletions(-) diff --git a/src/librustdoc/test.rs b/src/librustdoc/test.rs index 05f38378a410e..31b7d767c7a0a 100644 --- a/src/librustdoc/test.rs +++ b/src/librustdoc/test.rs @@ -27,6 +27,7 @@ use rustc::hir::intravisit; use rustc::session::{self, CompileIncomplete, config}; use rustc::session::config::{OutputType, OutputTypes, Externs, CodegenOptions}; use rustc::session::search_paths::{SearchPaths, PathKind}; +use rustc::util::nodemap::FxHashSet; use rustc_metadata::dynamic_lib::DynamicLibrary; use tempfile::Builder as TempFileBuilder; use rustc_driver::{self, driver, target_features, Compilation}; @@ -152,6 +153,7 @@ pub fn run(input_path: &Path, collector: &mut collector, map: &map, codes: ErrorCodes::from(sess.opts.unstable_features.is_nightly_build()), + tested_items: FxHashSet(), current_vis: true, }; hir_collector.visit_testable("".to_string(), &krate.attrs, true, |this| { @@ -673,6 +675,7 @@ struct HirCollector<'a, 'hir: 'a> { collector: &'a mut Collector, map: &'a hir::map::Map<'hir>, codes: ErrorCodes, + tested_items: FxHashSet, current_vis: bool, } @@ -745,8 +748,6 @@ impl<'a, 'hir> intravisit::Visitor<'hir> for HirCollector<'a, 'hir> { if is_public { if let Some(node) = self.map.get_if_local(path.def.def_id()) { // load doctests from the actual item, not the use statement - // TODO(misdreavus): this will run some doctests multiple times if an item is - // re-exported multiple times (say, into a prelude) match node { // if we've re-exported an item, continue with the new item, using the // exported visibility @@ -791,6 +792,11 @@ impl<'a, 'hir> intravisit::Visitor<'hir> for HirCollector<'a, 'hir> { } } + if !self.tested_items.insert(item.id) { + // item's been tested already, don't run them again + return; + } + let name = if let hir::ItemKind::Impl(.., ref ty, _) = item.node { self.map.node_to_pretty_string(ty.id) } else { @@ -815,12 +821,22 @@ impl<'a, 'hir> intravisit::Visitor<'hir> for HirCollector<'a, 'hir> { } fn visit_trait_item(&mut self, item: &'hir hir::TraitItem) { + if !self.tested_items.insert(item.id) { + // item's been tested already, don't run them again + return; + } + self.visit_testable(item.ident.to_string(), &item.attrs, true, |this| { intravisit::walk_trait_item(this, item); }); } fn visit_impl_item(&mut self, item: &'hir hir::ImplItem) { + if !self.tested_items.insert(item.id) { + // item's been tested already, don't run them again + return; + } + let is_public = item.vis.node.is_pub(); self.visit_testable(item.ident.to_string(), &item.attrs, is_public, |this| { intravisit::walk_impl_item(this, item); @@ -828,6 +844,11 @@ impl<'a, 'hir> intravisit::Visitor<'hir> for HirCollector<'a, 'hir> { } fn visit_foreign_item(&mut self, item: &'hir hir::ForeignItem) { + if !self.tested_items.insert(item.id) { + // item's been tested already, don't run them again + return; + } + let is_public = item.vis.node.is_pub(); self.visit_testable(item.name.to_string(), &item.attrs, @@ -841,12 +862,20 @@ impl<'a, 'hir> intravisit::Visitor<'hir> for HirCollector<'a, 'hir> { v: &'hir hir::Variant, g: &'hir hir::Generics, item_id: ast::NodeId) { + // FIXME(misdreavus): variants don't have their own NodeId so we can't insert them into the + // set + self.visit_testable(v.node.name.to_string(), &v.node.attrs, true, |this| { intravisit::walk_variant(this, v, g, item_id); }); } fn visit_struct_field(&mut self, f: &'hir hir::StructField) { + if !self.tested_items.insert(f.id) { + // item's been tested already, don't run them again + return; + } + let is_public = f.vis.node.is_pub(); self.visit_testable(f.ident.to_string(), &f.attrs, @@ -857,6 +886,11 @@ impl<'a, 'hir> intravisit::Visitor<'hir> for HirCollector<'a, 'hir> { } fn visit_macro_def(&mut self, macro_def: &'hir hir::MacroDef) { + if !self.tested_items.insert(macro_def.id) { + // item's been tested already, don't run them again + return; + } + // FIXME(misdreavus): does macro export status surface to us? is it in AccessLevels, does // its #[macro_export] attribute show up here? self.visit_testable(macro_def.name.to_string(), ¯o_def.attrs, true, |_| ()); From bc41a7afe916c5b5b90faf22d10d260d7742d034 Mon Sep 17 00:00:00 2001 From: QuietMisdreavus Date: Fri, 21 Sep 2018 10:33:43 -0500 Subject: [PATCH 4/5] add tests for private doctests --- src/test/rustdoc/private-doctests-private.rs | 49 ++++++++++++++++++++ src/test/rustdoc/private-doctests.rs | 45 ++++++++++++++++++ 2 files changed, 94 insertions(+) create mode 100644 src/test/rustdoc/private-doctests-private.rs create mode 100644 src/test/rustdoc/private-doctests.rs diff --git a/src/test/rustdoc/private-doctests-private.rs b/src/test/rustdoc/private-doctests-private.rs new file mode 100644 index 0000000000000..c168cbd8f6d27 --- /dev/null +++ b/src/test/rustdoc/private-doctests-private.rs @@ -0,0 +1,49 @@ +// Copyright 2018 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// compile-flags: --test --document-private-items +// should-fail + +// issue #30094: rustdoc runs doctests on private items even though those docs don't appear +// +// in this version, we show that passing --document-private-items causes the private doctests to +// run + +mod private { + /// Does all the work. + /// + /// ``` + /// panic!("oh no"); + /// ``` + pub fn real_job(a: u32, b: u32) -> u32 { + return a + b; + } +} + +pub mod public { + use super::private; + + /// ``` + /// // this was originally meant to link to public::function but we can't do that here + /// assert_eq!(2+2, 4); + /// ``` + pub fn function(a: u32, b: u32) -> u32 { + return complex_helper(a, b); + } + + /// Helps with stuff. + /// + /// ``` + /// panic!("oh no"); + /// ``` + fn complex_helper(a: u32, b: u32) -> u32 { + return private::real_job(a, b); + } +} diff --git a/src/test/rustdoc/private-doctests.rs b/src/test/rustdoc/private-doctests.rs new file mode 100644 index 0000000000000..480a525761489 --- /dev/null +++ b/src/test/rustdoc/private-doctests.rs @@ -0,0 +1,45 @@ +// Copyright 2018 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// compile-flags: --test + +// issue #30094: rustdoc runs doctests on private items even though those docs don't appear + +mod private { + /// Does all the work. + /// + /// ``` + /// panic!("oh no"); + /// ``` + pub fn real_job(a: u32, b: u32) -> u32 { + return a + b; + } +} + +pub mod public { + use super::private; + + /// ``` + /// // this was originally meant to link to public::function but we can't do that here + /// assert_eq!(2+2, 4); + /// ``` + pub fn function(a: u32, b: u32) -> u32 { + return complex_helper(a, b); + } + + /// Helps with stuff. + /// + /// ``` + /// panic!("oh no"); + /// ``` + fn complex_helper(a: u32, b: u32) -> u32 { + return private::real_job(a, b); + } +} From fcd525a14f342ed6acdf1baa2f41c4c1b97d44f5 Mon Sep 17 00:00:00 2001 From: QuietMisdreavus Date: Fri, 21 Sep 2018 15:19:56 -0500 Subject: [PATCH 5/5] fix test make_test_no_crate_inject --- src/librustdoc/test.rs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/librustdoc/test.rs b/src/librustdoc/test.rs index 31b7d767c7a0a..fac1efda7d257 100644 --- a/src/librustdoc/test.rs +++ b/src/librustdoc/test.rs @@ -955,11 +955,8 @@ assert_eq!(2+2, 4); fn make_test_no_crate_inject() { //even if you do use the crate within the test, setting `opts.no_crate_inject` will skip //adding it anyway - let opts = TestOptions { - no_crate_inject: true, - display_warnings: false, - attrs: vec![], - }; + let mut opts = TestOptions::default(); + opts.no_crate_inject = true; let input = "use asdf::qwop; assert_eq!(2+2, 4);";