Merge branch 'fix-339' into 'main'

Ensure all bindings have types

Closes #339

See merge request subplot/subplot!367

10 files changed, 224 insertions, 40 deletions

diff --git a/examples/muck/muck.yaml b/examples/muck/muck.yaml
index b22e088..967ab56 100644
--- a/examples/muck/muck.yaml
+++ b/examples/muck/muck.yaml
@@ -18,24 +18,34 @@
     python:
       function: fixme
   regex: true
+  types:
+    json: text
 
 - when: "I do PUT /res with Muck-Id: \\{(?P<id>\\S+)\\}, Muck-Revision: \\{(?P<rev>\\S+)\\}, and body (?P<json>\\{.*\\})"
   impl:
     python:
       function: fixme
   regex: true
+  types:
+    id: word
+    rev: word
+    json: text
 
 - when: "I do GET /res with Muck-Id: \\{(?P<id>\\S+)\\}"
   impl:
     python:
       function: fixme
   regex: true
+  types:
+    id: word
 
 - when: "I do DELETE /res with Muck-Id: \\{(?P<id>\\S+)\\}"
   impl:
     python:
       function: fixme
   regex: true
+  types:
+    id: word
 
 - when: "I restart Muck"
   impl:
@@ -58,15 +68,23 @@
     python:
       function: fixme
   regex: true
+  types:
+    header: word
+    name: word
 
 - then: "body matches (?P<json>\\{.*\\})"
   impl:
     python:
       function: fixme
   regex: true
+  types:
+    json: text
 
 - then: "revisions \\{(?P<rev1>\\S+)\\} and \\{(?P<rev2>\\S+)\\} are different"
   impl:
     python:
       function: fixme
   regex: true
+  types:
+    rev1: word
+    rev2: word
diff --git a/reference.md b/reference.md
index 1776e0b..9999636 100644
--- a/reference.md
+++ b/reference.md
@@ -12,7 +12,7 @@ set of subplot documents.
 given an installed subplot
 given a clone of https://gitlab.com/subplot/subplot.git in src at 5168420454b92205c13224a6801d3341d7f0c3d3
 when I docgen subplot.subplot to test.html, in src
-when I run, in src, subplot docgen subplot.subplot -o subplot.html -t python
+when I run, in src, subplot docgen subplot.subplot --merciful -o subplot.html -t python
 then file src/test.html exists
 ~~~
 
diff --git a/reference.py b/reference.py
index fa4b274..803716a 100644
--- a/reference.py
+++ b/reference.py
@@ -15,7 +15,7 @@ def docgen(ctx, filename=None, output=None, dirname=None):
 
     runcmd_run(
         ctx,
-        ["subplot", "docgen", filename, "--output", output, "-t", "python"],
+        ["subplot", "docgen", filename, "--merciful", "--output", output, "-t", "python"],
         cwd=dirname,
     )
     runcmd_exit_code_is_zero(ctx)
diff --git a/share/common/lib/files.yaml b/share/common/lib/files.yaml
index 8f3f932..689a5f3 100644
--- a/share/common/lib/files.yaml
+++ b/share/common/lib/files.yaml
@@ -242,6 +242,8 @@
     python:
       function: files_only_these_exist
   regex: true
+  types:
+    filenames: text
   doc: |
     Check that the test directory only contains specific files.
 
diff --git a/src/bin/subplot.rs b/src/bin/subplot.rs
index fd9500d..018ae5b 100644
--- a/src/bin/subplot.rs
+++ b/src/bin/subplot.rs
@@ -493,6 +493,7 @@ fn load_linted_doc(
     let template = template.to_string();
     trace!("Template: {:#?}", template);
     let mut warnings = Warnings::default();
+    doc.check_bindings(&mut warnings)?;
     doc.check_named_code_blocks_have_appropriate_class(&mut warnings)?;
     doc.check_named_files_exist(&template, &mut warnings)?;
     doc.check_matched_steps_have_impl(&template, &mut warnings);
diff --git a/src/bindings.rs b/src/bindings.rs
index b937edb..825b895 100644
--- a/src/bindings.rs
+++ b/src/bindings.rs
@@ -3,6 +3,7 @@ use super::MatchedSteps;
 use super::PartialStep;
 use super::ScenarioStep;
 use super::StepKind;
+use crate::Warning;
 use crate::{resource, SubplotError};
 
 use serde::{Deserialize, Serialize};
@@ -167,6 +168,7 @@ pub struct Binding {
     impls: HashMap<String, Arc<BindingImpl>>,
     types: HashMap<String, CaptureType>,
     doc: Option<String>,
+    filename: Arc<Path>,
 }
 
 impl Binding {
@@ -175,19 +177,14 @@ impl Binding {
         kind: StepKind,
         pattern: &str,
         case_sensitive: bool,
-        mut types: HashMap<String, CaptureType>,
+        types: HashMap<String, CaptureType>,
         doc: Option<String>,
+        filename: Arc<Path>,
     ) -> Result<Binding, SubplotError> {
         let regex = RegexBuilder::new(&format!("^{pattern}$"))
             .case_insensitive(!case_sensitive)
             .build()
             .map_err(|err| SubplotError::Regex(pattern.to_string(), err))?;
-        // For every named capture, ensure we have a known type for it.
-        // If the type is missing from the map, we default to `text` which is
-        // the .* pattern
-        for capture in regex.capture_names().flatten() {
-            types.entry(capture.into()).or_insert(CaptureType::Text);
-        }
 
         Ok(Binding {
             kind,
@@ -196,6 +193,7 @@ impl Binding {
             impls: HashMap::new(),
             types,
             doc,
+            filename,
         })
     }
 
@@ -283,15 +281,14 @@ impl Binding {
                     let cap = cap.as_str();
                     // These unwraps are safe because we ensured the map is complete
                     // in the constructor, and that all the types are known.
-                    let ty = self.types.get(name).unwrap();
-                    let rx = &KIND_PATTERNS.get(ty).unwrap();
+                    let kind = self.types.get(name).copied().unwrap_or(CaptureType::Text);
+                    let rx = KIND_PATTERNS.get(&kind).unwrap();
                     if !rx.is_match(cap) {
                         // This capture doesn't match the kind so it's not
                         // valid for this binding.
                         return None;
                     }
-                    let kind = self.types.get(name).unwrap_or(&CaptureType::Text);
-                    PartialStep::text(name, cap, *kind)
+                    PartialStep::text(name, cap, kind)
                 }
             };
 
@@ -307,6 +304,39 @@ impl Binding {
 
         Some(m)
     }
+
+    fn filename(&self) -> &Path {
+        &self.filename
+    }
+
+    fn check(&self, warnings: &mut crate::Warnings) -> Result<(), SubplotError> {
+        fn nth(i: usize) -> &'static str {
+            match i % 10 {
+                1 => "st",
+                2 => "nd",
+                3 => "rd",
+                _ => "th",
+            }
+        }
+        for (nr, capture) in self.regex.capture_names().enumerate().skip(1) {
+            if let Some(name) = capture {
+                if !self.types.contains_key(name) {
+                    warnings.push(Warning::MissingCaptureType(
+                        self.filename().to_owned(),
+                        format!("{}: {}", self.kind(), self.pattern()),
+                        name.to_string(),
+                    ));
+                }
+            } else {
+                warnings.push(Warning::MissingCaptureName(
+                    self.filename().to_owned(),
+                    format!("{}: {}", self.kind(), self.pattern()),
+                    format!("{nr}{}", nth(nr)),
+                ));
+            }
+        }
+        Ok(())
+    }
 }
 
 impl PartialEq for Binding {
@@ -325,10 +355,25 @@ mod test_binding {
     use crate::ScenarioStep;
     use crate::StepKind;
     use std::collections::HashMap;
+    use std::path::Path;
+    use std::path::PathBuf;
+    use std::sync::Arc;
+
+    fn path() -> Arc<Path> {
+        PathBuf::new().into()
+    }
 
     #[test]
     fn creates_new() {
-        let b = Binding::new(StepKind::Given, "I am Tomjon", false, HashMap::new(), None).unwrap();
+        let b = Binding::new(
+            StepKind::Given,
+            "I am Tomjon",
+            false,
+            HashMap::new(),
+            None,
+            path(),
+        )
+        .unwrap();
         assert_eq!(b.kind(), StepKind::Given);
         assert!(b.regex().is_match("I am Tomjon"));
         assert!(!b.regex().is_match("I am Tomjon of Lancre"));
@@ -337,20 +382,45 @@ mod test_binding {
 
     #[test]
     fn equal() {
-        let a = Binding::new(StepKind::Given, "I am Tomjon", false, HashMap::new(), None).unwrap();
-        let b = Binding::new(StepKind::Given, "I am Tomjon", false, HashMap::new(), None).unwrap();
+        let a = Binding::new(
+            StepKind::Given,
+            "I am Tomjon",
+            false,
+            HashMap::new(),
+            None,
+            path(),
+        )
+        .unwrap();
+        let b = Binding::new(
+            StepKind::Given,
+            "I am Tomjon",
+            false,
+            HashMap::new(),
+            None,
+            path(),
+        )
+        .unwrap();
         assert_eq!(a, b);
     }
 
     #[test]
     fn not_equal() {
-        let a = Binding::new(StepKind::Given, "I am Tomjon", false, HashMap::new(), None).unwrap();
+        let a = Binding::new(
+            StepKind::Given,
+            "I am Tomjon",
+            false,
+            HashMap::new(),
+            None,
+            path(),
+        )
+        .unwrap();
         let b = Binding::new(
             StepKind::Given,
             "I am Tomjon of Lancre",
             false,
             HashMap::new(),
             None,
+            path(),
         )
         .unwrap();
         assert_ne!(a, b);
@@ -359,21 +429,21 @@ mod test_binding {
     #[test]
     fn does_not_match_with_wrong_kind() {
         let step = ScenarioStep::new(StepKind::Given, "given", "yo", Location::Unknown);
-        let b = Binding::new(StepKind::When, "yo", false, HashMap::new(), None).unwrap();
+        let b = Binding::new(StepKind::When, "yo", false, HashMap::new(), None, path()).unwrap();
         assert!(b.match_with_step("", &step).is_none());
     }
 
     #[test]
     fn does_not_match_with_wrong_text() {
         let step = ScenarioStep::new(StepKind::Given, "given", "foo", Location::Unknown);
-        let b = Binding::new(StepKind::Given, "bar", false, HashMap::new(), None).unwrap();
+        let b = Binding::new(StepKind::Given, "bar", false, HashMap::new(), None, path()).unwrap();
         assert!(b.match_with_step("", &step).is_none());
     }
 
     #[test]
     fn match_with_fixed_pattern() {
         let step = ScenarioStep::new(StepKind::Given, "given", "foo", Location::Unknown);
-        let b = Binding::new(StepKind::Given, "foo", false, HashMap::new(), None).unwrap();
+        let b = Binding::new(StepKind::Given, "foo", false, HashMap::new(), None, path()).unwrap();
         let m = b.match_with_step("", &step).unwrap();
         assert_eq!(m.kind(), StepKind::Given);
         let mut parts = m.parts();
@@ -396,6 +466,7 @@ mod test_binding {
             false,
             HashMap::new(),
             None,
+            path(),
         )
         .unwrap();
         let m = b.match_with_step("", &step).unwrap();
@@ -413,9 +484,25 @@ mod test_binding {
     #[test]
     fn case_sensitive_mismatch() {
         let step = ScenarioStep::new(StepKind::Given, "given", "I am Tomjon", Location::Unknown);
-        let b = Binding::new(StepKind::Given, r"i am tomjon", false, HashMap::new(), None).unwrap();
+        let b = Binding::new(
+            StepKind::Given,
+            r"i am tomjon",
+            false,
+            HashMap::new(),
+            None,
+            path(),
+        )
+        .unwrap();
         assert!(b.match_with_step("", &step).is_some());
-        let b = Binding::new(StepKind::Given, r"i am tomjon", true, HashMap::new(), None).unwrap();
+        let b = Binding::new(
+            StepKind::Given,
+            r"i am tomjon",
+            true,
+            HashMap::new(),
+            None,
+            path(),
+        )
+        .unwrap();
         assert!(b.match_with_step("", &step).is_none());
     }
 }
@@ -493,11 +580,11 @@ impl Bindings {
     }
 
     /// Add bindings from a YAML string
-    pub fn add_from_yaml(&mut self, yaml: &str) -> Result<(), SubplotError> {
+    pub fn add_from_yaml(&mut self, yaml: &str, filename: Arc<Path>) -> Result<(), SubplotError> {
         let bindings: Vec<ParsedBindingWrapper> =
             serde_yaml::from_str(yaml).map_err(SubplotError::Metadata)?;
         for wrapper in bindings {
-            self.add(from_hashmap(&wrapper.binding)?);
+            self.add(from_hashmap(&wrapper.binding, Arc::clone(&filename))?);
         }
         Ok(())
     }
@@ -539,12 +626,12 @@ impl Bindings {
     where
         P: AsRef<Path> + Debug,
     {
-        let yaml = resource::read_as_string(filename.as_ref(), template)
-            .map_err(|e| SubplotError::BindingsFileNotFound(filename.as_ref().into(), e))?;
+        let filename = filename.as_ref();
+        let yaml = resource::read_as_string(filename, template)
+            .map_err(|e| SubplotError::BindingsFileNotFound(filename.into(), e))?;
         trace!("Loaded file content");
-        self.add_from_yaml(&yaml).map_err(|e| {
-            SubplotError::BindingFileParseError(filename.as_ref().to_owned(), Box::new(e))
-        })?;
+        self.add_from_yaml(&yaml, filename.to_owned().into())
+            .map_err(|e| SubplotError::BindingFileParseError(filename.to_owned(), Box::new(e)))?;
         Ok(())
     }
 
@@ -556,9 +643,17 @@ impl Bindings {
             .filter(|b| b.kind() == kind && b.pattern() == pattern);
         m.count() == 1
     }
+
+    /// Check these bindings for any warnings which users might need to know about
+    pub fn check(&self, warnings: &mut crate::Warnings) -> Result<(), SubplotError> {
+        for binding in self.bindings() {
+            binding.check(warnings)?;
+        }
+        Ok(())
+    }
 }
 
-fn from_hashmap(parsed: &ParsedBinding) -> Result<Binding, SubplotError> {
+fn from_hashmap(parsed: &ParsedBinding, filename: Arc<Path>) -> Result<Binding, SubplotError> {
     let given: i32 = parsed.given.is_some().into();
     let when: i32 = parsed.when.is_some().into();
     let then: i32 = parsed.then.is_some().into();
@@ -601,6 +696,7 @@ fn from_hashmap(parsed: &ParsedBinding) -> Result<Binding, SubplotError> {
         parsed.case_sensitive,
         types,
         parsed.doc.clone(),
+        filename,
     )?;
     trace!("Binding parsed OK");
     for (template, pimpl) in &parsed.impls {
@@ -622,6 +718,13 @@ mod test_bindings {
     use crate::SubplotError;
 
     use std::collections::HashMap;
+    use std::path::Path;
+    use std::path::PathBuf;
+    use std::sync::Arc;
+
+    fn path() -> Arc<Path> {
+        PathBuf::new().into()
+    }
 
     #[test]
     fn has_no_bindings_initially() {
@@ -637,6 +740,7 @@ mod test_bindings {
             false,
             HashMap::new(),
             None,
+            path(),
         )
         .unwrap();
         let mut bindings = Bindings::new();
@@ -672,7 +776,7 @@ mod test_bindings {
     total: word
 ";
         let mut bindings = Bindings::new();
-        bindings.add_from_yaml(yaml).unwrap();
+        bindings.add_from_yaml(yaml, path()).unwrap();
         println!("test: {bindings:?}");
         assert!(bindings.has(StepKind::Given, "I am Tomjon"));
         assert!(bindings.has(StepKind::When, "I declare myself king"));
@@ -692,7 +796,7 @@ mod test_bindings {
     python:
       FUNCTION: set_name
 ";
-        match Bindings::new().add_from_yaml(yaml) {
+        match Bindings::new().add_from_yaml(yaml, path()) {
             Ok(_) => unreachable!(),
             Err(SubplotError::BindingHasManyKeywords(_)) => (),
             Err(e) => panic!("Incorrect error: {}", e),
@@ -709,7 +813,7 @@ mod test_bindings {
   types:
     age: number
 ";
-        match Bindings::new().add_from_yaml(yaml) {
+        match Bindings::new().add_from_yaml(yaml, path()) {
             Ok(_) => unreachable!(),
             Err(SubplotError::SimplePatternKindMismatch(_)) => (),
             Err(e) => panic!("Incorrect error: {}", e),
@@ -719,8 +823,15 @@ mod test_bindings {
     #[test]
     fn does_not_find_match_for_unmatching_kind() {
         let step = ScenarioStep::new(StepKind::Given, "given", "I am Tomjon", Location::Unknown);
-        let binding =
-            Binding::new(StepKind::When, r"I am Tomjon", false, HashMap::new(), None).unwrap();
+        let binding = Binding::new(
+            StepKind::When,
+            r"I am Tomjon",
+            false,
+            HashMap::new(),
+            None,
+            path(),
+        )
+        .unwrap();
         let mut bindings = Bindings::new();
         bindings.add(binding);
         assert!(matches!(
@@ -738,6 +849,7 @@ mod test_bindings {
             false,
             HashMap::new(),
             None,
+            path(),
         )
         .unwrap();
         let mut bindings = Bindings::new();
@@ -753,7 +865,15 @@ mod test_bindings {
         let step = ScenarioStep::new(StepKind::Given, "given", "I am Tomjon", Location::Unknown);
         let mut bindings = Bindings::default();
         bindings.add(
-            Binding::new(StepKind::Given, r"I am Tomjon", false, HashMap::new(), None).unwrap(),
+            Binding::new(
+                StepKind::Given,
+                r"I am Tomjon",
+                false,
+                HashMap::new(),
+                None,
+                path(),
+            )
+            .unwrap(),
         );
         bindings.add(
             Binding::new(
@@ -763,6 +883,7 @@ mod test_bindings {
                 false,
                 HashMap::new(),
                 None,
+                path(),
             )
             .unwrap(),
         );
@@ -775,8 +896,15 @@ mod test_bindings {
     #[test]
     fn finds_match_for_fixed_string_pattern() {
         let step = ScenarioStep::new(StepKind::Given, "given", "I am Tomjon", Location::Unknown);
-        let binding =
-            Binding::new(StepKind::Given, r"I am Tomjon", false, HashMap::new(), None).unwrap();
+        let binding = Binding::new(
+            StepKind::Given,
+            r"I am Tomjon",
+            false,
+            HashMap::new(),
+            None,
+            path(),
+        )
+        .unwrap();
         let mut bindings = Bindings::new();
         bindings.add(binding);
         let m = bindings.find("", &step).unwrap();
@@ -799,6 +927,7 @@ mod test_bindings {
             false,
             HashMap::new(),
             None,
+            path(),
         )
         .unwrap();
         let mut bindings = Bindings::new();
diff --git a/src/doc.rs b/src/doc.rs
index 1e48e40..ff52b79 100644
--- a/src/doc.rs
+++ b/src/doc.rs
@@ -474,6 +474,11 @@ impl Document {
         set
     }
 
+    /// Check bindings for warnings
+    pub fn check_bindings(&self, warnings: &mut Warnings) -> Result<(), SubplotError> {
+        self.meta.bindings().check(warnings)
+    }
+
     /// Check labelled code blocks have some appropriate class
     pub fn check_named_code_blocks_have_appropriate_class(
         &self,
diff --git a/src/error.rs b/src/error.rs
index 7c63ca4..b89e94b 100644
--- a/src/error.rs
+++ b/src/error.rs
@@ -394,6 +394,14 @@ pub enum Warning {
     /// A code block has an identifier but is not marked as a file or example
     #[error("Code block has identifier but lacks file or example class.  Is this a mistake? #{0} at {1}")]
     MissingAppropriateClassOnNamedCodeBlock(String, String),
+
+    /// A capture in a binding is missing a name
+    #[error("{0}: {1} - missing a name for the {2} capture")]
+    MissingCaptureName(PathBuf, String, String),
+
+    /// A capture in a binding is missing a type
+    #[error("{0}: {1} - missing a type for the capture called {2}")]
+    MissingCaptureType(PathBuf, String, String),
 }
 
 /// A list of warnings.
diff --git a/subplot.yaml b/subplot.yaml
index 5ad35aa..1c384eb 100644
--- a/subplot.yaml
+++ b/subplot.yaml
@@ -28,6 +28,9 @@
     rust:
       function: step_was_run
   regex: true
+  types:
+    keyword: text
+    name: text
 
 - then: step "(?P<keyword1>given|when|then) (?P<name1>.+)" was run, and then step "(?P<keyword2>given|when|then) (?P<name2>.+)"
   impl:
@@ -36,6 +39,11 @@
     rust:
       function: step_was_run_and_then
   regex: true
+  types:
+    keyword1: text
+    keyword2: text
+    name1: text
+    name2: text
 
 - then: cleanup for "(?P<keyword1>given|when|then) (?P<name1>.+)" was run, and then for "(?P<keyword2>given|when|then) (?P<name2>.+)"
   impl:
@@ -44,6 +52,11 @@
     rust:
       function: cleanup_was_run
   regex: true
+  types:
+    keyword1: text
+    keyword2: text
+    name1: text
+    name2: text
 
 - then: cleanup for "(?P<keyword>given|when|then) (?P<name>.+)" was not run
   impl:
@@ -52,6 +65,9 @@
     rust:
       function: cleanup_was_not_run
   regex: true
+  types:
+    keyword: text
+    name: text
 
 - then: JSON output matches {filename}
   impl:
diff --git a/tests/bindings-ubm.rs b/tests/bindings-ubm.rs
index 4c4aaa0..e4232d1 100644
--- a/tests/bindings-ubm.rs
+++ b/tests/bindings-ubm.rs
@@ -4,12 +4,17 @@
 // there are a large number of them.
 
 use regex::RegexBuilder;
-use std::collections::HashMap;
+use std::path::{Path, PathBuf};
 use std::time::SystemTime;
+use std::{collections::HashMap, sync::Arc};
 use subplot::{html::Location, Binding, Bindings, ScenarioStep, StepKind};
 
 const N: i32 = 1000;
 
+fn path() -> Arc<Path> {
+    PathBuf::new().into()
+}
+
 #[test]
 fn bindings_microbenchmark() {
     let time = SystemTime::now();
@@ -34,7 +39,7 @@ fn bindings_microbenchmark() {
 
     let mut toadd = vec![];
     for t in texts.iter() {
-        toadd.push(Binding::new(StepKind::Given, t, false, HashMap::new(), None).unwrap());
+        toadd.push(Binding::new(StepKind::Given, t, false, HashMap::new(), None, path()).unwrap());
     }
     let created = time.elapsed().unwrap();