Various Fixes and Improvements

gsd-parser/src/lib.rs

@@ -1,3 +1,6 @@
+#![expect(clippy::result_large_err)]
+#![deny(clippy::as_conversions)]
+
 use std::collections::BTreeMap;
 use std::path::Path;
 use std::sync::Arc;
@@ -139,6 +142,23 @@ pub enum UserPrmDataType {
     BitArea(u8, u8),
 }
 
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub struct PrmValueRangeError(pub(crate) ());
+
+impl From<std::num::TryFromIntError> for PrmValueRangeError {
+    fn from(_value: std::num::TryFromIntError) -> Self {
+        PrmValueRangeError(())
+    }
+}
+
+impl std::fmt::Display for PrmValueRangeError {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        "value for user_prm is not in range for data type".fmt(f)
+    }
+}
+
+impl std::error::Error for PrmValueRangeError {}
+
 impl UserPrmDataType {
     pub fn size(self) -> usize {
         match self {
@@ -153,42 +173,42 @@ impl UserPrmDataType {
         }
     }
 
-    pub fn write_value_to_slice(self, value: i64, s: &mut [u8]) {
+    pub fn write_value_to_slice(self, value: i64, s: &mut [u8]) -> Result<(), PrmValueRangeError> {
         match self {
             UserPrmDataType::Unsigned8 => {
-                assert!(0 <= value && value <= 255);
-                s[..1].copy_from_slice(&(value as u8).to_be_bytes());
+                s[..1].copy_from_slice(&u8::try_from(value)?.to_be_bytes());
             }
             UserPrmDataType::Unsigned16 => {
-                assert!(0 <= value && value <= 65535);
-                s[..2].copy_from_slice(&(value as u16).to_be_bytes());
+                s[..2].copy_from_slice(&u16::try_from(value)?.to_be_bytes());
             }
             UserPrmDataType::Unsigned32 => {
-                assert!(0 <= value && value <= 4294967295);
-                s[..4].copy_from_slice(&(value as u32).to_be_bytes());
+                s[..4].copy_from_slice(&u32::try_from(value)?.to_be_bytes());
             }
             UserPrmDataType::Signed8 => {
-                assert!(-127 <= value && value <= 127);
-                s[..1].copy_from_slice(&(value as i8).to_be_bytes());
+                s[..1].copy_from_slice(&i8::try_from(value)?.to_be_bytes());
             }
             UserPrmDataType::Signed16 => {
-                assert!(-32767 <= value && value <= 32767);
-                s[..2].copy_from_slice(&(value as i16).to_be_bytes());
+                s[..2].copy_from_slice(&u16::try_from(value)?.to_be_bytes());
             }
             UserPrmDataType::Signed32 => {
-                assert!(2147483647 <= value && value <= 2147483647);
-                s[..4].copy_from_slice(&(value as i32).to_be_bytes());
+                s[..4].copy_from_slice(&i32::try_from(value)?.to_be_bytes());
             }
             UserPrmDataType::Bit(b) => {
+                if value != 0 && value != 1 {
+                    return Err(PrmValueRangeError(()));
+                }
                 assert!(value == 0 || value == 1);
-                s[0] |= (value as u8) << b;
+                s[0] |= u8::try_from(value)? << b;
             }
             UserPrmDataType::BitArea(first, last) => {
                 let bit_size = last - first + 1;
-                assert!(value >= 0 && value < 2i64.pow(bit_size as u32));
-                s[0] = (value as u8) << first;
+                if value < 0 || value >= 2i64.pow(u32::from(bit_size)) {
+                    return Err(PrmValueRangeError(()));
+                }
+                s[0] = u8::try_from(value)? << first;
             }
         }
+        Ok(())
     }
 }
 
@@ -199,6 +219,28 @@ pub enum PrmValueConstraint {
     Unconstrained,
 }
 
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub struct PrmValueConstraintError {
+    value: i64,
+    constraint: PrmValueConstraint,
+}
+
+impl std::fmt::Display for PrmValueConstraintError {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        match &self.constraint {
+            PrmValueConstraint::MinMax(min, max) => {
+                write!(f, "value {} not in range {min}..={max}", self.value)
+            }
+            PrmValueConstraint::Enum(values) => {
+                write!(f, "value {} not in set {values:?}", self.value)
+            }
+            PrmValueConstraint::Unconstrained => unreachable!(),
+        }
+    }
+}
+
+impl std::error::Error for PrmValueConstraintError {}
+
 impl PrmValueConstraint {
     pub fn is_valid(&self, value: i64) -> bool {
         match self {
@@ -208,21 +250,29 @@ impl PrmValueConstraint {
         }
     }
 
-    pub fn assert_valid(&self, value: i64) {
+    pub fn assert_valid(&self, value: i64) -> Result<(), PrmValueConstraintError> {
         match self {
             PrmValueConstraint::MinMax(min, max) => {
-                assert!(
-                    *min <= value && value <= *max,
-                    "value {value} not in range {min}..={max}",
-                );
+                if *min > value || value > *max {
+                    Err(PrmValueConstraintError {
+                        value,
+                        constraint: self.clone(),
+                    })
+                } else {
+                    Ok(())
+                }
             }
             PrmValueConstraint::Enum(values) => {
-                assert!(
-                    values.contains(&value),
-                    "value {value} not in set {values:?}",
-                );
+                if !values.contains(&value) {
+                    Err(PrmValueConstraintError {
+                        value,
+                        constraint: self.clone(),
+                    })
+                } else {
+                    Ok(())
+                }
             }
-            PrmValueConstraint::Unconstrained => (),
+            PrmValueConstraint::Unconstrained => Ok(()),
         }
     }
 }
@@ -350,20 +400,60 @@ pub struct GenericStationDescription {
     pub unit_diag: UnitDiag,
 }
 
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub enum SetPrmError {
+    PrmNotFound(String),
+    PrmWithoutTexts(String),
+    PrmTextNotFound { prm: String, text: String },
+    ValueConstraint(PrmValueConstraintError),
+    ValueRange { value: i64, ty: UserPrmDataType },
+}
+
+impl From<PrmValueConstraintError> for SetPrmError {
+    fn from(value: PrmValueConstraintError) -> Self {
+        Self::ValueConstraint(value)
+    }
+}
+
+impl std::fmt::Display for SetPrmError {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        match self {
+            SetPrmError::PrmNotFound(name) => {
+                write!(f, "prm {name} was not found")
+            }
+            SetPrmError::ValueConstraint(prm_value_constraint_error) => {
+                prm_value_constraint_error.fmt(f)
+            }
+            SetPrmError::ValueRange { value, ty } => {
+                write!(f, "value {value} is out of range for type {ty:?}")
+            }
+            SetPrmError::PrmWithoutTexts(name) => {
+                write!(f, "prm {name} does not have texts")
+            }
+            SetPrmError::PrmTextNotFound { prm, text } => {
+                write!(f, "prm {prm} does not have text {text:?}")
+            }
+        }
+    }
+}
+
+impl std::error::Error for SetPrmError {}
+
+#[derive(Debug, PartialEq, Eq)]
 pub struct PrmBuilder<'a> {
     desc: &'a UserPrmData,
     prm: Vec<u8>,
 }
 
 impl<'a> PrmBuilder<'a> {
-    pub fn new(desc: &'a UserPrmData) -> Self {
+    pub fn new(desc: &'a UserPrmData) -> Result<Self, PrmValueRangeError> {
         let mut this = Self {
             desc,
             prm: Vec::new(),
         };
         this.write_const_prm_data();
-        this.write_default_prm_data();
-        this
+        this.write_default_prm_data()?;
+        Ok(this)
     }
 
     fn update_prm_data_len(&mut self, offset: usize, size: usize) {
@@ -381,44 +471,61 @@ impl<'a> PrmBuilder<'a> {
         }
     }
 
-    fn write_default_prm_data(&mut self) {
+    fn write_default_prm_data(&mut self) -> Result<(), PrmValueRangeError> {
         for (offset, data_ref) in self.desc.data_ref.iter() {
             let size = data_ref.data_type.size();
             self.update_prm_data_len(*offset, size);
             data_ref
                 .data_type
-                .write_value_to_slice(data_ref.default_value, &mut self.prm[(*offset as usize)..]);
+                .write_value_to_slice(data_ref.default_value, &mut self.prm[(*offset)..])?;
         }
+        Ok(())
     }
 
-    pub fn set_prm(&mut self, prm: &str, value: i64) -> &mut Self {
+    pub fn set_prm(&mut self, prm: &str, value: i64) -> Result<&mut Self, SetPrmError> {
         let (offset, data_ref) = self
             .desc
             .data_ref
             .iter()
             .find(|(_, r)| r.name == prm)
-            .unwrap();
-        data_ref.constraint.assert_valid(value);
+            .ok_or_else(|| SetPrmError::PrmNotFound(prm.to_string()))?;
+        data_ref.constraint.assert_valid(value)?;
         data_ref
             .data_type
-            .write_value_to_slice(value, &mut self.prm[(*offset as usize)..]);
-        self
+            .write_value_to_slice(value, &mut self.prm[(*offset)..])
+            .map_err(|_e| SetPrmError::ValueRange {
+                value,
+                ty: data_ref.data_type,
+            })?;
+        Ok(self)
     }
 
-    pub fn set_prm_from_text(&mut self, prm: &str, value: &str) -> &mut Self {
+    pub fn set_prm_from_text(&mut self, prm: &str, value: &str) -> Result<&mut Self, SetPrmError> {
         let (offset, data_ref) = self
             .desc
             .data_ref
             .iter()
             .find(|(_, r)| r.name == prm)
-            .unwrap();
-        let text_ref = data_ref.text_ref.as_ref().unwrap();
-        let value = *text_ref.get(value).unwrap();
-        data_ref.constraint.assert_valid(value);
+            .ok_or_else(|| SetPrmError::PrmNotFound(prm.to_string()))?;
+        let text_ref = data_ref
+            .text_ref
+            .as_ref()
+            .ok_or_else(|| SetPrmError::PrmWithoutTexts(prm.to_string()))?;
+        let value = *text_ref
+            .get(value)
+            .ok_or_else(|| SetPrmError::PrmTextNotFound {
+                prm: prm.to_string(),
+                text: value.to_string(),
+            })?;
+        data_ref.constraint.assert_valid(value)?;
         data_ref
             .data_type
-            .write_value_to_slice(value, &mut self.prm[(*offset as usize)..]);
-        self
+            .write_value_to_slice(value, &mut self.prm[(*offset)..])
+            .map_err(|_e| SetPrmError::ValueRange {
+                value,
+                ty: data_ref.data_type,
+            })?;
+        Ok(self)
     }
 
     pub fn as_bytes(&self) -> &[u8] {
@@ -433,9 +540,9 @@ impl<'a> PrmBuilder<'a> {
 pub fn parse_from_file<P: AsRef<Path>>(file: P) -> GenericStationDescription {
     use std::io::Read;
 
-    let mut f = std::fs::File::open(file.as_ref()).unwrap();
+    let mut f = std::fs::File::open(file.as_ref()).expect("TODO");
     let mut source_bytes = Vec::new();
-    f.read_to_end(&mut source_bytes).unwrap();
+    f.read_to_end(&mut source_bytes).expect("TODO");
     let source = String::from_utf8_lossy(&source_bytes);
 
     match parser::parse(file.as_ref(), &source) {

gsd-parser/src/parser.rs

@@ -57,7 +57,6 @@ where
     Ok(match pair.as_rule() {
         gsd_parser::Rule::number_list => pair
             .into_inner()
-            .into_iter()
             .map(|p| parse_number::<T>(p))
             .collect::<ParseResult<Vec<T>>>()?,
         gsd_parser::Rule::dec_number | gsd_parser::Rule::hex_number => {
@@ -96,7 +95,7 @@ pub fn parse(
 fn parse_inner(source: &str) -> ParseResult<crate::GenericStationDescription> {
     use pest::Parser;
 
-    let gsd_pairs = gsd_parser::GsdParser::parse(gsd_parser::Rule::gsd, &source)?
+    let gsd_pairs = gsd_parser::GsdParser::parse(gsd_parser::Rule::gsd, source)?
         .next()
         .expect("pest grammar wrong?");
 
@@ -276,8 +275,10 @@ fn parse_inner(source: &str) -> ParseResult<crate::GenericStationDescription> {
                                 "ext_user_prm_data_ref" => {
                                     let offset = parse_number(value_pair)?;
                                     let data_id = parse_number(pairs.next().unwrap())?;
-                                    let data_ref =
-                                        user_prm_data_definitions.get(&data_id).unwrap().clone();
+                                    let data_ref = user_prm_data_definitions
+                                        .get(&data_id)
+                                        .expect("TODO")
+                                        .clone();
                                     module_prm_data.data_ref.push((offset, data_ref));
                                 }
                                 "ext_user_prm_data_const" => {
@@ -483,7 +484,10 @@ fn parse_inner(source: &str) -> ParseResult<crate::GenericStationDescription> {
                     "ext_user_prm_data_ref" => {
                         let offset = parse_number(value_pair)?;
                         let data_id = parse_number(pairs.next().unwrap())?;
-                        let data_ref = user_prm_data_definitions.get(&data_id).unwrap().clone();
+                        let data_ref = user_prm_data_definitions
+                            .get(&data_id)
+                            .expect("TODO")
+                            .clone();
                         gsd.user_prm_data.data_ref.push((offset, data_ref));
                         // The presence of this keywords means `User_Prm_Data` and
                         // `User_Prm_Data_Len` should be ignored.

gsd-parser/tests/regress.rs

@@ -11,7 +11,7 @@ fn regress(#[files("tests/data/*.[gG][sS][dD]")] gsd_file: PathBuf) {
 fn regress_prm(#[files("tests/data/*.[gG][sS][dD]")] gsd_file: PathBuf) {
     let name = gsd_file.file_stem().unwrap().to_string_lossy().to_string();
     let gsd = gsd_parser::parse_from_file(gsd_file);
-    let mut prm = gsd_parser::PrmBuilder::new(&gsd.user_prm_data);
+    let mut prm = gsd_parser::PrmBuilder::new(&gsd.user_prm_data).unwrap();
 
     // Try setting all the available parameters to some reasonable values.
     for (_, prm_ref) in gsd.user_prm_data.data_ref.iter() {
@@ -22,16 +22,32 @@ fn regress_prm(#[files("tests/data/*.[gG][sS][dD]")] gsd_file: PathBuf) {
                 // Fallback when the list only has one text...
                 texts.keys().next().unwrap()
             };
-            prm.set_prm_from_text(&prm_ref.name, text);
+            prm.set_prm_from_text(&prm_ref.name, text).unwrap();
+
+            // Test that trying a wrong text doens't panic
+            let res = prm.set_prm_from_text(&prm_ref.name, "InvalidTextAllTheWay");
+            assert!(res.is_err());
         } else {
             let v = match &prm_ref.constraint {
                 gsd_parser::PrmValueConstraint::MinMax(_, max) => *max,
                 gsd_parser::PrmValueConstraint::Enum(values) => *values.last().unwrap(),
                 gsd_parser::PrmValueConstraint::Unconstrained => 1,
             };
-            prm.set_prm(&prm_ref.name, v);
+            prm.set_prm(&prm_ref.name, v).unwrap();
+
+            // Test that an invalid value results in an error rather than a panic
+            let res = prm.set_prm(&prm_ref.name, i64::MIN);
+            assert!(res.is_err());
+
+            // Test that trying a text PRM doesn't panic
+            let res = prm.set_prm_from_text(&prm_ref.name, "InvalidTextAllTheWay");
+            assert!(res.is_err());
         }
     }
 
+    // Test that a non-existent PRM doesn't panic
+    let res = prm.set_prm("ThisPrmNeverEverExistsEver", 0);
+    assert!(res.is_err());
+
     insta::assert_debug_snapshot!(format!("{}-PRM", name).as_ref(), prm.as_bytes());
 }