Merge branch 'kinnison/fix-59' into 'master'

feat: Support reporting error when more than one binding matches a step

Closes #59

See merge request larswirzenius/subplot!50

5 files changed, 122 insertions, 40 deletions

diff --git a/src/ast.rs b/src/ast.rs
index bc3547d..4e1d33b 100644
--- a/src/ast.rs
+++ b/src/ast.rs
@@ -803,15 +803,16 @@ fn step(
     }
     let step = step.unwrap();
 
-    let m = bindings.find(&step);
-    if m.is_none() {
-        eprintln!("Could not finding binding for: {}", text);
-        return (
-            error_msg(&format!("Could not find binding for: {}", text)),
-            defkind,
-        );
-    }
-    let m = m.unwrap();
+    let m = match bindings.find(&step) {
+        Ok(m) => m,
+        Err(e) => {
+            eprintln!("Could not select binding: {:?}", e);
+            return (
+                error_msg(&format!("Could not select binding for: {}", text)),
+                defkind,
+            );
+        }
+    };
 
     let mut inlines = Vec::new();
 
diff --git a/src/bindings.rs b/src/bindings.rs
index 4233528..40ef0b4 100644
--- a/src/bindings.rs
+++ b/src/bindings.rs
@@ -274,15 +274,15 @@ impl Bindings {
     }
 
     /// Add a binding to the set.
-    pub fn add(&mut self, binding: &Binding) {
-        self.bindings.push(binding.clone());
+    pub fn add(&mut self, binding: Binding) {
+        self.bindings.push(binding);
     }
 
     /// Add bindings from a YAML string
     pub fn add_from_yaml(&mut self, yaml: &str) -> Result<()> {
         let bindings: Vec<ParsedBinding> = serde_yaml::from_str(yaml)?;
         for b in bindings {
-            self.add(&from_hashmap(&b)?);
+            self.add(from_hashmap(&b)?);
         }
         Ok(())
     }
@@ -294,20 +294,17 @@ impl Bindings {
 
     /// Find the binding matching a given scenario step, if there is
     /// exactly one.
-    pub fn find(&self, step: &ScenarioStep) -> Option<MatchedStep> {
+    pub fn find(&self, step: &ScenarioStep) -> Result<MatchedStep> {
         let mut matches = self
             .bindings()
             .iter()
-            .map(|b| b.match_with_step(step))
-            .filter(|o| o.is_some());
-        if let Some(m) = matches.next() {
-            if matches.count() == 0 {
-                m
-            } else {
-                None
-            }
-        } else {
-            None
+            .filter_map(|b| b.match_with_step(step));
+        match matches.next() {
+            None => Err(SubplotError::BindingUnknown(step.to_string())),
+            Some(matched) => match matches.next() {
+                None => Ok(matched),
+                Some(_) => Err(SubplotError::BindingNotUnique(step.to_string())),
+            },
         }
     }
 
@@ -398,7 +395,7 @@ mod test_bindings {
         let binding =
             Binding::new(StepKind::Given, r"I am (?P<name>\S+)", "set_name", None).unwrap();
         let mut bindings = Bindings::new();
-        bindings.add(&binding);
+        bindings.add(binding.clone());
         assert_eq!(bindings.bindings(), &[binding]);
     }
 
@@ -440,8 +437,11 @@ mod test_bindings {
         let step = ScenarioStep::new(StepKind::Given, "given", "I am Tomjon");
         let binding = Binding::new(StepKind::When, r"I am Tomjon", "set_foo", None).unwrap();
         let mut bindings = Bindings::new();
-        bindings.add(&binding);
-        assert!(bindings.find(&step).is_none());
+        bindings.add(binding);
+        assert!(match bindings.find(&step) {
+            Err(SubplotError::BindingUnknown(_)) => true,
+            _ => false,
+        });
     }
 
     #[test]
@@ -450,8 +450,31 @@ mod test_bindings {
         let binding =
             Binding::new(StepKind::Given, r"I am Tomjon of Lancre", "set_foo", None).unwrap();
         let mut bindings = Bindings::new();
-        bindings.add(&binding);
-        assert!(bindings.find(&step).is_none());
+        bindings.add(binding);
+        assert!(match bindings.find(&step) {
+            Err(SubplotError::BindingUnknown(_)) => true,
+            _ => false,
+        });
+    }
+
+    #[test]
+    fn two_matching_bindings() {
+        let step = ScenarioStep::new(StepKind::Given, "given", "I am Tomjon");
+        let mut bindings = Bindings::default();
+        bindings.add(Binding::new(StepKind::Given, r"I am Tomjon", "set_foo", None).unwrap());
+        bindings.add(
+            Binding::new(
+                StepKind::Given,
+                &super::regex_from_simple_pattern(r"I am {name}", false).unwrap(),
+                "set_foo",
+                None,
+            )
+            .unwrap(),
+        );
+        assert!(match bindings.find(&step) {
+            Err(SubplotError::BindingNotUnique(_)) => true,
+            _ => false,
+        });
     }
 
     #[test]
@@ -459,7 +482,7 @@ mod test_bindings {
         let step = ScenarioStep::new(StepKind::Given, "given", "I am Tomjon");
         let binding = Binding::new(StepKind::Given, r"I am Tomjon", "set_name", None).unwrap();
         let mut bindings = Bindings::new();
-        bindings.add(&binding);
+        bindings.add(binding);
         let m = bindings.find(&step).unwrap();
         assert_eq!(m.kind(), StepKind::Given);
         let mut parts = m.parts();
@@ -477,7 +500,7 @@ mod test_bindings {
         let binding =
             Binding::new(StepKind::Given, r"I am (?P<name>\S+)", "set_name", None).unwrap();
         let mut bindings = Bindings::new();
-        bindings.add(&binding);
+        bindings.add(binding);
         let m = bindings.find(&step).unwrap();
         assert_eq!(m.kind(), StepKind::Given);
         let mut parts = m.parts();
diff --git a/src/error.rs b/src/error.rs
index f988c86..daef85c 100644
--- a/src/error.rs
+++ b/src/error.rs
@@ -34,6 +34,13 @@ pub enum SubplotError {
     #[error("do not understand binding: {0}")]
     BindingUnknown(String),
 
+    /// Scenario step matches more than one binding
+    ///
+    /// THis may be due to bindings being too general, or having unusual
+    /// overlaps in their matching
+    #[error("more than one binding matches: {0}")]
+    BindingNotUnique(String),
+
     /// A binding in the bindings file doesn't specify a known keyword.
     #[error("binding doesn't specify known keyword: {0}")]
     BindingWithoutKnownKeyword(String),
diff --git a/src/matches.rs b/src/matches.rs
index 27a71e1..2c8f196 100644
--- a/src/matches.rs
+++ b/src/matches.rs
@@ -1,9 +1,7 @@
 use crate::Bindings;
 use crate::Result;
 use crate::Scenario;
-use crate::ScenarioStep;
 use crate::StepKind;
-use crate::SubplotError;
 
 use serde::{Deserialize, Serialize};
 
@@ -20,7 +18,7 @@ impl MatchedScenario {
         let steps: Result<Vec<MatchedStep>> = scen
             .steps()
             .iter()
-            .map(|step| find_binding(step, bindings))
+            .map(|step| bindings.find(step))
             .collect();
         Ok(MatchedScenario {
             title: scen.title().to_string(),
@@ -29,13 +27,6 @@ impl MatchedScenario {
     }
 }
 
-fn find_binding(step: &ScenarioStep, bindings: &Bindings) -> Result<MatchedStep> {
-    match bindings.find(step) {
-        None => Err(SubplotError::BindingUnknown(step.to_string())),
-        Some(ms) => Ok(ms),
-    }
-}
-
 /// A matched binding and scenario step, with captured parts.
 ///
 /// A MatchedStep is a sequence of partial steps, each representing
diff --git a/subplot.md b/subplot.md
index b2cdecf..b99f397 100644
--- a/subplot.md
+++ b/subplot.md
@@ -1811,6 +1811,66 @@ This is another embedded file, and has the same name in uppercase.
 ```
 ~~~~
 
+## Steps must match bindings
+
+Subplot permits the binding author to define arbitrarily complex regular
+expressions for binding matches.  In order to ensure that associating steps
+to bindings is both reliable and tractable, a step must match _exactly one_
+binding.
+
+```{#badbindings.yaml .file .yaml}
+- given: a binding
+  function: a_binding
+- given: a (?:broken)? binding
+  function: a_broken_binding
+  regex: true
+```
+
+### Steps which do not match bindings do not work
+
+~~~~{#nobinding.md .file .markdown}
+---
+title: No bindings available
+bindings:
+- badbindings.yaml
+...
+# Broken scenario because step has no binding
+
+```scenario
+given a missing binding
+then nothing works
+```
+~~~~
+
+```scenario
+given file nobinding.md
+and file badbindings.yaml
+when I try to run sp-codegen --run nobinding.md -o test.py
+then exit code is non-zero
+```
+
+### Steps which match more than one binding do not work
+
+~~~~{#twobindings.md .file .markdown}
+---
+title: Two bindings match
+bindings:
+- badbindings.yaml
+...
+# Broken scenario because step has two possible bindings
+
+```scenario
+given a binding
+```
+~~~~
+
+```scenario
+given file twobindings.md
+and file badbindings.yaml
+when I try to run sp-codegen --run twobindings.md -o test.py
+then exit code is non-zero
+```
+
 
 ## Embedded graphs