preprocess/links: fail for invalid links

Before, `mdbook` would continue processing even when
errors, such as invalid links, are encountered. Moreover,
it would exit with a `0` return code. Such behavior is
unexpected and leads to confusion when run in CI.

Now, when links that don't point to existing files are
encounter and error is returned which yields to `mdbook`
exiting with an error code. The change in behavior has
revealed that some tests were run with invalid links.

src/book/book.rs

@@ -102,11 +102,11 @@ impl Book {
     /// Unlike the `iter()` method, this requires a closure instead of returning
     /// an iterator. This is because using iterators can possibly allow you
     /// to have iterator invalidation errors.
-    pub fn for_each_mut<F>(&mut self, mut func: F)
+    pub fn for_each_mut<F>(&mut self, mut func: F) -> Result<()>
     where
-        F: FnMut(&mut BookItem),
+        F: FnMut(&mut BookItem) -> Result<()>,
     {
-        for_each_mut(&mut func, &mut self.sections);
+        for_each_mut(&mut func, &mut self.sections)
     }
 
     /// Append a `BookItem` to the `Book`.
@@ -116,18 +116,19 @@ impl Book {
     }
 }
 
-pub fn for_each_mut<'a, F, I>(func: &mut F, items: I)
+pub fn for_each_mut<'a, F, I>(func: &mut F, items: I) -> Result<()>
 where
-    F: FnMut(&mut BookItem),
+    F: FnMut(&mut BookItem) -> Result<()>,
     I: IntoIterator<Item = &'a mut BookItem>,
 {
     for item in items {
         if let BookItem::Chapter(ch) = item {
-            for_each_mut(func, &mut ch.sub_items);
+            for_each_mut(func, &mut ch.sub_items)?;
         }
 
-        func(item);
+        func(item)?;
     }
+    Ok(())
 }
 
 /// Enum representing any type of item which can be added to a book.
@@ -595,7 +596,7 @@ And here is some \
         let num_items = book.iter().count();
         let mut visited = 0;
 
-        book.for_each_mut(|_| visited += 1);
+        book.for_each_mut(|_| Ok(visited += 1)).unwrap();
 
         assert_eq!(visited, num_items);
     }

src/preprocess/index.rs

@@ -41,7 +41,8 @@ impl Preprocessor for IndexPreprocessor {
                     }
                 }
             }
-        });
+            Ok(())
+        })?;
 
         Ok(book)
     }

src/preprocess/links.rs

@@ -56,7 +56,7 @@ impl Preprocessor for LinkPreprocessor {
 
                     let mut chapter_title = ch.name.clone();
                     let content =
-                        replace_all(&ch.content, base, chapter_path, 0, &mut chapter_title);
+                        replace_all(&ch.content, base, chapter_path, 0, &mut chapter_title)?;
                     ch.content = content;
                     if chapter_title != ch.name {
                         ctx.chapter_titles
@@ -65,7 +65,8 @@ impl Preprocessor for LinkPreprocessor {
                     }
                 }
             }
-        });
+            Ok(())
+        })?;
 
         Ok(book)
     }
@@ -77,7 +78,7 @@ fn replace_all<P1, P2>(
     source: P2,
     depth: usize,
     chapter_title: &mut String,
-) -> String
+) -> Result<String>
 where
     P1: AsRef<Path>,
     P2: AsRef<Path>,
@@ -93,8 +94,9 @@ where
     for link in find_links(s) {
         replaced.push_str(&s[previous_end_index..link.start_index]);
 
-        match link.render_with_path(path, chapter_title) {
+        let new_content = link
-            Ok(new_content) => {
+            .render_with_path(path, chapter_title)
+            .context(format!("Error updating \"{}\"", link.link_text))?;
         if depth < MAX_LINK_NESTED_DEPTH {
             if let Some(rel_path) = link.link_type.relative_path(path) {
                 replaced.push_str(&replace_all(
@@ -103,7 +105,7 @@ where
                     source,
                     depth + 1,
                     chapter_title,
-                        ));
+                )?);
             } else {
                 replaced.push_str(&new_content);
             }
@@ -115,21 +117,9 @@ where
         }
         previous_end_index = link.end_index;
     }
-            Err(e) => {
-                error!("Error updating \"{}\", {}", link.link_text, e);
-                for cause in e.chain().skip(1) {
-                    warn!("Caused By: {}", cause);
-                }
-
-                // This should make sure we include the raw `{{# ... }}` snippet
-                // in the page content if there are any errors.
-                previous_end_index = link.start_index;
-            }
-        }
-    }
 
     replaced.push_str(&s[previous_end_index..]);
-    replaced
+    Ok(replaced)
 }
 
 #[derive(PartialEq, Debug, Clone)]
@@ -444,7 +434,10 @@ mod tests {
         {{#include file.rs}} << an escaped link!
         ```";
         let mut chapter_title = "test_replace_all_escaped".to_owned();
-        assert_eq!(replace_all(start, "", "", 0, &mut chapter_title), end);
+        assert_eq!(
+            replace_all(start, "", "", 0, &mut chapter_title).unwrap(),
+            end
+        );
     }
 
     #[test]
@@ -456,7 +449,10 @@ mod tests {
         # My Chapter
         ";
         let mut chapter_title = "test_set_chapter_title".to_owned();
-        assert_eq!(replace_all(start, "", "", 0, &mut chapter_title), end);
+        assert_eq!(
+            replace_all(start, "", "", 0, &mut chapter_title).unwrap(),
+            end
+        );
         assert_eq!(chapter_title, "My Title");
     }
 

tests/dummy_book/mod.rs

@@ -136,10 +136,10 @@ fn recursive_copy<A: AsRef<Path>, B: AsRef<Path>>(from: A, to: B) -> Result<()>
 
 pub fn new_copy_of_example_book() -> Result<TempDir> {
     let temp = TempFileBuilder::new().prefix("guide").tempdir()?;
+    let manifest_dir = Path::new(env!("CARGO_MANIFEST_DIR"));
 
-    let guide = Path::new(env!("CARGO_MANIFEST_DIR")).join("guide");
+    recursive_copy(manifest_dir.join("guide"), temp.path().join("guide"))?;
-
+    recursive_copy(manifest_dir.join("examples"), temp.path().join("examples"))?;
-    recursive_copy(guide, temp.path())?;
 
     Ok(temp)
 }

tests/rendered_output.rs

@@ -420,7 +420,7 @@ Around the world, around the world"];
 fn example_book_can_build() {
     let example_book_dir = dummy_book::new_copy_of_example_book().unwrap();
 
-    let md = MDBook::load(example_book_dir.path()).unwrap();
+    let md = MDBook::load(example_book_dir.path().join("guide")).unwrap();
 
     md.build().unwrap();
 }
@@ -484,7 +484,7 @@ fn first_chapter_is_copied_as_index_even_if_not_first_elem() {
 #[test]
 fn theme_dir_overrides_work_correctly() {
     let book_dir = dummy_book::new_copy_of_example_book().unwrap();
-    let book_dir = book_dir.path();
+    let book_dir = &book_dir.path().join("guide");
     let theme_dir = book_dir.join("theme");
 
     let mut index = mdbook::theme::INDEX.to_vec();