Merge branch 'liw/eltloc' into 'main'

feat: add location information to markdown parsing errors

Closes #16

See merge request subplot/subplot!331

6 files changed, 258 insertions, 82 deletions

diff --git a/src/doc.rs b/src/doc.rs
index f08f795..fe263ad 100644
--- a/src/doc.rs
+++ b/src/doc.rs
@@ -104,20 +104,20 @@ impl Document {
     {
         let meta = Metadata::from_yaml_metadata(basedir, yamlmeta, template)?;
         trace!("metadata from YAML: {:#?}", meta);
-        let files = Self::all_files(&markdowns);
+        let files = Self::all_files(&markdowns)?;
         let doc = Document::new(subplot, markdowns, meta, files, style);
         trace!("Loaded from JSON OK");
         Ok(doc)
     }
 
-    fn all_files(markdowns: &[Markdown]) -> EmbeddedFiles {
+    fn all_files(markdowns: &[Markdown]) -> Result<EmbeddedFiles, SubplotError> {
         let mut files = EmbeddedFiles::default();
         for md in markdowns {
-            for file in md.embedded_files().files() {
+            for file in md.embedded_files()?.files() {
                 files.push(file.clone());
             }
         }
-        files
+        Ok(files)
     }
 
     /// Construct a Document from a named file.
diff --git a/src/error.rs b/src/error.rs
index f7dfe52..db85e83 100644
--- a/src/error.rs
+++ b/src/error.rs
@@ -1,4 +1,4 @@
-use crate::html::HtmlError;
+use crate::html::{HtmlError, Location};
 use crate::matches::MatchedSteps;
 use crate::md::MdError;
 
@@ -148,8 +148,8 @@ pub enum SubplotError {
     /// scenario block before the first heading in the document.
     ///
     /// To fix, add a heading or move the scenario after a heading.
-    #[error("first scenario is before first heading")]
-    ScenarioBeforeHeading,
+    #[error("{0}: first scenario is before first heading")]
+    ScenarioBeforeHeading(Location),
 
     /// Step does not have a keyword.
     #[error("step has no keyword: {0}")]
@@ -299,10 +299,6 @@ pub enum SubplotError {
     #[error("Failed to parse YAML metadata in {0}")]
     MetadataFile(PathBuf, #[source] serde_yaml::Error),
 
-    /// Abstract syntax tree error.
-    #[error(transparent)]
-    Ast(#[from] crate::metadata::Error),
-
     /// UTF8 conversion error.
     #[error("failed to parse UTF8 in file {0}")]
     FileUtf8(PathBuf, #[source] std::string::FromUtf8Error),
diff --git a/src/html.rs b/src/html.rs
index 7237258..d9788d9 100644
--- a/src/html.rs
+++ b/src/html.rs
@@ -72,7 +72,7 @@ impl HtmlPage {
 }
 
 /// Parse Markdown text into an HTML element.
-pub fn parse(markdown: &str) -> Result<Element, HtmlError> {
+pub fn parse(filename: &Path, markdown: &str) -> Result<Element, HtmlError> {
     let mut options = Options::empty();
     options.insert(Options::ENABLE_HEADING_ATTRIBUTES);
     options.insert(Options::ENABLE_STRIKETHROUGH);
@@ -85,7 +85,7 @@ pub fn parse(markdown: &str) -> Result<Element, HtmlError> {
     for (event, loc) in p {
         trace!("event {:?}", event);
         let (line, col) = linecol.get(loc.start);
-        let loc = Location::new(line, col);
+        let loc = Location::new(filename, line, col);
         match event {
             Event::Start(tag) => match tag {
                 Tag::Paragraph => stack.push_tag(ElementTag::P, loc),
@@ -162,7 +162,7 @@ pub fn parse(markdown: &str) -> Result<Element, HtmlError> {
                     let s = as_plain_text(e.children());
                     trace!("paragraph text: {:?}", s);
                     if s.starts_with(": ") || s.contains("\n: ") {
-                        return Err(HtmlError::DefinitionList(loc.line, loc.col));
+                        return Err(HtmlError::DefinitionList(loc));
                     }
                     stack.append_child(Content::Elt(e));
                 }
@@ -254,8 +254,12 @@ impl Element {
     }
 
     /// Get location.
-    pub fn location(&self) -> &Option<Location> {
-        &self.loc
+    pub fn location(&self) -> Location {
+        if let Some(loc) = &self.loc {
+            loc.clone()
+        } else {
+            Location::unknown()
+        }
     }
 
     fn set_block_attributes(&mut self, block_attrs: Vec<BlockAttr>) {
@@ -543,15 +547,76 @@ impl Content {
 }
 
 /// Location of element in source file.
-#[derive(Debug, Clone, Copy)]
-pub struct Location {
-    line: usize,
-    col: usize,
+#[derive(Debug, Clone, Eq, PartialEq)]
+pub enum Location {
+    /// A known location.
+    Known {
+        /// Name of file.
+        filename: PathBuf,
+        /// Line in file.
+        line: usize,
+        /// Column in line.
+        col: usize,
+    },
+    /// An unknown location.
+    Unknown,
 }
 
 impl Location {
-    fn new(line: usize, col: usize) -> Self {
-        Self { line, col }
+    fn new(filename: &Path, line: usize, col: usize) -> Self {
+        Self::Known {
+            filename: filename.into(),
+            line,
+            col,
+        }
+    }
+
+    /// Create an unknown location.
+    pub fn unknown() -> Self {
+        Self::Unknown
+    }
+
+    /// Report name of source file from where this element comes from.
+    pub fn filename(&self) -> &Path {
+        if let Self::Known {
+            filename,
+            line: _,
+            col: _,
+        } = self
+        {
+            filename
+        } else {
+            Path::new("")
+        }
+    }
+
+    /// Report row and column in source where this element comes from.
+    pub fn rowcol(&self) -> (usize, usize) {
+        if let Self::Known {
+            filename: _,
+            line,
+            col,
+        } = self
+        {
+            (*line, *col)
+        } else {
+            (0, 0)
+        }
+    }
+}
+
+impl std::fmt::Display for Location {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> Result<(), std::fmt::Error> {
+        if let Self::Known {
+            filename,
+            line,
+            col,
+        } = self
+        {
+            write!(f, "{}:{}:{}", filename.display(), line, col)
+        } else {
+            write!(f, "(unknown location)")
+        }
     }
 }
 
@@ -616,8 +681,8 @@ pub enum HtmlError {
 
     /// Input contains an attempt to use a definition list in
     /// Markdown.
-    #[error("attempt to use definition lists in Markdown: line {0}, column {1}")]
-    DefinitionList(usize, usize),
+    #[error("{0}: attempt to use definition lists in Markdown")]
+    DefinitionList(Location),
 
     /// String formatting error. This is likely a programming error.
     #[error("string formatting error: {0}")]
diff --git a/src/md.rs b/src/md.rs
index 5e56198..02efa0c 100644
--- a/src/md.rs
+++ b/src/md.rs
@@ -1,7 +1,7 @@
 //! A parsed Markdown document.
 
 use crate::{
-    html::{parse, Attribute, Content, Element, ElementTag},
+    html::{parse, Attribute, Content, Element, ElementTag, Location},
     parse_scenario_snippet, Bindings, EmbeddedFile, EmbeddedFiles, Scenario, ScenarioStep, Style,
     SubplotError, Warnings,
 };
@@ -22,11 +22,11 @@ impl Markdown {
         let text = std::fs::read(filename)
             .map_err(|e| SubplotError::InputFileUnreadable(filename.into(), e))?;
         let text = std::str::from_utf8(&text).map_err(SubplotError::Utf8Error)?;
-        Self::new_from_str(text)
+        Self::new_from_str(filename, text)
     }
 
-    fn new_from_str(text: &str) -> Result<Self, SubplotError> {
-        let html = parse(text)?;
+    fn new_from_str(filename: &Path, text: &str) -> Result<Self, SubplotError> {
+        let html = parse(filename, text)?;
         Ok(Self::new(html))
     }
 
@@ -142,16 +142,16 @@ impl Markdown {
 
     /// Find embedded files.
     // FIXME: this should return a result
-    pub fn embedded_files(&self) -> EmbeddedFiles {
+    pub fn embedded_files(&self) -> Result<EmbeddedFiles, MdError> {
         let mut files = EmbeddedFiles::default();
 
         for e in Self::visit(&self.html) {
-            if let Ok(file) = embedded_file(e) {
+            if let MaybeEmbeddedFile::IsFile(file) = embedded_file(e)? {
                 files.push(file);
             }
         }
 
-        files
+        Ok(files)
     }
 }
 
@@ -159,42 +159,47 @@ impl Markdown {
 #[derive(Debug)]
 enum StructureElement {
     // Headings consist of the text and the level of the heading.
-    Heading(String, i64),
+    Heading(String, i64, Location),
 
     // Scenario snippets consist just of the unparsed text.
-    Snippet(String),
+    Snippet(String, Location),
 }
 
 impl StructureElement {
     fn heading(e: &Element, level: i64) -> Self {
-        Self::Heading(e.content(), level)
+        Self::Heading(e.content(), level, e.location())
     }
 
     fn snippet(e: &Element) -> Self {
-        Self::Snippet(e.content())
+        Self::Snippet(e.content(), e.location())
     }
 }
 
-fn embedded_file(e: &Element) -> Result<EmbeddedFile, MdError> {
+enum MaybeEmbeddedFile {
+    IsFile(EmbeddedFile),
+    NotFile,
+}
+
+fn embedded_file(e: &Element) -> Result<MaybeEmbeddedFile, MdError> {
     if e.tag() != ElementTag::Pre {
-        return Err(MdError::NotCodeBlockElement(e.tag().name().to_string()));
+        return Ok(MaybeEmbeddedFile::NotFile);
     }
 
     if !e.has_attr("class", "file") {
-        return Err(MdError::NotFile);
+        return Ok(MaybeEmbeddedFile::NotFile);
     }
 
     let id = e.attr("id");
     if id.is_none() {
-        return Err(MdError::NoId);
+        return Ok(MaybeEmbeddedFile::NotFile);
     }
     let id = id.unwrap();
     if id.value().is_none() {
-        return Err(MdError::NoIdValue);
+        return Err(MdError::NoIdValue(e.location()));
     }
     let id = id.value().unwrap();
     if id.is_empty() {
-        return Err(MdError::NoIdValue);
+        return Err(MdError::NoIdValue(e.location()));
     }
 
     // The contents we get from the pulldown_cmark parser for a code
@@ -209,7 +214,8 @@ fn embedded_file(e: &Element) -> Result<EmbeddedFile, MdError> {
     if contents.ends_with('\n') {
         contents.truncate(contents.len() - 1);
     }
-    match AddNewline::parse(e.attr("add-newline"))? {
+    let addnl = AddNewline::parse(e.attr("add-newline"), e.location());
+    match addnl? {
         AddNewline::No => {
             // Newline already isn't there.
         }
@@ -225,7 +231,12 @@ fn embedded_file(e: &Element) -> Result<EmbeddedFile, MdError> {
         }
     };
 
-    Ok(EmbeddedFile::new(id.into(), contents))
+    eprintln!("ok");
+
+    Ok(MaybeEmbeddedFile::IsFile(EmbeddedFile::new(
+        id.into(),
+        contents,
+    )))
 }
 
 #[derive(Debug, Eq, PartialEq, Copy, Clone)]
@@ -236,14 +247,14 @@ enum AddNewline {
 }
 
 impl AddNewline {
-    fn parse(attr: Option<&Attribute>) -> Result<Self, MdError> {
+    fn parse(attr: Option<&Attribute>, loc: Location) -> Result<Self, MdError> {
         if let Some(attr) = attr {
             if let Some(value) = attr.value() {
                 let value = match value {
                     "yes" => Self::Yes,
                     "no" => Self::No,
                     "auto" => Self::Auto,
-                    _ => return Err(MdError::BadAddNewline(value.into())),
+                    _ => return Err(MdError::BadAddNewline(value.into(), loc)),
                 };
                 return Ok(value);
             }
@@ -259,13 +270,13 @@ fn extract_scenario(e: &[StructureElement]) -> Result<(Option<Scenario>, usize),
     }
 
     match &e[0] {
-        StructureElement::Snippet(_) => Err(SubplotError::ScenarioBeforeHeading),
-        StructureElement::Heading(title, level) => {
+        StructureElement::Snippet(_, loc) => Err(SubplotError::ScenarioBeforeHeading(loc.clone())),
+        StructureElement::Heading(title, level, _loc) => {
             let mut scen = Scenario::new(title);
             let mut prevkind = None;
             for (i, item) in e.iter().enumerate().skip(1) {
                 match item {
-                    StructureElement::Heading(_, level2) => {
+                    StructureElement::Heading(_, level2, _loc) => {
                         let is_subsection = *level2 > *level;
                         if is_subsection {
                             if scen.has_steps() {
@@ -278,7 +289,7 @@ fn extract_scenario(e: &[StructureElement]) -> Result<(Option<Scenario>, usize),
                             return Ok((None, i));
                         }
                     }
-                    StructureElement::Snippet(text) => {
+                    StructureElement::Snippet(text, _) => {
                         for line in parse_scenario_snippet(text) {
                             let step = ScenarioStep::new_from_str(line, prevkind)?;
                             scen.add(&step);
@@ -407,39 +418,40 @@ mod typeset {
 #[derive(Debug, thiserror::Error, Eq, PartialEq)]
 pub enum MdError {
     /// Trie to treat a non-PRE element as an embedded file.
-    #[error("tried to treat wrong element as an embedded file: {0}")]
-    NotCodeBlockElement(String),
+    #[error("{1}: tried to treat wrong element as an embedded file: {0}")]
+    NotCodeBlockElement(String, Location),
 
     /// Code block lacks the "file" attribute.
-    #[error("code block is not a file")]
-    NotFile,
+    #[error("{0}; code block is not a file")]
+    NotFile(Location),
 
     /// Code block lacks an identifile to use as th filename.
-    #[error("code block lacks a filename identifier")]
-    NoId,
+    #[error("{0}: code block lacks a filename identifier")]
+    NoId(Location),
 
     /// Identifier is empty.
-    #[error("code block has an empty filename identifier")]
-    NoIdValue,
+    #[error("{0}: code block has an empty filename identifier")]
+    NoIdValue(Location),
 
     /// Value ofv add-newline attribute ie not understood.
-    #[error("value of add-newline attirubte is not understood: {0}")]
-    BadAddNewline(String),
+    #[error("{1}: value of add-newline attribute is not understood: {0}")]
+    BadAddNewline(String, Location),
 }
 
 #[cfg(test)]
 mod test_extract {
     use super::extract_scenario;
+    use super::Location;
     use super::StructureElement;
     use crate::Scenario;
     use crate::SubplotError;
 
     fn h(title: &str, level: i64) -> StructureElement {
-        StructureElement::Heading(title.to_string(), level)
+        StructureElement::Heading(title.to_string(), level, Location::unknown())
     }
 
     fn s(text: &str) -> StructureElement {
-        StructureElement::Snippet(text.to_string())
+        StructureElement::Snippet(text.to_string(), Location::unknown())
     }
 
     fn check_result(
@@ -530,7 +542,7 @@ mod test_extract {
         let elements = vec![s("given something")];
         let r = extract_scenario(&elements);
         match r {
-            Err(SubplotError::ScenarioBeforeHeading) => (),
+            Err(SubplotError::ScenarioBeforeHeading(_)) => (),
             _ => panic!("unexpected result {:?}", r),
         }
     }
@@ -538,24 +550,25 @@ mod test_extract {
 
 #[cfg(test)]
 mod test {
-    use super::{AddNewline, Attribute, Markdown, MdError};
-    use std::path::PathBuf;
+    use super::{AddNewline, Attribute, Location, Markdown, MdError};
+    use std::path::{Path, PathBuf};
 
     #[test]
     fn loads_empty_doc() {
-        let md = Markdown::new_from_str("").unwrap();
+        let md = Markdown::new_from_str(Path::new(""), "").unwrap();
         assert!(md.html.content().is_empty());
     }
 
     #[test]
     fn finds_no_images_in_empty_doc() {
-        let md = Markdown::new_from_str("").unwrap();
+        let md = Markdown::new_from_str(Path::new(""), "").unwrap();
         assert!(md.images().is_empty());
     }
 
     #[test]
     fn finds_images() {
         let md = Markdown::new_from_str(
+            Path::new(""),
             r#"
 ![alt text](filename.jpg)
 "#,
@@ -566,13 +579,14 @@ mod test {
 
     #[test]
     fn finds_no_blocks_in_empty_doc() {
-        let md = Markdown::new_from_str("").unwrap();
+        let md = Markdown::new_from_str(Path::new(""), "").unwrap();
         assert!(md.block_classes().is_empty());
     }
 
     #[test]
     fn finds_no_classes_when_no_blocks_have_them() {
         let md = Markdown::new_from_str(
+            Path::new(""),
             r#"
 ~~~
 ~~~
@@ -585,6 +599,7 @@ mod test {
     #[test]
     fn finds_block_classes() {
         let md = Markdown::new_from_str(
+            Path::new(""),
             r#"
 ~~~scenario
 ~~~
@@ -597,7 +612,7 @@ mod test {
 
     #[test]
     fn finds_no_scenarios_in_empty_doc() {
-        let md = Markdown::new_from_str("").unwrap();
+        let md = Markdown::new_from_str(Path::new(""), "").unwrap();
         let scenarios = md.scenarios().unwrap();
         assert!(scenarios.is_empty());
     }
@@ -605,6 +620,7 @@ mod test {
     #[test]
     fn finds_scenarios() {
         let md = Markdown::new_from_str(
+            Path::new(""),
             r#"
 # Super trooper
 
@@ -627,14 +643,15 @@ given ABBA
 
     #[test]
     fn finds_no_embedded_files_in_empty_doc() {
-        let md = Markdown::new_from_str("").unwrap();
+        let md = Markdown::new_from_str(Path::new(""), "").unwrap();
         let files = md.embedded_files();
-        assert!(files.files().is_empty());
+        assert!(files.unwrap().files().is_empty());
     }
 
     #[test]
     fn finds_embedded_files() {
         let md = Markdown::new_from_str(
+            Path::new(""),
             r#"
 ~~~{#fileid .file .text}
 hello, world
@@ -642,7 +659,7 @@ hello, world
 "#,
         )
         .unwrap();
-        let files = md.embedded_files();
+        let files = md.embedded_files().unwrap();
         assert_eq!(files.files().len(), 1);
         let file = files.files().get(0).unwrap();
         assert_eq!(file.filename(), "fileid");
@@ -651,33 +668,45 @@ hello, world
 
     #[test]
     fn parses_no_auto_newline_as_auto() {
-        assert_eq!(AddNewline::parse(None).unwrap(), AddNewline::Auto);
+        assert_eq!(
+            AddNewline::parse(None, Location::unknown()).unwrap(),
+            AddNewline::Auto
+        );
     }
 
     #[test]
     fn parses_auto_as_auto() {
         let attr = Attribute::new("add-newline", "auto");
-        assert_eq!(AddNewline::parse(Some(&attr)).unwrap(), AddNewline::Auto);
+        assert_eq!(
+            AddNewline::parse(Some(&attr), Location::unknown()).unwrap(),
+            AddNewline::Auto
+        );
     }
 
     #[test]
     fn parses_yes_as_yes() {
         let attr = Attribute::new("add-newline", "yes");
-        assert_eq!(AddNewline::parse(Some(&attr)).unwrap(), AddNewline::Yes);
+        assert_eq!(
+            AddNewline::parse(Some(&attr), Location::unknown()).unwrap(),
+            AddNewline::Yes
+        );
     }
 
     #[test]
     fn parses_no_as_no() {
         let attr = Attribute::new("add-newline", "no");
-        assert_eq!(AddNewline::parse(Some(&attr)).unwrap(), AddNewline::No);
+        assert_eq!(
+            AddNewline::parse(Some(&attr), Location::unknown()).unwrap(),
+            AddNewline::No
+        );
     }
 
     #[test]
     fn parses_empty_as_error() {
         let attr = Attribute::new("add-newline", "");
         assert_eq!(
-            AddNewline::parse(Some(&attr)),
-            Err(MdError::BadAddNewline("".into()))
+            AddNewline::parse(Some(&attr), Location::unknown()),
+            Err(MdError::BadAddNewline("".into(), Location::unknown()))
         );
     }
 
@@ -685,8 +714,11 @@ hello, world
     fn parses_garbage_as_error() {
         let attr = Attribute::new("add-newline", "garbage");
         assert_eq!(
-            AddNewline::parse(Some(&attr)),
-            Err(MdError::BadAddNewline("garbage".into()))
+            AddNewline::parse(Some(&attr), Location::unknown()),
+            Err(MdError::BadAddNewline(
+                "garbage".into(),
+                Location::unknown()
+            ))
         );
     }
 }
diff --git a/src/metadata.rs b/src/metadata.rs
index e4d3d53..263e13e 100644
--- a/src/metadata.rs
+++ b/src/metadata.rs
@@ -24,9 +24,6 @@ pub enum Error {
     #[error(transparent)]
     Regex(#[from] regex::Error),
 
-    #[error("Markdown doesn't contain a YAML block for document metadata")]
-    NoMetadata,
-
     #[error(transparent)]
     Yaml(#[from] serde_yaml::Error),
 }
diff --git a/subplot.md b/subplot.md
index 9929e8b..129fc8b 100644
--- a/subplot.md
+++ b/subplot.md
@@ -3566,3 +3566,89 @@ This is a test file.
 ~~~{#expected.txt .file}
 This is a test file.
 ~~~
+## Mistakes in markdown
+
+When there are mistakes in the markdown input, Subplot should report
+the location (filename, line, column) where the mistake is, and what
+the mistake is. The scenarios in this section verify that.
+
+### Scenario before the first heading
+
+_Requirement: A scenario must follow a heading._
+
+Justification: the heading can be used as the title for the scenario.
+
+~~~scenario
+given an installed subplot
+given file scenario-before-heading.subplot
+given file scenario-before-heading.md
+when I try to run subplot docgen scenario-before-heading.subplot -o /dev/null
+then command fails
+then stderr contains "ERROR: scenario-before-heading.md:1:1: first scenario is before first heading"
+~~~
+
+~~~{#scenario-before-heading.subplot .file .yaml}
+title: Foo
+markdowns:
+  - scenario-before-heading.md
+~~~
+
+~~~~~~{#scenario-before-heading.md .file .markdown}
+~~~scenario
+~~~
+~~~~~~
+
+### Attempt to use definition list
+
+_Requirement: Attempt to use definition lists is reported._
+
+Justification: the markdown parser we use in Subplot doesn't support
+them, and it would be unhelpful to not tell the user if they try to
+use them.
+
+~~~scenario
+given an installed subplot
+given file dl.subplot
+given file dl.md
+when I try to run subplot docgen dl.subplot -o /dev/null
+then command fails
+then stderr contains "ERROR: dl.md:3:1: attempt to use definition lists in Markdown"
+~~~
+
+~~~{#dl.subplot .file .yaml}
+title: Foo
+markdowns:
+  - dl.md
+~~~
+
+~~~~~~{#dl.md .file .markdown}
+# Foo
+
+Some term
+: Definition of term.
+~~~~~~
+
+### Bad "add-newline" value
+
+_Requirement: Only specific values for the "add-newline" attribute are
+allowed for an embedded file._
+
+~~~scenario
+given an installed subplot
+given file add-newline.subplot
+given file add-newline.md
+when I try to run subplot docgen add-newline.subplot -o /dev/null
+then command fails
+then stderr contains "ERROR: add-newline.md:1:1: value of add-newline attribute is not understood: xyzzy"
+~~~
+
+~~~{#add-newline.subplot .file .yaml}
+title: Foo
+markdowns:
+  - add-newline.md
+~~~
+
+~~~~~~{#add-newline.md .file .markdown}
+~~~{#foo.txt .file add-newline=xyzzy}
+~~~
+~~~~~~