Interpret while

[helpmehelpus]

This PR does the following

• Applies standard formatting to the code. rustfmt is a standard for formatting rust code, and can be integrated into IDE's on save. cargo fmt applies it to the codebase for us
• Gets the While arm of the match stmt in execute to compile. To achieve this, a few changes to the signatures of execute and eval are necessary. This also makes the code looks more like what Rust expect from us. Please see the explanation below

Why the current code does not work

In Rust, when we call a function and the pass the values directly, we give ownership of the value to the inner scope.
This means that if we have something like

let val = 123;
do_something(val);

println!(val); // error

ownership of val will be moved to do_something. Moreover, values are automatically dropped when they go out of scope.
Think of it as the compiler automatically inserting a call to drop at the end of the scope. Visually, it looks like:

{
    let outer_var = 123;
    {
        // outer_var is visible here
        let inner_var = 456;
        // inner_var gets dropped here
    }
    // inner_var is gone, outer_var still valid
    ...
    // outer_var gets dropped here
}

Because of this, we will get a few compile errors in our current code for Statement::While.

Statement::While(cond, stmt) => {
    let mut value = eval(*cond, &env)?; // cond is moved here, so it gets dropped when eval() returns
    let mut new_env = env.clone();
    while value > 0 {
        new_env = execute(*stmt, &mut new_env)?.clone(); // stmt is moved/dropped in the first iteration,
                                                         // thus becoming unavailable
        value = eval(*cond, &env)?; // "use after free" error, as cond will have already been dropped
        // new_env gets dropped here
    }
    // new_env here cannot refer to new_env within the while loop, as that will get dropped when it goes out
    // of scope Also, we are borrowing new_env inside this very function when we make a call to env.clone(). 
    // Rust will not allow us to send this value to outer scopes, our new_env goes out of scope when we we 
    // return from this arm
    Ok(&new_env)
}

To work around this, we leverage the concept of immutable references. As their name suggests, they are references that
can be passed around but don't allow mutation. A very brief summary of the rules of Rust's borrow checker is:

• Values that are owned get consumed when passed around and are dropped when they go out of scope
• We can have multiple immutable references at any point in time
• We can have at most one mutable reference at any point in time

Since we want to be able to call eval multiple times with a Box<Expression> that we do not plan to mutate, we change the
signature of eval to take an immutable reference to Expression:

pub fn eval(exp: &Expression, env: &Environment) -> Result<i32, String>

This makes it so we do not need to dereference every single expression throughout the code, and instead only the i32
that CInt wraps.

Since we want to call execute on a statement an arbitrary number of times, we can also pass an immutable reference to
stmt instead:

pub fn execute(stmt: &Statement, env: &mut Environment) -> Result<&Environment, String>

Upon adjusting the test code to pass references to the calls, if we run cargo check now, we get a lifetime error:

error[E0106]: missing lifetime specifier
  --> src/interpreter/interpreter.rs:22:67
   |
22 | pub fn execute(stmt: &Statement, env: &mut Environment) -> Result<&Environment, String> {
   |                      ----------       ----------------            ^ expected named lifetime parameter
   |
   = help: this function's return type contains a borrowed value, but the signature does not say whether it is borrowed from `stmt` or `env`
help: consider introducing a named lifetime parameter

Lifetimes are my least understood aspect of Rust so far, but Rust's compiler errors are very informative. What this is
telling us is that we need to:

• Tell Rust to bind the lifetime of the output to the lifetime of at least one of the inputs (env in our case)
• Make it clear that the output reference will be valid. In other words, we need to tell Rust that our result will live
  at least as long as the input reference.

We do this by changing the signature of execute to the following

pub fn execute<'a>(stmt: &Statement, env: &'a mut Environment) -> Result<&'a Environment, String>

With this, and a few fixes to env.get() and env.insert(), we are almost done. cargo check now throws one final
error:

error[E0515]: cannot return value referencing local variable `new_env`
  --> src/interpreter/interpreter.rs:44:13
   |
44 |             Ok(&new_env)
   |             ^^^--------^
   |             |  |
   |             |  `new_env` is borrowed here
   |             returns a value referencing data owned by the currenthttps://doc.rust-lang.org/book/ function

For more information about this error, try `rustc --explain E0515`.

Since env is already a mutable reference, we don't really need to clone it, and can mutate it in place, like we are
already doing in the other arms of the match.

Statement::While(cond, stmt) => {
    let mut value = eval(cond, env)?;
    while value > 0 {
        execute(stmt, env)?;
        value = eval(cond, env)?;
    }
    Ok(env)
}

Note that, since env is itself a reference, we are not required to do &env in intenral calls to eval or execute.
The same applies to cond; now that we have made them all references, we can pass them around without worrying about
ownership. We still do that in the tests because we need to pass references to the functions we call. But inside them,
we don't need * and & all over, and it now looks a bit cleaner.

Now cargo check passes, and so does cargo test. We obviously need to add tests for our while statements, but at least
we have gotten the ownership.

Summary

Passing ownership around can cause us to not be able to use values again, as they are consumed and become unavailable.
This statically prevents a lot of memory safety errors such as use after free. Having arbitrarily as many immutable references
at our disposal is also nice. Since Rust knows the objects represented by immutable references cannot be modified, "all"
we need to do is to ensure that our references will live long enough, and hence why Rust gives us control over lifetimes.

Reference

Rust book https://doc.rust-lang.org/book/

[rbonifacio]

Hi @helpmehelpus, thanks for the detailed pull request!

Question: In this line, should we make it more straightforward that we accumulate changes in the environment during each iteration of the while loop? Would something like env = execute(stmt, env) improve readability and make the code more "functional"? Otherwise, it currently feels as though env might be intended as a global variable.

[helpmehelpus]

@rbonifacio yes, we can. It looks like we two main options here.

If we just do

while value > 0 {
    env = execute(stmt, env)?;
    value = eval(cond, env)?;
}

we get two errors, both related to the current signature of execute:

pub fn execute<'a>(stmt: &Statement, env: &'a mut Environment) -> Result<&'a Environment, String> {

The first error is that we cannot assign multiple times to the immutable variable env. Note that, even though it holds a mutable reference, the parameter itself is not annotated with mut, and is therefore immutable.

The second error is that we are returning an immutable reference from the function, so there is a type mismatch between the input env and the output.

Option 1 (least preferred imo)

We can do

Statement::While(cond, stmt) => {
    let mut value = eval(cond, env)?;
    while value > 0 {
        let env = execute(stmt, env)?; // could also be let _ = execute(stmt, env)?; still smells bad
        value = eval(cond, env)?;
    }
    Ok(env)
}

Here we are basically leveraging Rust's shadowing to define an env variable that is scoped to the while block. We assume that we are mutating the outer env on each call to execute inside the while block and storing its result in a new variable to compute the result of eval.
Keep in mind that the inner let env goes out of scope at the end of the while block, and that Ok(env) will return the mutated environment parameter. This does not look good to me, as we are essentially using two different env objects in execute(stmt, env)? and in eval(cond, env)?. Just to explain this is confusing, let alone keeping this in mind as the code gets more complicated.

Option 2 (probably best)

If I understand what you are asking, we want to use the same env parameter throughout the entire function. So we can change the signature of execute to:

pub fn execute<'a>(stmt: &Statement, mut env: &'a mut Environment) -> Result<&'a mut Environment, String> {

and now we can do what you asked:

while value > 0 {
    env = execute(stmt, env)?;
    value = eval(cond, env)?;
}

There is now a single env, the mutation is explicitly and there is less confusion.

Please let me know if this is the desired outcome, and I will commit the changes.