From fac788affeab3aee3c1d3369ec0f8764e10e9163 Mon Sep 17 00:00:00 2001 From: Zettroke Date: Thu, 8 May 2025 09:14:56 +0300 Subject: [PATCH 1/8] fix function closures not correctly updating `TypeState` --- src/compiler/expression/function_call.rs | 30 ++++++++++++++++++++++-- src/compiler/function.rs | 12 ++++++++-- src/compiler/type_def.rs | 2 +- src/stdlib/filter.rs | 1 + src/stdlib/for_each.rs | 1 + src/stdlib/map_keys.rs | 1 + src/stdlib/map_values.rs | 1 + 7 files changed, 43 insertions(+), 5 deletions(-) diff --git a/src/compiler/expression/function_call.rs b/src/compiler/expression/function_call.rs index 4b650b37da..25c96377aa 100644 --- a/src/compiler/expression/function_call.rs +++ b/src/compiler/expression/function_call.rs @@ -506,11 +506,16 @@ impl<'a> Builder<'a> { let block = closure_block.expect("closure must contain block"); + let mut variables_types = vec![]; // At this point, we've compiled the block, so we can remove the // closure variables from the compiler's local environment. for ident in &variables { match locals.remove_variable(ident) { - Some(details) => state.local.insert_variable(ident.clone(), details), + Some(details) => { + variables_types.push(variable_details); + + state.local.insert_variable(ident.clone(), details) + }, None => { state.local.remove_variable(ident); } @@ -536,7 +541,7 @@ impl<'a> Builder<'a> { }); } - let fnclosure = Closure::new(variables, block, block_type_def); + let fnclosure = Closure::new(variables, variables_types, block, block_type_def); self.list.set_closure(fnclosure.clone()); // closure = Some(fnclosure); @@ -699,6 +704,27 @@ impl Expression for FunctionCall { let mut expr_result = self.expr.apply_type_info(&mut state); + // Closure can change state of locals in our `state`, so we need to update it. + if let Some(closure) = &self.closure { + // To get correct `type_info()` from closure we need to add closure arguments into current state + let mut closure_state = state.clone(); + for (ident, details) in closure + .variables + .iter() + .cloned() + .zip(closure.variables_types.iter().cloned()) + { + closure_state.local.insert_variable(ident, details); + } + let mut closure_info = closure.block.type_info(&closure_state); + // No interaction with closure arguments can't affect parent state, so remove them before merge + for ident in &closure.variables { + closure_info.state.local.remove_variable(ident); + } + + state = state.merge(closure_info.state); + } + // If one of the arguments only partially matches the function type // definition, then we mark the entire function as fallible. // diff --git a/src/compiler/function.rs b/src/compiler/function.rs index 5ab0bbd686..bb874e4f88 100644 --- a/src/compiler/function.rs +++ b/src/compiler/function.rs @@ -14,6 +14,7 @@ use super::{ CompileConfig, Span, TypeDef, expression::{Block, Container, Expr, Expression, container::Variant}, state::TypeState, + type_def::Details, value::{Kind, kind}, }; @@ -627,15 +628,22 @@ mod test_impls { #[derive(Debug, Clone, PartialEq)] pub struct Closure { pub variables: Vec, + pub variables_types: Vec
, pub block: Block, pub block_type_def: TypeDef, } impl Closure { #[must_use] - pub fn new>(variables: Vec, block: Block, block_type_def: TypeDef) -> Self { + pub fn new( + variables: Vec, + variables_types: Vec
, + block: Block, + block_type_def: TypeDef, + ) -> Self { Self { - variables: variables.into_iter().map(Into::into).collect(), + variables, + variables_types, block, block_type_def, } diff --git a/src/compiler/type_def.rs b/src/compiler/type_def.rs index 767a6a5b8d..51542ff30b 100644 --- a/src/compiler/type_def.rs +++ b/src/compiler/type_def.rs @@ -538,7 +538,7 @@ impl From for Kind { } #[derive(Debug, Clone, PartialEq)] -pub(crate) struct Details { +pub struct Details { pub(crate) type_def: TypeDef, pub(crate) value: Option, } diff --git a/src/stdlib/filter.rs b/src/stdlib/filter.rs index e15eb68cff..cba09009d8 100644 --- a/src/stdlib/filter.rs +++ b/src/stdlib/filter.rs @@ -161,6 +161,7 @@ impl FunctionExpression for FilterFn { variables, block, block_type_def: _, + .. } = &self.closure; let runner = closure::Runner::new(variables, |ctx| block.resolve(ctx)); diff --git a/src/stdlib/for_each.rs b/src/stdlib/for_each.rs index 3654fc32c7..c9bf2173b8 100644 --- a/src/stdlib/for_each.rs +++ b/src/stdlib/for_each.rs @@ -155,6 +155,7 @@ impl FunctionExpression for ForEachFn { variables, block, block_type_def: _, + .. } = &self.closure; let runner = closure::Runner::new(variables, |ctx| block.resolve(ctx)); diff --git a/src/stdlib/map_keys.rs b/src/stdlib/map_keys.rs index 9efa35a3ad..6b0ab7837a 100644 --- a/src/stdlib/map_keys.rs +++ b/src/stdlib/map_keys.rs @@ -191,6 +191,7 @@ impl FunctionExpression for MapKeysFn { variables, block, block_type_def: _, + .. } = &self.closure; let runner = closure::Runner::new(variables, |ctx| block.resolve(ctx)); diff --git a/src/stdlib/map_values.rs b/src/stdlib/map_values.rs index cabf3964c6..93824f3f84 100644 --- a/src/stdlib/map_values.rs +++ b/src/stdlib/map_values.rs @@ -180,6 +180,7 @@ impl FunctionExpression for MapValuesFn { variables, block, block_type_def: _, + .. } = &self.closure; let runner = closure::Runner::new(variables, |ctx| block.resolve(ctx)); From 94bde99a61974c4a1c563b4e07c4502b69c9d032 Mon Sep 17 00:00:00 2001 From: Zettroke Date: Thu, 8 May 2025 20:48:10 +0300 Subject: [PATCH 2/8] add changelog entry --- changelog.d/1399.fix-closure-type-state.md | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 changelog.d/1399.fix-closure-type-state.md diff --git a/changelog.d/1399.fix-closure-type-state.md b/changelog.d/1399.fix-closure-type-state.md new file mode 100644 index 0000000000..c6fae2a1f8 --- /dev/null +++ b/changelog.d/1399.fix-closure-type-state.md @@ -0,0 +1,3 @@ +Function closures now correctly update type state of the program. + +authors: zettroke From f785ceb76b8638e62283e8225a9b5f00ea2fa5a9 Mon Sep 17 00:00:00 2001 From: Zettroke Date: Fri, 9 May 2025 02:08:56 +0300 Subject: [PATCH 3/8] add test for function closures correctly updating `TypeState` --- src/compiler/compiler.rs | 27 +++++++++++++----------- src/compiler/expression/function_call.rs | 25 ++++++++++++++++++++-- 2 files changed, 38 insertions(+), 14 deletions(-) diff --git a/src/compiler/compiler.rs b/src/compiler/compiler.rs index 4fd1b42127..ecbaab8b61 100644 --- a/src/compiler/compiler.rs +++ b/src/compiler/compiler.rs @@ -84,6 +84,19 @@ impl CompilerError { } impl<'a> Compiler<'a> { + pub(crate) fn new(fns: &'a [Box], config: CompileConfig) -> Self { + Self { + fns, + diagnostics: vec![], + fallible: false, + abortable: false, + external_queries: vec![], + external_assignments: vec![], + skip_missing_query_target: vec![], + fallible_expression_error: None, + config, + } + } /// Compiles a given source into the final [`Program`]. /// /// # Arguments @@ -106,17 +119,7 @@ impl<'a> Compiler<'a> { let initial_state = state.clone(); let mut state = state.clone(); - let mut compiler = Self { - fns, - diagnostics: vec![], - fallible: false, - abortable: false, - external_queries: vec![], - external_assignments: vec![], - skip_missing_query_target: vec![], - fallible_expression_error: None, - config, - }; + let mut compiler = Compiler::new(fns, config); let expressions = compiler.compile_root_exprs(ast, &mut state); let (errors, warnings): (Vec<_>, Vec<_>) = @@ -272,7 +275,7 @@ impl<'a> Compiler<'a> { Some(Group::new(expr)) } - fn compile_root_exprs( + pub(crate) fn compile_root_exprs( &mut self, nodes: impl IntoIterator>, state: &mut TypeState, diff --git a/src/compiler/expression/function_call.rs b/src/compiler/expression/function_call.rs index 25c96377aa..6cb74db1d7 100644 --- a/src/compiler/expression/function_call.rs +++ b/src/compiler/expression/function_call.rs @@ -1273,8 +1273,9 @@ impl DiagnosticMessage for FunctionCallError { #[cfg(test)] mod tests { - use crate::compiler::{Category, FunctionExpression, value::kind}; - + use crate::compiler::{Category, Compiler, FunctionExpression, value::kind, , FunctionExpression}; + use crate::parser::parse; + use crate::stdlib::ForEach; use super::*; #[derive(Clone, Debug)] @@ -1462,4 +1463,24 @@ mod tests { assert_eq!(Ok(expected), params); } + + #[test] + fn closure_type_state() { + let program = parse(r#" + v = "" + + for_each({}) -> |key, value| { + v = 0 + } + "#).unwrap(); + + let fns = vec![Box::new(ForEach) as Box]; + let mut compiler = Compiler::new(&fns, CompileConfig::default()); + + let mut state = TypeState::default(); + compiler.compile_root_exprs(program, &mut state); + let var = state.local.variable(&Ident::new("v")).unwrap(); + + assert_eq!(var.type_def.kind(), &Kind::bytes().or_integer()); + } } From 9ad6d11655520bb8502ebde7151c71f84b4705ad Mon Sep 17 00:00:00 2001 From: Pavlos Rontidis Date: Fri, 9 May 2025 09:38:56 -0400 Subject: [PATCH 4/8] cargo fmt --- src/compiler/expression/function_call.rs | 7 +++++-- src/compiler/program.rs | 2 +- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/compiler/expression/function_call.rs b/src/compiler/expression/function_call.rs index 6cb74db1d7..7835aff5fc 100644 --- a/src/compiler/expression/function_call.rs +++ b/src/compiler/expression/function_call.rs @@ -1466,13 +1466,16 @@ mod tests { #[test] fn closure_type_state() { - let program = parse(r#" + let program = parse( + r#" v = "" for_each({}) -> |key, value| { v = 0 } - "#).unwrap(); + "#, + ) + .unwrap(); let fns = vec![Box::new(ForEach) as Box]; let mut compiler = Compiler::new(&fns, CompileConfig::default()); diff --git a/src/compiler/program.rs b/src/compiler/program.rs index 3ce1708a7d..e83d87a3e3 100644 --- a/src/compiler/program.rs +++ b/src/compiler/program.rs @@ -48,7 +48,7 @@ pub struct ProgramInfo { /// Returns whether the compiled program can fail at runtime. /// /// A program can only fail at runtime if the fallible-function-call - /// (`foo!()`) is used within the source. + /// (`foo!()`) is used within the source.vrl pub fallible: bool, /// Returns whether the compiled program can be aborted at runtime. From 1d77fc437a04d30f5780e9545f91dabcc9341d0a Mon Sep 17 00:00:00 2001 From: Pavlos Rontidis Date: Fri, 9 May 2025 09:39:43 -0400 Subject: [PATCH 5/8] rename changelog file --- changelog.d/{1399.fix-closure-type-state.md => 1399.fix.md} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename changelog.d/{1399.fix-closure-type-state.md => 1399.fix.md} (100%) diff --git a/changelog.d/1399.fix-closure-type-state.md b/changelog.d/1399.fix.md similarity index 100% rename from changelog.d/1399.fix-closure-type-state.md rename to changelog.d/1399.fix.md From 5296ad75f9ad4875df9bce231713a516418fdd1b Mon Sep 17 00:00:00 2001 From: Pavlos Rontidis Date: Fri, 9 May 2025 10:13:48 -0400 Subject: [PATCH 6/8] code improvements --- src/compiler/expression/function_call.rs | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/compiler/expression/function_call.rs b/src/compiler/expression/function_call.rs index 7835aff5fc..6e2584ac3d 100644 --- a/src/compiler/expression/function_call.rs +++ b/src/compiler/expression/function_call.rs @@ -500,16 +500,14 @@ impl<'a> Builder<'a> { state: &mut TypeState, ) -> Result<(Option, bool), FunctionCallError> { // Check if we have a closure we need to compile. - if let Some((variables, input)) = self.closure.clone() { + if let Some((variables, input)) = &self.closure { // TODO: This assumes the closure will run exactly once, which is incorrect. // see: https://github.com/vectordotdev/vector/issues/13782 - let block = closure_block.expect("closure must contain block"); - let mut variables_types = vec![]; // At this point, we've compiled the block, so we can remove the // closure variables from the compiler's local environment. - for ident in &variables { + for ident in variables { match locals.remove_variable(ident) { Some(details) => { variables_types.push(variable_details); @@ -522,13 +520,18 @@ impl<'a> Builder<'a> { } } - let (block_span, (block, block_type_def)) = block.take(); + let (block_span, (block, block_type_def)) = closure_block + .ok_or(FunctionCallError::MissingClosure { + call_span: Span::default(), // TODO can we provide a better span? + example: None, + })? + .take(); let closure_fallible = block_type_def.is_fallible(); // Check the type definition of the resulting block.This needs to match // whatever is configured by the closure input type. - let expected_kind = input.output.into_kind(); + let expected_kind = input.clone().output.into_kind(); let found_kind = block_type_def .kind() .union(block_type_def.returns().clone()); @@ -541,7 +544,7 @@ impl<'a> Builder<'a> { }); } - let fnclosure = Closure::new(variables, variables_types, block, block_type_def); + let fnclosure = Closure::new(variables.clone(), variables_types, block, block_type_def); self.list.set_closure(fnclosure.clone()); // closure = Some(fnclosure); From 6105d065a26cca1b06dd0d836e381598b3c2727c Mon Sep 17 00:00:00 2001 From: Zettroke Date: Tue, 24 Mar 2026 17:14:45 +0300 Subject: [PATCH 7/8] fix rebase errors --- src/compiler/expression/function_call.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/compiler/expression/function_call.rs b/src/compiler/expression/function_call.rs index 6e2584ac3d..10c97ce753 100644 --- a/src/compiler/expression/function_call.rs +++ b/src/compiler/expression/function_call.rs @@ -510,10 +510,10 @@ impl<'a> Builder<'a> { for ident in variables { match locals.remove_variable(ident) { Some(details) => { - variables_types.push(variable_details); + variables_types.push(details.clone()); state.local.insert_variable(ident.clone(), details) - }, + } None => { state.local.remove_variable(ident); } @@ -1276,10 +1276,10 @@ impl DiagnosticMessage for FunctionCallError { #[cfg(test)] mod tests { - use crate::compiler::{Category, Compiler, FunctionExpression, value::kind, , FunctionExpression}; + use super::*; + use crate::compiler::{Category, Compiler, FunctionExpression, value::kind}; use crate::parser::parse; use crate::stdlib::ForEach; - use super::*; #[derive(Clone, Debug)] struct Fn; From 0f01b67090657dd6a551bf5c574f1b3a4b19f1b1 Mon Sep 17 00:00:00 2001 From: Zettroke Date: Wed, 1 Apr 2026 11:25:16 +0300 Subject: [PATCH 8/8] fix incorrect rebase --- src/compiler/expression/function_call.rs | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/src/compiler/expression/function_call.rs b/src/compiler/expression/function_call.rs index 10c97ce753..c52b75d0bb 100644 --- a/src/compiler/expression/function_call.rs +++ b/src/compiler/expression/function_call.rs @@ -508,15 +508,15 @@ impl<'a> Builder<'a> { // At this point, we've compiled the block, so we can remove the // closure variables from the compiler's local environment. for ident in variables { - match locals.remove_variable(ident) { - Some(details) => { - variables_types.push(details.clone()); - - state.local.insert_variable(ident.clone(), details) - } - None => { - state.local.remove_variable(ident); - } + let variable_details = state + .local + .remove_variable(ident) + .expect("Closure variable must be present"); + variables_types.push(variable_details); + + // If outer scope has this variable, restore its state + if let Some(details) = locals.remove_variable(ident) { + state.local.insert_variable(ident.clone(), details); } } @@ -711,6 +711,7 @@ impl Expression for FunctionCall { if let Some(closure) = &self.closure { // To get correct `type_info()` from closure we need to add closure arguments into current state let mut closure_state = state.clone(); + dbg!(&closure.variables_types); for (ident, details) in closure .variables .iter() @@ -720,7 +721,7 @@ impl Expression for FunctionCall { closure_state.local.insert_variable(ident, details); } let mut closure_info = closure.block.type_info(&closure_state); - // No interaction with closure arguments can't affect parent state, so remove them before merge + // Interaction with closure arguments can't affect parent state, so remove them before merge for ident in &closure.variables { closure_info.state.local.remove_variable(ident); }