rustdoc: Fix some local inlining issues

ollie27 · ollie27 · commit 6fe7786db6d4 · 2016-11-14T18:24:47.000Z
* Only inline public items when inlining glob imports.
* Never inline while in a private module or a child of a private module.
* Never inline impls. This allowed the removal of a workaround in the
rendering code.
diff --git a/src/librustdoc/html/render.rs b/src/librustdoc/html/render.rs
@@ -257,8 +257,6 @@ pub struct Cache {
     parent_stack: Vec<DefId>,
     parent_is_trait_impl: bool,
     search_index: Vec<IndexItem>,
-    seen_modules: FxHashSet<DefId>,
-    seen_mod: bool,
     stripped_mod: bool,
     deref_trait_did: Option<DefId>,
     deref_mut_trait_did: Option<DefId>,
@@ -520,8 +518,6 @@ pub fn run(mut krate: clean::Crate,
         parent_is_trait_impl: false,
         extern_locations: FxHashMap(),
         primitive_locations: FxHashMap(),
-        seen_modules: FxHashSet(),
-        seen_mod: false,
         stripped_mod: false,
         access_levels: krate.access_levels.clone(),
         orphan_impl_items: Vec::new(),
@@ -977,37 +973,26 @@ impl DocFolder for Cache {
             _ => self.stripped_mod,
         };
 
-        // Inlining can cause us to visit the same item multiple times.
-        // (i.e. relevant for gathering impls and implementors)
-        let orig_seen_mod = if item.is_mod() {
-            let seen_this = self.seen_mod || !self.seen_modules.insert(item.def_id);
-            mem::replace(&mut self.seen_mod, seen_this)
-        } else {
-            self.seen_mod
-        };
-
         // Register any generics to their corresponding string. This is used
         // when pretty-printing types
         if let Some(generics) = item.inner.generics() {
             self.generics(generics);
         }
 
-        if !self.seen_mod {
-            // Propagate a trait methods' documentation to all implementors of the
-            // trait
-            if let clean::TraitItem(ref t) = item.inner {
-                self.traits.insert(item.def_id, t.clone());
-            }
+        // Propagate a trait methods' documentation to all implementors of the
+        // trait
+        if let clean::TraitItem(ref t) = item.inner {
+            self.traits.entry(item.def_id).or_insert_with(|| t.clone());
+        }
 
-            // Collect all the implementors of traits.
-            if let clean::ImplItem(ref i) = item.inner {
-                if let Some(did) = i.trait_.def_id() {
-                    self.implementors.entry(did).or_insert(vec![]).push(Implementor {
-                        def_id: item.def_id,
-                        stability: item.stability.clone(),
-                        impl_: i.clone(),
-                    });
-                }
+        // Collect all the implementors of traits.
+        if let clean::ImplItem(ref i) = item.inner {
+            if let Some(did) = i.trait_.def_id() {
+                self.implementors.entry(did).or_insert(vec![]).push(Implementor {
+                    def_id: item.def_id,
+                    stability: item.stability.clone(),
+                    impl_: i.clone(),
+                });
             }
         }
 
@@ -1186,12 +1171,10 @@ impl DocFolder for Cache {
                 } else {
                     unreachable!()
                 };
-                if !self.seen_mod {
-                    if let Some(did) = did {
-                        self.impls.entry(did).or_insert(vec![]).push(Impl {
-                            impl_item: item,
-                        });
-                    }
+                if let Some(did) = did {
+                    self.impls.entry(did).or_insert(vec![]).push(Impl {
+                        impl_item: item,
+                    });
                 }
                 None
             } else {
@@ -1201,7 +1184,6 @@ impl DocFolder for Cache {
 
         if pushed { self.stack.pop().unwrap(); }
         if parent_pushed { self.parent_stack.pop().unwrap(); }
-        self.seen_mod = orig_seen_mod;
         self.stripped_mod = orig_stripped_mod;
         self.parent_is_trait_impl = orig_parent_is_trait_impl;
         ret
diff --git a/src/librustdoc/visit_ast.rs b/src/librustdoc/visit_ast.rs
@@ -44,7 +44,9 @@ pub struct RustdocVisitor<'a, 'tcx: 'a> {
     pub attrs: hir::HirVec<ast::Attribute>,
     pub cx: &'a core::DocContext<'a, 'tcx>,
     view_item_stack: FxHashSet<ast::NodeId>,
-    inlining_from_glob: bool,
+    inlining: bool,
+    /// Is the current module and all of its parents public?
+    inside_public_path: bool,
 }
 
 impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> {
@@ -57,7 +59,8 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> {
             attrs: hir::HirVec::new(),
             cx: cx,
             view_item_stack: stack,
-            inlining_from_glob: false,
+            inlining: false,
+            inside_public_path: true,
         }
     }
 
@@ -189,10 +192,14 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> {
         om.stab = self.stability(id);
         om.depr = self.deprecation(id);
         om.id = id;
+        // Keep track of if there were any private modules in the path.
+        let orig_inside_public_path = self.inside_public_path;
+        self.inside_public_path &= vis == hir::Public;
         for i in &m.item_ids {
             let item = self.cx.map.expect_item(i.id);
             self.visit_item(item, None, &mut om);
         }
+        self.inside_public_path = orig_inside_public_path;
         if let Some(exports) = self.cx.export_map.get(&id) {
             for export in exports {
                 if let Def::Macro(def_id) = export.def {
@@ -336,8 +343,8 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> {
 
         let ret = match tcx.map.get(def_node_id) {
             hir_map::NodeItem(it) => {
+                let prev = mem::replace(&mut self.inlining, true);
                 if glob {
-                    let prev = mem::replace(&mut self.inlining_from_glob, true);
                     match it.node {
                         hir::ItemMod(ref m) => {
                             for i in &m.item_ids {
@@ -348,10 +355,10 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> {
                         hir::ItemEnum(..) => {}
                         _ => { panic!("glob not mapped to a module or enum"); }
                     }
-                    self.inlining_from_glob = prev;
                 } else {
                     self.visit_item(it, renamed, om);
                 }
+                self.inlining = prev;
                 true
             }
             _ => false,
@@ -365,6 +372,19 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> {
         debug!("Visiting item {:?}", item);
         let name = renamed.unwrap_or(item.name);
         match item.node {
+            hir::ItemForeignMod(ref fm) => {
+                // If inlining we only want to include public functions.
+                om.foreigns.push(if self.inlining {
+                    hir::ForeignMod {
+                        abi: fm.abi,
+                        items: fm.items.iter().filter(|i| i.vis == hir::Public).cloned().collect(),
+                    }
+                } else {
+                    fm.clone()
+                });
+            }
+            // If we're inlining, skip private items.
+            _ if self.inlining && item.vis != hir::Public => {}
             hir::ItemExternCrate(ref p) => {
                 let cstore = &self.cx.sess().cstore;
                 om.extern_crates.push(ExternCrate {
@@ -379,7 +399,9 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> {
             }
             hir::ItemUse(ref vpath) => {
                 let node = vpath.node.clone();
-                let node = if item.vis == hir::Public {
+                // If there was a private module in the current path then don't bother inlining
+                // anything as it will probably be stripped anyway.
+                let node = if item.vis == hir::Public && self.inside_public_path {
                     let please_inline = item.attrs.iter().any(|item| {
                         match item.meta_item_list() {
                             Some(list) if item.check_name("doc") => {
@@ -479,43 +501,41 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> {
                 };
                 om.traits.push(t);
             },
+
             hir::ItemImpl(unsafety, polarity, ref gen, ref tr, ref ty, ref items) => {
-                let i = Impl {
-                    unsafety: unsafety,
-                    polarity: polarity,
-                    generics: gen.clone(),
-                    trait_: tr.clone(),
-                    for_: ty.clone(),
-                    items: items.clone(),
-                    attrs: item.attrs.clone(),
-                    id: item.id,
-                    whence: item.span,
-                    vis: item.vis.clone(),
-                    stab: self.stability(item.id),
-                    depr: self.deprecation(item.id),
-                };
-                // Don't duplicate impls when inlining glob imports, we'll pick
-                // them up regardless of where they're located.
-                if !self.inlining_from_glob {
+                // Don't duplicate impls when inlining, we'll pick them up
+                // regardless of where they're located.
+                if !self.inlining {
+                    let i = Impl {
+                        unsafety: unsafety,
+                        polarity: polarity,
+                        generics: gen.clone(),
+                        trait_: tr.clone(),
+                        for_: ty.clone(),
+                        items: items.clone(),
+                        attrs: item.attrs.clone(),
+                        id: item.id,
+                        whence: item.span,
+                        vis: item.vis.clone(),
+                        stab: self.stability(item.id),
+                        depr: self.deprecation(item.id),
+                    };
                     om.impls.push(i);
                 }
             },
             hir::ItemDefaultImpl(unsafety, ref trait_ref) => {
-                let i = DefaultImpl {
-                    unsafety: unsafety,
-                    trait_: trait_ref.clone(),
-                    id: item.id,
-                    attrs: item.attrs.clone(),
-                    whence: item.span,
-                };
-                // see comment above about ItemImpl
-                if !self.inlining_from_glob {
+                // See comment above about ItemImpl.
+                if !self.inlining {
+                    let i = DefaultImpl {
+                        unsafety: unsafety,
+                        trait_: trait_ref.clone(),
+                        id: item.id,
+                        attrs: item.attrs.clone(),
+                        whence: item.span,
+                    };
                     om.def_traits.push(i);
                 }
             }
-            hir::ItemForeignMod(ref fm) => {
-                om.foreigns.push(fm.clone());
-            }
         }
     }
 
diff --git a/src/test/rustdoc/inline_local/glob-extern-no-defaults.rs b/src/test/rustdoc/inline_local/glob-extern-no-defaults.rs
@@ -0,0 +1,35 @@
+// Copyright 2016 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: --no-defaults
+
+#![crate_name = "foo"]
+
+mod mod1 {
+    extern {
+        pub fn public_fn();
+        fn private_fn();
+    }
+}
+
+pub use mod1::*;
+
+// @has foo/index.html
+// @has - "mod1"
+// @has - "public_fn"
+// @!has - "private_fn"
+// @has foo/fn.public_fn.html
+// @!has foo/fn.private_fn.html
+
+// @has foo/mod1/index.html
+// @has - "public_fn"
+// @has - "private_fn"
+// @has foo/mod1/fn.public_fn.html
+// @has foo/mod1/fn.private_fn.html
diff --git a/src/test/rustdoc/inline_local/glob-extern.rs b/src/test/rustdoc/inline_local/glob-extern.rs
@@ -0,0 +1,31 @@
+// Copyright 2016 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.
+
+#![crate_name = "foo"]
+
+mod mod1 {
+    extern {
+        pub fn public_fn();
+        fn private_fn();
+    }
+}
+
+pub use mod1::*;
+
+// @has foo/index.html
+// @!has - "mod1"
+// @has - "public_fn"
+// @!has - "private_fn"
+// @has foo/fn.public_fn.html
+// @!has foo/fn.private_fn.html
+
+// @!has foo/mod1/index.html
+// @has foo/mod1/fn.public_fn.html
+// @!has foo/mod1/fn.private_fn.html
diff --git a/src/test/rustdoc/inline_local/glob-private-no-defaults.rs b/src/test/rustdoc/inline_local/glob-private-no-defaults.rs
@@ -0,0 +1,58 @@
+// Copyright 2016 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: --no-defaults
+
+#![crate_name = "foo"]
+
+mod mod1 {
+    mod mod2 {
+        pub struct Mod2Public;
+        struct Mod2Private;
+    }
+    pub use self::mod2::*;
+
+    pub struct Mod1Public;
+    struct Mod1Private;
+}
+pub use mod1::*;
+
+// @has foo/index.html
+// @has - "mod1"
+// @has - "Mod1Public"
+// @!has - "Mod1Private"
+// @!has - "mod2"
+// @has - "Mod2Public"
+// @!has - "Mod2Private"
+// @has foo/struct.Mod1Public.html
+// @!has foo/struct.Mod1Private.html
+// @has foo/struct.Mod2Public.html
+// @!has foo/struct.Mod2Private.html
+
+// @has foo/mod1/index.html
+// @has - "mod2"
+// @has - "Mod1Public"
+// @has - "Mod1Private"
+// @!has - "Mod2Public"
+// @!has - "Mod2Private"
+// @has foo/mod1/struct.Mod1Public.html
+// @has foo/mod1/struct.Mod1Private.html
+// @!has foo/mod1/struct.Mod2Public.html
+// @!has foo/mod1/struct.Mod2Private.html
+
+// @has foo/mod1/mod2/index.html
+// @has - "Mod2Public"
+// @has - "Mod2Private"
+// @has foo/mod1/mod2/struct.Mod2Public.html
+// @has foo/mod1/mod2/struct.Mod2Private.html
+
+// @!has foo/mod2/index.html
+// @!has foo/mod2/struct.Mod2Public.html
+// @!has foo/mod2/struct.Mod2Private.html
diff --git a/src/test/rustdoc/inline_local/glob-private.rs b/src/test/rustdoc/inline_local/glob-private.rs
