Cleanups and comments in src/eval

src/eval/env.rs

@@ -2,15 +2,28 @@ use crate::eval::Value;
 use internment::ArcIntern;
 use std::sync::Arc;
 
+/// An evaluation environment, which maps variable names to their
+/// current values.
+/// 
+/// One key difference between `EvalEnvironment` and `HashMap` is that
+/// `EvalEnvironment` uses an `extend` mechanism to add keys, rather
+/// than an `insert`. This difference allows you to add mappings for
+/// a subcomputation while still retaining the old version without those
+/// keys, which is really handy for implementing variable scoping.
 pub struct EvalEnvironment {
     inner: Arc<EvalEnvInternal>,
 }
 
-pub enum EvalEnvInternal {
+enum EvalEnvInternal {
     Empty,
     Value(ArcIntern<String>, Value, Arc<EvalEnvInternal>),
 }
 
+/// Errors that can happen when looking up a variable.
+/// 
+/// This enumeration may be extended in the future, depending on if we
+/// get more subtle with our keys. But for now, this is just a handy
+/// way to make lookup failures be `thiserror::Error`s.
 #[derive(Clone, Debug, PartialEq, thiserror::Error)]
 pub enum LookupError {
     #[error("Could not find variable '{0}' in environment")]
@@ -24,28 +37,38 @@ impl Default for EvalEnvironment {
 }
 
 impl EvalEnvironment {
+    /// Create a new, empty environment.
     pub fn empty() -> Self {
         EvalEnvironment {
             inner: Arc::new(EvalEnvInternal::Empty),
         }
     }
 
+    /// Extend the environment with a new mapping.
+    /// 
+    /// Note the types: the result of this method is a new `EvalEnvironment`,
+    /// with its own lifetime, and the original environment is left unmodified.
     pub fn extend(&self, name: ArcIntern<String>, value: Value) -> Self {
         EvalEnvironment {
             inner: Arc::new(EvalEnvInternal::Value(name, value, self.inner.clone())),
         }
     }
 
+    /// Look up a variable in the environment, returning an error if it isn't there.
     pub fn lookup(&self, n: ArcIntern<String>) -> Result<Value, LookupError> {
         self.inner.lookup(n)
     }
 }
 
 impl EvalEnvInternal {
+    /// Look up a variable in the environment, returning an error if it isn't there.
     fn lookup(&self, n: ArcIntern<String>) -> Result<Value, LookupError> {
         match self {
+            // if this is an empty dictionary, never mind, couldn't find it
             EvalEnvInternal::Empty => Err(LookupError::CouldNotFind(n)),
+            // is this the key we have right here? if yes, return our value
             EvalEnvInternal::Value(name, value, _) if *name == n => Ok(value.clone()),
+            // otherwise, recurse up our chain of environments
             EvalEnvInternal::Value(_, _, rest) => rest.lookup(n),
         }
     }
@@ -70,6 +93,9 @@ mod tests {
         assert!(tester.lookup(arced("baz")).is_err());
     }
 
+    // added this test to make sure that our nesting property works propertly.
+    // it's not a big deal now, but it'll be really handy later when we add any
+    // kind of variable scoping.
     #[test]
     fn nested() {
         let tester = EvalEnvironment::default();

src/eval/primop.rs

@@ -1,5 +1,6 @@
 use crate::eval::value::Value;
 
+/// Errors that can occur running primitive operations in the evaluators.
 #[derive(Clone, Debug, PartialEq, thiserror::Error)]
 pub enum PrimOpError {
     #[error("Math error (underflow or overflow) computing {0} operator")]
@@ -14,6 +15,16 @@ pub enum PrimOpError {
     UnknownPrimOp(String),
 }
 
+// Implementing primitives in an interpreter like this is *super* tedious,
+// and the only way to make it even somewhat manageable is to use macros.
+// This particular macro works for binary operations, and assumes that
+// you've already worked out that the `calculate` call provided two arguments.
+//
+// In those cases, it will rul the operations we know about, and error if
+// it doesn't.
+//
+// This macro then needs to be instantiated for every type, which is super
+// fun.
 macro_rules! run_op {
     ($op: ident, $left: expr, $right: expr) => {
         match $op {
@@ -23,15 +34,15 @@ macro_rules! run_op {
                 .map(Into::into),
             "-" => $left
                 .checked_sub($right)
-                .ok_or(PrimOpError::MathFailure("+"))
+                .ok_or(PrimOpError::MathFailure("-"))
                 .map(Into::into),
             "*" => $left
                 .checked_mul($right)
-                .ok_or(PrimOpError::MathFailure("+"))
+                .ok_or(PrimOpError::MathFailure("*"))
                 .map(Into::into),
             "/" => $left
                 .checked_div($right)
-                .ok_or(PrimOpError::MathFailure("+"))
+                .ok_or(PrimOpError::MathFailure("/"))
                 .map(Into::into),
             _ => Err(PrimOpError::UnknownPrimOp($op.to_string())),
         }
@@ -41,6 +52,8 @@ macro_rules! run_op {
 impl Value {
     fn binary_op(operation: &str, left: &Value, right: &Value) -> Result<Value, PrimOpError> {
         match left {
+            // for now we only have one type, but in the future this is
+            // going to be very irritating.
             Value::I64(x) => match right {
                 Value::I64(y) => run_op!(operation, x, *y),
                 //                _ => Err(PrimOpError::TypeMismatch(
@@ -52,6 +65,10 @@ impl Value {
         }
     }
 
+    /// Calculate the result of running the given primitive on the given arguments.
+    /// 
+    /// This can cause errors in a whole mess of ways, so be careful about your
+    /// inputs.
     pub fn calculate(operation: &str, values: Vec<Value>) -> Result<Value, PrimOpError> {
         if values.len() == 2 {
             Value::binary_op(operation, &values[0], &values[1])

src/eval/value.rs

@@ -1,5 +1,10 @@
 use std::fmt::Display;
 
+/// Values in the interpreter.
+/// 
+/// Yes, this is yet another definition of a structure called `Value`, which
+/// are almost entirely identical. However, it's nice to have them separated
+/// by type so that we don't mix them up.
 #[derive(Clone, Debug, PartialEq)]
 pub enum Value {
     I64(i64),