Rework error handling in mdbook-spec This enhances mdbook-spec's error handling with multiple changes: - Several errors are now conditionally warnings. This makes doing local development with `mdbook serve` much easier, as sometimes you are OK with temporarily having broken links. SPEC_DENY_WARNINGS still rejects these. - Collect multiple errors, and display all of them, instead of bailing on the very first error. - Show a count of the number of warnings or errors. This is done by introducing a new `Diagnostics` struct for emitting warnings and errors. The `warn_or_err!` macro provide the primary interface for emitting warnings. I also added a `bug!` macro for internal errors that will immediately exit. This is slightly nicer output than using `panic!`.
diff --git a/mdbook-spec/src/lib.rs b/mdbook-spec/src/lib.rs index 37f6550..e52ca40 100644 --- a/mdbook-spec/src/lib.rs +++ b/mdbook-spec/src/lib.rs
@@ -9,6 +9,7 @@ use once_cell::sync::Lazy; use regex::{Captures, Regex}; use semver::{Version, VersionReq}; +use std::fmt; use std::io; use std::ops::Range; use std::path::PathBuf; @@ -46,15 +47,58 @@ Ok(()) } -pub struct Spec { +/// Handler for errors and warnings. +pub struct Diagnostics { /// Whether or not warnings should be errors (set by SPEC_DENY_WARNINGS /// environment variable). deny_warnings: bool, + /// Number of messages generated. + count: u32, +} + +impl Diagnostics { + fn new() -> Diagnostics { + let deny_warnings = std::env::var("SPEC_DENY_WARNINGS").as_deref() == Ok("1"); + Diagnostics { + deny_warnings, + count: 0, + } + } + + /// Displays a warning or error (depending on whether warnings are denied). + /// + /// Usually you want the [`warn_or_err!`] macro. + fn warn_or_err(&mut self, args: fmt::Arguments<'_>) { + if self.deny_warnings { + eprintln!("error: {args}"); + } else { + eprintln!("warning: {args}"); + } + self.count += 1; + } +} + +/// Displays a warning or error (depending on whether warnings are denied). +#[macro_export] +macro_rules! warn_or_err { + ($diag:expr, $($arg:tt)*) => { + $diag.warn_or_err(format_args!($($arg)*)); + }; +} + +/// Displays a message for an internal error, and immediately exits. +#[macro_export] +macro_rules! bug { + ($($arg:tt)*) => { + eprintln!("mdbook-spec internal error: {}", format_args!($($arg)*)); + std::process::exit(1); + }; +} + +pub struct Spec { /// Path to the rust-lang/rust git repository (set by SPEC_RUST_ROOT /// environment variable). rust_root: Option<PathBuf>, - /// The git ref that can be used in a URL to the rust-lang/rust repository. - git_ref: String, } impl Spec { @@ -64,30 +108,10 @@ /// the rust git checkout. If `None`, it will use the `SPEC_RUST_ROOT` /// environment variable. If the root is not specified, then no tests will /// be linked unless `SPEC_DENY_WARNINGS` is set in which case this will - /// return an error.. + /// return an error. pub fn new(rust_root: Option<PathBuf>) -> Result<Spec> { - let deny_warnings = std::env::var("SPEC_DENY_WARNINGS").as_deref() == Ok("1"); let rust_root = rust_root.or_else(|| std::env::var_os("SPEC_RUST_ROOT").map(PathBuf::from)); - if deny_warnings && rust_root.is_none() { - bail!("SPEC_RUST_ROOT environment variable must be set"); - } - let git_ref = match git_ref(&rust_root) { - Ok(s) => s, - Err(e) => { - if deny_warnings { - eprintln!("error: {e:?}"); - std::process::exit(1); - } else { - eprintln!("warning: {e:?}"); - "master".into() - } - } - }; - Ok(Spec { - deny_warnings, - rust_root, - git_ref, - }) + Ok(Spec { rust_root }) } /// Generates link references to all rules on all pages, so you can easily @@ -180,9 +204,20 @@ } fn run(&self, _ctx: &PreprocessorContext, mut book: Book) -> Result<Book, Error> { - let rules = self.collect_rules(&book); + let mut diag = Diagnostics::new(); + if diag.deny_warnings && self.rust_root.is_none() { + bail!("error: SPEC_RUST_ROOT environment variable must be set"); + } + let rules = self.collect_rules(&book, &mut diag); let tests = self.collect_tests(&rules); let summary_table = test_links::make_summary_table(&book, &tests, &rules); + let git_ref = match git_ref(&self.rust_root) { + Ok(s) => s, + Err(e) => { + warn_or_err!(&mut diag, "{e:?}"); + "master".into() + } + }; book.for_each_mut(|item| { let BookItem::Chapter(ch) = item else { @@ -193,7 +228,7 @@ } ch.content = self.admonitions(&ch); ch.content = self.auto_link_references(&ch, &rules); - ch.content = self.render_rule_definitions(&ch.content, &tests); + ch.content = self.render_rule_definitions(&ch.content, &tests, &git_ref); if ch.name == "Test summary" { ch.content = ch.content.replace("{{summary-table}}", &summary_table); } @@ -201,7 +236,15 @@ // Final pass will resolve everything as a std link (or error if the // link is unknown). - std_links::std_links(&mut book); + std_links::std_links(&mut book, &mut diag); + + if diag.count > 0 { + if diag.deny_warnings { + eprintln!("mdbook-spec exiting due to {} errors", diag.count); + std::process::exit(1); + } + eprintln!("mdbook-spec generated {} warnings", diag.count); + } Ok(book) }
diff --git a/mdbook-spec/src/rules.rs b/mdbook-spec/src/rules.rs index 1149826..dcab26d 100644 --- a/mdbook-spec/src/rules.rs +++ b/mdbook-spec/src/rules.rs
@@ -1,7 +1,7 @@ //! Handling for rule identifiers. use crate::test_links::RuleToTests; -use crate::Spec; +use crate::{warn_or_err, Diagnostics, Spec}; use mdbook::book::Book; use mdbook::BookItem; use once_cell::sync::Lazy; @@ -34,7 +34,7 @@ impl Spec { /// Collects all rule definitions in the book. - pub fn collect_rules(&self, book: &Book) -> Rules { + pub fn collect_rules(&self, book: &Book, diag: &mut Diagnostics) -> Rules { let mut rules = Rules::default(); for item in book.iter() { let BookItem::Chapter(ch) = item else { @@ -53,16 +53,12 @@ .def_paths .insert(rule_id.to_string(), (source_path.clone(), path.clone())) { - let message = format!( + warn_or_err!( + diag, "rule `{rule_id}` defined multiple times\n\ First location: {old:?}\n\ Second location: {source_path:?}" ); - if self.deny_warnings { - panic!("error: {message}"); - } else { - eprintln!("warning: {message}"); - } } let mut parts: Vec<_> = rule_id.split('.').collect(); while !parts.is_empty() { @@ -78,7 +74,12 @@ /// Converts lines that start with `r[…]` into a "rule" which has special /// styling and can be linked to. - pub fn render_rule_definitions(&self, content: &str, tests: &RuleToTests) -> String { + pub fn render_rule_definitions( + &self, + content: &str, + tests: &RuleToTests, + git_ref: &str, + ) -> String { RULE_RE .replace_all(content, |caps: &Captures<'_>| { let rule_id = &caps[1]; @@ -96,7 +97,6 @@ test_html, "<li><a href=\"https://github.com/rust-lang/rust/blob/{git_ref}/{test_path}\">{test_path}</a></li>", test_path = test.path, - git_ref = self.git_ref ) .unwrap(); }
diff --git a/mdbook-spec/src/std_links.rs b/mdbook-spec/src/std_links.rs index fc3d8f9..8a80388 100644 --- a/mdbook-spec/src/std_links.rs +++ b/mdbook-spec/src/std_links.rs
@@ -1,7 +1,8 @@ //! Support for translating links to the standard library. -use mdbook::book::Book; -use mdbook::book::Chapter; +use crate::{bug, warn_or_err, Diagnostics}; +use anyhow::{bail, Result}; +use mdbook::book::{Book, Chapter}; use mdbook::BookItem; use once_cell::sync::Lazy; use pulldown_cmark::{BrokenLink, CowStr, Event, LinkType, Options, Parser, Tag}; @@ -9,10 +10,9 @@ use std::collections::HashMap; use std::fmt::Write as _; use std::fs; -use std::io::{self, Write as _}; use std::ops::Range; use std::path::PathBuf; -use std::process::{self, Command}; +use std::process::Command; use tempfile::TempDir; /// The Regex used to extract the std links from the HTML generated by rustdoc. @@ -31,7 +31,7 @@ /// Converts links to the standard library to the online documentation in a /// fashion similar to rustdoc intra-doc links. -pub fn std_links(book: &mut Book) { +pub fn std_links(book: &mut Book, diag: &mut Diagnostics) { // Collect all links in all chapters. let mut chapter_links = HashMap::new(); for item in book.iter() { @@ -42,15 +42,18 @@ continue; } let key = ch.source_path.as_ref().unwrap(); - chapter_links.insert(key, collect_markdown_links(&ch)); + chapter_links.insert(key, collect_markdown_links(&ch, diag)); } // Write a Rust source file to use with rustdoc to generate intra-doc links. let tmp = TempDir::with_prefix("mdbook-spec-").unwrap(); - run_rustdoc(&tmp, &chapter_links); + if let Err(e) = run_rustdoc(&tmp, &chapter_links, diag) { + warn_or_err!(diag, "{e:?}"); + return; + } // Extract the links from the generated html. - let generated = - fs::read_to_string(tmp.path().join("doc/a/index.html")).expect("index.html generated"); + let generated = fs::read_to_string(tmp.path().join("doc/a/index.html")) + .expect("index.html failed to generate"); let mut urls: Vec<_> = STD_LINK_EXTRACT_RE .captures_iter(&generated) .map(|cap| cap.get(1).unwrap().as_str()) @@ -58,12 +61,11 @@ let mut urls = &mut urls[..]; let expected_len: usize = chapter_links.values().map(|l| l.len()).sum(); if urls.len() != expected_len { - eprintln!( - "error: expected rustdoc to generate {} links, but found {}", + bug!( + "expected rustdoc to generate {} links, but found {}", expected_len, urls.len(), ); - process::exit(1); } // Unflatten the urls list so that it is split back by chapter. let mut ch_urls: HashMap<&PathBuf, Vec<_>> = HashMap::new(); @@ -84,7 +86,7 @@ } let key = ch.source_path.as_ref().unwrap(); // Create a list of replacements to make in the raw markdown to point to the new url. - let replacements = compute_replacements(&ch, &chapter_links[key], &ch_urls[key]); + let replacements = compute_replacements(&ch, &chapter_links[key], &ch_urls[key], diag); let mut new_contents = ch.content.clone(); for (md_link, url, range) in replacements { @@ -133,7 +135,7 @@ } /// Collects all markdown links that look like they might be standard library links. -fn collect_markdown_links(chapter: &Chapter) -> Vec<Link<'_>> { +fn collect_markdown_links<'a>(chapter: &'a Chapter, diag: &mut Diagnostics) -> Vec<Link<'a>> { let mut opts = Options::empty(); opts.insert(Options::ENABLE_TABLES); opts.insert(Options::ENABLE_FOOTNOTES); @@ -180,13 +182,13 @@ continue; } if !title.is_empty() { - eprintln!( - "error: titles in links are not supported\n\ + warn_or_err!( + diag, + "titles in links are not supported\n\ Link {dest_url} has title `{title}` found in chapter {} ({:?})", chapter.name, chapter.source_path.as_ref().unwrap() ); - process::exit(1); } links.push(Link { link_type, @@ -208,14 +210,19 @@ /// generate intra-doc links on them. /// /// The output will be in the given `tmp` directory. -fn run_rustdoc(tmp: &TempDir, chapter_links: &HashMap<&PathBuf, Vec<Link<'_>>>) { +fn run_rustdoc( + tmp: &TempDir, + chapter_links: &HashMap<&PathBuf, Vec<Link<'_>>>, + diag: &mut Diagnostics, +) -> Result<()> { let src_path = tmp.path().join("a.rs"); // Allow redundant since there could some in-scope things that are // technically not necessary, but we don't care about (like // [`Option`](std::option::Option)). let mut src = format!( - "#![deny(rustdoc::broken_intra_doc_links)]\n\ - #![allow(rustdoc::redundant_explicit_links)]\n" + "#![{}(rustdoc::broken_intra_doc_links)]\n\ + #![allow(rustdoc::redundant_explicit_links)]\n", + if diag.deny_warnings { "deny" } else { "warn" } ); // This uses a list to make easy to pull the links out of the generated HTML. for (_ch_path, links) in chapter_links { @@ -231,10 +238,10 @@ | LinkType::CollapsedUnknown | LinkType::ShortcutUnknown => { // These should only happen due to broken link replacements. - panic!("unexpected link type unknown {link:?}"); + bug!("unexpected link type unknown {link:?}"); } LinkType::Autolink | LinkType::Email => { - panic!("link type should have been filtered {link:?}"); + bug!("link type should have been filtered {link:?}"); } } } @@ -256,10 +263,13 @@ .output() .expect("rustdoc installed"); if !output.status.success() { - eprintln!("error: failed to extract std links ({:?})\n", output.status,); - io::stderr().write_all(&output.stderr).unwrap(); - process::exit(1); + let stderr = String::from_utf8_lossy(&output.stderr); + bail!( + "failed to extract std links ({:?})\n{stderr}", + output.status + ); } + Ok(()) } static DOC_URL: Lazy<Regex> = Lazy::new(|| { @@ -271,8 +281,7 @@ // Set SPEC_RELATIVE=0 to disable this, which can be useful for working locally. if std::env::var("SPEC_RELATIVE").as_deref() != Ok("0") { let Some(url_start) = DOC_URL.shortest_match(url) else { - eprintln!("error: expected rustdoc URL to start with {DOC_URL:?}, got {url}"); - std::process::exit(1); + bug!("expected rustdoc URL to start with {DOC_URL:?}, got {url}"); }; let url_path = &url[url_start..]; let num_dots = chapter.path.as_ref().unwrap().components().count(); @@ -294,21 +303,23 @@ chapter: &'a Chapter, links: &[Link<'_>], urls: &[&'a str], + diag: &mut Diagnostics, ) -> Vec<(&'a str, &'a str, Range<usize>)> { let mut replacements = Vec::new(); for (url, link) in urls.iter().zip(links) { let Some(cap) = ANCHOR_URL.captures(url) else { let line = super::line_from_range(&chapter.content, &link.range); - eprintln!( - "error: broken markdown link found in {}\n\ + warn_or_err!( + diag, + "broken markdown link found in {}\n\ Line is: {line}\n\ Link to `{}` could not be resolved by rustdoc to a known URL (result was `{}`).\n", chapter.source_path.as_ref().unwrap().display(), link.dest_url, url ); - process::exit(1); + continue; }; let url = cap.get(1).unwrap().as_str(); let md_link = &chapter.content[link.range.clone()]; @@ -316,11 +327,10 @@ let range = link.range.clone(); let add_link = |re: &Regex| { let Some(cap) = re.captures(md_link) else { - eprintln!( - "error: expected link `{md_link}` of type {:?} to match regex {re}", + bug!( + "expected link `{md_link}` of type {:?} to match regex {re}", link.link_type ); - process::exit(1); }; let md_link = cap.get(1).unwrap().as_str(); replacements.push((md_link, url, range)); @@ -337,7 +347,7 @@ add_link(&MD_LINK_SHORTCUT); } _ => { - panic!("unexpected link type: {link:#?}"); + bug!("unexpected link type: {link:#?}"); } } }