fix: Handle builtin out-parameter calls in RVM compiler

- 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 <anakrish@microsoft.com>
This commit is contained in:
Anand Krishnamoorthi
2025-12-02 13:29:12 -06:00
parent 5aefd51cb6
commit 30bd134a0b
5 changed files with 334 additions and 37 deletions

View File

@@ -37,12 +37,13 @@ pub fn create_loop_index_binding_plan<T: VariableBindingContext>(
pub fn create_parameter_binding_plan<T: VariableBindingContext>( pub fn create_parameter_binding_plan<T: VariableBindingContext>(
param_expr: &ExprRef, param_expr: &ExprRef,
context: &T, context: &T,
scoping: ScopingMode,
) -> Result<BindingPlan> { ) -> Result<BindingPlan> {
let mut newly_bound = BTreeSet::new(); let mut newly_bound = BTreeSet::new();
let destructuring_plan = create_destructuring_plan_with_tracking( let destructuring_plan = create_destructuring_plan_with_tracking(
param_expr, param_expr,
context, context,
ScopingMode::AllowShadowing, scoping,
&mut newly_bound, &mut newly_bound,
) )
.ok_or_else(|| BindingPlannerError::FailedToCreateDestructuringPlan { .ok_or_else(|| BindingPlannerError::FailedToCreateDestructuringPlan {
@@ -50,7 +51,9 @@ pub fn create_parameter_binding_plan<T: VariableBindingContext>(
span: param_expr.span().clone(), 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 { Ok(BindingPlan::Parameter {
param_expr: param_expr.clone(), param_expr: param_expr.clone(),

View File

@@ -381,7 +381,9 @@ impl LoopHoister {
for param in args { for param in args {
// Create binding plan for function parameter // Create binding plan for function parameter
match super::destructuring_planner::create_parameter_binding_plan( match super::destructuring_planner::create_parameter_binding_plan(
param, &context, param,
&context,
ScopingMode::AllowShadowing,
) { ) {
Ok(binding_plan) => { Ok(binding_plan) => {
let expr_idx = param.as_ref().eidx(); 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 the last parameter expression contains unbound vars, create a binding plan
if let Some(last_param) = params.last() { if let Some(last_param) = params.last() {
match super::destructuring_planner::create_parameter_binding_plan( match super::destructuring_planner::create_parameter_binding_plan(
last_param, context, last_param,
context,
ScopingMode::RespectParent,
) { ) {
Ok(binding_plan) => { Ok(binding_plan) => {
let expr_idx = last_param.as_ref().eidx(); let expr_idx = last_param.as_ref().eidx();

View File

@@ -3,11 +3,24 @@
use super::{Compiler, CompilerError, Register, Result}; use super::{Compiler, CompilerError, Register, Result};
use crate::ast::ExprRef; use crate::ast::ExprRef;
use crate::builtins;
use crate::compiler::destructuring_planner::plans::BindingPlan;
use crate::lexer::Span; use crate::lexer::Span;
use crate::rvm::instructions::{BuiltinCallParams, FunctionCallParams}; use crate::rvm::instructions::{BuiltinCallParams, FunctionCallParams};
use crate::rvm::Instruction; use crate::rvm::Instruction;
use crate::utils::get_path_string; 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<usize>,
},
Builtin {
builtin_index: u16,
expected_args: Option<usize>,
},
}
impl<'a> Compiler<'a> { impl<'a> Compiler<'a> {
pub(super) fn compile_function_call( pub(super) fn compile_function_call(
@@ -27,51 +40,138 @@ impl<'a> Compiler<'a> {
.map_err(|_| CompilerError::InvalidFunctionExpressionWithPackage.at(&span))? .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(); 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)?; let param_reg = self.compile_rego_expr_with_span(param, param.span(), false)?;
arg_regs.push(param_reg); arg_regs.push(param_reg);
} }
let dest = self.alloc_register(); let dest = self.alloc_register();
if self.is_user_defined_function(&full_fcn_path) { match call_target {
let rule_index = self.get_or_assign_rule_index(&full_fcn_path)?; CallTarget::User { rule_index, .. } => {
let mut args_array = [0u8; 8]; let mut args_array = [0u8; 8];
let num_args = arg_regs.len().min(8) as u8; let num_args = arg_regs.len().min(8) as u8;
for (i, &reg) in arg_regs.iter().take(8).enumerate() { for (i, &reg) in arg_regs.iter().take(8).enumerate() {
args_array[i] = reg; args_array[i] = reg;
} }
let params_index = self.program.add_function_call_params(FunctionCallParams { let params_index = self.program.add_function_call_params(FunctionCallParams {
func_rule_index: rule_index, func_rule_index: rule_index,
dest, dest,
num_args, num_args,
args: args_array, args: args_array,
}); });
self.emit_instruction(Instruction::FunctionCall { params_index }, &span); 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, &reg) in arg_regs.iter().take(8).enumerate() {
args_array[i] = reg;
} }
CallTarget::Builtin { builtin_index, .. } => {
let mut args_array = [0u8; 8];
let num_args = arg_regs.len().min(8) as u8;
for (i, &reg) in arg_regs.iter().take(8).enumerate() {
args_array[i] = reg;
}
let params_index = self.program.add_builtin_call_params(BuiltinCallParams { let params_index = self.program.add_builtin_call_params(BuiltinCallParams {
dest, dest,
builtin_index, builtin_index,
num_args, num_args,
args: args_array, args: args_array,
}); });
self.emit_instruction(Instruction::BuiltinCall { params_index }, &span); self.emit_instruction(Instruction::BuiltinCall { params_index }, &span);
} else {
return Err(CompilerError::UnknownFunction {
name: original_fcn_path,
} }
.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) Ok(dest)
} }
fn lookup_builtin_arity(&self, name: &str) -> Option<usize> {
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<CallTarget> {
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))
}
}
} }

View File

@@ -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"

View File

@@ -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"