Refactoring, block scoping

forge_core/src/compiler.rs

@@ -6,7 +6,7 @@ use std::fmt::{Display, Formatter};
 use crate::forge_parser::{ParseError, parse};
 
 #[derive(Eq, Clone, PartialEq, Debug)]
-pub struct CompileError(usize, usize, String);
+pub struct CompileError(pub usize, pub usize, pub String);
 
 impl From<ParseError> for CompileError {
     fn from(value: ParseError) -> Self {
@@ -41,8 +41,7 @@ pub enum Variable {
 ///
 /// A complete function implementation consists of:
 /// - A label for the entrypoint
-/// - Preamble code to set up the stack frame
+/// - The function body (including preamble code to set up the stack frame)
-/// - The function body
 ///
 /// The stack frame is managed by the function through a global pointer called "frame". When
 /// the function is called, it can assume that all memory after "frame" is free for use (this
@@ -143,6 +142,32 @@ impl CompiledFn {
 
         Ok(())
     }
+
+    /// Takes a new frame size and removes all names from the local scope that would be placed
+    /// beyond that frame length: used when compiling blocks to de-scope names that should only
+    /// be visible in the block
+    fn reduce_frame_size_to(&mut self, new_size: usize) {
+        self.frame_size = new_size;
+        let mut to_remove: Vec<String> = Vec::new();
+        for (name, var) in self.local_scope.iter() {
+            if let Variable::Local(offset) = var {
+                if *offset >= self.frame_size {
+                    to_remove.push(name.clone());
+                }
+            }
+        }
+
+        for name in to_remove {
+            self.local_scope.remove(name.as_str());
+        }
+    }
+
+    /// Reduces the frame size by one word, to throw out the most recently declared local.
+    /// Used for things like repeat loops where there's a name declared outside a block but which
+    /// should only be visible in the block anyway.
+    fn forget_last_local(&mut self) {
+        self.reduce_frame_size_to(self.frame_size - 3);
+    }
 }
 
 /// Maps from names to the variables they represent
@@ -269,6 +294,8 @@ impl Compilable for Block {
     fn process(self, state: &mut State, sig: Option<&mut CompiledFn>, _loc: Location) -> Result<(), CompileError> {
         let sig = sig.expect("Block outside function");
 
+        let frame_size_before_block = sig.frame_size;
+
         // Compile each statement:
         for stmt in self.0 {
             let loc = stmt.location;
@@ -316,38 +343,71 @@ impl Compilable for Block {
                     body.process(state, Some(sig), loc)?;
                     sig.emit("#end")
                 }
-                Statement::RepeatLoop(RepeatLoop{ count, name: Some(name), body }) => {
+                Statement::RepeatLoop(repeat_loop) => {
-                    // A repeat loop with a name essentially starts with a vardecl, so, we'll make
+                    repeat_loop.process(state, Some(sig), loc)?;
-                    // a decl and process it (and deal with the error if it's a duplicate name)
+                }
-                    // TODO: This makes some assumptions about names and scoping, and opens a can of
+            }
-                    // worms. I think what we'd really like to do here is implement block scoping for real,
+        }
-                    // which actually shouldn't be too hard: after a block, reset the scope to whatever
+
-                    // it was before the block, and make the repeat loop counter go in its block.
+        // If we declared anything inside the block, we'll know that because the frame size will
+        // have increased, so, simply blow away those names to make them block scoped. This doesn't
+        // give us shadowing, it's not a true scope chain, but this is much simpler and fixes some
+        // common cases where you want that.
+        sig.reduce_frame_size_to(frame_size_before_block);
+
+        Ok(())
+    }
+}
+
+///////////////////////////////////////////////////////////
+
+impl Compilable for RepeatLoop {
+    fn process(self, state: &mut State, sig: Option<&mut CompiledFn>, loc: Location) -> Result<(), CompileError> {
+        let sig = sig.expect("Repeat loop outside function");
+
+        if let RepeatLoop{ count, name, body } = self {
+            let named_counter = name.is_some();
+            let mut counter_name = String::new();
+            if let Some(name) = name {
+                counter_name = name;
+            }
+
+            if named_counter {
                 let decl = VarDecl{
-                        name: name.clone(),
+                    name: counter_name.clone(),
                     size: None,
                     initial: Some(Expr::Number(0)),
                 };
                 decl.process(state, Some(sig), loc)?;
+            }
 
-                    // Okay, the counter is now declared and in scope; we'll eval the limit once
+            // Okay, the counter (if present) is now declared and in scope; we'll eval the limit once
             count.process(state, Some(sig), loc)?;
 
             // Now we have a fairly normal while loop:
             sig.emit("#while");
             // Stack currently has the limit on top. We need to cmp the counter to that.
             sig.emit("dup");
-                    // Load the counter
+
-                    Expr::Name(name.clone()).process(state, Some(sig), loc)?;
+            // Load the counter, if present:
+            if named_counter {
+                Expr::Name(counter_name.clone()).process(state, Some(sig), loc)?;
                 // Subtract the counter from (the copy of) the limit.
                 sig.emit("sub");
-                    // cmp that to zero, for our flag
+            }
+
+            // cmp that to zero, for our flag: this is either limit - ctr or limit, depending if
+            // there's a counter
             sig.emit_arg("agt", 0);
+
             // Loop body:
             sig.emit("#do");
             body.process(state, Some(sig), loc)?;
-                    // After the body we need to increment the counter. Put its address on top:
+
-                    Expr::Address(Expr::Name(name.clone()).into()).process(state, Some(sig), loc)?;
+            // After the body we need to increment the counter if there is one
+            if named_counter {
+                // Put its address on top:
+                Expr::Address(Expr::Name(counter_name.clone()).into()).process(state, Some(sig), loc)?;
                 // Dup and load it:
                 sig.emit("dup");
                 sig.emit("loadw");
@@ -356,29 +416,20 @@ impl Compilable for Block {
                 // Now we have ( counted-addr new-ctr-val ) so swap and store
                 sig.emit("swap");
                 sig.emit("storew");
+            } else {
+                // No counter, so just decrement the limit, which is already on top:
+                sig.emit_arg("sub", 1);
+            }
+
             // Back to the check!
             sig.emit("#end");
             // Done with the loop, but the limit is still on the stack, pop it:
             sig.emit("pop");
-                }
+
-                Statement::RepeatLoop(RepeatLoop{ count, name: _, body }) => {
+            // If we have added a counter variable, we need to forget that because (even
-                    // No name, so, what we'll do is, eval the count:
+            // though declared outside the block) it should be scoped to the block:
-                    count.process(state, Some(sig), loc)?;
+            if named_counter {
-                    // Now the count is on top of the stack, so we'll do a #while loop counting it
+                sig.forget_last_local();
-                    // down to zero:
-                    sig.emit("#while");
-                    sig.emit("dup");
-                    // Only go through the loop if the counter is positive:
-                    sig.emit_arg("agt", 0);
-                    sig.emit("#do");
-                    body.process(state, Some(sig), loc)?;
-                    // After the body we need to decrement the counter:
-                    sig.emit_arg("sub", 1);
-                    // Back to the check!
-                    sig.emit("#end");
-                    // Loop is over so we're left with the dead counter on the top, drop it:
-                    sig.emit("pop");
-                }
             }
         }
 
@@ -1357,4 +1408,41 @@ mod test {
             ".db 0"
         ].join("\n"))
     }
+
+    #[test]
+    fn test_block_scoping() {
+        assert_eq!(
+            test_body(state_for("fn test() { repeat(5) c { 2; } var k = 5; return k; }")),
+            vec![
+                "push 0", // Create the 'c' var and store 0 in it
+                "loadw frame",
+                "storew",
+                "push 5",
+                "#while", // Starting the loop:
+                "dup", // Copy the limit
+                "loadw frame", // Load c
+                "loadw",
+                "sub", // Subtract c from a
+                "agt 0", // Are we still positive?
+                "#do",
+                "push 2", // Pointless loop body
+                "pop",
+                "loadw frame", // Load c as an lvalue
+                "dup", // Dup it, load it, add 1
+                "loadw",
+                "add 1",
+                "swap", // Swap the addr on top and store it
+                "storew",
+                "#end", // End of the loop body!
+                "pop", // Drop the limit off the top
+                "push 5", // Push the rvalue we'll put in 'k'
+                "loadw frame", // THIS IS THE TEST: 'k' should go at frame + 0, because it's
+                "storew", // taking the same (now freed) frame slot that c took, because c is
+                "loadw frame", // now out of scope
+                "loadw",
+                "ret"
+            ]
+                .join("\n")
+        );
+    }
 }

vtest/src/forge_tests.rs

@@ -16,6 +16,13 @@ fn main_return(src: &str) -> i32 {
     cpu.pop_data().into()
 }
 
+fn error_message(src: &str) -> Option<String> {
+    match forge_core::compiler::build_boot(src) {
+        Ok(_) => None,
+        Err(forge_core::compiler::CompileError(_, _, s)) => Some(s)
+    }
+}
+
 #[test]
 fn basic_forge_test() {
     assert_eq!(
@@ -51,3 +58,26 @@ fn arg_test() {
         2
     )
 }
+
+#[test]
+fn scope_test() {
+    // Refer to a var that's now out of scope, to ensure block scoping works
+    assert_eq!(
+        error_message(
+            "fn main() {
+               if (1) { var a = 3; } // scoped to the block!
+               return a; // should error!
+             }"),
+        Some("Unknown name a".into())
+    );
+
+    // Refer to a var that's still in scope, to ensure we're not wiping too much out
+    assert_eq!(
+        error_message(
+            "fn main(a) {
+               if (1) { var b = 3; } // scoped to the block!
+               return a; // should work, we didn't remove the thing that dropped
+             }"),
+        None
+    )
+}