Merge pull request #2846 from ehuss/fix-unique-id-loop

Fix ID collisions when the numeric suffix gets used

Author: ehuss

crates/mdbook-html/src/html/print.rs

@@ -8,7 +8,7 @@ use super::Node;
 use crate::html::{ChapterTree, Element, serialize};
 use crate::utils::{ToUrlPath, id_from_content, normalize_path, unique_id};
 use mdbook_core::static_regex;
-use std::collections::HashMap;
+use std::collections::{HashMap, HashSet};
 use std::path::{Component, PathBuf};
 
 /// Takes all the chapter trees, modifies them to be suitable to render for
@@ -42,12 +42,9 @@ pub(crate) fn render_print_page(mut chapter_trees: Vec<ChapterTree<'_>>) -> Stri
 /// been seen. This is used to generate unique IDs.
 fn make_ids_unique(
     chapter_trees: &mut [ChapterTree<'_>],
-) -> (
-    HashMap<PathBuf, HashMap<String, String>>,
-    HashMap<String, u32>,
-) {
+) -> (HashMap<PathBuf, HashMap<String, String>>, HashSet<String>) {
     let mut id_remap = HashMap::new();
-    let mut id_counter = HashMap::new();
+    let mut id_counter = HashSet::new();
     for ChapterTree {
         html_path, tree, ..
     } in chapter_trees
@@ -76,7 +73,7 @@ fn make_ids_unique(
 /// print output has something to link to.
 fn make_root_id_map(
     chapter_trees: &mut [ChapterTree<'_>],
-    id_counter: &mut HashMap<String, u32>,
+    id_counter: &mut HashSet<String>,
 ) -> HashMap<PathBuf, String> {
     let mut path_to_root_id = HashMap::new();
     for ChapterTree {

crates/mdbook-html/src/html/tree.rs

@@ -821,7 +821,7 @@ where
     /// to all header elements, and to also add an `<a>` tag so that clicking
     /// the header will set the current URL to that header's fragment.
     fn add_header_links(&mut self) {
-        let mut id_counter = HashMap::new();
+        let mut id_counter = HashSet::new();
         let headings =
             self.node_ids_for_tag(&|name| matches!(name, "h1" | "h2" | "h3" | "h4" | "h5" | "h6"));
         for heading in headings {

crates/mdbook-html/src/utils.rs

@@ -1,7 +1,6 @@
 //! Utilities for processing HTML.
 
-use std::collections::HashMap;
-use std::fmt::Write;
+use std::collections::HashSet;
 use std::path::{Component, Path, PathBuf};
 
 /// Utility function to normalize path elements like `..`.
@@ -54,18 +53,23 @@ impl ToUrlPath for Path {
 
 /// Make sure an HTML id is unique.
 ///
-/// The `id_counter` map is used to ensure the ID is globally unique. If the
-/// same id appears more than once, then it will have a number added to make
-/// it unique.
-pub(crate) fn unique_id(id: &str, id_counter: &mut HashMap<String, u32>) -> String {
-    let mut id = id.to_string();
-    let id_count = id_counter.entry(id.to_string()).or_insert(0);
-    if *id_count != 0 {
-        // FIXME: This should be a loop to ensure that the new ID is also unique.
-        write!(id, "-{id_count}").unwrap();
+/// Keeps a set of all previously returned IDs; if the requested id is already
+/// used, numeric suffixes (-1, -2, ...) are tried until an unused one is found.
+pub(crate) fn unique_id(id: &str, used: &mut HashSet<String>) -> String {
+    if used.insert(id.to_string()) {
+        return id.to_string();
+    }
+
+    // This ID is already in use. Generate one that is not by appending a
+    // numeric suffix.
+    let mut counter: u32 = 1;
+    loop {
+        let candidate = format!("{id}-{counter}");
+        if used.insert(candidate.clone()) {
+            return candidate;
+        }
+        counter += 1;
     }
-    *id_count += 1;
-    id
 }
 
 /// Generates an HTML id from the given text.

tests/testsuite/rendering/header_links/expected/header_links.html

@@ -6,5 +6,5 @@ <h2 id="hï"><a class="header" href="#hï">Hï</a></h2>
 <h2 id="repeat"><a class="header" href="#repeat">Repeat</a></h2>
 <h2 id="repeat-1"><a class="header" href="#repeat-1">Repeat</a></h2>
 <h2 id="repeat-2"><a class="header" href="#repeat-2">Repeat</a></h2>
-<h2 id="repeat-1"><a class="header" href="#repeat-1">Repeat 1</a></h2>
+<h2 id="repeat-1-1"><a class="header" href="#repeat-1-1">Repeat 1</a></h2>
 <h2 id="with-emphasis-bold-bold_emphasis-code-escaped-html-link-httpsexamplecom"><a class="header" href="#with-emphasis-bold-bold_emphasis-code-escaped-html-link-httpsexamplecom"><!--comment--> With <em>emphasis</em> <strong>bold</strong> <strong><em>bold_emphasis</em></strong> <code>code</code> &lt;escaped&gt; <span>html</span> <a href="https://example.com/link">link</a> <a href="https://example.com/">https://example.com/</a></a></h2>