Respond to comments and start adding tests

P1n3appl3 · P1n3appl3 · commit 867ac8da355c · 2020-08-14T06:09:37.000-05:00
diff --git a/src/librustdoc/clean/mod.rs b/src/librustdoc/clean/mod.rs
@@ -2262,7 +2262,7 @@ impl Clean<Vec<Item>> for doctree::Import<'_> {
             name: None,
             attrs: self.attrs.clean(cx),
             source: self.whence.clean(cx),
-            def_id: DefId::local(CRATE_DEF_INDEX),
+            def_id: cx.tcx.hir().local_def_id(self.id).to_def_id(),
             visibility: self.vis.clean(cx),
             stability: None,
             deprecation: None,
diff --git a/src/librustdoc/clean/types.rs b/src/librustdoc/clean/types.rs
@@ -279,6 +279,42 @@ pub enum ItemEnum {
 }
 
 impl ItemEnum {
+    /// Some items contain others such as structs (for their fields) and Enums
+    /// (for their variants). This method returns those contained items.
+    pub fn inner_items(&self) -> impl Iterator<Item = &Item> {
+        match self {
+            StructItem(s) => s.fields.iter(),
+            UnionItem(u) => u.fields.iter(),
+            VariantItem(Variant { kind: VariantKind::Struct(v) }) => v.fields.iter(),
+            EnumItem(e) => e.variants.iter(),
+            TraitItem(t) => t.items.iter(),
+            ImplItem(i) => i.items.iter(),
+            ModuleItem(m) => m.items.iter(),
+            ExternCrateItem(_, _)
+            | ImportItem(_)
+            | FunctionItem(_)
+            | TypedefItem(_, _)
+            | OpaqueTyItem(_, _)
+            | StaticItem(_)
+            | ConstantItem(_)
+            | TraitAliasItem(_)
+            | TyMethodItem(_)
+            | MethodItem(_)
+            | StructFieldItem(_)
+            | VariantItem(_)
+            | ForeignFunctionItem(_)
+            | ForeignStaticItem(_)
+            | ForeignTypeItem
+            | MacroItem(_)
+            | ProcMacroItem(_)
+            | PrimitiveItem(_)
+            | AssocConstItem(_,_)
+            | AssocTypeItem(_,_)
+            | StrippedItem(_)
+            | KeywordItem(_) => [].iter(),
+        }
+    }
+
     pub fn is_associated(&self) -> bool {
         match *self {
             ItemEnum::TypedefItem(_, _) | ItemEnum::AssocTypeItem(_, _) => true,
diff --git a/src/librustdoc/json/conversions.rs b/src/librustdoc/json/conversions.rs
@@ -1,3 +1,7 @@
+//! These from impls are used to create the JSON types which get serialized. They're very close to
+//! the `clean` types but with some fields removed or stringified to simplify the output and not
+//! expose unstable compiler internals.
+
 use std::convert::From;
 
 use rustc_ast::ast;
@@ -22,7 +26,7 @@ impl From<clean::Item> for Item {
             deprecation,
         } = item;
         Item {
-            crate_num: def_id.krate.as_u32(),
+            crate_id: def_id.krate.as_u32(),
             name,
             source: source.into(),
             visibility: visibility.into(),
@@ -329,14 +333,13 @@ impl From<clean::Type> for Type {
             ResolvedPath { path, param_names, did, is_generic: _ } => Type::ResolvedPath {
                 name: path.whole_name(),
                 id: did.into(),
-                args: Box::new(path.segments.last().map(|args| args.clone().args.into())),
+                args: path.segments.last().map(|args| Box::new(args.clone().args.into())),
                 param_names: param_names
                     .map(|v| v.into_iter().map(Into::into).collect())
                     .unwrap_or_default(),
             },
             Generic(s) => Type::Generic(s),
             Primitive(p) => Type::Primitive(p.as_str().to_string()),
-            // TODO: check if there's a more idiomatic way of calling `into` on Box<T>
             BareFunction(f) => Type::FunctionPointer(Box::new((*f).into())),
             Tuple(t) => Type::Tuple(t.into_iter().map(Into::into).collect()),
             Slice(t) => Type::Slice(Box::new((*t).into())),
diff --git a/src/librustdoc/json/mod.rs b/src/librustdoc/json/mod.rs
@@ -1,8 +1,15 @@
+//! Rustdoc's JSON backend
+//!
+//! This module contains the logic for rendering a crate as JSON rather than the normal static HTML
+//! output. See [the RFC](https://github.com/rust-lang/rfcs/pull/2963) and the [`types`] module
+//! docs for usage and details.
+
 mod conversions;
 mod types;
 
 use std::cell::RefCell;
 use std::fs::File;
+use std::path::PathBuf;
 use std::rc::Rc;
 
 use rustc_data_structures::fx::FxHashMap;
@@ -17,10 +24,17 @@ use crate::html::render::cache::ExternalLocation;
 
 #[derive(Clone)]
 pub struct JsonRenderer {
+    /// A mapping of IDs that contains all local items for this crate which gets output as a top
+    /// level field of the JSON blob.
     index: Rc<RefCell<FxHashMap<types::Id, types::Item>>>,
+    /// The directory where the blob will be written to.
+    out_path: PathBuf,
 }
 
 impl JsonRenderer {
+    /// Inserts an item into the index. This should be used rather than directly calling insert on
+    /// the hashmap because certain items (traits and types) need to have their mappings for trait
+    /// implementations filled out before they're inserted.
     fn insert(&self, item: clean::Item, cache: &Cache) {
         let id = item.def_id;
         let mut new_item: types::Item = item.into();
@@ -75,33 +89,74 @@ impl JsonRenderer {
             })
             .unwrap_or_default()
     }
+
+    fn get_trait_items(&self, cache: &Cache) -> Vec<(types::Id, types::Item)> {
+        cache
+            .traits
+            .iter()
+            .filter_map(|(id, trait_item)| {
+                // only need to synthesize items for external traits
+                if !id.is_local() {
+                    trait_item.items.clone().into_iter().for_each(|i| self.insert(i, cache));
+                    Some((
+                        (*id).into(),
+                        types::Item {
+                            crate_id: id.krate.as_u32(),
+                            name: cache
+                                .paths
+                                .get(&id)
+                                .unwrap_or_else(|| {
+                                    cache
+                                        .external_paths
+                                        .get(&id)
+                                        .expect("Trait should either be in local or external paths")
+                                })
+                                .0
+                                .last()
+                                .map(Clone::clone),
+                            visibility: types::Visibility::Public,
+                            kind: types::ItemKind::Trait,
+                            inner: types::ItemEnum::TraitItem(trait_item.clone().into()),
+                            source: None,
+                            docs: Default::default(),
+                            links: Default::default(),
+                            attrs: Default::default(),
+                            deprecation: Default::default(),
+                        },
+                    ))
+                } else {
+                    None
+                }
+            })
+            .collect()
+    }
 }
 
 impl FormatRenderer for JsonRenderer {
     fn init(
         krate: clean::Crate,
-        _options: RenderOptions,
+        options: RenderOptions,
         _render_info: RenderInfo,
         _edition: Edition,
         _cache: &mut Cache,
     ) -> Result<(Self, clean::Crate), Error> {
         debug!("Initializing json renderer");
-        Ok((JsonRenderer { index: Rc::new(RefCell::new(FxHashMap::default())) }, krate))
+        Ok((
+            JsonRenderer {
+                index: Rc::new(RefCell::new(FxHashMap::default())),
+                out_path: options.output,
+            },
+            krate,
+        ))
     }
 
     fn item(&mut self, item: clean::Item, cache: &Cache) -> Result<(), Error> {
         use clean::ItemEnum::*;
-        // Flatten items that recursively store other items by putting their children in the index
-        match item.inner.clone() {
-            StructItem(s) => s.fields.into_iter().for_each(|i| self.insert(i, cache)),
-            UnionItem(u) => u.fields.into_iter().for_each(|i| self.insert(i, cache)),
-            VariantItem(clean::Variant { kind: clean::VariantKind::Struct(v) }) => {
-                v.fields.into_iter().for_each(|i| self.insert(i, cache));
-            }
-            EnumItem(e) => e.variants.into_iter().for_each(|i| self.item(i, cache).unwrap()),
-            TraitItem(t) => t.items.into_iter().for_each(|i| self.insert(i, cache)),
-            ImplItem(i) => i.items.into_iter().for_each(|i| self.insert(i, cache)),
-            _ => {}
+        // Flatten items that recursively store other items by inserting them into the index
+        if let ModuleItem(_) = &item.inner {
+            // but ignore modules because we handle recursing into them separately
+        } else {
+            item.inner.inner_items().for_each(|i| self.item(i.clone(), cache).unwrap())
         }
         self.insert(item.clone(), cache);
         Ok(())
@@ -124,41 +179,7 @@ impl FormatRenderer for JsonRenderer {
     fn after_krate(&mut self, krate: &clean::Crate, cache: &Cache) -> Result<(), Error> {
         debug!("Done with crate");
         let mut index = (*self.index).clone().into_inner();
-        let trait_items = cache.traits.iter().filter_map(|(id, trait_item)| {
-            // only need to synthesize items for external traits
-            if !id.is_local() {
-                trait_item.items.clone().into_iter().for_each(|i| self.insert(i, cache));
-                Some((
-                    (*id).into(),
-                    types::Item {
-                        crate_num: id.krate.as_u32(),
-                        name: cache
-                            .paths
-                            .get(&id)
-                            .unwrap_or_else(|| {
-                                cache
-                                    .external_paths
-                                    .get(&id)
-                                    .expect("Trait should either be in local or external paths")
-                            })
-                            .0
-                            .last()
-                            .map(Clone::clone),
-                        visibility: types::Visibility::Public,
-                        kind: types::ItemKind::Trait,
-                        inner: types::ItemEnum::TraitItem(trait_item.clone().into()),
-                        source: None,
-                        docs: Default::default(),
-                        links: Default::default(),
-                        attrs: Default::default(),
-                        deprecation: Default::default(),
-                    },
-                ))
-            } else {
-                None
-            }
-        });
-        index.extend(trait_items);
+        index.extend(self.get_trait_items(cache));
         let output = types::Crate {
             root: types::Id(String::from("0:0")),
             version: krate.version.clone(),
@@ -172,7 +193,7 @@ impl FormatRenderer for JsonRenderer {
                 .map(|(k, (path, kind))| {
                     (
                         k.into(),
-                        types::ItemSummary { crate_num: k.krate.as_u32(), path, kind: kind.into() },
+                        types::ItemSummary { crate_id: k.krate.as_u32(), path, kind: kind.into() },
                     )
                 })
                 .collect(),
@@ -194,7 +215,10 @@ impl FormatRenderer for JsonRenderer {
                 .collect(),
             format_version: 1,
         };
-        serde_json::ser::to_writer_pretty(&File::create("test.json").unwrap(), &output).unwrap();
+        let mut p = self.out_path.clone();
+        p.push(output.index.get(&output.root).unwrap().name.clone().unwrap());
+        p.set_extension("json");
+        serde_json::ser::to_writer_pretty(&File::create(p).unwrap(), &output).unwrap();
         Ok(())
     }
 
diff --git a/src/librustdoc/json/types.rs b/src/librustdoc/json/types.rs
@@ -1,6 +1,6 @@
 //! Rustdoc's JSON output interface
 //!
-//! These types are the public API exposed through the `--output-format json` flag. The [`Crate`][]
+//! These types are the public API exposed through the `--output-format json` flag. The [`Crate`]
 //! struct is the root of the JSON blob and all other items are contained within.
 
 use std::path::PathBuf;
@@ -13,7 +13,7 @@ use serde::{Deserialize, Serialize};
 /// tools to find or link to them.
 #[derive(Clone, Debug, Serialize, Deserialize)]
 pub struct Crate {
-    /// The id of the root [`Module`][] item of the local crate.
+    /// The id of the root [`Module`] item of the local crate.
     pub root: Id,
     /// The version string given to `--crate-version`, if any.
     pub version: Option<String>,
@@ -22,10 +22,9 @@ pub struct Crate {
     /// A collection of all items in the local crate as well as some external traits and their
     /// items that are referenced locally.
     pub index: FxHashMap<Id, Item>,
-    /// Maps ids to fully qualified paths (e.g. `["std", "io", "lazy", "Lazy"]` for
-    /// `std::io::lazy::Lazy`) as well as their `ItemKind`
+    /// Maps IDs to fully qualified paths and other info helpful for generating links.
     pub paths: FxHashMap<Id, ItemSummary>,
-    /// Maps `crate_num` of items to a crate name and html_root_url if it exists
+    /// Maps `crate_id` of items to a crate name and html_root_url if it exists.
     pub external_crates: FxHashMap<u32, ExternalCrate>,
     /// A single version number to be used in the future when making backwards incompatible changes
     /// to the JSON output.
@@ -38,31 +37,36 @@ pub struct ExternalCrate {
     pub html_root_url: Option<String>,
 }
 
-/// For external items (stuff not defined in the local crate), you don't get the same level of
+/// For external (not defined in the local crate) items, you don't get the same level of
 /// information. This struct should contain enough to generate a link/reference to the item in
 /// question, or can be used by a tool that takes the json output of multiple crates to find
 /// the actual item definition with all the relevant info.
 #[derive(Clone, Debug, Serialize, Deserialize)]
 pub struct ItemSummary {
-    pub crate_num: u32,
+    /// Can be used to look up the name and html_root_url of the crate this item came from in the
+    /// `external_crates` map.
+    pub crate_id: u32,
+    /// The list of path components for the fully qualified path of this item (e.g.
+    /// `["std", "io", "lazy", "Lazy"]` for `std::io::lazy::Lazy`).
     pub path: Vec<String>,
+    /// Whether this item is a struct, trait, macro, etc.
     pub kind: ItemKind,
 }
 
 #[derive(Clone, Debug, Serialize, Deserialize)]
 pub struct Item {
-    /// This can be used as a key to the `external_crates` map of [`Crate`][] to see which crate
+    /// This can be used as a key to the `external_crates` map of [`Crate`] to see which crate
     /// this item came from.
-    pub crate_num: u32,
+    pub crate_id: u32,
     /// Some items such as impls don't have names.
     pub name: Option<String>,
-    /// The source location of this item. May not be present if it came from a macro expansion,
-    /// inline assembly, other "virtual" files.
+    /// The source location of this item (absent if it came from a macro expansion or inline
+    /// assembly).
     pub source: Option<Span>,
-    /// Usually documented items are all public, but you can tell rustdoc to output private items
+    /// By default all documented items are public, but you can tell rustdoc to output private items
     /// so this field is needed to differentiate.
     pub visibility: Visibility,
-    /// The full docstring of this item.
+    /// The full markdown docstring of this item.
     pub docs: String,
     /// This mapping resolves [intradoc links](https://github.com/rust-lang/rfcs/blob/master/text/1946-intra-rustdoc-links.md) from the docstring to their IDs
     pub links: FxHashMap<String, Id>,
@@ -71,10 +75,6 @@ pub struct Item {
     pub deprecation: Option<Deprecation>,
     pub kind: ItemKind,
     pub inner: ItemEnum,
-    // TODO: should we stringify the cfg attrs as well, or should we preserve their structure so
-    // the consumer doesn't have to parse an arbitrarily nested tree to figure out what platforms
-    // the item is available on?
-    // TODO: should we have a "stability" field if it's only used by the standard library?
 }
 
 #[derive(Clone, Debug, Serialize, Deserialize)]
@@ -97,6 +97,8 @@ pub struct Deprecation {
 #[derive(Clone, Debug, Serialize, Deserialize)]
 pub enum Visibility {
     Public,
+    /// For the most part items are private by default. The exceptions are associated items of
+    /// public traits and variants of public enums.
     Default,
     Crate,
     // TODO: Restricted(Id, String),
@@ -336,7 +338,7 @@ pub enum Type {
     ResolvedPath {
         name: String,
         id: Id,
-        args: Box<Option<GenericArgs>>,
+        args: Option<Box<GenericArgs>>,
         param_names: Vec<GenericBound>,
     },
     /// Parameterized types
@@ -429,9 +431,6 @@ pub struct Impl {
     pub blanket_impl: Option<Type>,
 }
 
-// TODO: this is currently broken because imports have the same ID as the module that contains
-// them. The only obvious fix is to modify the clean types to renumber imports so that IDs are
-// actually unique.
 #[serde(rename_all = "snake_case")]
 #[derive(Clone, Debug, Serialize, Deserialize)]
 pub struct Import {
diff --git a/src/test/run-make-fulldeps/rustdoc-json/Makefile b/src/test/run-make-fulldeps/rustdoc-json/Makefile
@@ -0,0 +1,5 @@
+-include ../tools.mk
+
+tests: *.rs
+	$(RUSTDOC) $< -o $(TMPDIR) --output-format json
+	$(PYTHON) check_missing_items.py $(TMPDIR)/$(basename $<).json
diff --git a/src/test/run-make-fulldeps/rustdoc-json/check_missing_items.py b/src/test/run-make-fulldeps/rustdoc-json/check_missing_items.py
diff --git a/src/test/run-make-fulldeps/rustdoc-json/test.rs b/src/test/run-make-fulldeps/rustdoc-json/test.rs
