From 7bea518d3a792d9c8df508b0af83ceb857ce75b7 Mon Sep 17 00:00:00 2001 From: Mark Rousskov Date: Sun, 22 Jul 2018 07:25:00 -0600 Subject: [PATCH] Remove global derive_id and reset_ids functions Previously these functions relied on TLS but we can instead thread the relevant state through explicitly. --- src/librustdoc/externalfiles.rs | 12 +- src/librustdoc/html/markdown.rs | 154 ++++++++++++++++-------- src/librustdoc/html/render.rs | 147 +++++++++------------- src/librustdoc/lib.rs | 6 +- src/librustdoc/markdown.rs | 11 +- src/tools/error_index_generator/main.rs | 13 +- 6 files changed, 190 insertions(+), 153 deletions(-) diff --git a/src/librustdoc/externalfiles.rs b/src/librustdoc/externalfiles.rs index 67502ab8e153..9631ea059cc4 100644 --- a/src/librustdoc/externalfiles.rs +++ b/src/librustdoc/externalfiles.rs @@ -13,7 +13,8 @@ use std::path::Path; use std::str; use errors; use syntax::feature_gate::UnstableFeatures; -use html::markdown::{ErrorCodes, Markdown}; +use html::markdown::{IdMap, ErrorCodes, Markdown}; +use std::cell::RefCell; #[derive(Clone)] pub struct ExternalHtml { @@ -30,7 +31,8 @@ pub struct ExternalHtml { impl ExternalHtml { pub fn load(in_header: &[String], before_content: &[String], after_content: &[String], - md_before_content: &[String], md_after_content: &[String], diag: &errors::Handler) + md_before_content: &[String], md_after_content: &[String], diag: &errors::Handler, + id_map: &mut IdMap) -> Option { let codes = ErrorCodes::from(UnstableFeatures::from_environment().is_nightly_build()); load_external_files(in_header, diag) @@ -40,7 +42,8 @@ impl ExternalHtml { ) .and_then(|(ih, bc)| load_external_files(md_before_content, diag) - .map(|m_bc| (ih, format!("{}{}", bc, Markdown(&m_bc, &[], codes)))) + .map(|m_bc| (ih, + format!("{}{}", bc, Markdown(&m_bc, &[], RefCell::new(id_map), codes)))) ) .and_then(|(ih, bc)| load_external_files(after_content, diag) @@ -48,7 +51,8 @@ impl ExternalHtml { ) .and_then(|(ih, bc, ac)| load_external_files(md_after_content, diag) - .map(|m_ac| (ih, bc, format!("{}{}", ac, Markdown(&m_ac, &[], codes)))) + .map(|m_ac| (ih, bc, + format!("{}{}", ac, Markdown(&m_ac, &[], RefCell::new(id_map), codes)))) ) .map(|(ih, bc, ac)| ExternalHtml { diff --git a/src/librustdoc/html/markdown.rs b/src/librustdoc/html/markdown.rs index 8f8973875649..706671f21eff 100644 --- a/src/librustdoc/html/markdown.rs +++ b/src/librustdoc/html/markdown.rs @@ -18,10 +18,12 @@ //! ``` //! #![feature(rustc_private)] //! -//! use rustdoc::html::markdown::{Markdown, ErrorCodes}; +//! use rustdoc::html::markdown::{IdMap, Markdown, ErrorCodes}; +//! use std::cell::RefCell; //! //! let s = "My *markdown* _text_"; -//! let html = format!("{}", Markdown(s, &[], ErrorCodes::Yes)); +//! let mut id_map = IdMap::new(); +//! let html = format!("{}", Markdown(s, &[], RefCell::new(&mut id_map), ErrorCodes::Yes)); //! // ... something using html //! ``` @@ -35,7 +37,6 @@ use std::borrow::Cow; use std::ops::Range; use std::str; -use html::render::derive_id; use html::toc::TocBuilder; use html::highlight; use test; @@ -47,12 +48,13 @@ use pulldown_cmark::{Options, OPTION_ENABLE_FOOTNOTES, OPTION_ENABLE_TABLES}; /// formatted, this struct will emit the HTML corresponding to the rendered /// version of the contained markdown string. /// The second parameter is a list of link replacements -pub struct Markdown<'a>(pub &'a str, pub &'a [(String, String)], pub ErrorCodes); +pub struct Markdown<'a>( + pub &'a str, pub &'a [(String, String)], pub RefCell<&'a mut IdMap>, pub ErrorCodes); /// A unit struct like `Markdown`, that renders the markdown with a /// table of contents. -pub struct MarkdownWithToc<'a>(pub &'a str, pub ErrorCodes); +pub struct MarkdownWithToc<'a>(pub &'a str, pub RefCell<&'a mut IdMap>, pub ErrorCodes); /// A unit struct like `Markdown`, that renders the markdown escaping HTML tags. -pub struct MarkdownHtml<'a>(pub &'a str, pub ErrorCodes); +pub struct MarkdownHtml<'a>(pub &'a str, pub RefCell<&'a mut IdMap>, pub ErrorCodes); /// A unit struct like `Markdown`, that renders only the first paragraph. pub struct MarkdownSummaryLine<'a>(pub &'a str, pub &'a [(String, String)]); @@ -287,23 +289,25 @@ impl<'a, 'b, I: Iterator>> Iterator for LinkReplacer<'a, 'b, I> } /// Make headings links with anchor ids and build up TOC. -struct HeadingLinks<'a, 'b, I: Iterator>> { +struct HeadingLinks<'a, 'b, 'ids, I: Iterator>> { inner: I, toc: Option<&'b mut TocBuilder>, buf: VecDeque>, + id_map: &'ids mut IdMap, } -impl<'a, 'b, I: Iterator>> HeadingLinks<'a, 'b, I> { - fn new(iter: I, toc: Option<&'b mut TocBuilder>) -> Self { +impl<'a, 'b, 'ids, I: Iterator>> HeadingLinks<'a, 'b, 'ids, I> { + fn new(iter: I, toc: Option<&'b mut TocBuilder>, ids: &'ids mut IdMap) -> Self { HeadingLinks { inner: iter, toc, buf: VecDeque::new(), + id_map: ids, } } } -impl<'a, 'b, I: Iterator>> Iterator for HeadingLinks<'a, 'b, I> { +impl<'a, 'b, 'ids, I: Iterator>> Iterator for HeadingLinks<'a, 'b, 'ids, I> { type Item = Event<'a>; fn next(&mut self) -> Option { @@ -322,7 +326,7 @@ impl<'a, 'b, I: Iterator>> Iterator for HeadingLinks<'a, 'b, I> } self.buf.push_back(event); } - let id = derive_id(id); + let id = self.id_map.derive(id); if let Some(ref mut builder) = self.toc { let mut html_header = String::new(); @@ -641,7 +645,8 @@ impl LangString { impl<'a> fmt::Display for Markdown<'a> { fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result { - let Markdown(md, links, codes) = *self; + let Markdown(md, links, ref ids, codes) = *self; + let mut ids = ids.borrow_mut(); // This is actually common enough to special-case if md.is_empty() { return Ok(()) } @@ -661,7 +666,7 @@ impl<'a> fmt::Display for Markdown<'a> { let mut s = String::with_capacity(md.len() * 3 / 2); - let p = HeadingLinks::new(p, None); + let p = HeadingLinks::new(p, None, &mut ids); let p = LinkReplacer::new(p, links); let p = CodeBlocks::new(p, codes); let p = Footnotes::new(p); @@ -673,7 +678,8 @@ impl<'a> fmt::Display for Markdown<'a> { impl<'a> fmt::Display for MarkdownWithToc<'a> { fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result { - let MarkdownWithToc(md, codes) = *self; + let MarkdownWithToc(md, ref ids, codes) = *self; + let mut ids = ids.borrow_mut(); let mut opts = Options::empty(); opts.insert(OPTION_ENABLE_TABLES); @@ -686,7 +692,7 @@ impl<'a> fmt::Display for MarkdownWithToc<'a> { let mut toc = TocBuilder::new(); { - let p = HeadingLinks::new(p, Some(&mut toc)); + let p = HeadingLinks::new(p, Some(&mut toc), &mut ids); let p = CodeBlocks::new(p, codes); let p = Footnotes::new(p); html::push_html(&mut s, p); @@ -700,7 +706,8 @@ impl<'a> fmt::Display for MarkdownWithToc<'a> { impl<'a> fmt::Display for MarkdownHtml<'a> { fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result { - let MarkdownHtml(md, codes) = *self; + let MarkdownHtml(md, ref ids, codes) = *self; + let mut ids = ids.borrow_mut(); // This is actually common enough to special-case if md.is_empty() { return Ok(()) } @@ -718,7 +725,7 @@ impl<'a> fmt::Display for MarkdownHtml<'a> { let mut s = String::with_capacity(md.len() * 3 / 2); - let p = HeadingLinks::new(p, None); + let p = HeadingLinks::new(p, None, &mut ids); let p = CodeBlocks::new(p, codes); let p = Footnotes::new(p); html::push_html(&mut s, p); @@ -835,7 +842,10 @@ pub fn markdown_links(md: &str) -> Vec<(String, Option>)> { let p = Parser::new_with_broken_link_callback(md, opts, Some(&push)); - let iter = Footnotes::new(HeadingLinks::new(p, None)); + // There's no need to thread an IdMap through to here because + // the IDs generated aren't going to be emitted anywhere. + let mut ids = IdMap::new(); + let iter = Footnotes::new(HeadingLinks::new(p, None, &mut ids)); for ev in iter { if let Event::Start(Tag::Link(dest, _)) = ev { @@ -854,11 +864,67 @@ pub fn markdown_links(md: &str) -> Vec<(String, Option>)> { links } +#[derive(Default)] +pub struct IdMap { + map: HashMap, +} + +impl IdMap { + pub fn new() -> Self { + IdMap::default() + } + + pub fn populate>(&mut self, ids: I) { + for id in ids { + let _ = self.derive(id); + } + } + + pub fn reset(&mut self) { + self.map = HashMap::new(); + } + + pub fn derive(&mut self, candidate: String) -> String { + let id = match self.map.get_mut(&candidate) { + None => candidate, + Some(a) => { + let id = format!("{}-{}", candidate, *a); + *a += 1; + id + } + }; + + self.map.insert(id.clone(), 1); + id + } +} + +#[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", "search", "methods", + "examples-3", "method.into_iter-2", "assoc_type.Item", "assoc_type.Item-1"]; + + let map = RefCell::new(IdMap::new()); + let test = || { + let mut map = map.borrow_mut(); + let actual: Vec = input.iter().map(|s| map.derive(s.to_string())).collect(); + assert_eq!(&actual[..], expected); + }; + test(); + map.borrow_mut().reset(); + test(); +} + #[cfg(test)] mod tests { - use super::{ErrorCodes, LangString, Markdown, MarkdownHtml}; + use super::{ErrorCodes, LangString, Markdown, MarkdownHtml, IdMap}; use super::plain_summary_line; - use html::render::reset_ids; + use std::cell::RefCell; #[test] fn test_lang_string_parse() { @@ -901,19 +967,12 @@ mod tests { t("text,no_run", false, true, false, false, false, false, false, v()); } - #[test] - fn issue_17736() { - let markdown = "# title"; - Markdown(markdown, &[], ErrorCodes::Yes).to_string(); - reset_ids(true); - } - #[test] fn test_header() { fn t(input: &str, expect: &str) { - let output = Markdown(input, &[], ErrorCodes::Yes).to_string(); + let mut map = IdMap::new(); + let output = Markdown(input, &[], RefCell::new(&mut map), ErrorCodes::Yes).to_string(); assert_eq!(output, expect, "original: {}", input); - reset_ids(true); } t("# Foo bar", "

\ @@ -932,28 +991,24 @@ mod tests { #[test] fn test_header_ids_multiple_blocks() { - fn t(input: &str, expect: &str) { - let output = Markdown(input, &[], ErrorCodes::Yes).to_string(); + let mut map = IdMap::new(); + fn t(map: &mut IdMap, input: &str, expect: &str) { + let output = Markdown(input, &[], RefCell::new(map), ErrorCodes::Yes).to_string(); assert_eq!(output, expect, "original: {}", input); } - let test = || { - t("# Example", "

\ - Example

"); - t("# Panics", "

\ - Panics

"); - t("# Example", "

\ - Example

"); - t("# Main", "

\ - Main

"); - t("# Example", "

\ - Example

"); - t("# Panics", "

\ - Panics

"); - }; - test(); - reset_ids(true); - test(); + t(&mut map, "# Example", "

\ + Example

"); + t(&mut map, "# Panics", "

\ + Panics

"); + t(&mut map, "# Example", "

\ + Example

"); + t(&mut map, "# Main", "

\ + Main

"); + t(&mut map, "# Example", "

\ + Example

"); + t(&mut map, "# Panics", "

\ + Panics

"); } #[test] @@ -974,7 +1029,8 @@ mod tests { #[test] fn test_markdown_html_escape() { fn t(input: &str, expect: &str) { - let output = MarkdownHtml(input, ErrorCodes::Yes).to_string(); + let mut idmap = IdMap::new(); + let output = MarkdownHtml(input, RefCell::new(&mut idmap), ErrorCodes::Yes).to_string(); assert_eq!(output, expect, "original: {}", input); } diff --git a/src/librustdoc/html/render.rs b/src/librustdoc/html/render.rs index b9b058cb5482..c1375fd37fd6 100644 --- a/src/librustdoc/html/render.rs +++ b/src/librustdoc/html/render.rs @@ -50,6 +50,7 @@ use std::mem; use std::path::{PathBuf, Path, Component}; use std::str; use std::sync::Arc; +use std::rc::Rc; use externalfiles::ExternalHtml; @@ -73,7 +74,7 @@ use html::format::{GenericBounds, WhereClause, href, AbiSpace}; use html::format::{VisSpace, Method, UnsafetySpace, MutableSpace}; use html::format::fmt_impl_for_trait_page; use html::item_type::ItemType; -use html::markdown::{self, Markdown, MarkdownHtml, MarkdownSummaryLine, ErrorCodes}; +use html::markdown::{self, Markdown, MarkdownHtml, MarkdownSummaryLine, ErrorCodes, IdMap}; use html::{highlight, layout}; use minifier; @@ -89,7 +90,7 @@ pub type NameDoc = (String, Option); /// easily cloned because it is cloned per work-job (about once per item in the /// rustdoc tree). #[derive(Clone)] -pub struct Context { +struct Context { /// Current hierarchy of components leading down to what's currently being /// rendered pub current: Vec, @@ -101,10 +102,12 @@ pub struct Context { /// publicly reused items to redirect to the right location. pub render_redirect_pages: bool, pub codes: ErrorCodes, + /// The map used to ensure all generated 'id=' attributes are unique. + id_map: Rc>, pub shared: Arc, } -pub struct SharedContext { +struct SharedContext { /// The path to the crate root source minus the file name. /// Used for simplifying paths to the highlighted source code files. pub src_root: PathBuf, @@ -452,9 +455,8 @@ impl ToJson for IndexItemFunctionType { thread_local!(static CACHE_KEY: RefCell> = Default::default()); thread_local!(pub static CURRENT_LOCATION_KEY: RefCell> = RefCell::new(Vec::new())); -thread_local!(pub static USED_ID_MAP: RefCell> = RefCell::new(init_ids())); -fn init_ids() -> FxHashMap { +pub fn initial_ids() -> Vec { [ "main", "search", @@ -472,36 +474,7 @@ fn init_ids() -> FxHashMap { "methods", "deref-methods", "implementations", - ].into_iter().map(|id| (String::from(*id), 1)).collect() -} - -/// 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(embedded: bool) { - USED_ID_MAP.with(|s| { - *s.borrow_mut() = if embedded { - init_ids() - } else { - FxHashMap() - }; - }); -} - -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 - }) + ].into_iter().map(|id| (String::from(*id))).collect() } /// Generates the documentation for `crate` into the directory `dst` @@ -515,7 +488,8 @@ pub fn run(mut krate: clean::Crate, renderinfo: RenderInfo, sort_modules_alphabetically: bool, themes: Vec, - enable_minification: bool) -> Result<(), Error> { + enable_minification: bool, + id_map: IdMap) -> Result<(), Error> { let src_root = match krate.src { FileName::Real(ref p) => match p.parent() { Some(p) => p.to_path_buf(), @@ -584,6 +558,7 @@ pub fn run(mut krate: clean::Crate, dst, render_redirect_pages: false, codes: ErrorCodes::from(UnstableFeatures::from_environment().is_nightly_build()), + id_map: Rc::new(RefCell::new(id_map)), shared: Arc::new(scx), }; @@ -1711,6 +1686,11 @@ impl<'a> fmt::Display for Settings<'a> { } impl Context { + fn derive_id(&self, id: String) -> String { + let mut map = self.id_map.borrow_mut(); + map.derive(id) + } + /// String representation of how to get back to the root path of the 'doc/' /// folder in terms of a relative URL. fn root_path(&self) -> String { @@ -1865,7 +1845,10 @@ impl Context { resource_suffix: &self.shared.resource_suffix, }; - reset_ids(true); + { + self.id_map.borrow_mut().reset(); + self.id_map.borrow_mut().populate(initial_ids()); + } if !self.render_redirect_pages { layout::render(writer, &self.shared.layout, &page, @@ -2222,16 +2205,18 @@ fn document(w: &mut fmt::Formatter, cx: &Context, item: &clean::Item) -> fmt::Re /// Render md_text as markdown. fn render_markdown(w: &mut fmt::Formatter, + cx: &Context, md_text: &str, links: Vec<(String, String)>, - prefix: &str, - codes: ErrorCodes) + prefix: &str) -> fmt::Result { - write!(w, "
{}{}
", prefix, Markdown(md_text, &links, codes)) + let mut ids = cx.id_map.borrow_mut(); + write!(w, "
{}{}
", + prefix, Markdown(md_text, &links, RefCell::new(&mut ids), cx.codes)) } -fn document_short(w: &mut fmt::Formatter, item: &clean::Item, link: AssocItemLink, - prefix: &str, codes: ErrorCodes) -> fmt::Result { +fn document_short(w: &mut fmt::Formatter, cx: &Context, item: &clean::Item, link: AssocItemLink, + prefix: &str) -> fmt::Result { if let Some(s) = item.doc_value() { let markdown = if s.contains('\n') { format!("{} [Read more]({})", @@ -2239,7 +2224,7 @@ fn document_short(w: &mut fmt::Formatter, item: &clean::Item, link: AssocItemLin } else { plain_summary_line(Some(s)).to_string() }; - render_markdown(w, &markdown, item.links(), prefix, codes)?; + render_markdown(w, cx, &markdown, item.links(), prefix)?; } else if !prefix.is_empty() { write!(w, "
{}
", prefix)?; } @@ -2265,7 +2250,7 @@ fn document_full(w: &mut fmt::Formatter, item: &clean::Item, cx: &Context, prefix: &str) -> fmt::Result { if let Some(s) = cx.shared.maybe_collapsed_doc_value(item) { debug!("Doc block: =====\n{}\n=====", s); - render_markdown(w, &*s, item.links(), prefix, cx.codes)?; + render_markdown(w, cx, &*s, item.links(), prefix)?; } else if !prefix.is_empty() { write!(w, "
{}
", prefix)?; } @@ -2431,7 +2416,7 @@ fn item_module(w: &mut fmt::Formatter, cx: &Context, let (short, name) = item_ty_to_strs(&myty.unwrap()); write!(w, "

\ {name}

\n", - id = derive_id(short.to_owned()), name = name)?; + id = cx.derive_id(short.to_owned()), name = name)?; } match myitem.inner { @@ -2526,7 +2511,8 @@ fn short_stability(item: &clean::Item, cx: &Context, show_reason: bool) -> Vec Vec", unstable_extra)); } else { + let mut ids = cx.id_map.borrow_mut(); let text = format!("🔬 \ This is a nightly-only experimental API. {}\ {}", unstable_extra, MarkdownHtml( &stab.unstable_reason, + RefCell::new(&mut ids), error_codes)); stability.push(format!("
{}
", text)); @@ -2583,14 +2571,15 @@ fn short_stability(item: &clean::Item, cx: &Context, show_reason: bool) -> Vec{}", text)) } @@ -2831,8 +2820,8 @@ fn item_trait( -> fmt::Result { let name = m.name.as_ref().unwrap(); let item_type = m.type_(); - let id = derive_id(format!("{}.{}", item_type, name)); - let ns_id = derive_id(format!("{}.{}", name, item_type.name_space())); + let id = cx.derive_id(format!("{}.{}", item_type, name)); + let ns_id = cx.derive_id(format!("{}.{}", name, item_type.name_space())); write!(w, "{extra}

\

")?; if let Some(ref dox) = cx.shared.maybe_collapsed_doc_value(&i.impl_item) { + let mut ids = cx.id_map.borrow_mut(); write!(w, "
{}
", - Markdown(&*dox, &i.impl_item.links(), cx.codes))?; + Markdown(&*dox, &i.impl_item.links(), RefCell::new(&mut ids), cx.codes))?; } } @@ -3836,8 +3826,8 @@ fn render_impl(w: &mut fmt::Formatter, cx: &Context, i: &Impl, link: AssocItemLi clean::TyMethodItem(clean::TyMethod{ ref decl, .. }) => { // Only render when the method is not static or we allow static methods if render_method_item { - let id = derive_id(format!("{}.{}", item_type, name)); - let ns_id = derive_id(format!("{}.{}", name, item_type.name_space())); + let id = cx.derive_id(format!("{}.{}", item_type, name)); + let ns_id = cx.derive_id(format!("{}.{}", name, item_type.name_space())); write!(w, "

", id, item_type)?; write!(w, "{}", spotlight_decl(decl)?)?; write!(w, "