From 3f961e3c55e07614e2f9ace58eed2faa0b71b43c Mon Sep 17 00:00:00 2001 From: Yago Iglesias Date: Fri, 11 Aug 2023 00:58:23 +0200 Subject: [PATCH 1/4] Add falling tests to diagnose the bug --- crates/vm/src/vm_tests.rs | 196 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 196 insertions(+) diff --git a/crates/vm/src/vm_tests.rs b/crates/vm/src/vm_tests.rs index 105ae7a..b38d526 100644 --- a/crates/vm/src/vm_tests.rs +++ b/crates/vm/src/vm_tests.rs @@ -940,6 +940,202 @@ mod tests { expected: Object::INTEGER(610), }]; + run_vm_tests(tests); + } + #[test] + fn test_use_same_varaible_multiple_times() { + let tests = vec![ + VmTestCase { + input: r#" + let a = 1; + let b = a; + let c = a + b + 1; + c"# + .to_string(), + expected: Object::INTEGER(3), + }, + VmTestCase { + input: r#" + let a = 1; + let b = a; + let c = a + b + 1; + let a = 2; + let b = a; + let c = a + b + 1; + c"# + .to_string(), + expected: Object::INTEGER(5), + }, + VmTestCase { + input: r#" + let a = "hello"; + let b = "world"; + let c = a + b; + let d = a + b + c; + d"# + .to_string(), + expected: Object::STRING("helloworldhelloworld".to_string()), + }, + ]; + + run_vm_tests(tests); + } + + #[test] + fn test_multiple_use_same_variable_same_line_() { + let tests = vec![ + VmTestCase { + input: r#" + let a = 1; + let b = a * a + 2 + b"# + .to_string(), + expected: Object::INTEGER(3), + }, + VmTestCase { + input: r#" + let a = 1; + let a = a + 1; + a"# + .to_string(), + expected: Object::INTEGER(2), + }, + VmTestCase { + input: r#" + let a = 1; + let b = a + 1; + let a = a + b; + a"# + .to_string(), + expected: Object::INTEGER(3), + }, + VmTestCase { + input: r#" + let a = "hello"; + let a = a + "world"; + let a = a + "world"; + a"# + .to_string(), + expected: Object::STRING("helloworldworld".to_string()), + }, + ]; + + run_vm_tests(tests); + } + + #[test] + fn test_array_multiple_ussage() { + let tests = vec![ + VmTestCase { + input: r#" + let array = [1,2,3]; + let value = array[1] + array[2]; + value"# + .to_string(), + expected: Object::INTEGER(5), + }, + VmTestCase { + input: r#" + let array = [1,2,3]; + let array = push(array, 4); + array"# + .to_string(), + expected: Object::ARRAY(vec![ + Object::INTEGER(1), + Object::INTEGER(2), + Object::INTEGER(3), + Object::INTEGER(4), + ]), + }, + ]; + run_vm_tests(tests); + } + + #[test] + fn test_shadowing_same_type() { + let tests = vec![ + VmTestCase { + input: r#" + let x = 4; + let x = 5; + x"# + .to_string(), + expected: Object::INTEGER(5), + }, + VmTestCase { + input: r#" + let x = [1,2,3]; + let x = [4,5,6]; + x"# + .to_string(), + expected: Object::ARRAY(vec![ + Object::INTEGER(4), + Object::INTEGER(5), + Object::INTEGER(6), + ]), + }, + VmTestCase { + input: r#" + let x = fn() { 1 }; + let x = fn() { 2 }; + x()"# + .to_string(), + expected: Object::INTEGER(2), + }, + ]; + + run_vm_tests(tests); + } + + #[test] + fn test_shadowing_with_new_type() { + let tests = vec![ + VmTestCase { + input: r#" + let x = 4; + let x = "string"; + x"# + .to_string(), + expected: Object::STRING("string".to_string()), + }, + VmTestCase { + input: r#" + let x = "string"; + let x = fn() { 1 }; + x()"# + .to_string(), + expected: Object::INTEGER(1), + }, + VmTestCase { + input: r#" + let x = fn() { 1 }; + let x = 5, + x"# + .to_string(), + expected: Object::INTEGER(5), + }, + VmTestCase { + input: r#" + let x = 5; + let x = [1,2,3]; + x"# + .to_string(), + expected: Object::ARRAY(vec![ + Object::INTEGER(1), + Object::INTEGER(2), + Object::INTEGER(3), + ]), + }, + VmTestCase { + input: r#" + let x = [1,2,3]; + let x = 5; + x"# + .to_string(), + expected: Object::INTEGER(5), + }, + ]; + run_vm_tests(tests); } } From 03ea58ceb6b44423838a0579fd13b039ed849cb2 Mon Sep 17 00:00:00 2001 From: Yago Iglesias Date: Fri, 11 Aug 2023 11:21:29 +0200 Subject: [PATCH 2/4] Add more falling tests --- crates/compiler/src/compiler_tests.rs | 32 ++++++ crates/vm/src/vm_tests.rs | 159 +++++++++++++++++++------- 2 files changed, 149 insertions(+), 42 deletions(-) diff --git a/crates/compiler/src/compiler_tests.rs b/crates/compiler/src/compiler_tests.rs index 545384b..d5d8e61 100644 --- a/crates/compiler/src/compiler_tests.rs +++ b/crates/compiler/src/compiler_tests.rs @@ -317,6 +317,16 @@ pub mod tests { #[test] fn test_let_statements() { let tests = vec![ + CompilerTestCase { + input: r#" + let one = 1;"# + .to_string(), + expected_constants: vec![Object::INTEGER(1)], + expected_instructions: flatten_instructions(vec![ + Opcode::Constant.make(vec![0]), + Opcode::SetGlobal.make(vec![0]), + ]), + }, CompilerTestCase { input: r#" let one = 1; @@ -1151,4 +1161,26 @@ pub mod tests { run_compiler(tests); } + + #[test] + fn test_shadowing_with_itself() { + let tests = vec![CompilerTestCase { + input: r#" + let a = 1; + let a = a + 1;"# + .to_string(), + + expected_constants: vec![Object::INTEGER(1), Object::INTEGER(1)], + expected_instructions: flatten_instructions(vec![ + Opcode::Constant.make(vec![0]), + Opcode::SetGlobal.make(vec![0]), + Opcode::GetGlobal.make(vec![0]), + Opcode::Constant.make(vec![1]), + Opcode::Add.make(vec![]), + Opcode::SetGlobal.make(vec![0]), + ]), + }]; + + run_compiler(tests); + } } diff --git a/crates/vm/src/vm_tests.rs b/crates/vm/src/vm_tests.rs index b38d526..5bdce7b 100644 --- a/crates/vm/src/vm_tests.rs +++ b/crates/vm/src/vm_tests.rs @@ -981,48 +981,6 @@ mod tests { run_vm_tests(tests); } - #[test] - fn test_multiple_use_same_variable_same_line_() { - let tests = vec![ - VmTestCase { - input: r#" - let a = 1; - let b = a * a + 2 - b"# - .to_string(), - expected: Object::INTEGER(3), - }, - VmTestCase { - input: r#" - let a = 1; - let a = a + 1; - a"# - .to_string(), - expected: Object::INTEGER(2), - }, - VmTestCase { - input: r#" - let a = 1; - let b = a + 1; - let a = a + b; - a"# - .to_string(), - expected: Object::INTEGER(3), - }, - VmTestCase { - input: r#" - let a = "hello"; - let a = a + "world"; - let a = a + "world"; - a"# - .to_string(), - expected: Object::STRING("helloworldworld".to_string()), - }, - ]; - - run_vm_tests(tests); - } - #[test] fn test_array_multiple_ussage() { let tests = vec![ @@ -1138,4 +1096,121 @@ mod tests { run_vm_tests(tests); } + + #[test] + fn test_shadowing_using_previous_value() { + let tests = vec![ + VmTestCase { + input: r#" + let a = 1; + let b = a * a + 2 + b"# + .to_string(), + expected: Object::INTEGER(3), + }, + VmTestCase { + input: r#" + let a = 1; + let a = a + 1; + a"# + .to_string(), + expected: Object::INTEGER(2), + }, + VmTestCase { + input: r#" + let a = fn() { + let a = 1; + a + }; + a() + "# + .to_string(), + expected: Object::INTEGER(1), + }, + VmTestCase { + input: r#" + let f = fn(a){ + let a = 1; + a + }; + f(1) + "# + .to_string(), + expected: Object::INTEGER(1), + }, + VmTestCase { + input: r#" + let f = fn(){ + let a = 1; + let h = fn(){ + let a = 2; + a + }; + h() + a + }; + f() + "# + .to_string(), + expected: Object::INTEGER(3), + }, + // Addition of a global variable a with 10 as its value + VmTestCase { + input: r#" + let a = 10; + let a = fn() { + let a = 1; + a + }; + a() + "# + .to_string(), + expected: Object::INTEGER(1), + }, + VmTestCase { + input: r#" + let a = 10; + let f = fn(a){ + let a = 1; + a + }; + f(1) + a + "# + .to_string(), + expected: Object::INTEGER(11), + }, + VmTestCase { + input: r#" + let a = 10; + let f = fn(){ + let h = fn(){ + let a = 2; + a + }; + h() + }; + f() + a + "# + .to_string(), + expected: Object::INTEGER(12), + }, + VmTestCase { + input: r#" + let a = 10; + let f = fn(){ + let a = 1; + let h = fn(){ + let a = 2; + a + }; + h() + a + }; + f() + a + "# + .to_string(), + expected: Object::INTEGER(13), + }, + ]; + + run_vm_tests(tests); + } } From d99ec2094e9ed279974572fc2555e51191575750 Mon Sep 17 00:00:00 2001 From: Yago Iglesias Date: Fri, 11 Aug 2023 11:21:48 +0200 Subject: [PATCH 3/4] fix: fix shadowing with same variable We now make sure to see if a name has already been defined in the correct scope and if it is the case when do not define it again. --- crates/compiler/src/compiler.rs | 26 +++++++++++++++++++++++++- crates/compiler/src/symbol_table.rs | 4 ++++ 2 files changed, 29 insertions(+), 1 deletion(-) mode change 100755 => 100644 crates/compiler/src/compiler.rs diff --git a/crates/compiler/src/compiler.rs b/crates/compiler/src/compiler.rs old mode 100755 new mode 100644 index 514a93d..f69beca --- a/crates/compiler/src/compiler.rs +++ b/crates/compiler/src/compiler.rs @@ -102,7 +102,31 @@ impl Compiler { self.emit(Opcode::Pop, vec![]); } Statement::Let(s) => { - let symbol = self.symbol_table.define(s.name.value); + // This step is extremely important. If it is not done then when shadowing variables + // and using the previous value we get an error. Because we would have assigned + // a new index to the symbol and the GetGlobal instruction would get a NULL + // value instead of the previous value. (corresponds to issue #11) + let symbol = match self.symbol_table.resolve(&s.name.value) { + Some(symbol) => match symbol.scope { + SymbolScope::Global => { + // A Local variable should never replace a global one + if self.symbol_table.has_outer() { + // This means that the symbol will + // be local and not global + self.symbol_table.define(s.name.value) + } else { + symbol + } + } + SymbolScope::Local => symbol, + + // We only want to do in in the case of "normal" variable assignation. + // The special cases should not be touched, since the program should not + // have access to them, only the compiler/vm + _ => self.symbol_table.define(s.name.value), + }, + None => self.symbol_table.define(s.name.value), + }; self.compile_expression(s.value)?; diff --git a/crates/compiler/src/symbol_table.rs b/crates/compiler/src/symbol_table.rs index 53791af..cf12455 100644 --- a/crates/compiler/src/symbol_table.rs +++ b/crates/compiler/src/symbol_table.rs @@ -115,6 +115,10 @@ impl SymbolTable { self.store.insert(symbol.name.clone(), symbol.clone()); symbol } + + pub fn has_outer(&self) -> bool{ + self.outer.is_some() + } } #[cfg(test)] From fbbdff8a42aea87d5a8ef9f4aad5aa03a6debd2f Mon Sep 17 00:00:00 2001 From: Yago Iglesias Date: Fri, 11 Aug 2023 23:08:44 +0200 Subject: [PATCH 4/4] comment: update wrong comments --- crates/compiler/src/compiler.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/crates/compiler/src/compiler.rs b/crates/compiler/src/compiler.rs index f69beca..ae685f4 100644 --- a/crates/compiler/src/compiler.rs +++ b/crates/compiler/src/compiler.rs @@ -105,14 +105,15 @@ impl Compiler { // This step is extremely important. If it is not done then when shadowing variables // and using the previous value we get an error. Because we would have assigned // a new index to the symbol and the GetGlobal instruction would get a NULL - // value instead of the previous value. (corresponds to issue #11) + // value instead of the previous value. (corresponds to issue #8) let symbol = match self.symbol_table.resolve(&s.name.value) { Some(symbol) => match symbol.scope { SymbolScope::Global => { // A Local variable should never replace a global one if self.symbol_table.has_outer() { // This means that the symbol will - // be local and not global + // be local and not global, and thus not + // replace the global one self.symbol_table.define(s.name.value) } else { symbol