From da1774ea591a7c0bc241e4d0c16649a1cf485c13 Mon Sep 17 00:00:00 2001
From: Daniel Silverstone <dsilvers@digital-scurf.org>
Date: Wed, 27 Dec 2023 18:19:12 +0000
Subject: bindings: Add warnings for missing capture names or missing capture
 types

Signed-off-by: Daniel Silverstone <dsilvers@digital-scurf.org>
---
 src/bin/subplot.rs |  1 +
 src/bindings.rs    | 53 ++++++++++++++++++++++++++++++++++++++++++-----------
 src/doc.rs         |  5 +++++
 src/error.rs       |  8 ++++++++
 4 files changed, 56 insertions(+), 11 deletions(-)

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 c342432..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};
@@ -176,7 +177,7 @@ 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> {
@@ -184,12 +185,6 @@ impl Binding {
             .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,
@@ -286,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)
                 }
             };
 
@@ -314,6 +308,35 @@ impl Binding {
     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 {
@@ -620,6 +643,14 @@ 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, filename: Arc<Path>) -> Result<Binding, SubplotError> {
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.
-- 
cgit v1.2.1