Skip to content

Commit

Permalink
Rollup merge of #133000 - GuillaumeGomez:footnote-ids, r=notriddle
Browse files Browse the repository at this point in the history
[rustdoc] Fix duplicated footnote IDs

Fixes #131901.

Footnote IDs were increased locally (ie, on the docblock) and not globally (ie, on the whole item page).

cc `@aDotInTheVoid`
r? `@notriddle`
  • Loading branch information
matthiaskrgr authored Nov 13, 2024
2 parents 59c5d7d + 7a8257d commit 292cac9
Show file tree
Hide file tree
Showing 4 changed files with 92 additions and 32 deletions.
46 changes: 30 additions & 16 deletions src/librustdoc/html/markdown.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1333,12 +1333,14 @@ impl Markdown<'_> {

let mut s = String::with_capacity(md.len() * 3 / 2);

let p = HeadingLinks::new(p, None, ids, heading_offset);
let p = footnotes::Footnotes::new(p);
let p = LinkReplacer::new(p.map(|(ev, _)| ev), links);
let p = TableWrapper::new(p);
let p = CodeBlocks::new(p, codes, edition, playground);
html::push_html(&mut s, p);
ids.handle_footnotes(|ids, existing_footnotes| {
let p = HeadingLinks::new(p, None, ids, heading_offset);
let p = footnotes::Footnotes::new(p, existing_footnotes);
let p = LinkReplacer::new(p.map(|(ev, _)| ev), links);
let p = TableWrapper::new(p);
let p = CodeBlocks::new(p, codes, edition, playground);
html::push_html(&mut s, p);
});

s
}
Expand Down Expand Up @@ -1367,13 +1369,13 @@ impl MarkdownWithToc<'_> {

let mut toc = TocBuilder::new();

{
ids.handle_footnotes(|ids, existing_footnotes| {
let p = HeadingLinks::new(p, Some(&mut toc), ids, HeadingOffset::H1);
let p = footnotes::Footnotes::new(p);
let p = footnotes::Footnotes::new(p, existing_footnotes);
let p = TableWrapper::new(p.map(|(ev, _)| ev));
let p = CodeBlocks::new(p, codes, edition, playground);
html::push_html(&mut s, p);
}
});

(toc.into_toc(), s)
}
Expand Down Expand Up @@ -1401,13 +1403,15 @@ impl MarkdownItemInfo<'_> {

let mut s = String::with_capacity(md.len() * 3 / 2);

let p = HeadingLinks::new(p, None, ids, HeadingOffset::H1);
let p = footnotes::Footnotes::new(p);
let p = TableWrapper::new(p.map(|(ev, _)| ev));
let p = p.filter(|event| {
!matches!(event, Event::Start(Tag::Paragraph) | Event::End(TagEnd::Paragraph))
ids.handle_footnotes(|ids, existing_footnotes| {
let p = HeadingLinks::new(p, None, ids, HeadingOffset::H1);
let p = footnotes::Footnotes::new(p, existing_footnotes);
let p = TableWrapper::new(p.map(|(ev, _)| ev));
let p = p.filter(|event| {
!matches!(event, Event::Start(Tag::Paragraph) | Event::End(TagEnd::Paragraph))
});
html::push_html(&mut s, p);
});
html::push_html(&mut s, p);

s
}
Expand Down Expand Up @@ -1882,6 +1886,7 @@ pub(crate) fn rust_code_blocks(md: &str, extra_info: &ExtraInfo<'_>) -> Vec<Rust
#[derive(Clone, Default, Debug)]
pub struct IdMap {
map: FxHashMap<Cow<'static, str>, usize>,
existing_footnotes: usize,
}

// The map is pre-initialized and cloned each time to avoid reinitializing it repeatedly.
Expand Down Expand Up @@ -1943,7 +1948,7 @@ fn init_id_map() -> FxHashMap<Cow<'static, str>, usize> {

impl IdMap {
pub fn new() -> Self {
IdMap { map: DEFAULT_ID_MAP.get_or_init(init_id_map).clone() }
IdMap { map: DEFAULT_ID_MAP.get_or_init(init_id_map).clone(), existing_footnotes: 0 }
}

pub(crate) fn derive<S: AsRef<str> + ToString>(&mut self, candidate: S) -> String {
Expand All @@ -1959,4 +1964,13 @@ impl IdMap {
self.map.insert(id.clone().into(), 1);
id
}

/// Method to handle `existing_footnotes` increment automatically (to prevent forgetting
/// about it).
pub(crate) fn handle_footnotes<F: FnOnce(&mut Self, &mut usize)>(&mut self, closure: F) {
let mut existing_footnotes = self.existing_footnotes;

closure(self, &mut existing_footnotes);
self.existing_footnotes = existing_footnotes;
}
}
33 changes: 19 additions & 14 deletions src/librustdoc/html/markdown/footnotes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,36 +8,35 @@ use super::SpannedEvent;

/// Moves all footnote definitions to the end and add back links to the
/// references.
pub(super) struct Footnotes<'a, I> {
pub(super) struct Footnotes<'a, 'b, I> {
inner: I,
footnotes: FxIndexMap<String, FootnoteDef<'a>>,
existing_footnotes: &'b mut usize,
}

/// The definition of a single footnote.
struct FootnoteDef<'a> {
content: Vec<Event<'a>>,
/// The number that appears in the footnote reference and list.
id: u16,
id: usize,
}

impl<'a, I> Footnotes<'a, I> {
pub(super) fn new(iter: I) -> Self {
Footnotes { inner: iter, footnotes: FxIndexMap::default() }
impl<'a, 'b, I> Footnotes<'a, 'b, I> {
pub(super) fn new(iter: I, existing_footnotes: &'b mut usize) -> Self {
Footnotes { inner: iter, footnotes: FxIndexMap::default(), existing_footnotes }
}

fn get_entry(&mut self, key: &str) -> (&mut Vec<Event<'a>>, u16) {
let new_id = self.footnotes.len() + 1;
fn get_entry(&mut self, key: &str) -> (&mut Vec<Event<'a>>, usize) {
let new_id = self.footnotes.len() + 1 + *self.existing_footnotes;
let key = key.to_owned();
let FootnoteDef { content, id } = self
.footnotes
.entry(key)
.or_insert(FootnoteDef { content: Vec::new(), id: new_id as u16 });
let FootnoteDef { content, id } =
self.footnotes.entry(key).or_insert(FootnoteDef { content: Vec::new(), id: new_id });
// Don't allow changing the ID of existing entrys, but allow changing the contents.
(content, *id)
}
}

impl<'a, I: Iterator<Item = SpannedEvent<'a>>> Iterator for Footnotes<'a, I> {
impl<'a, 'b, I: Iterator<Item = SpannedEvent<'a>>> Iterator for Footnotes<'a, 'b, I> {
type Item = SpannedEvent<'a>;

fn next(&mut self) -> Option<Self::Item> {
Expand All @@ -47,8 +46,13 @@ impl<'a, I: Iterator<Item = SpannedEvent<'a>>> Iterator for Footnotes<'a, I> {
// When we see a reference (to a footnote we may not know) the definition of,
// reserve a number for it, and emit a link to that number.
let (_, id) = self.get_entry(reference);
let reference =
format!("<sup id=\"fnref{0}\"><a href=\"#fn{0}\">{0}</a></sup>", id);
let reference = format!(
"<sup id=\"fnref{0}\"><a href=\"#fn{0}\">{1}</a></sup>",
id,
// Although the ID count is for the whole page, the footnote reference
// are local to the item so we make this ID "local" when displayed.
id - *self.existing_footnotes
);
return Some((Event::Html(reference.into()), range));
}
Some((Event::Start(Tag::FootnoteDefinition(def)), _)) => {
Expand All @@ -64,6 +68,7 @@ impl<'a, I: Iterator<Item = SpannedEvent<'a>>> Iterator for Footnotes<'a, I> {
// After all the markdown is emmited, emit an <hr> then all the footnotes
// in a list.
let defs: Vec<_> = self.footnotes.drain(..).map(|(_, x)| x).collect();
*self.existing_footnotes += defs.len();
let defs_html = render_footnotes_defs(defs);
return Some((Event::Html(defs_html.into()), 0..0));
} else {
Expand Down
4 changes: 2 additions & 2 deletions src/librustdoc/html/render/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,9 +76,9 @@ pub(crate) struct Context<'tcx> {

// `Context` is cloned a lot, so we don't want the size to grow unexpectedly.
#[cfg(all(not(windows), target_pointer_width = "64"))]
rustc_data_structures::static_assert_size!(Context<'_>, 184);
#[cfg(all(windows, target_pointer_width = "64"))]
rustc_data_structures::static_assert_size!(Context<'_>, 192);
#[cfg(all(windows, target_pointer_width = "64"))]
rustc_data_structures::static_assert_size!(Context<'_>, 200);

/// Shared mutable state used in [`Context`] and elsewhere.
pub(crate) struct SharedContext<'tcx> {
Expand Down
41 changes: 41 additions & 0 deletions tests/rustdoc/footnote-ids.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
// This test ensures that footnotes ID are not duplicated across an item page.
// This is a regression test for <https://github.com/rust-lang/rust/issues/131901>.

#![crate_name = "foo"]

//@ has 'foo/struct.Foo.html'

pub struct Foo;

impl Foo {
//@ has - '//a[@href="#fn1"]' '1'
//@ has - '//li[@id="fn1"]' 'Hiya'
//@ has - '//a[@href="#fn2"]' '2'
//@ has - '//li[@id="fn2"]' 'Tiya'
/// Link 1 [^1]
/// Link 1.1 [^2]
///
/// [^1]: Hiya
/// [^2]: Tiya
pub fn l1(){}

//@ has - '//a[@href="#fn3"]' '1'
//@ has - '//li[@id="fn3"]' 'Yiya'
//@ has - '//a[@href="#fn4"]' '2'
//@ has - '//li[@id="fn4"]' 'Biya'
/// Link 2 [^1]
/// Link 3 [^2]
///
/// [^1]: Yiya
/// [^2]: Biya
pub fn l2() {}
}

impl Foo {
//@ has - '//a[@href="#fn5"]' '1'
//@ has - '//li[@id="fn5"]' 'Ciya'
/// Link 3 [^1]
///
/// [^1]: Ciya
pub fn l3(){}
}

0 comments on commit 292cac9

Please sign in to comment.