From 30bd134a0be0d83cdeecc59ec91980c4b60a1d34 Mon Sep 17 00:00:00 2001 From: Anand Krishnamoorthi Date: Tue, 2 Dec 2025 13:29:12 -0600 Subject: [PATCH] fix: Handle builtin out-parameter calls in RVM compiler MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Teach the hoister/destructuring planner to respect parent scope when building binding plans for extra arguments, so already-bound vars yield equality checks. - Update the compiler’s function-call path to drop the trailing out-argument, run its binding plan after the call, and share call-target resolution logic. - Add regression suites for builtin and user-defined out-parameter scenarios plus align the CLI example output when RVM returns undefined. Signed-off-by: Anand Krishnamoorthi --- .../destructuring_planner/parameters.rs | 7 +- src/compiler/hoist.rs | 8 +- src/languages/rego/compiler/function_calls.rs | 166 ++++++++++++++---- tests/rvm/rego/cases/builtins_out_params.yaml | 83 +++++++++ tests/rvm/rego/cases/user_fcn_out_params.yaml | 107 +++++++++++ 5 files changed, 334 insertions(+), 37 deletions(-) create mode 100644 tests/rvm/rego/cases/builtins_out_params.yaml create mode 100644 tests/rvm/rego/cases/user_fcn_out_params.yaml diff --git a/src/compiler/destructuring_planner/parameters.rs b/src/compiler/destructuring_planner/parameters.rs index fd5574a..d921bdf 100644 --- a/src/compiler/destructuring_planner/parameters.rs +++ b/src/compiler/destructuring_planner/parameters.rs @@ -37,12 +37,13 @@ pub fn create_loop_index_binding_plan( pub fn create_parameter_binding_plan( param_expr: &ExprRef, context: &T, + scoping: ScopingMode, ) -> Result { let mut newly_bound = BTreeSet::new(); let destructuring_plan = create_destructuring_plan_with_tracking( param_expr, context, - ScopingMode::AllowShadowing, + scoping, &mut newly_bound, ) .ok_or_else(|| BindingPlannerError::FailedToCreateDestructuringPlan { @@ -50,7 +51,9 @@ pub fn create_parameter_binding_plan( span: param_expr.span().clone(), })?; - validate_pattern_bindings(param_expr, &newly_bound, context)?; + if scoping == ScopingMode::AllowShadowing { + validate_pattern_bindings(param_expr, &newly_bound, context)?; + } Ok(BindingPlan::Parameter { param_expr: param_expr.clone(), diff --git a/src/compiler/hoist.rs b/src/compiler/hoist.rs index cbae7a4..bba31e0 100644 --- a/src/compiler/hoist.rs +++ b/src/compiler/hoist.rs @@ -381,7 +381,9 @@ impl LoopHoister { for param in args { // Create binding plan for function parameter match super::destructuring_planner::create_parameter_binding_plan( - param, &context, + param, + &context, + ScopingMode::AllowShadowing, ) { Ok(binding_plan) => { let expr_idx = param.as_ref().eidx(); @@ -708,7 +710,9 @@ impl LoopHoister { // If the last parameter expression contains unbound vars, create a binding plan if let Some(last_param) = params.last() { match super::destructuring_planner::create_parameter_binding_plan( - last_param, context, + last_param, + context, + ScopingMode::RespectParent, ) { Ok(binding_plan) => { let expr_idx = last_param.as_ref().eidx(); diff --git a/src/languages/rego/compiler/function_calls.rs b/src/languages/rego/compiler/function_calls.rs index cf2d91c..f709292 100644 --- a/src/languages/rego/compiler/function_calls.rs +++ b/src/languages/rego/compiler/function_calls.rs @@ -3,11 +3,24 @@ use super::{Compiler, CompilerError, Register, Result}; use crate::ast::ExprRef; +use crate::builtins; +use crate::compiler::destructuring_planner::plans::BindingPlan; use crate::lexer::Span; use crate::rvm::instructions::{BuiltinCallParams, FunctionCallParams}; use crate::rvm::Instruction; use crate::utils::get_path_string; -use alloc::vec::Vec; +use alloc::{format, string::ToString, vec::Vec}; + +enum CallTarget { + User { + rule_index: u16, + expected_args: Option, + }, + Builtin { + builtin_index: u16, + expected_args: Option, + }, +} impl<'a> Compiler<'a> { pub(super) fn compile_function_call( @@ -27,51 +40,138 @@ impl<'a> Compiler<'a> { .map_err(|_| CompilerError::InvalidFunctionExpressionWithPackage.at(&span))? }; + let mut out_param_plan: Option<(BindingPlan, Span)> = None; + let mut params_to_compile = params.len(); + + let call_target = self.determine_call_target(&original_fcn_path, &full_fcn_path, &span)?; + + let expected_args = match &call_target { + CallTarget::User { expected_args, .. } => *expected_args, + CallTarget::Builtin { expected_args, .. } => *expected_args, + }; + + if let Some(expected) = expected_args { + if params.len() == expected + 1 { + if let Some(last_param) = params.last() { + let plan = self.expect_binding_plan_for_expr( + last_param, + &format!("extra argument for function '{}'", original_fcn_path), + )?; + + match plan { + BindingPlan::Parameter { .. } => { + out_param_plan = Some((plan, last_param.span().clone())); + params_to_compile -= 1; + } + other => { + return Err(CompilerError::UnexpectedBindingPlan { + context: "function extra argument".to_string(), + found: format!("{other:?}"), + } + .at(last_param.span())); + } + } + } + } + } + let mut arg_regs = Vec::new(); - for param in params.iter() { + for param in params.iter().take(params_to_compile) { let param_reg = self.compile_rego_expr_with_span(param, param.span(), false)?; arg_regs.push(param_reg); } let dest = self.alloc_register(); - if self.is_user_defined_function(&full_fcn_path) { - let rule_index = self.get_or_assign_rule_index(&full_fcn_path)?; - let mut args_array = [0u8; 8]; - let num_args = arg_regs.len().min(8) as u8; - for (i, ®) in arg_regs.iter().take(8).enumerate() { - args_array[i] = reg; - } + match call_target { + CallTarget::User { rule_index, .. } => { + let mut args_array = [0u8; 8]; + let num_args = arg_regs.len().min(8) as u8; + for (i, ®) in arg_regs.iter().take(8).enumerate() { + args_array[i] = reg; + } - let params_index = self.program.add_function_call_params(FunctionCallParams { - func_rule_index: rule_index, - dest, - num_args, - args: args_array, - }); - self.emit_instruction(Instruction::FunctionCall { params_index }, &span); - } else if self.is_builtin(&original_fcn_path) { - let builtin_index = self.get_builtin_index(&original_fcn_path)?; - let mut args_array = [0u8; 8]; - let num_args = arg_regs.len().min(8) as u8; - for (i, ®) in arg_regs.iter().take(8).enumerate() { - args_array[i] = reg; + let params_index = self.program.add_function_call_params(FunctionCallParams { + func_rule_index: rule_index, + dest, + num_args, + args: args_array, + }); + self.emit_instruction(Instruction::FunctionCall { params_index }, &span); } + CallTarget::Builtin { builtin_index, .. } => { + let mut args_array = [0u8; 8]; + let num_args = arg_regs.len().min(8) as u8; + for (i, ®) in arg_regs.iter().take(8).enumerate() { + args_array[i] = reg; + } - let params_index = self.program.add_builtin_call_params(BuiltinCallParams { - dest, - builtin_index, - num_args, - args: args_array, - }); - self.emit_instruction(Instruction::BuiltinCall { params_index }, &span); - } else { - return Err(CompilerError::UnknownFunction { - name: original_fcn_path, + let params_index = self.program.add_builtin_call_params(BuiltinCallParams { + dest, + builtin_index, + num_args, + args: args_array, + }); + self.emit_instruction(Instruction::BuiltinCall { params_index }, &span); } - .at(&span)); + } + + if let Some((plan, plan_span)) = &out_param_plan { + self.apply_binding_plan(plan, dest, plan_span) + .map_err(|err| CompilerError::from(err).at(plan_span))?; + self.emit_instruction(Instruction::LoadBool { dest, value: true }, &span); } Ok(dest) } + + fn lookup_builtin_arity(&self, name: &str) -> Option { + if name == "print" { + Some(2) + } else { + builtins::BUILTINS + .get(name) + .map(|(_, arity)| *arity as usize) + } + } +} + +impl<'a> Compiler<'a> { + fn determine_call_target( + &mut self, + original_fcn_path: &str, + full_fcn_path: &str, + span: &Span, + ) -> Result { + if self.is_user_defined_function(full_fcn_path) { + let rule_index = self.get_or_assign_rule_index(full_fcn_path)?; + let expected_args = self + .policy + .inner + .functions + .get(full_fcn_path) + .map(|(_, arity, _)| *arity as usize) + .or_else(|| { + self.rule_function_param_count + .get(rule_index as usize) + .and_then(|count| *count) + }); + Ok(CallTarget::User { + rule_index, + expected_args, + }) + } else if self.is_builtin(original_fcn_path) { + let builtin_index = self.get_builtin_index(original_fcn_path)?; + let expected_args = self.lookup_builtin_arity(original_fcn_path); + Ok(CallTarget::Builtin { + builtin_index, + expected_args, + }) + } else { + Err(CompilerError::UnknownFunction { + name: original_fcn_path.to_string(), + } + .at(span)) + } + } } diff --git a/tests/rvm/rego/cases/builtins_out_params.yaml b/tests/rvm/rego/cases/builtins_out_params.yaml new file mode 100644 index 0000000..93f6c68 --- /dev/null +++ b/tests/rvm/rego/cases/builtins_out_params.yaml @@ -0,0 +1,83 @@ +# Copyright (c) Microsoft Corporation. +# Licensed under the MIT License. + +# Builtin Out-Parameter Test Suite +# Verifies the compiler handles builtin return-argument syntax consistently +# across variable bindings, equality checks, scheduler ordering, and literals. + +cases: + - note: builtin_out_param_simple_binding + data: {} + modules: + - | + package test + + rule1 if { + floor(1.001, x) + x == 1 + } + query: data.test.rule1 + want_result: true + + - note: builtin_out_param_scheduler_reordering + data: {} + modules: + - | + package test + + rule2 if { + x == 1 + floor(1.001, x) + } + query: data.test.rule2 + want_result: true + + - note: builtin_out_param_existing_binding_equality + data: {} + modules: + - | + package test + + rule3 if { + x := 1 + floor(1.001, x) + } + query: data.test.rule3 + want_result: true + + - note: builtin_out_param_existing_binding_mismatch + data: {} + modules: + - | + package test + + rule31 if { + x := 2 + floor(1.001, x) + } + query: data.test.rule31 + want_result: "#undefined" + + - note: builtin_out_param_literal_success + data: {} + modules: + - | + package test + + rule4 if { + floor(1.001, 1) + } + query: data.test.rule4 + want_result: true + + - note: builtin_out_param_literal_failure + data: {} + modules: + - | + package test + + rule5 if { + floor(1.001, 2) + } + query: data.test.rule5 + want_result: "#undefined" diff --git a/tests/rvm/rego/cases/user_fcn_out_params.yaml b/tests/rvm/rego/cases/user_fcn_out_params.yaml new file mode 100644 index 0000000..1feb9ac --- /dev/null +++ b/tests/rvm/rego/cases/user_fcn_out_params.yaml @@ -0,0 +1,107 @@ +# Copyright (c) Microsoft Corporation. +# Licensed under the MIT License. + +# User-Defined Out-Parameter Test Suite +# Mirrors builtin coverage but routes through the my_floor helper rule +# to ensure planner handling stays correct for user-defined function rules. + +cases: + - note: user_fcn_out_param_simple_binding + data: {} + modules: + - | + package test + + my_floor(x) := y if { + floor(x, y) + } + + rule1 if { + my_floor(1.001, x) + x == 1 + } + query: data.test.rule1 + want_result: true + + - note: user_fcn_out_param_scheduler_reordering + data: {} + modules: + - | + package test + + my_floor(x) := y if { + floor(x, y) + } + + rule2 if { + x == 1 + my_floor(1.001, x) + } + query: data.test.rule2 + want_result: true + + - note: user_fcn_out_param_existing_binding_equality + data: {} + modules: + - | + package test + + my_floor(x) := y if { + floor(x, y) + } + + rule3 if { + x := 1 + my_floor(1.001, x) + } + query: data.test.rule3 + want_result: true + + - note: user_fcn_out_param_existing_binding_mismatch + data: {} + modules: + - | + package test + + my_floor(x) := y if { + floor(x, y) + } + + rule31 if { + x := 2 + my_floor(1.001, x) + } + query: data.test.rule31 + want_result: "#undefined" + + - note: user_fcn_out_param_literal_success + data: {} + modules: + - | + package test + + my_floor(x) := y if { + floor(x, y) + } + + rule4 if { + my_floor(1.001, 1) + } + query: data.test.rule4 + want_result: true + + - note: user_fcn_out_param_literal_failure + data: {} + modules: + - | + package test + + my_floor(x) := y if { + floor(x, y) + } + + rule5 if { + my_floor(1.001, 2) + } + query: data.test.rule5 + want_result: "#undefined"