Change Unit#new_base to receive CanonicalName

Before this, it accepted a string and a AcceptsPrefix that were used to
construct a CanonicalName internally.

This also applies fixes suggested on the PR review [1], such as
AcceptsPrefix inconsistencies on tests, code duplication on
get_canonical_unit_name.

[1]:#309

numbat/src/bytecode_interpreter.rs

@@ -10,7 +10,7 @@ use crate::prefix::Prefix;
 use crate::prefix_parser::AcceptsPrefix;
 use crate::pretty_print::PrettyPrint;
 use crate::typed_ast::{BinaryOperator, Expression, Statement, StringPart, UnaryOperator};
-use crate::unit::Unit;
+use crate::unit::{CanonicalName, Unit};
 use crate::unit_registry::{UnitMetadata, UnitRegistry};
 use crate::vm::{Constant, ExecutionContext, Op, Vm};
 use crate::{decorator, ffi};
@@ -301,8 +301,7 @@ impl BytecodeInterpreter {
 
                 let constant_idx = self.vm.add_constant(Constant::Unit(Unit::new_base(
                     unit_name,
-                    &crate::decorator::get_canonical_unit_name(unit_name.as_str(), &decorators[..]),
-                    crate::decorator::get_canonical_accepts_prefix(&decorators[..]),
+                    crate::decorator::get_canonical_unit_name(unit_name.as_str(), &decorators[..]),
                 )));
                 for (name, _) in decorator::name_and_aliases(unit_name, decorators) {
                     self.unit_name_to_constant_index
@@ -316,15 +315,20 @@ impl BytecodeInterpreter {
 
                 let constant_idx = self.vm.add_constant(Constant::Unit(Unit::new_base(
                     "<dummy>",
-                    "<dummy>",
-                    AcceptsPrefix::both(),
+                    CanonicalName {
+                        name: "<dummy>".to_string(),
+                        accepts_prefix: AcceptsPrefix::both(),
+                    },
                 ))); // TODO: dummy is just a temp. value until the SetUnitConstant op runs
                 let unit_information_idx = self.vm.add_unit_information(
                     unit_name,
-                    Some(&crate::decorator::get_canonical_unit_name(
-                        unit_name.as_str(),
-                        &decorators[..],
-                    )),
+                    Some(
+                        &crate::decorator::get_canonical_unit_name(
+                            unit_name.as_str(),
+                            &decorators[..],
+                        )
+                        .name,
+                    ),
                     UnitMetadata {
                         type_: type_.clone(),
                         readable_type: readable_type.clone(),

numbat/src/decorator.rs

@@ -1,4 +1,4 @@
-use crate::prefix_parser::AcceptsPrefix;
+use crate::{prefix_parser::AcceptsPrefix, unit::CanonicalName};
 
 #[derive(Debug, Clone, PartialEq, Eq)]
 pub enum Decorator {
@@ -36,30 +36,28 @@ pub fn name_and_aliases<'a>(
     }
 }
 
-pub fn get_canonical_unit_name(unit_name: &str, decorators: &[Decorator]) -> String {
+pub fn get_canonical_unit_name(unit_name: &str, decorators: &[Decorator]) -> CanonicalName {
     for decorator in decorators {
         if let Decorator::Aliases(aliases) = decorator {
             for (alias, accepts_prefix) in aliases {
                 if accepts_prefix.map(|ap| ap.short).unwrap_or(false) {
-                    return alias.into();
+                    let name = alias.into();
+                    let apr = match accepts_prefix {
+                        &Some(ap) if ap.short => ap,
+                        _ => AcceptsPrefix::only_long(),
+                    };
+                    return CanonicalName {
+                        name,
+                        accepts_prefix: apr,
+                    };
                 }
             }
         }
     }
-    unit_name.into()
-}
-
-pub fn get_canonical_accepts_prefix(decorators: &[Decorator]) -> AcceptsPrefix {
-    for decorator in decorators {
-        if let Decorator::Aliases(aliases) = decorator {
-            for (_alias, accepts_prefix) in aliases {
-                if accepts_prefix.map(|ap| ap.short).unwrap_or(false) {
-                    return accepts_prefix.unwrap();
-                }
-            }
-        }
+    CanonicalName {
+        name: unit_name.into(),
+        accepts_prefix: AcceptsPrefix::only_long(),
     }
-    AcceptsPrefix::only_long()
 }
 
 pub fn name(decorators: &[Decorator]) -> Option<String> {

numbat/src/interpreter.rs

@@ -128,7 +128,7 @@ pub trait Interpreter {
 #[cfg(test)]
 mod tests {
     use crate::prefix_parser::AcceptsPrefix;
-    use crate::unit::Unit;
+    use crate::unit::{CanonicalName, Unit};
     use crate::{bytecode_interpreter::BytecodeInterpreter, prefix_transformer::Transformer};
 
     use super::*;
@@ -229,11 +229,19 @@ mod tests {
         assert_runtime_error(
             "1 meter > alternative_length_base_unit",
             RuntimeError::QuantityError(QuantityError::IncompatibleUnits(
-                Unit::new_base("meter", "m", AcceptsPrefix::only_short()),
+                Unit::new_base(
+                    "meter",
+                    CanonicalName {
+                        name: "m".to_string(),
+                        accepts_prefix: AcceptsPrefix::only_short(),
+                    },
+                ),
                 Unit::new_base(
                     "alternative_length_base_unit",
-                    "alternative_length_base_unit",
-                    AcceptsPrefix::only_long(),
+                    CanonicalName {
+                        name: "alternative_length_base_unit".to_string(),
+                        accepts_prefix: AcceptsPrefix::only_long(),
+                    },
                 ),
             )),
         );
@@ -254,7 +262,13 @@ mod tests {
              unit pixel : Pixel
              2 * pixel",
             Quantity::from_scalar(2.0)
-                * Quantity::from_unit(Unit::new_base("pixel", "px", AcceptsPrefix::only_short())),
+                * Quantity::from_unit(Unit::new_base(
+                    "pixel",
+                    CanonicalName {
+                        name: "px".to_string(),
+                        accepts_prefix: AcceptsPrefix::only_short(),
+                    },
+                )),
         );
 
         assert_evaluates_to(

numbat/src/unit.rs

@@ -23,8 +23,8 @@ pub enum UnitKind {
 
 #[derive(Debug, Clone, PartialEq, Eq)]
 pub struct CanonicalName {
-    name: String,
-    accepts_prefix: AcceptsPrefix,
+    pub name: String,
+    pub accepts_prefix: AcceptsPrefix,
 }
 
 #[derive(Debug, Clone, PartialEq, Eq)]
@@ -52,11 +52,7 @@ impl UnitIdentifier {
     pub fn unit_and_factor(&self) -> BaseUnitAndFactor {
         match &self.kind {
             UnitKind::Base => BaseUnitAndFactor(
-                Unit::new_base(
-                    &self.name,
-                    &self.canonical_name.name,
-                    self.canonical_name.accepts_prefix,
-                ),
+                Unit::new_base(&self.name, self.canonical_name.clone()),
                 Number::from_f64(1.0),
             ),
             UnitKind::Derived(factor, defining_unit) => {
@@ -68,11 +64,7 @@ impl UnitIdentifier {
     pub fn base_unit_and_factor(&self) -> BaseUnitAndFactor {
         match &self.kind {
             UnitKind::Base => BaseUnitAndFactor(
-                Unit::new_base(
-                    &self.name,
-                    &self.canonical_name.name,
-                    self.canonical_name.accepts_prefix,
-                ),
+                Unit::new_base(&self.name, self.canonical_name.clone()),
                 Number::from_f64(1.0),
             ),
             UnitKind::Derived(factor, defining_unit) => {
@@ -230,15 +222,12 @@ impl Unit {
         self == &Self::scalar()
     }
 
-    pub fn new_base(name: &str, canonical_name: &str, accepts_prefix: AcceptsPrefix) -> Self {
+    pub fn new_base(name: &str, canonical_name: CanonicalName) -> Self {
         Unit::from_factor(UnitFactor {
             prefix: Prefix::none(),
             unit_id: UnitIdentifier {
                 name: name.into(),
-                canonical_name: CanonicalName {
-                    name: canonical_name.into(),
-                    accepts_prefix,
-                },
+                canonical_name,
                 kind: UnitKind::Base,
             },
             exponent: Rational::from_integer(1),
@@ -308,32 +297,71 @@ impl Unit {
 
     #[cfg(test)]
     pub fn meter() -> Self {
-        Self::new_base("meter", "m", AcceptsPrefix::only_short())
+        Self::new_base(
+            "meter",
+            CanonicalName {
+                name: "m".to_string(),
+                accepts_prefix: AcceptsPrefix::only_short(),
+            },
+        )
     }
 
     #[cfg(test)]
     pub fn centimeter() -> Self {
-        Self::new_base("meter", "m", AcceptsPrefix::only_short()).with_prefix(Prefix::centi())
+        Self::new_base(
+            "meter",
+            CanonicalName {
+                name: "m".to_string(),
+                accepts_prefix: AcceptsPrefix::only_short(),
+            },
+        )
+        .with_prefix(Prefix::centi())
     }
 
     #[cfg(test)]
     pub fn millimeter() -> Self {
-        Self::new_base("meter", "m", AcceptsPrefix::only_short()).with_prefix(Prefix::milli())
+        Self::new_base(
+            "meter",
+            CanonicalName {
+                name: "m".to_string(),
+                accepts_prefix: AcceptsPrefix::only_short(),
+            },
+        )
+        .with_prefix(Prefix::milli())
     }
 
     #[cfg(test)]
     pub fn kilometer() -> Self {
-        Self::new_base("meter", "m", AcceptsPrefix::only_short()).with_prefix(Prefix::kilo())
+        Self::new_base(
+            "meter",
+            CanonicalName {
+                name: "m".to_string(),
+                accepts_prefix: AcceptsPrefix::only_short(),
+            },
+        )
+        .with_prefix(Prefix::kilo())
     }
 
     #[cfg(test)]
     pub fn second() -> Self {
-        Self::new_base("second", "s", AcceptsPrefix::only_short())
+        Self::new_base(
+            "second",
+            CanonicalName {
+                name: "s".to_string(),
+                accepts_prefix: AcceptsPrefix::only_short(),
+            },
+        )
     }
 
     #[cfg(test)]
     pub fn gram() -> Self {
-        Self::new_base("gram", "g", AcceptsPrefix::both())
+        Self::new_base(
+            "gram",
+            CanonicalName {
+                name: "g".to_string(),
+                accepts_prefix: AcceptsPrefix::only_short(),
+            },
+        )
     }
 
     #[cfg(test)]
@@ -343,7 +371,13 @@ impl Unit {
 
     #[cfg(test)]
     pub fn kelvin() -> Self {
-        Self::new_base("kelvin", "K", AcceptsPrefix::none())
+        Self::new_base(
+            "kelvin",
+            CanonicalName {
+                name: "K".to_string(),
+                accepts_prefix: AcceptsPrefix::only_short(),
+            },
+        )
     }
 
     #[cfg(test)]
@@ -443,7 +477,13 @@ impl Unit {
 
     #[cfg(test)]
     pub fn bit() -> Self {
-        Self::new_base("bit", "B", AcceptsPrefix::none())
+        Self::new_base(
+            "bit",
+            CanonicalName {
+                name: "bit".to_string(),
+                accepts_prefix: AcceptsPrefix::only_long(),
+            },
+        )
     }
 
     #[cfg(test)]