rustdoc: don't run doctests on private items (by default)

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")),

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;
@@ -26,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};
@@ -41,7 +43,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 +53,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<String>,
 }
@@ -65,7 +69,8 @@ pub fn run(input_path: &Path,
            display_warnings: bool,
            linker: Option<PathBuf>,
            edition: Edition,
-           cg: CodegenOptions)
+           cg: CodegenOptions,
+           document_private_items: bool)
            -> isize {
     let input = config::Input::File(input_path.to_owned());
 
@@ -124,6 +129,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 +153,10 @@ 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, |this| {
+            hir_collector.visit_testable("".to_string(), &krate.attrs, true, |this| {
                 intravisit::walk_crate(this, krate);
             });
         }
@@ -169,9 +177,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 +675,15 @@ struct HirCollector<'a, 'hir: 'a> {
     collector: &'a mut Collector,
     map: &'a hir::map::Map<'hir>,
     codes: ErrorCodes,
+    tested_items: FxHashSet<ast::NodeId>,
+    current_vis: bool,
 }
 
 impl<'a, 'hir> HirCollector<'a, 'hir> {
     fn visit_testable<F: FnOnce(&mut Self)>(&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 +692,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 +727,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();
         }
@@ -709,32 +742,118 @@ 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
+                    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,
+                    }
+                }
+            }
+        }
+
+        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 {
             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| {
+        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) {
-        self.visit_testable(item.ident.to_string(), &item.attrs, |this| {
+        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);
         });
     }
 
     fn visit_foreign_item(&mut self, item: &'hir hir::ForeignItem) {
-        self.visit_testable(item.name.to_string(), &item.attrs, |this| {
+        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,
+                            is_public,
+                            |this| {
             intravisit::walk_foreign_item(this, item);
         });
     }
@@ -743,19 +862,38 @@ 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| {
+        // 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) {
-        self.visit_testable(f.ident.to_string(), &f.attrs, |this| {
+        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,
+                            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(), &macro_def.attrs, |_| ());
+        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(), &macro_def.attrs, true, |_| ());
     }
 }
 
@@ -817,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);";

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 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, 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);
+    }
+}