From 1142c37b16b17130fb71e989f3e4ebbcdac67db4 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 6 Sep 2026 13:32:57 +0000 Subject: [PATCH] fix: environment leak when a call expression errors `evaluate_call_expression` pushed a new environment and then used `?` while evaluating the arguments and the function's block, so any error inside a call returned early without ever popping that environment. Because the REPL reuses a single `Evaluator` across lines, each such error permanently orphaned a frame: subsequent bindings landed in the leaked environment and the chain grew without bound. The body of the call is now evaluated by a helper, so the result is propagated only after the environment has been popped, pairing every push with a pop on both the success and the error paths. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01P9XgLUX4gdGGRvKhnCAUs6 --- src/evaluator/expression/call/mod.rs | 31 ++++++---- src/tests/environment/mod.rs | 38 ++++++++++++ ...nment_call_error_case_1_1_environment.snap | 31 ++++++++++ ...onment_call_error_case_1_1_evaluation.snap | 5 ++ ...nment_call_error_case_1_2_environment.snap | 31 ++++++++++ ..._call_error_case_1_2_evaluation_error.snap | 7 +++ ...nment_call_error_case_1_3_environment.snap | 34 +++++++++++ ...onment_call_error_case_1_3_evaluation.snap | 5 ++ ...nment_call_error_case_2_1_environment.snap | 25 ++++++++ ...onment_call_error_case_2_1_evaluation.snap | 5 ++ ...nment_call_error_case_2_2_environment.snap | 25 ++++++++ ..._call_error_case_2_2_evaluation_error.snap | 7 +++ ...nment_call_error_case_2_3_environment.snap | 28 +++++++++ ...onment_call_error_case_2_3_evaluation.snap | 5 ++ ...nment_call_error_case_3_1_environment.snap | 57 ++++++++++++++++++ ...onment_call_error_case_3_1_evaluation.snap | 5 ++ ...nment_call_error_case_3_2_environment.snap | 57 ++++++++++++++++++ ..._call_error_case_3_2_evaluation_error.snap | 7 +++ ...nment_call_error_case_3_3_environment.snap | 60 +++++++++++++++++++ ...onment_call_error_case_3_3_evaluation.snap | 5 ++ src/tests/macros.rs | 21 +++++++ 21 files changed, 479 insertions(+), 10 deletions(-) create mode 100644 src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_1_1_environment.snap create mode 100644 src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_1_1_evaluation.snap create mode 100644 src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_1_2_environment.snap create mode 100644 src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_1_2_evaluation_error.snap create mode 100644 src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_1_3_environment.snap create mode 100644 src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_1_3_evaluation.snap create mode 100644 src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_2_1_environment.snap create mode 100644 src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_2_1_evaluation.snap create mode 100644 src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_2_2_environment.snap create mode 100644 src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_2_2_evaluation_error.snap create mode 100644 src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_2_3_environment.snap create mode 100644 src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_2_3_evaluation.snap create mode 100644 src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_3_1_environment.snap create mode 100644 src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_3_1_evaluation.snap create mode 100644 src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_3_2_environment.snap create mode 100644 src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_3_2_evaluation_error.snap create mode 100644 src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_3_3_environment.snap create mode 100644 src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_3_3_evaluation.snap diff --git a/src/evaluator/expression/call/mod.rs b/src/evaluator/expression/call/mod.rs index 8ee675f7..27f13fc5 100644 --- a/src/evaluator/expression/call/mod.rs +++ b/src/evaluator/expression/call/mod.rs @@ -1,5 +1,6 @@ use crate::evaluator::Evaluator; use crate::evaluator::Object; +use crate::syntax_analysis::Block; use crate::syntax_analysis::Expression; impl Evaluator { @@ -19,17 +20,11 @@ impl Evaluator { } self.environment.push(); - - for (argument, parameter_identifier) in arguments.into_iter().zip(parameters) { - let argument_evaluation = self.evaluate_expression(argument)?; - - self.environment - .set(parameter_identifier, argument_evaluation); - } - - let block_call_evaluation = self.evaluate_block(block)?; + // The call's environment is popped before the result is propagated, so an error + // inside the call does not leave an orphaned environment behind. + let call_evaluation = self.evaluate_call(arguments, parameters, block); self.environment.pop(); - Ok(block_call_evaluation) + call_evaluation } object => anyhow::bail!( "Cannot call an object of type {}, only functions are callable.", @@ -37,4 +32,20 @@ impl Evaluator { ), } } + + fn evaluate_call( + &mut self, + arguments: Vec, + parameters: Vec, + block: Block, + ) -> anyhow::Result { + for (argument, parameter_identifier) in arguments.into_iter().zip(parameters) { + let argument_evaluation = self.evaluate_expression(argument)?; + + self.environment + .set(parameter_identifier, argument_evaluation); + } + + self.evaluate_block(block) + } } diff --git a/src/tests/environment/mod.rs b/src/tests/environment/mod.rs index 49e72d6e..3adc2e7d 100644 --- a/src/tests/environment/mod.rs +++ b/src/tests/environment/mod.rs @@ -18,3 +18,41 @@ fn test_environment(code_1: &str, code_2: &str, code_3: &str, snapshot_name: &st assert_successive_environment!(evaluator, code_2, format!("{snapshot_name}_2")); assert_successive_environment!(evaluator, code_3, format!("{snapshot_name}_3")); } + +#[template] +#[rstest( + code_1, + code_2, + code_3, + snapshot_name, + case( + "let divide = fn(x) { x / 0 };", + "divide(1);", + "let a = 25;", + "environment_call_error_case_1" + ), + case( + "let identity = fn(x) { x };", + "identity(1 / 0);", + "let a = 25;", + "environment_call_error_case_2" + ), + case( + "let call_divide = fn(x) { let divide = fn(y) { y / 0 }; divide(x) };", + "call_divide(1);", + "let a = 25;", + "environment_call_error_case_3" + ) +)] +fn environment_call_error_cases(code_1: &str, code_2: &str, code_3: &str, snapshot_name: &str) {} + +// A call which errors must still pop its environment, otherwise the erroring call's environment +// leaks and every subsequent binding lands in it. +#[apply(environment_call_error_cases)] +fn test_environment_call_error(code_1: &str, code_2: &str, code_3: &str, snapshot_name: &str) { + let mut evaluator = crate::evaluator::Evaluator::new(); + + assert_successive_environment!(evaluator, code_1, format!("{snapshot_name}_1")); + assert_successive_environment_error!(evaluator, code_2, format!("{snapshot_name}_2")); + assert_successive_environment!(evaluator, code_3, format!("{snapshot_name}_3")); +} diff --git a/src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_1_1_environment.snap b/src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_1_1_environment.snap new file mode 100644 index 00000000..3c9d2346 --- /dev/null +++ b/src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_1_1_environment.snap @@ -0,0 +1,31 @@ +--- +source: src/tests/environment/mod.rs +expression: evaluator +--- +Evaluator { + environment: Environment { + variables: { + "divide": Function { + parameters: [ + "x", + ], + block: Block { + nodes: [ + Expression { + expression: Infix { + left_hand: Identifier { + identifier: "x", + }, + operator: Divide, + right_hand: Integer { + literal: 0, + }, + }, + }, + ], + }, + }, + }, + sub_environment: None, + }, +} diff --git a/src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_1_1_evaluation.snap b/src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_1_1_evaluation.snap new file mode 100644 index 00000000..11a9f425 --- /dev/null +++ b/src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_1_1_evaluation.snap @@ -0,0 +1,5 @@ +--- +source: src/tests/environment/mod.rs +expression: evaluation +--- +Null diff --git a/src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_1_2_environment.snap b/src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_1_2_environment.snap new file mode 100644 index 00000000..3c9d2346 --- /dev/null +++ b/src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_1_2_environment.snap @@ -0,0 +1,31 @@ +--- +source: src/tests/environment/mod.rs +expression: evaluator +--- +Evaluator { + environment: Environment { + variables: { + "divide": Function { + parameters: [ + "x", + ], + block: Block { + nodes: [ + Expression { + expression: Infix { + left_hand: Identifier { + identifier: "x", + }, + operator: Divide, + right_hand: Integer { + literal: 0, + }, + }, + }, + ], + }, + }, + }, + sub_environment: None, + }, +} diff --git a/src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_1_2_evaluation_error.snap b/src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_1_2_evaluation_error.snap new file mode 100644 index 00000000..ca4e007e --- /dev/null +++ b/src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_1_2_evaluation_error.snap @@ -0,0 +1,7 @@ +--- +source: src/tests/environment/mod.rs +expression: error +--- +Err( + "Division by zero, cannot apply the / infix operator to a right hand operand of 0.", +) diff --git a/src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_1_3_environment.snap b/src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_1_3_environment.snap new file mode 100644 index 00000000..994ab12b --- /dev/null +++ b/src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_1_3_environment.snap @@ -0,0 +1,34 @@ +--- +source: src/tests/environment/mod.rs +expression: evaluator +--- +Evaluator { + environment: Environment { + variables: { + "a": Integer { + value: 25, + }, + "divide": Function { + parameters: [ + "x", + ], + block: Block { + nodes: [ + Expression { + expression: Infix { + left_hand: Identifier { + identifier: "x", + }, + operator: Divide, + right_hand: Integer { + literal: 0, + }, + }, + }, + ], + }, + }, + }, + sub_environment: None, + }, +} diff --git a/src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_1_3_evaluation.snap b/src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_1_3_evaluation.snap new file mode 100644 index 00000000..11a9f425 --- /dev/null +++ b/src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_1_3_evaluation.snap @@ -0,0 +1,5 @@ +--- +source: src/tests/environment/mod.rs +expression: evaluation +--- +Null diff --git a/src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_2_1_environment.snap b/src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_2_1_environment.snap new file mode 100644 index 00000000..98623acb --- /dev/null +++ b/src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_2_1_environment.snap @@ -0,0 +1,25 @@ +--- +source: src/tests/environment/mod.rs +expression: evaluator +--- +Evaluator { + environment: Environment { + variables: { + "identity": Function { + parameters: [ + "x", + ], + block: Block { + nodes: [ + Expression { + expression: Identifier { + identifier: "x", + }, + }, + ], + }, + }, + }, + sub_environment: None, + }, +} diff --git a/src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_2_1_evaluation.snap b/src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_2_1_evaluation.snap new file mode 100644 index 00000000..11a9f425 --- /dev/null +++ b/src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_2_1_evaluation.snap @@ -0,0 +1,5 @@ +--- +source: src/tests/environment/mod.rs +expression: evaluation +--- +Null diff --git a/src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_2_2_environment.snap b/src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_2_2_environment.snap new file mode 100644 index 00000000..98623acb --- /dev/null +++ b/src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_2_2_environment.snap @@ -0,0 +1,25 @@ +--- +source: src/tests/environment/mod.rs +expression: evaluator +--- +Evaluator { + environment: Environment { + variables: { + "identity": Function { + parameters: [ + "x", + ], + block: Block { + nodes: [ + Expression { + expression: Identifier { + identifier: "x", + }, + }, + ], + }, + }, + }, + sub_environment: None, + }, +} diff --git a/src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_2_2_evaluation_error.snap b/src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_2_2_evaluation_error.snap new file mode 100644 index 00000000..ca4e007e --- /dev/null +++ b/src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_2_2_evaluation_error.snap @@ -0,0 +1,7 @@ +--- +source: src/tests/environment/mod.rs +expression: error +--- +Err( + "Division by zero, cannot apply the / infix operator to a right hand operand of 0.", +) diff --git a/src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_2_3_environment.snap b/src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_2_3_environment.snap new file mode 100644 index 00000000..e0de6fd9 --- /dev/null +++ b/src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_2_3_environment.snap @@ -0,0 +1,28 @@ +--- +source: src/tests/environment/mod.rs +expression: evaluator +--- +Evaluator { + environment: Environment { + variables: { + "a": Integer { + value: 25, + }, + "identity": Function { + parameters: [ + "x", + ], + block: Block { + nodes: [ + Expression { + expression: Identifier { + identifier: "x", + }, + }, + ], + }, + }, + }, + sub_environment: None, + }, +} diff --git a/src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_2_3_evaluation.snap b/src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_2_3_evaluation.snap new file mode 100644 index 00000000..11a9f425 --- /dev/null +++ b/src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_2_3_evaluation.snap @@ -0,0 +1,5 @@ +--- +source: src/tests/environment/mod.rs +expression: evaluation +--- +Null diff --git a/src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_3_1_environment.snap b/src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_3_1_environment.snap new file mode 100644 index 00000000..deeaf398 --- /dev/null +++ b/src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_3_1_environment.snap @@ -0,0 +1,57 @@ +--- +source: src/tests/environment/mod.rs +expression: evaluator +--- +Evaluator { + environment: Environment { + variables: { + "call_divide": Function { + parameters: [ + "x", + ], + block: Block { + nodes: [ + Statement { + statement: Let { + identifier: "divide", + expression: Function { + parameters: [ + "y", + ], + block: Block { + nodes: [ + Expression { + expression: Infix { + left_hand: Identifier { + identifier: "y", + }, + operator: Divide, + right_hand: Integer { + literal: 0, + }, + }, + }, + ], + }, + }, + }, + }, + Expression { + expression: Call { + function: Identifier { + identifier: "divide", + }, + arguments: [ + Identifier { + identifier: "x", + }, + ], + }, + }, + ], + }, + }, + }, + sub_environment: None, + }, +} diff --git a/src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_3_1_evaluation.snap b/src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_3_1_evaluation.snap new file mode 100644 index 00000000..11a9f425 --- /dev/null +++ b/src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_3_1_evaluation.snap @@ -0,0 +1,5 @@ +--- +source: src/tests/environment/mod.rs +expression: evaluation +--- +Null diff --git a/src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_3_2_environment.snap b/src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_3_2_environment.snap new file mode 100644 index 00000000..deeaf398 --- /dev/null +++ b/src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_3_2_environment.snap @@ -0,0 +1,57 @@ +--- +source: src/tests/environment/mod.rs +expression: evaluator +--- +Evaluator { + environment: Environment { + variables: { + "call_divide": Function { + parameters: [ + "x", + ], + block: Block { + nodes: [ + Statement { + statement: Let { + identifier: "divide", + expression: Function { + parameters: [ + "y", + ], + block: Block { + nodes: [ + Expression { + expression: Infix { + left_hand: Identifier { + identifier: "y", + }, + operator: Divide, + right_hand: Integer { + literal: 0, + }, + }, + }, + ], + }, + }, + }, + }, + Expression { + expression: Call { + function: Identifier { + identifier: "divide", + }, + arguments: [ + Identifier { + identifier: "x", + }, + ], + }, + }, + ], + }, + }, + }, + sub_environment: None, + }, +} diff --git a/src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_3_2_evaluation_error.snap b/src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_3_2_evaluation_error.snap new file mode 100644 index 00000000..ca4e007e --- /dev/null +++ b/src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_3_2_evaluation_error.snap @@ -0,0 +1,7 @@ +--- +source: src/tests/environment/mod.rs +expression: error +--- +Err( + "Division by zero, cannot apply the / infix operator to a right hand operand of 0.", +) diff --git a/src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_3_3_environment.snap b/src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_3_3_environment.snap new file mode 100644 index 00000000..fbc314e9 --- /dev/null +++ b/src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_3_3_environment.snap @@ -0,0 +1,60 @@ +--- +source: src/tests/environment/mod.rs +expression: evaluator +--- +Evaluator { + environment: Environment { + variables: { + "a": Integer { + value: 25, + }, + "call_divide": Function { + parameters: [ + "x", + ], + block: Block { + nodes: [ + Statement { + statement: Let { + identifier: "divide", + expression: Function { + parameters: [ + "y", + ], + block: Block { + nodes: [ + Expression { + expression: Infix { + left_hand: Identifier { + identifier: "y", + }, + operator: Divide, + right_hand: Integer { + literal: 0, + }, + }, + }, + ], + }, + }, + }, + }, + Expression { + expression: Call { + function: Identifier { + identifier: "divide", + }, + arguments: [ + Identifier { + identifier: "x", + }, + ], + }, + }, + ], + }, + }, + }, + sub_environment: None, + }, +} diff --git a/src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_3_3_evaluation.snap b/src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_3_3_evaluation.snap new file mode 100644 index 00000000..11a9f425 --- /dev/null +++ b/src/tests/environment/snapshots/monkey_interpreter__tests__environment__test_environment_call_error_case_3_3_evaluation.snap @@ -0,0 +1,5 @@ +--- +source: src/tests/environment/mod.rs +expression: evaluation +--- +Null diff --git a/src/tests/macros.rs b/src/tests/macros.rs index e9717c8c..fa0ffc1d 100644 --- a/src/tests/macros.rs +++ b/src/tests/macros.rs @@ -111,3 +111,24 @@ macro_rules! assert_ok { } }; } + +macro_rules! assert_successive_environment_error { + ($evaluator:expr, $code:expr, $snapshot_name:expr) => { + INIT.call_once(|| { + pretty_env_logger::init(); + }); + + // When + let error = $evaluator.evaluate( + crate::syntax_analysis::SyntaxAnalysis::from( + crate::lexical_analysis::LexicalAnalysis::from($code).unwrap(), + ) + .unwrap(), + ); + + // Then + assert!(error.is_err()); + insta::assert_debug_snapshot!(format!("test_{}_evaluation_error", $snapshot_name), error); + insta::assert_debug_snapshot!(format!("test_{}_environment", $snapshot_name), $evaluator); + }; +}