From 28b3b193bcf993e2dd2a9399ebbe61e6bab36202 Mon Sep 17 00:00:00 2001 From: Zettroke Date: Thu, 8 May 2025 09:14:56 +0300 Subject: [PATCH 1/9] fix function closures not correctly updating `TypeState` --- src/compiler/expression/function_call.rs | 44 +++++++++++++++++++----- src/compiler/function.rs | 25 +++++++++----- 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, 56 insertions(+), 19 deletions(-) diff --git a/src/compiler/expression/function_call.rs b/src/compiler/expression/function_call.rs index 35579a6e4e..779e9ea71d 100644 --- a/src/compiler/expression/function_call.rs +++ b/src/compiler/expression/function_call.rs @@ -507,16 +507,21 @@ 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. - variables - .iter() - .for_each(|ident| match locals.remove_variable(ident) { - Some(details) => state.local.insert_variable(ident.clone(), details), - None => { - state.local.remove_variable(ident); - } - }); + for ident in &variables { + 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); + } + } let (block_span, (block, block_type_def)) = block.take(); @@ -537,7 +542,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); @@ -700,6 +705,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 0e4262aec2..931c3ceb64 100644 --- a/src/compiler/function.rs +++ b/src/compiler/function.rs @@ -1,6 +1,13 @@ #![allow(clippy::missing_errors_doc)] pub mod closure; +use super::{ + expression::{container::Variant, Block, Container, Expr, Expression}, + state::TypeState, + value::{kind, Kind}, + CompileConfig, Span, TypeDef, +}; +use crate::compiler::type_def::Details; use crate::diagnostic::{DiagnosticMessage, Label, Note}; use crate::parser::ast::Ident; use crate::path::OwnedTargetPath; @@ -10,13 +17,6 @@ use std::{ fmt, }; -use super::{ - expression::{container::Variant, Block, Container, Expr, Expression}, - state::TypeState, - value::{kind, Kind}, - CompileConfig, Span, TypeDef, -}; - pub type Compiled = Result, Box>; pub type CompiledArgument = Result>, Box>; @@ -448,15 +448,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 5352ad8b43..71a1d9af5c 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 402b4b3311..706e6a964c 100644 --- a/src/stdlib/filter.rs +++ b/src/stdlib/filter.rs @@ -122,6 +122,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 4e46860865..ce021d8170 100644 --- a/src/stdlib/for_each.rs +++ b/src/stdlib/for_each.rs @@ -98,6 +98,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 bb5bb98625..0ef24fe8bf 100644 --- a/src/stdlib/map_keys.rs +++ b/src/stdlib/map_keys.rs @@ -122,6 +122,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 a0e1f5040b..a399aaa472 100644 --- a/src/stdlib/map_values.rs +++ b/src/stdlib/map_values.rs @@ -121,6 +121,7 @@ impl FunctionExpression for MapValuesFn { variables, block, block_type_def: _, + .. } = &self.closure; let runner = closure::Runner::new(variables, |ctx| block.resolve(ctx)); From b040cd72d5f2bfe9fcb3847ac5cf974951de4613 Mon Sep 17 00:00:00 2001 From: Zettroke Date: Thu, 8 May 2025 20:48:10 +0300 Subject: [PATCH 2/9] 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 035c257aa704417869d254d26a68660d07f03859 Mon Sep 17 00:00:00 2001 From: Zettroke Date: Fri, 9 May 2025 02:08:56 +0300 Subject: [PATCH 3/9] 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 b1b618af69..f6dc01e61d 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 779e9ea71d..8b15d19796 100644 --- a/src/compiler/expression/function_call.rs +++ b/src/compiler/expression/function_call.rs @@ -1250,8 +1250,9 @@ impl DiagnosticMessage for FunctionCallError { #[cfg(test)] mod tests { - use crate::compiler::{value::kind, FunctionExpression}; - + use crate::compiler::{value::kind, Compiler, FunctionExpression}; + use crate::parser::parse; + use crate::stdlib::ForEach; use super::*; #[derive(Clone, Debug)] @@ -1437,4 +1438,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 9b04cf7fea4d4228aa2285d6e8297fee7d172439 Mon Sep 17 00:00:00 2001 From: Pavlos Rontidis Date: Fri, 9 May 2025 09:38:56 -0400 Subject: [PATCH 4/9] cargo fmt --- src/compiler/expression/function_call.rs | 9 ++++++--- src/compiler/program.rs | 2 +- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/compiler/expression/function_call.rs b/src/compiler/expression/function_call.rs index 8b15d19796..4beec8b665 100644 --- a/src/compiler/expression/function_call.rs +++ b/src/compiler/expression/function_call.rs @@ -1250,10 +1250,10 @@ impl DiagnosticMessage for FunctionCallError { #[cfg(test)] mod tests { + use super::*; use crate::compiler::{value::kind, Compiler, FunctionExpression}; use crate::parser::parse; use crate::stdlib::ForEach; - use super::*; #[derive(Clone, Debug)] struct Fn; @@ -1441,13 +1441,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 18a8b2b96b..a4f4bb806a 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 46c33688b5519cfc57cb1c1d31d213b9698e8c1d Mon Sep 17 00:00:00 2001 From: Pavlos Rontidis Date: Fri, 9 May 2025 09:39:43 -0400 Subject: [PATCH 5/9] 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 425ebc93acf2ff9526b610c6c227a5ec916ce5df Mon Sep 17 00:00:00 2001 From: Pavlos Rontidis Date: Fri, 9 May 2025 09:43:55 -0400 Subject: [PATCH 6/9] make Vector integration check work with forks --- .github/workflows/vector_integration_check.yaml | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/.github/workflows/vector_integration_check.yaml b/.github/workflows/vector_integration_check.yaml index 581f841629..83cede1fea 100644 --- a/.github/workflows/vector_integration_check.yaml +++ b/.github/workflows/vector_integration_check.yaml @@ -35,8 +35,9 @@ jobs: git clone https://github.com/vectordotdev/vector.git cd vector git switch master - VRL_BRANCH=$(cd ../vrl && git rev-parse --abbrev-ref HEAD) - sed -i.bak "s|\(vrl = {[^}]*branch = \)\"[^\"]*\"|\1\"${{ github.head_ref }}\"|" Cargo.toml + VRL_GITHUB_REPO="${{ github.event.pull_request.head.repo.full_name }}" + VRL_GITHUB_BRANCH="${{ github.head_ref }}" + sed -i.bak "s|git = \".*\"|git = \"https://github.com/${VRL_GITHUB_REPO}.git\"|; s|branch = \".*\"|branch = \"${VRL_GITHUB_BRANCH}\"|" Cargo.toml cargo update -p vrl - name: Cargo Check Vector From 8b5e5a9bd97a9131e8e4c6641f9eaf6f6fdeab57 Mon Sep 17 00:00:00 2001 From: Pavlos Rontidis Date: Fri, 9 May 2025 09:52:19 -0400 Subject: [PATCH 7/9] second attempt to fix the workflow --- .github/workflows/vector_integration_check.yaml | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/.github/workflows/vector_integration_check.yaml b/.github/workflows/vector_integration_check.yaml index 83cede1fea..8c904461dd 100644 --- a/.github/workflows/vector_integration_check.yaml +++ b/.github/workflows/vector_integration_check.yaml @@ -35,9 +35,15 @@ jobs: git clone https://github.com/vectordotdev/vector.git cd vector git switch master + VRL_GITHUB_REPO="${{ github.event.pull_request.head.repo.full_name }}" VRL_GITHUB_BRANCH="${{ github.head_ref }}" - sed -i.bak "s|git = \".*\"|git = \"https://github.com/${VRL_GITHUB_REPO}.git\"|; s|branch = \".*\"|branch = \"${VRL_GITHUB_BRANCH}\"|" Cargo.toml + + REPLACEMENT_LINE="vrl = { git = \"https://github.com/${VRL_GITHUB_REPO}.git\", branch = \"${VRL_GITHUB_BRANCH}\" }" + sed -i.bak -e "/^vrl = {/c\\ + $REPLACEMENT_LINE + " Cargo.toml + cargo update -p vrl - name: Cargo Check Vector From e2a00093c5161107ca059a5e51010cf397cdf0a5 Mon Sep 17 00:00:00 2001 From: Pavlos Rontidis Date: Fri, 9 May 2025 10:13:48 -0400 Subject: [PATCH 8/9] 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 4beec8b665..bfcf4c4eba 100644 --- a/src/compiler/expression/function_call.rs +++ b/src/compiler/expression/function_call.rs @@ -501,16 +501,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 { let variable_details = state .local .remove_variable(ident) @@ -523,13 +521,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()); @@ -542,7 +545,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 893dc98807cf0a95ff142cd11bddeadda64b2cf9 Mon Sep 17 00:00:00 2001 From: Zettroke Date: Thu, 15 May 2025 08:28:41 +0300 Subject: [PATCH 9/9] Update vector_integration_check.yaml --- .github/workflows/vector_integration_check.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/vector_integration_check.yaml b/.github/workflows/vector_integration_check.yaml index faa49cd2ce..861c77b44e 100644 --- a/.github/workflows/vector_integration_check.yaml +++ b/.github/workflows/vector_integration_check.yaml @@ -61,4 +61,4 @@ jobs: run: | cd vector cargo update -p vrl - cargo check --workspace \ No newline at end of file + cargo check --workspace