Merge pull request #1733 from ehuss/mdbook-spec-warnings

Rework error handling in mdbook-spec

Author: traviscross

mdbook-spec/src/lib.rs

@@ -9,6 +9,7 @@ use mdbook::BookItem;
 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 @@ pub fn handle_preprocessing() -> Result<(), Error> {
     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 @@ impl Spec {
     /// 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 @@ impl Preprocessor for Spec {
     }
 
     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,15 +228,23 @@ impl Preprocessor for Spec {
             }
             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);
             }
         });
 
         // 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)
     }

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 @@ pub struct Rules {
 
 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 @@ impl Spec {
                         .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 @@ impl Spec {
 
     /// 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 @@ impl Spec {
                             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();
                     }