Auto merge of #30036 - mitaa:doc_id, r=alexcrichton

bors · bors · commit 461c46052b86 · 2015-12-08T20:15:53.000Z
This expands the code which generates unique IDs for Markdown headers within a single block to each rendered page. fixes #25001 fixes #29449
diff --git a/src/librustdoc/html/markdown.rs b/src/librustdoc/html/markdown.rs
@@ -14,7 +14,7 @@
 //! (bundled into the rust runtime). This module self-contains the C bindings
 //! and necessary legwork to render markdown, and exposes all of the
 //! functionality through a unit-struct, `Markdown`, which has an implementation
-//! of `fmt::String`. Example usage:
+//! of `fmt::Display`. Example usage:
 //!
 //! ```rust,ignore
 //! use rustdoc::html::markdown::Markdown;
@@ -29,19 +29,19 @@
 use libc;
 use std::ascii::AsciiExt;
 use std::cell::RefCell;
-use std::collections::HashMap;
 use std::default::Default;
 use std::ffi::CString;
 use std::fmt;
 use std::slice;
 use std::str;
 
+use html::render::derive_id;
 use html::toc::TocBuilder;
 use html::highlight;
 use html::escape::Escape;
 use test;
 
-/// A unit struct which has the `fmt::String` trait implemented. When
+/// A unit struct which has the `fmt::Display` trait implemented. When
 /// formatted, this struct will emit the HTML corresponding to the rendered
 /// version of the contained markdown string.
 pub struct Markdown<'a>(pub &'a str);
@@ -210,10 +210,6 @@ fn collapse_whitespace(s: &str) -> String {
     s.split_whitespace().collect::<Vec<_>>().join(" ")
 }
 
-thread_local!(static USED_HEADER_MAP: RefCell<HashMap<String, usize>> = {
-    RefCell::new(HashMap::new())
-});
-
 thread_local!(pub static PLAYGROUND_KRATE: RefCell<Option<Option<String>>> = {
     RefCell::new(None)
 });
@@ -311,16 +307,7 @@ pub fn render(w: &mut fmt::Formatter, s: &str, print_toc: bool) -> fmt::Result {
         let opaque = unsafe { (*data).opaque as *mut hoedown_html_renderer_state };
         let opaque = unsafe { &mut *((*opaque).opaque as *mut MyOpaque) };
 
-        // Make sure our hyphenated ID is unique for this page
-        let id = USED_HEADER_MAP.with(|map| {
-            let id = match map.borrow_mut().get_mut(&id) {
-                None => id,
-                Some(a) => { *a += 1; format!("{}-{}", id, *a - 1) }
-            };
-            map.borrow_mut().insert(id.clone(), 1);
-            id
-        });
-
+        let id = derive_id(id);
 
         let sec = opaque.toc_builder.as_mut().map_or("".to_owned(), |builder| {
             format!("{} ", builder.push(level as u32, s.clone(), id.clone()))
@@ -335,8 +322,6 @@ pub fn render(w: &mut fmt::Formatter, s: &str, print_toc: bool) -> fmt::Result {
         unsafe { hoedown_buffer_puts(ob, text.as_ptr()) }
     }
 
-    reset_headers();
-
     extern fn codespan(
         ob: *mut hoedown_buffer,
         text: *const hoedown_buffer,
@@ -500,18 +485,6 @@ impl LangString {
     }
 }
 
-/// By default this markdown renderer generates anchors for each header in the
-/// rendered document. The anchor name is the contents of the header separated
-/// by hyphens, and a thread-local map is used to disambiguate among duplicate
-/// headers (numbers are appended).
-///
-/// This method will reset the local table for these headers. This is typically
-/// used at the beginning of rendering an entire HTML page to reset from the
-/// previous state (if any).
-pub fn reset_headers() {
-    USED_HEADER_MAP.with(|s| s.borrow_mut().clear());
-}
-
 impl<'a> fmt::Display for Markdown<'a> {
     fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result {
         let Markdown(md) = *self;
@@ -579,6 +552,7 @@ pub fn plain_summary_line(md: &str) -> String {
 mod tests {
     use super::{LangString, Markdown};
     use super::plain_summary_line;
+    use html::render::reset_ids;
 
     #[test]
     fn test_lang_string_parse() {
@@ -611,13 +585,15 @@ mod tests {
     fn issue_17736() {
         let markdown = "# title";
         format!("{}", Markdown(markdown));
+        reset_ids();
     }
 
     #[test]
     fn test_header() {
         fn t(input: &str, expect: &str) {
             let output = format!("{}", Markdown(input));
             assert_eq!(output, expect);
+            reset_ids();
         }
 
         t("# Foo bar", "\n<h1 id='foo-bar' class='section-header'>\
@@ -634,6 +610,32 @@ mod tests {
           <em><code>baz</code></em> ❤ #qux</a></h4>");
     }
 
+    #[test]
+    fn test_header_ids_multiple_blocks() {
+        fn t(input: &str, expect: &str) {
+            let output = format!("{}", Markdown(input));
+            assert_eq!(output, expect);
+        }
+
+        let test = || {
+            t("# Example", "\n<h1 id='example' class='section-header'>\
+              <a href='#example'>Example</a></h1>");
+            t("# Panics", "\n<h1 id='panics' class='section-header'>\
+              <a href='#panics'>Panics</a></h1>");
+            t("# Example", "\n<h1 id='example-1' class='section-header'>\
+              <a href='#example-1'>Example</a></h1>");
+            t("# Main", "\n<h1 id='main-1' class='section-header'>\
+              <a href='#main-1'>Main</a></h1>");
+            t("# Example", "\n<h1 id='example-2' class='section-header'>\
+              <a href='#example-2'>Example</a></h1>");
+            t("# Panics", "\n<h1 id='panics-1' class='section-header'>\
+              <a href='#panics-1'>Panics</a></h1>");
+        };
+        test();
+        reset_ids();
+        test();
+    }
+
     #[test]
     fn test_plain_summary_line() {
         fn t(input: &str, expect: &str) {
diff --git a/src/librustdoc/html/render.rs b/src/librustdoc/html/render.rs
@@ -342,6 +342,51 @@ impl fmt::Display for IndexItemFunctionType {
 thread_local!(static CACHE_KEY: RefCell<Arc<Cache>> = Default::default());
 thread_local!(pub static CURRENT_LOCATION_KEY: RefCell<Vec<String>> =
                     RefCell::new(Vec::new()));
+thread_local!(static USED_ID_MAP: RefCell<HashMap<String, usize>> =
+                    RefCell::new(init_ids()));
+
+fn init_ids() -> HashMap<String, usize> {
+    [
+     "main",
+     "search",
+     "help",
+     "TOC",
+     "render-detail",
+     "associated-types",
+     "associated-const",
+     "required-methods",
+     "provided-methods",
+     "implementors",
+     "implementors-list",
+     "methods",
+     "deref-methods",
+     "implementations",
+     "derived_implementations"
+     ].into_iter().map(|id| (String::from(*id), 1)).collect::<HashMap<_, _>>()
+}
+
+/// This method resets the local table of used ID attributes. This is typically
+/// used at the beginning of rendering an entire HTML page to reset from the
+/// previous state (if any).
+pub fn reset_ids() {
+    USED_ID_MAP.with(|s| *s.borrow_mut() = init_ids());
+}
+
+pub fn derive_id(candidate: String) -> String {
+    USED_ID_MAP.with(|map| {
+        let id = match map.borrow_mut().get_mut(&candidate) {
+            None => candidate,
+            Some(a) => {
+                let id = format!("{}-{}", candidate, *a);
+                *a += 1;
+                id
+            }
+        };
+
+        map.borrow_mut().insert(id.clone(), 1);
+        id
+    })
+}
 
 /// Generates the documentation for `crate` into the directory `dst`
 pub fn run(mut krate: clean::Crate,
@@ -1276,7 +1321,7 @@ impl Context {
                 keywords: &keywords,
             };
 
-            markdown::reset_headers();
+            reset_ids();
 
             // We have a huge number of calls to write, so try to alleviate some
             // of the pain by using a buffered writer instead of invoking the
@@ -1698,10 +1743,9 @@ fn item_module(w: &mut fmt::Formatter, cx: &Context,
                 ItemType::AssociatedType  => ("associated-types", "Associated Types"),
                 ItemType::AssociatedConst => ("associated-consts", "Associated Constants"),
             };
-            try!(write!(w,
-                        "<h2 id='{id}' class='section-header'>\
-                        <a href=\"#{id}\">{name}</a></h2>\n<table>",
-                        id = short, name = name));
+            try!(write!(w, "<h2 id='{id}' class='section-header'>\
+                           <a href=\"#{id}\">{name}</a></h2>\n<table>",
+                           id = derive_id(short.to_owned()), name = name));
         }
 
         match myitem.inner {
@@ -1922,10 +1966,11 @@ fn item_trait(w: &mut fmt::Formatter, cx: &Context, it: &clean::Item,
 
     fn trait_item(w: &mut fmt::Formatter, cx: &Context, m: &clean::Item)
                   -> fmt::Result {
-        try!(write!(w, "<h3 id='{ty}.{name}' class='method stab {stab}'><code>",
-                    ty = shortty(m),
-                    name = *m.name.as_ref().unwrap(),
-                    stab = m.stability_class()));
+        let name = m.name.as_ref().unwrap();
+        let id = derive_id(format!("{}.{}", shortty(m), name));
+        try!(write!(w, "<h3 id='{id}' class='method stab {stab}'><code>",
+                       id = id,
+                       stab = m.stability_class()));
         try!(render_assoc_item(w, m, AssocItemLink::Anchor));
         try!(write!(w, "</code></h3>"));
         try!(document(w, cx, m));
@@ -2420,44 +2465,38 @@ fn render_impl(w: &mut fmt::Formatter, cx: &Context, i: &Impl, link: AssocItemLi
 
     fn doctraititem(w: &mut fmt::Formatter, cx: &Context, item: &clean::Item,
                     link: AssocItemLink, render_static: bool) -> fmt::Result {
+        let name = item.name.as_ref().unwrap();
         match item.inner {
             clean::MethodItem(..) | clean::TyMethodItem(..) => {
                 // Only render when the method is not static or we allow static methods
                 if !is_static_method(item) || render_static {
-                    try!(write!(w, "<h4 id='method.{}' class='{}'><code>",
-                                *item.name.as_ref().unwrap(),
-                                shortty(item)));
+                    let id = derive_id(format!("method.{}", name));
+                    try!(write!(w, "<h4 id='{}' class='{}'><code>", id, shortty(item)));
                 try!(render_assoc_item(w, item, link));
                     try!(write!(w, "</code></h4>\n"));
                 }
             }
             clean::TypedefItem(ref tydef, _) => {
-                let name = item.name.as_ref().unwrap();
-                try!(write!(w, "<h4 id='assoc_type.{}' class='{}'><code>",
-                            *name,
-                            shortty(item)));
+                let id = derive_id(format!("assoc_type.{}", name));
+                try!(write!(w, "<h4 id='{}' class='{}'><code>", id, shortty(item)));
                 try!(write!(w, "type {} = {}", name, tydef.type_));
                 try!(write!(w, "</code></h4>\n"));
             }
             clean::AssociatedConstItem(ref ty, ref default) => {
-                let name = item.name.as_ref().unwrap();
-                try!(write!(w, "<h4 id='assoc_const.{}' class='{}'><code>",
-                            *name, shortty(item)));
+                let id = derive_id(format!("assoc_const.{}", name));
+                try!(write!(w, "<h4 id='{}' class='{}'><code>", id, shortty(item)));
                 try!(assoc_const(w, item, ty, default.as_ref()));
                 try!(write!(w, "</code></h4>\n"));
             }
             clean::ConstantItem(ref c) => {
-                let name = item.name.as_ref().unwrap();
-                try!(write!(w, "<h4 id='assoc_const.{}' class='{}'><code>",
-                            *name, shortty(item)));
+                let id = derive_id(format!("assoc_const.{}", name));
+                try!(write!(w, "<h4 id='{}' class='{}'><code>", id, shortty(item)));
                 try!(assoc_const(w, item, &c.type_, Some(&c.expr)));
                 try!(write!(w, "</code></h4>\n"));
             }
             clean::AssociatedTypeItem(ref bounds, ref default) => {
-                let name = item.name.as_ref().unwrap();
-                try!(write!(w, "<h4 id='assoc_type.{}' class='{}'><code>",
-                            *name,
-                            shortty(item)));
+                let id = derive_id(format!("assoc_type.{}", name));
+                try!(write!(w, "<h4 id='{}' class='{}'><code>", id, shortty(item)));
                 try!(assoc_type(w, item, bounds, default));
                 try!(write!(w, "</code></h4>\n"));
             }
@@ -2671,3 +2710,22 @@ fn get_index_type_name(clean_type: &clean::Type) -> Option<String> {
 pub fn cache() -> Arc<Cache> {
     CACHE_KEY.with(|c| c.borrow().clone())
 }
+
+#[cfg(test)]
+#[test]
+fn test_unique_id() {
+    let input = ["foo", "examples", "examples", "method.into_iter","examples",
+                 "method.into_iter", "foo", "main", "search", "methods",
+                 "examples", "method.into_iter", "assoc_type.Item", "assoc_type.Item"];
+    let expected = ["foo", "examples", "examples-1", "method.into_iter", "examples-2",
+                    "method.into_iter-1", "foo-1", "main-1", "search-1", "methods-1",
+                    "examples-3", "method.into_iter-2", "assoc_type.Item", "assoc_type.Item-1"];
+
+    let test = || {
+        let actual: Vec<String> = input.iter().map(|s| derive_id(s.to_string())).collect();
+        assert_eq!(&actual[..], expected);
+    };
+    test();
+    reset_ids();
+    test();
+}
diff --git a/src/librustdoc/markdown.rs b/src/librustdoc/markdown.rs
@@ -21,9 +21,10 @@ use rustc::session::search_paths::SearchPaths;
 
 use externalfiles::ExternalHtml;
 
+use html::render::reset_ids;
 use html::escape::Escape;
 use html::markdown;
-use html::markdown::{Markdown, MarkdownWithToc, find_testable_code, reset_headers};
+use html::markdown::{Markdown, MarkdownWithToc, find_testable_code};
 use test::{TestOptions, Collector};
 
 /// Separate any lines at the start of the file that begin with `%`.
@@ -82,7 +83,7 @@ pub fn render(input: &str, mut output: PathBuf, matches: &getopts::Matches,
     }
     let title = metadata[0];
 
-    reset_headers();
+    reset_ids();
 
     let rendered = if include_toc {
         format!("{}", MarkdownWithToc(text))
diff --git a/src/test/rustdoc/issue-25001.rs b/src/test/rustdoc/issue-25001.rs
@@ -0,0 +1,53 @@
+// Copyright 2015 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.
+
+// @has issue_25001/struct.Foo.html
+pub struct Foo<T>(T);
+
+pub trait Bar {
+    type Item;
+
+    fn quux(self);
+}
+
+impl<T> Foo<T> {
+    // @has - '//*[@id="method.pass"]//code' 'fn pass()'
+    pub fn pass() {}
+}
+impl<T> Foo<T> {
+    // @has - '//*[@id="method.pass-1"]//code' 'fn pass() -> usize'
+    pub fn pass() -> usize { 42 }
+}
+impl<T> Foo<T> {
+    // @has - '//*[@id="method.pass-2"]//code' 'fn pass() -> isize'
+    pub fn pass() -> isize { 42 }
+}
+
+impl<T> Bar for Foo<T> {
+    // @has - '//*[@id="assoc_type.Item"]//code' 'type Item = T'
+    type Item=T;
+
+    // @has - '//*[@id="method.quux"]//code' 'fn quux(self)'
+    fn quux(self) {}
+}
+impl<'a, T> Bar for &'a Foo<T> {
+    // @has - '//*[@id="assoc_type.Item-1"]//code' "type Item = &'a T"
+    type Item=&'a T;
+
+    // @has - '//*[@id="method.quux-1"]//code' 'fn quux(self)'
+    fn quux(self) {}
+}
+impl<'a, T> Bar for &'a mut Foo<T> {
+    // @has - '//*[@id="assoc_type.Item-2"]//code' "type Item = &'a mut T"
+    type Item=&'a mut T;
+
+    // @has - '//*[@id="method.quux-2"]//code' 'fn quux(self)'
+    fn quux(self) {}
+}
diff --git a/src/test/rustdoc/issue-29449.rs b/src/test/rustdoc/issue-29449.rs
