warn about confusion when operating of multiple units with offsets

Signed-off-by: eternal-flame-AD <yume@yumechi.jp>

Cargo.toml

@@ -14,6 +14,7 @@ readme = "README.md"
 # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
 
 [dependencies]
+itertools = "0.13.0"
 log = "0.4.19"
 num-bigint = { version = "0.4.3", features = ["serde"] }
 num-rational = { version = "0.4.1", features = ["serde"] }

src/interpreter/mod.rs

@@ -11,6 +11,9 @@ use crate::{
     tokenizer::{token::Token, ReaderCursor, Tokenizer, TokenizerError},
 };
 
+use itertools::Itertools;
+use num_bigint::BigInt;
+use num_traits::Zero;
 use thiserror::Error;
 
 /// All other operations.
@@ -67,6 +70,30 @@ impl<'a> Interpreter<'a> {
             output,
         }
     }
+    /// Warns about quantities with offset derived units that are used in multiple quantities.
+    pub fn warn_confusing_unit_conversions(&self, qs: &[&Quantity]) {
+        let offset_base_units = qs
+            .iter()
+            .flat_map(|q| q.use_derived_unit.iter())
+            .filter(|d| *d.offset.numer() != BigInt::zero())
+            .flat_map(|d| {
+                d.exponents
+                    .0
+                    .iter()
+                    .unique_by(|d| &d.unit)
+                    .map(|e| (d.clone(), e.unit.clone()))
+            });
+
+        for (u, c) in offset_base_units.into_group_map_by(|(_, u)| u.clone()) {
+            if c.len() > 1 {
+                (self.output)(Output::Message(format!(
+                    "Warning: {} it is used in multiple quantities with an offset. This may lead to unexpected results. Affected derived units: [{}]",
+                    u,
+                    c.iter().map(|(d, _)| d.symbol.clone()).join(", ")
+                )));
+            }
+        }
+    }
     /// Reads all tokens from the tokenizer and processes them.
     pub fn process_tokens<R: Read>(
         &mut self,

src/interpreter/ops.rs

@@ -79,6 +79,8 @@ impl<'a> Interpreter<'a> {
         let rhs = self.stack.pop().ok_or(InterpreterError::StackUnderflow)?;
         let lhs = self.stack.pop().ok_or(InterpreterError::StackUnderflow)?;
 
+        self.warn_confusing_unit_conversions(&[&lhs, &rhs]);
+
         self.stack
             .push((lhs + rhs).map_err(InterpreterError::QuantityError)?);
 
@@ -89,6 +91,8 @@ impl<'a> Interpreter<'a> {
         let rhs = self.stack.pop().ok_or(InterpreterError::StackUnderflow)?;
         let lhs = self.stack.pop().ok_or(InterpreterError::StackUnderflow)?;
 
+        self.warn_confusing_unit_conversions(&[&lhs, &rhs]);
+
         self.stack
             .push((lhs - rhs).map_err(InterpreterError::QuantityError)?);
 
@@ -99,6 +103,8 @@ impl<'a> Interpreter<'a> {
         let rhs = self.stack.pop().ok_or(InterpreterError::StackUnderflow)?;
         let lhs = self.stack.pop().ok_or(InterpreterError::StackUnderflow)?;
 
+        self.warn_confusing_unit_conversions(&[&lhs, &rhs]);
+
         self.stack.push(lhs * rhs);
 
         Ok(())
@@ -108,6 +114,8 @@ impl<'a> Interpreter<'a> {
         let rhs = self.stack.pop().ok_or(InterpreterError::StackUnderflow)?;
         let lhs = self.stack.pop().ok_or(InterpreterError::StackUnderflow)?;
 
+        self.warn_confusing_unit_conversions(&[&lhs, &rhs]);
+
         self.stack.push(lhs / rhs);
 
         Ok(())

src/quantity/mod.rs

@@ -3,6 +3,7 @@ use std::{
     ops::{Add, Div, Mul, Sub},
 };
 
+use itertools::Itertools;
 use num_rational::BigRational;
 use num_traits::ToPrimitive;
 use serde::{ser::SerializeStruct, Deserialize, Serialize, Serializer};
@@ -147,6 +148,16 @@ impl Mul for Quantity {
                 use_derived_unit.push(lhs_d.clone() * rhs_d.clone());
             }
         }
+        for lhs_d in &self.use_derived_unit {
+            use_derived_unit.push(lhs_d.clone());
+        }
+        for rhs_d in &rhs.use_derived_unit {
+            use_derived_unit.push(rhs_d.clone());
+        }
+        use_derived_unit = use_derived_unit
+            .into_iter()
+            .unique_by(|d| d.symbol.clone())
+            .collect();
 
         Quantity {
             number,
@@ -168,6 +179,16 @@ impl Div for Quantity {
                 use_derived_unit.push(lhs_d.clone() / rhs_d.clone());
             }
         }
+        for lhs_d in &self.use_derived_unit {
+            use_derived_unit.push(lhs_d.clone());
+        }
+        for rhs_d in &rhs.use_derived_unit {
+            use_derived_unit.push(rhs_d.clone());
+        }
+        use_derived_unit = use_derived_unit
+            .into_iter()
+            .unique_by(|d| d.symbol.clone())
+            .collect();
 
         Quantity {
             number,

src/quantity/units.rs

@@ -79,7 +79,7 @@ impl UnitSystem {
     }
 }
 
-#[derive(Debug, PartialEq, Clone, Serialize, Deserialize)]
+#[derive(Debug, PartialEq, Eq, Hash, Clone, Serialize, Deserialize)]
 pub struct BaseUnit {
     pub symbol: String,
 }

tests/interpreter_test.rs

@@ -58,3 +58,73 @@ fn test_interpreter() {
         _ => panic!("output should be a quantity"),
     }
 }
+
+#[test]
+fn test_warn_confusing_units() {
+    let outputs = Mutex::new(Vec::new());
+    let output_fn = |output| outputs.lock().unwrap().push(output);
+    let mut interpreter = Interpreter::new(Box::new(output_fn));
+    interpreter
+        .run_str("@base(K)")
+        .expect("command should succeed");
+
+    interpreter
+        .run_str("273.15 (K) 1 @derived(degC)")
+        .expect("command should succeed");
+
+    interpreter
+        .run_str("0 (degC) 0 (degC) + p")
+        .expect("command should succeed");
+
+    let output = outputs.lock().unwrap().pop().expect("output should exist");
+    let msg = outputs.lock().unwrap().pop().expect("output should exist");
+
+    match msg {
+        unitdc::interpreter::Output::Message(e) => {
+            assert_eq!(
+                e.to_ascii_lowercase().contains("warning"),
+                true,
+                "output should contain 'warning'"
+            );
+        }
+        _ => panic!("output should be a message"),
+    }
+
+    match output {
+        unitdc::interpreter::Output::Quantity(q) => {
+            assert_eq!(q.number.to_f64().unwrap(), 273.15 * 2.);
+            assert_eq!(
+                q.unit,
+                UnitCombo(vec![UnitExponent {
+                    unit: BaseUnit {
+                        symbol: "K".to_string(),
+                    },
+                    exponent: 1,
+                },])
+            );
+        }
+        _ => panic!("output should be a quantity"),
+    }
+
+    interpreter
+        .run_str("@base(m)")
+        .expect("command should succeed");
+
+    interpreter
+        .run_str("0 (degC) 1 (m) * 0 (degC) 1 (m) * + p")
+        .expect("command should succeed");
+
+    let _ = outputs.lock().unwrap().pop().expect("output should exist");
+    let msg = outputs.lock().unwrap().pop().expect("output should exist");
+
+    match msg {
+        unitdc::interpreter::Output::Message(e) => {
+            assert_eq!(
+                e.to_ascii_lowercase().contains("warning"),
+                true,
+                "output should contain 'warning'"
+            );
+        }
+        _ => panic!("output should be a message"),
+    }
+}

unitdc.rc

@@ -18,11 +18,16 @@
 0 (m) 1 1.093613298 / @derived(yd)
 0 (m) 1 0.0006213712 / @derived(mi)
 
-@base(degC)
-0 (degC) 273.15 @derived(K)
+@base(K)
+273.15 (K) 1 @derived(degC)
 _-5 9 / 32 * (degC) 5 9 / @derived(degF)
 
 @base(mol)
+0 (mol) 1e3 @derived(kmol)
+0 (mol) 1e-3 @derived(mmol)
+0 (mol) 1e-6 @derived(umol)
+0 (mol) 1e-9 @derived(nmol)
+0 (mol) 1e-12 @derived(pmol)
 
 @base(l)
 0 (l) 1e-1 @derived(dl)