From 126cc12eb5b6f96266ef4e68296773120ca1d406 Mon Sep 17 00:00:00 2001 From: Anand Krishnamoorthi <35780660+anakrish@users.noreply.github.com> Date: Wed, 1 Apr 2026 05:34:33 -0500 Subject: [PATCH] refactor: consolidate RVM instruction variants and clean up VM internals (#651) Merge the three separate Assert* instructions (AssertNot, AssertCondition, AssertNotUndefined) into a single `Guard { register, mode }` instruction with a GuardMode enum. This cuts duplicated match arms across display, listing, parser, dispatch, and all compiler emit sites. Drop the unnecessary `#[repr(C)]` from the Instruction enum. It was never exposed across FFI, so the C-compatible 4-byte discriminant was pure waste. Without it Rust picks a 1-byte discriminant, shrinking every instruction from 8 bytes to 6. A new `instruction_size` unit test locks this at 6. While touching these files, also clean up several long-standing issues: - Deduplicate the iteration-state setup in loops.rs by extracting a shared resolve_iteration_state() helper -- the stack-based and stackless paths had near-identical 40-line blocks. - Collapse the ExitWithSuccess / ExitWithFailure match arms into one. - In rules.rs, stop cloning Arc just to borrow a RuleInfo -- clone the small RuleInfo struct directly and extract a get_rule_info() helper. - Move the memory check into dispatch (runs per instruction) and remove the now-dead enforce_memory_check() entry-point calls. - Apply map_or_else style throughout listing.rs for consistency. --- src/languages/rego/compiler/destructuring.rs | 110 ++-- src/languages/rego/compiler/expressions.rs | 11 +- src/languages/rego/compiler/queries.rs | 18 +- src/languages/rego/compiler/rules.rs | 2 - src/rvm/instructions/display.rs | 61 +- src/rvm/instructions/mod.rs | 43 +- src/rvm/instructions/types.rs | 14 +- src/rvm/program/listing.rs | 602 ++++++++++--------- src/rvm/tests/instruction_parser.rs | 32 +- src/rvm/vm/arithmetic.rs | 22 +- src/rvm/vm/comprehension.rs | 4 +- src/rvm/vm/dispatch.rs | 46 +- src/rvm/vm/execution.rs | 10 +- src/rvm/vm/loops.rs | 185 +++--- src/rvm/vm/machine.rs | 10 - src/rvm/vm/rules.rs | 71 ++- tests/rvm/compiler.rs | 67 ++- 17 files changed, 699 insertions(+), 609 deletions(-) diff --git a/src/languages/rego/compiler/destructuring.rs b/src/languages/rego/compiler/destructuring.rs index 69cc298..b23779e 100644 --- a/src/languages/rego/compiler/destructuring.rs +++ b/src/languages/rego/compiler/destructuring.rs @@ -7,7 +7,7 @@ use crate::compiler::destructuring_planner::plans::{ AssignmentPlan, BindingPlan, DestructuringPlan, WildcardSide, }; use crate::lexer::Span; -use crate::rvm::instructions::Instruction; +use crate::rvm::instructions::{GuardMode, Instruction}; use crate::value::Value; use anyhow::{bail, Result}; @@ -91,19 +91,6 @@ impl<'a> Compiler<'a> { AssignmentPlan::EqualityCheck { lhs_expr, rhs_expr } => { let lhs_reg = self.compile_rego_expr_with_span(lhs_expr, lhs_expr.span(), false)?; let rhs_reg = self.compile_rego_expr_with_span(rhs_expr, rhs_expr.span(), false)?; - if !self.soft_assert_mode { - // AssertEq handles the equality assertion inline; the returned - // register is not used as a boolean by the caller — it is the - // expression's "result register" for potential downstream use. - self.emit_instruction( - Instruction::AssertEq { - left: lhs_reg, - right: rhs_reg, - }, - span, - ); - return Ok(lhs_reg); - } let dest = self.alloc_register(); self.emit_instruction( Instruction::Eq { @@ -113,6 +100,15 @@ impl<'a> Compiler<'a> { }, span, ); + if !self.soft_assert_mode { + self.emit_instruction( + Instruction::Guard { + register: dest, + mode: GuardMode::Condition, + }, + span, + ); + } Ok(dest) } AssignmentPlan::WildcardMatch { @@ -125,7 +121,10 @@ impl<'a> Compiler<'a> { let rhs_reg = self.compile_rego_expr_with_span(rhs_expr, rhs_expr.span(), false)?; self.emit_instruction( - Instruction::AssertNotUndefined { register: rhs_reg }, + Instruction::Guard { + register: rhs_reg, + mode: GuardMode::NotUndefined, + }, span, ); Ok(self.load_bool_literal(true, span)) @@ -134,7 +133,10 @@ impl<'a> Compiler<'a> { let lhs_reg = self.compile_rego_expr_with_span(lhs_expr, lhs_expr.span(), false)?; self.emit_instruction( - Instruction::AssertNotUndefined { register: lhs_reg }, + Instruction::Guard { + register: lhs_reg, + mode: GuardMode::NotUndefined, + }, span, ); Ok(self.load_bool_literal(true, span)) @@ -206,44 +208,44 @@ impl<'a> Compiler<'a> { DestructuringPlan::EqualityExpr(expected_expr) => { let expected_reg = self.compile_rego_expr_with_span(expected_expr, expected_expr.span(), false)?; + let cmp_reg = self.alloc_register(); + self.emit_instruction( + Instruction::Eq { + dest: cmp_reg, + left: value_register, + right: expected_reg, + }, + span, + ); if self.soft_assert_mode { - let cmp_reg = self.alloc_register(); - self.emit_instruction( - Instruction::Eq { - dest: cmp_reg, - left: value_register, - right: expected_reg, - }, - span, - ); return Ok(Some(cmp_reg)); } self.emit_instruction( - Instruction::AssertEq { - left: value_register, - right: expected_reg, + Instruction::Guard { + register: cmp_reg, + mode: GuardMode::Condition, }, span, ); } DestructuringPlan::EqualityValue(expected_value) => { let expected_reg = self.load_literal_value(expected_value, span); + let cmp_reg = self.alloc_register(); + self.emit_instruction( + Instruction::Eq { + dest: cmp_reg, + left: value_register, + right: expected_reg, + }, + span, + ); if self.soft_assert_mode { - let cmp_reg = self.alloc_register(); - self.emit_instruction( - Instruction::Eq { - dest: cmp_reg, - left: value_register, - right: expected_reg, - }, - span, - ); return Ok(Some(cmp_reg)); } self.emit_instruction( - Instruction::AssertEq { - left: value_register, - right: expected_reg, + Instruction::Guard { + register: cmp_reg, + mode: GuardMode::Condition, }, span, ); @@ -263,8 +265,9 @@ impl<'a> Compiler<'a> { ); if context.require_defined_values() { self.emit_instruction( - Instruction::AssertNotUndefined { + Instruction::Guard { register: element_reg, + mode: GuardMode::NotUndefined, }, span, ); @@ -289,8 +292,9 @@ impl<'a> Compiler<'a> { span, ); self.emit_instruction( - Instruction::AssertNotUndefined { + Instruction::Guard { register: field_reg, + mode: GuardMode::NotUndefined, }, span, ); @@ -310,8 +314,9 @@ impl<'a> Compiler<'a> { span, ); self.emit_instruction( - Instruction::AssertNotUndefined { + Instruction::Guard { register: field_reg, + mode: GuardMode::NotUndefined, }, span, ); @@ -349,7 +354,13 @@ impl<'a> Compiler<'a> { self.add_variable(var_name, dest); if context.require_defined_values() { - self.emit_instruction(Instruction::AssertNotUndefined { register: dest }, span); + self.emit_instruction( + Instruction::Guard { + register: dest, + mode: GuardMode::NotUndefined, + }, + span, + ); } Ok(()) @@ -393,13 +404,22 @@ impl<'a> Compiler<'a> { span, ); + let cmp_reg = self.alloc_register(); self.emit_instruction( - Instruction::AssertEq { + Instruction::Eq { + dest: cmp_reg, left: actual_len_reg, right: expected_len_reg, }, span, ); + self.emit_instruction( + Instruction::Guard { + register: cmp_reg, + mode: GuardMode::Condition, + }, + span, + ); Ok(()) } } diff --git a/src/languages/rego/compiler/expressions.rs b/src/languages/rego/compiler/expressions.rs index ff184a6..9727012 100644 --- a/src/languages/rego/compiler/expressions.rs +++ b/src/languages/rego/compiler/expressions.rs @@ -11,6 +11,7 @@ use super::{Compiler, CompilerError, Register, Result}; use crate::ast::{Expr, ExprRef}; use crate::compiler::destructuring_planner::plans::BindingPlan; use crate::lexer::Span; +use crate::rvm::instructions::GuardMode; use crate::rvm::Instruction; use crate::Value; use alloc::{format, string::ToString}; @@ -32,8 +33,9 @@ impl<'a> Compiler<'a> { let result_reg = reg; if assert_condition { self.emit_instruction( - Instruction::AssertCondition { - condition: result_reg, + Instruction::Guard { + register: result_reg, + mode: GuardMode::Condition, }, span, ); @@ -113,8 +115,9 @@ impl<'a> Compiler<'a> { if assert_condition { self.emit_instruction( - Instruction::AssertCondition { - condition: result_reg, + Instruction::Guard { + register: result_reg, + mode: GuardMode::Condition, }, span, ); diff --git a/src/languages/rego/compiler/queries.rs b/src/languages/rego/compiler/queries.rs index 4043a84..f25c855 100644 --- a/src/languages/rego/compiler/queries.rs +++ b/src/languages/rego/compiler/queries.rs @@ -8,6 +8,7 @@ use super::{Compiler, CompilerError, ComprehensionType, ContextType, Result}; use crate::ast::{self, LiteralStmt, Query}; +use crate::rvm::instructions::GuardMode; use crate::rvm::program::RuleType; use crate::rvm::Instruction; use alloc::format; @@ -242,7 +243,22 @@ impl<'a> Compiler<'a> { compiler.compile_rego_expr_with_span(expr, expr.span(), false) })?; - self.emit_instruction(Instruction::AssertNot { operand: expr_reg }, &stmt.span); + let negated_reg = self.alloc_register(); + self.emit_instruction( + Instruction::Not { + dest: negated_reg, + operand: expr_reg, + }, + &stmt.span, + ); + + self.emit_instruction( + Instruction::Guard { + register: negated_reg, + mode: GuardMode::Condition, + }, + &stmt.span, + ); } } Ok(()) diff --git a/src/languages/rego/compiler/rules.rs b/src/languages/rego/compiler/rules.rs index 6fa12f9..7227700 100644 --- a/src/languages/rego/compiler/rules.rs +++ b/src/languages/rego/compiler/rules.rs @@ -543,14 +543,12 @@ impl<'a> Compiler<'a> { // A definition has a known static value if every body (including // else-branches) would produce the same literal. let def_static_value = if bodies.is_empty() { - // No bodies — value comes from the head's value_expr. let head_value = self .context_stack .last() .and_then(|ctx| ctx.value_expr.clone()); Self::static_value_of_expr(&head_value) } else { - // Replay the same value_expr resolution as the body loop. let head_value = self .context_stack .last() diff --git a/src/rvm/instructions/display.rs b/src/rvm/instructions/display.rs index 73ee97e..8454f8d 100644 --- a/src/rvm/instructions/display.rs +++ b/src/rvm/instructions/display.rs @@ -5,6 +5,7 @@ use alloc::format; use alloc::string::String; use alloc::vec::Vec; +use super::types::GuardMode; use super::{Instruction, InstructionData, LiteralOrRegister}; impl Instruction { @@ -65,32 +66,25 @@ impl Instruction { ) }, ), - Instruction::ObjectCreate { params_index } => { - instruction_data - .get_object_create_params(params_index) - .map_or_else( - || format!("OBJECT_CREATE P({}) [INVALID INDEX]", params_index), - |params| { - let mut field_parts = Vec::new(); - - // Add literal key fields - for &(literal_idx, value_reg) in params.literal_key_field_pairs() { - field_parts.push(format!("L({}):R({})", literal_idx, value_reg)); - } - - // Add non-literal key fields - for &(key_reg, value_reg) in params.field_pairs() { - field_parts.push(format!("R({}):R({})", key_reg, value_reg)); - } - - let fields_str = field_parts.join(" "); - format!( - "OBJECT_CREATE R({}) L({}) [{}]", - params.dest, params.template_literal_idx, fields_str - ) - }, - ) - } + Instruction::ObjectCreate { params_index } => instruction_data + .get_object_create_params(params_index) + .map_or_else( + || format!("OBJECT_CREATE P({}) [INVALID INDEX]", params_index), + |params| { + let mut field_parts = Vec::new(); + for &(literal_idx, value_reg) in params.literal_key_field_pairs() { + field_parts.push(format!("L({}):R({})", literal_idx, value_reg)); + } + for &(key_reg, value_reg) in params.field_pairs() { + field_parts.push(format!("R({}):R({})", key_reg, value_reg)); + } + let fields_str = field_parts.join(" "); + format!( + "OBJECT_CREATE R({}) L({}) [{}]", + params.dest, params.template_literal_idx, fields_str + ) + }, + ), Instruction::VirtualDataDocumentLookup { params_index } => instruction_data .get_virtual_data_document_lookup_params(params_index) .map_or_else( @@ -245,14 +239,13 @@ impl core::fmt::Display for Instruction { Instruction::AssertEq { left, right } => { format!("ASSERT_EQ R({}) R({})", left, right) } - Instruction::AssertNot { operand } => { - format!("ASSERT_NOT R({})", operand) - } - Instruction::AssertCondition { condition } => { - format!("ASSERT_CONDITION R({})", condition) - } - Instruction::AssertNotUndefined { register } => { - format!("ASSERT_NOT_UNDEFINED R({})", register) + Instruction::Guard { register, mode } => { + let name = match mode { + GuardMode::Not => "ASSERT_NOT", + GuardMode::Condition => "ASSERT_CONDITION", + GuardMode::NotUndefined => "ASSERT_NOT_UNDEFINED", + }; + format!("{} R({})", name, register) } Instruction::LoopStart { params_index } => { format!("LOOP_START P({})", params_index) diff --git a/src/rvm/instructions/mod.rs b/src/rvm/instructions/mod.rs index ea8af94..80e43a2 100644 --- a/src/rvm/instructions/mod.rs +++ b/src/rvm/instructions/mod.rs @@ -10,12 +10,11 @@ pub use params::{ FunctionCallParams, InstructionData, LoopStartParams, ObjectCreateParams, SetCreateParams, VirtualDataDocumentLookupParams, }; -pub use types::{ComprehensionMode, LiteralOrRegister, LoopMode}; +pub use types::{ComprehensionMode, GuardMode, LiteralOrRegister, LoopMode}; use serde::{Deserialize, Serialize}; /// RVM Instructions - simplified enum-based design -#[repr(C)] #[derive(Debug, Clone, Copy, Serialize, Deserialize)] pub enum Instruction { /// Load literal value from literal table into register @@ -131,8 +130,6 @@ pub enum Instruction { left: u8, right: u8, }, - /// Rego negation - produces `true` if operand is `false` or undefined, - /// `false` for any other defined value (including non-booleans). Not { dest: u8, operand: u8, @@ -251,19 +248,10 @@ pub enum Instruction { right: u8, }, - /// Assert negation - succeed if operand is false or undefined, fail if true - AssertNot { - operand: u8, - }, - - /// Assert condition - if register contains false or undefined, return undefined immediately - AssertCondition { - condition: u8, - }, - - /// Assert not undefined - if register contains undefined, return undefined immediately - AssertNotUndefined { + /// Consolidated guard instruction — replaces AssertNot, AssertCondition, AssertNotUndefined. + Guard { register: u8, + mode: GuardMode, }, /// Start a loop over a collection with specified semantics - uses parameter table @@ -392,3 +380,26 @@ impl Instruction { Self::ComprehensionEnd {} } } + +#[cfg(test)] +mod tests { + use super::*; + use core::mem::size_of; + + #[test] + fn instruction_size() { + // Lock the instruction size to detect unintended growth. + // Without repr(C), Rust picks a 1-byte discriminant (< 256 variants) + // plus 4 bytes for the largest payload variant + 1 byte alignment + // padding for u16 fields = 6 bytes total. + // + // TODO: Reduce to 4 bytes by making LoopNext zero-payload (both fields + // are redundant with LoopStart params) and moving IndexLiteral to a + // params table. + let size = size_of::(); + assert_eq!( + size, 6, + "Instruction size changed from 6 to {size} — review new variants for bloat" + ); + } +} diff --git a/src/rvm/instructions/types.rs b/src/rvm/instructions/types.rs index 04629f1..ccff662 100644 --- a/src/rvm/instructions/types.rs +++ b/src/rvm/instructions/types.rs @@ -15,7 +15,7 @@ pub enum LiteralOrRegister { /// Loop execution modes for different Rego iteration constructs #[repr(C)] -#[derive(Debug, Clone, Copy, Serialize, Deserialize)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] pub enum LoopMode { /// Any quantification: some x in arr, x := arr[_], etc. /// Succeeds if ANY iteration succeeds, exits early on first success @@ -45,3 +45,15 @@ pub enum ComprehensionMode { /// Collects successful key-value pairs into an object Object, } + +/// Guard sub-modes for the consolidated `Guard` instruction. +#[repr(u8)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +pub enum GuardMode { + /// Assert negation — succeed if operand is false/undefined, fail if true. + Not, + /// Assert condition — fail (return undefined) if register is false/undefined. + Condition, + /// Assert not undefined — fail (return undefined) if register is undefined. + NotUndefined, +} diff --git a/src/rvm/program/listing.rs b/src/rvm/program/listing.rs index a79d7f6..56c94f2 100644 --- a/src/rvm/program/listing.rs +++ b/src/rvm/program/listing.rs @@ -1,6 +1,5 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. -#![allow(clippy::option_if_let_else)] use alloc::format; use alloc::string::{String, ToString as _}; @@ -106,6 +105,36 @@ pub fn generate_assembly_listing(program: &Program, config: &AssemblyListingConf } } + // Add metadata section + { + push_line(&mut output, format_args!(";")); + push_line(&mut output, format_args!("; METADATA:")); + push_line( + &mut output, + format_args!( + "; compiler_version: {}", + program.metadata.compiler_version + ), + ); + push_line( + &mut output, + format_args!("; compiled_at: {}", program.metadata.compiled_at), + ); + if !program.metadata.source_info.is_empty() { + push_line( + &mut output, + format_args!("; source_info: {}", program.metadata.source_info), + ); + } + push_line( + &mut output, + format_args!( + "; optimization_level: {}", + program.metadata.optimization_level + ), + ); + } + push_line(&mut output, format_args!(";")); for (pc, instruction) in program.instructions.iter().enumerate() { @@ -230,14 +259,14 @@ fn format_instruction_readable( match *instruction { Instruction::Load { dest, literal_idx } => { let base = format!("{}Load r{} ← L{}", indent, dest, literal_idx); - let comment = match program.literals.get(usize::from(literal_idx)) { - Some(literal) => { + let comment = program.literals.get(usize::from(literal_idx)).map_or_else( + || "Load literal: ".to_string(), + |literal| { let literal_json = serde_json::to_string(literal).unwrap_or_else(|_| "".to_string()); format!("Load literal: {}", literal_json) - } - None => "Load literal: ".to_string(), - }; + }, + ); align_comment(&base, &comment, config.comment_column) } Instruction::LoadTrue { dest } => { @@ -358,78 +387,81 @@ fn format_instruction_readable( } Instruction::Not { dest, operand } => { let base = format!("{}Not r{} ← ¬r{}", indent, dest, operand); - let comment = format!( - "Rego negation: true if r{} is false/undefined, false otherwise", - operand - ); + let comment = format!("Logical NOT: !r{}", operand); align_comment(&base, &comment, config.comment_column) } - Instruction::BuiltinCall { params_index } => { - if let Some(params) = instruction_data.get_builtin_call_params(params_index) { - let args_str = params - .arg_registers() - .iter() - .map(|&r| format!("r{}", r)) - .collect::>() - .join(", "); + Instruction::BuiltinCall { params_index } => instruction_data + .get_builtin_call_params(params_index) + .map_or_else( + || { + let base = format!("{}BuiltinCall [INVALID P({})]", indent, params_index); + align_comment( + &base, + "ERROR: Invalid builtin call parameters", + config.comment_column, + ) + }, + |params| { + let args_str = params + .arg_registers() + .iter() + .map(|&r| format!("r{}", r)) + .collect::>() + .join(", "); - let builtin_name = program - .builtin_info_table - .get(usize::from(params.builtin_index)) - .map(|info| info.name.as_str()) - .unwrap_or(""); + let builtin_name = program + .builtin_info_table + .get(usize::from(params.builtin_index)) + .map(|info| info.name.as_str()) + .unwrap_or(""); - let base = format!( - "{}BuiltinCall r{} ← {}({})", - indent, params.dest, builtin_name, args_str - ); - let comment = format!( - "Call builtin '{}' (B{}) with {} args", - builtin_name, params.builtin_index, params.num_args - ); - align_comment(&base, &comment, config.comment_column) - } else { - let base = format!("{}BuiltinCall [INVALID P({})]", indent, params_index); - align_comment( - &base, - "ERROR: Invalid builtin call parameters", - config.comment_column, - ) - } - } - Instruction::FunctionCall { params_index } => { - if let Some(params) = instruction_data.get_function_call_params(params_index) { - let args_str = params - .arg_registers() - .iter() - .map(|&r| format!("r{}", r)) - .collect::>() - .join(", "); + let base = format!( + "{}BuiltinCall r{} ← {}({})", + indent, params.dest, builtin_name, args_str + ); + let comment = format!( + "Call builtin '{}' (B{}) with {} args", + builtin_name, params.builtin_index, params.num_args + ); + align_comment(&base, &comment, config.comment_column) + }, + ), + Instruction::FunctionCall { params_index } => instruction_data + .get_function_call_params(params_index) + .map_or_else( + || { + let base = format!("{}FunctionCall [INVALID P({})]", indent, params_index); + align_comment( + &base, + "ERROR: Invalid function call parameters", + config.comment_column, + ) + }, + |params| { + let args_str = params + .arg_registers() + .iter() + .map(|&r| format!("r{}", r)) + .collect::>() + .join(", "); - let func_name = program - .rule_infos - .get(usize::from(params.func_rule_index)) - .map(|info| info.name.as_str()) - .unwrap_or(""); + let func_name = program + .rule_infos + .get(usize::from(params.func_rule_index)) + .map(|info| info.name.as_str()) + .unwrap_or(""); - let base = format!( - "{}FunctionCall r{} ← {}({})", - indent, params.dest, func_name, args_str - ); - let comment = format!( - "Call function '{}' (R{}) with {} args", - func_name, params.func_rule_index, params.num_args - ); - align_comment(&base, &comment, config.comment_column) - } else { - let base = format!("{}FunctionCall [INVALID P({})]", indent, params_index); - align_comment( - &base, - "ERROR: Invalid function call parameters", - config.comment_column, - ) - } - } + let base = format!( + "{}FunctionCall r{} ← {}({})", + indent, params.dest, func_name, args_str + ); + let comment = format!( + "Call function '{}' (R{}) with {} args", + func_name, params.func_rule_index, params.num_args + ); + align_comment(&base, &comment, config.comment_column) + }, + ), Instruction::HostAwait { dest, arg, id } => { let base = format!( "{}HostAwait r{} ← await r{} (id r{})", @@ -463,14 +495,16 @@ fn format_instruction_readable( indent, params.map_or(0, |p| p.dest) ); - let comment = match params { - Some(p) => format!( - "Create object with {} fields (P{})", - p.field_count(), - params_index - ), - None => format!("Create object (P{} - INVALID)", params_index), - }; + let comment = params.map_or_else( + || format!("Create object (P{} - INVALID)", params_index), + |p| { + format!( + "Create object with {} fields (P{})", + p.field_count(), + params_index, + ) + }, + ); align_comment(&base, &comment, config.comment_column) } Instruction::Index { @@ -494,17 +528,19 @@ fn format_instruction_readable( "{}IndexLiteral r{} ← r{}[L{}]", indent, dest, container, literal_idx ); - let comment = match program.literals.get(usize::from(literal_idx)) { - Some(literal) => { + let comment = program.literals.get(usize::from(literal_idx)).map_or_else( + || { + format!( + "Index with literal: r{}[L{}] (invalid index)", + container, literal_idx, + ) + }, + |literal| { let literal_json = serde_json::to_string(literal).unwrap_or_else(|_| "".to_string()); format!("Index with literal key: r{}[{}]", container, literal_json) - } - None => format!( - "Index with literal: r{}[L{}] (invalid index)", - container, literal_idx - ), - }; + }, + ); align_comment(&base, &comment, config.comment_column) } Instruction::ArrayNew { dest } => { @@ -516,24 +552,25 @@ fn format_instruction_readable( let comment = format!("Append r{} to array r{}", value, arr); align_comment(&base, &comment, config.comment_column) } - Instruction::ArrayCreate { params_index } => { - if let Some(params) = instruction_data.get_array_create_params(params_index) { - let elements = params - .element_registers() - .iter() - .map(|r| format!("r{}", r)) - .collect::>() - .join(", "); - let base = format!("{}ArrayCreate r{} ← [{}]", indent, params.dest, elements); - let comment = format!( - "Create array from {} elements (undefined if any element is undefined)", - params.element_count() - ); - align_comment(&base, &comment, config.comment_column) - } else { - format!("{}ArrayCreate ", indent, params_index) - } - } + Instruction::ArrayCreate { params_index } => instruction_data + .get_array_create_params(params_index) + .map_or_else( + || format!("{}ArrayCreate ", indent, params_index), + |params| { + let elements = params + .element_registers() + .iter() + .map(|r| format!("r{}", r)) + .collect::>() + .join(", "); + let base = format!("{}ArrayCreate r{} ← [{}]", indent, params.dest, elements); + let comment = format!( + "Create array from {} elements (undefined if any element is undefined)", + params.element_count() + ); + align_comment(&base, &comment, config.comment_column) + }, + ), Instruction::SetNew { dest } => { let base = format!("{}SetNew r{} ← set()", indent, dest); align_comment(&base, "Create new empty set", config.comment_column) @@ -543,24 +580,26 @@ fn format_instruction_readable( let comment = format!("Add r{} to set r{}", value, set); align_comment(&base, &comment, config.comment_column) } - Instruction::SetCreate { params_index } => { - if let Some(params) = instruction_data.get_set_create_params(params_index) { - let elements = params - .element_registers() - .iter() - .map(|r| format!("r{}", r)) - .collect::>() - .join(", "); - let base = format!("{}SetCreate r{} ← {{{}}}", indent, params.dest, elements); - let comment = format!( - "Create set from {} elements (undefined if any element is undefined)", - params.element_count() - ); - align_comment(&base, &comment, config.comment_column) - } else { - format!("{}SetCreate ", indent, params_index) - } - } + Instruction::SetCreate { params_index } => instruction_data + .get_set_create_params(params_index) + .map_or_else( + || format!("{}SetCreate ", indent, params_index), + |params| { + let elements = params + .element_registers() + .iter() + .map(|r| format!("r{}", r)) + .collect::>() + .join(", "); + let base = + format!("{}SetCreate r{} ← {{{}}}", indent, params.dest, elements); + let comment = format!( + "Create set from {} elements (undefined if any element is undefined)", + params.element_count() + ); + align_comment(&base, &comment, config.comment_column) + }, + ), Instruction::Contains { dest, collection, @@ -581,61 +620,67 @@ fn format_instruction_readable( Instruction::AssertEq { left, right } => { let base = format!("{}AssertEq assert r{} == r{}", indent, left, right); let comment = format!( - "Assert r{} equals r{} (exit if unequal/undefined)", + "Assert r{} equals r{} (exit if either undefined or different)", left, right ); align_comment(&base, &comment, config.comment_column) } - Instruction::AssertNot { operand } => { - let base = format!("{}AssertNot assert !r{}", indent, operand); - let comment = format!( - "Assert r{} is false/undefined (exit if any defined truthy value)", - operand - ); - align_comment(&base, &comment, config.comment_column) - } - Instruction::AssertCondition { condition } => { - let base = format!("{}Assert assert r{}", indent, condition); - let comment = format!("Assert r{} is true (exit if false/undefined)", condition); - align_comment(&base, &comment, config.comment_column) - } - Instruction::AssertNotUndefined { register } => { - let base = format!( - "{}AssertNotUndefined assert_not_undefined r{}", - indent, register - ); - let comment = format!("Assert r{} is not undefined (exit if undefined)", register); - align_comment(&base, &comment, config.comment_column) + Instruction::Guard { register, mode } => { + let (keyword, comment) = match mode { + crate::rvm::instructions::GuardMode::Not => ( + format!("{}AssertNot assert !r{}", indent, register), + format!("Assert r{} is false/undefined (exit if true)", register), + ), + crate::rvm::instructions::GuardMode::Condition => ( + format!("{}Assert assert r{}", indent, register), + format!("Assert r{} is true (exit if false/undefined)", register), + ), + crate::rvm::instructions::GuardMode::NotUndefined => ( + format!( + "{}AssertNotUndefined assert_not_undefined r{}", + indent, register + ), + format!("Assert r{} is not undefined (exit if undefined)", register), + ), + }; + align_comment(&keyword, &comment, config.comment_column) } Instruction::LoopStart { params_index } => { - if let Some(params) = instruction_data.get_loop_params(params_index) { - let mode_str = match params.mode { - LoopMode::Any => "any", - LoopMode::Every => "every", - LoopMode::ForEach => "foreach", - }; - let base = format!( - "{}LoopStart {} r{},r{} in r{} → r{} {{", - indent, - mode_str, - params.key_reg, - params.value_reg, - params.collection, - params.result_reg - ); - let comment = format!( - "{} loop over r{}, body: {}-{} (P{})", - mode_str, params.collection, params.body_start, params.loop_end, params_index - ); - align_comment(&base, &comment, config.comment_column) - } else { - let base = format!("{}LoopStart [INVALID P({})] {{", indent, params_index); - align_comment( - &base, - "ERROR: Invalid loop parameters", - config.comment_column, - ) - } + instruction_data.get_loop_params(params_index).map_or_else( + || { + let base = format!("{}LoopStart [INVALID P({})] {{", indent, params_index); + align_comment( + &base, + "ERROR: Invalid loop parameters", + config.comment_column, + ) + }, + |params| { + let mode_str = match params.mode { + LoopMode::Any => "any", + LoopMode::Every => "every", + LoopMode::ForEach => "foreach", + }; + let base = format!( + "{}LoopStart {} r{},r{} in r{} → r{} {{", + indent, + mode_str, + params.key_reg, + params.value_reg, + params.collection, + params.result_reg + ); + let comment = format!( + "{} loop over r{}, body: {}-{} (P{})", + mode_str, + params.collection, + params.body_start, + params.loop_end, + params_index + ); + align_comment(&base, &comment, config.comment_column) + }, + ) } Instruction::LoopNext { body_start, @@ -684,52 +729,58 @@ fn format_instruction_readable( align_comment(&base, "End of rule evaluation", config.comment_column) } Instruction::ChainedIndex { params_index } => { - let (base, comment) = - if let Some(params) = instruction_data.get_chained_index_params(params_index) { - let chain_parts: Vec = params - .path_components - .iter() - .map(|component| match *component { - crate::rvm::instructions::LiteralOrRegister::Literal(idx) => { - if let Some(literal) = program.literals.get(usize::from(idx)) { - match *literal { - crate::Value::String(ref s) => format!(".{}", s.as_ref()), - ref other => format!( - "[{}]", - serde_json::to_string(other) - .unwrap_or_else(|_| "?".to_string()) - ), - } - } else { - format!("[L{}?]", idx) + let (base, comment) = instruction_data + .get_chained_index_params(params_index) + .map_or_else( + || { + let base_str = format!("{}ChainedIndex chained_index", indent); + let comment_str = + "Multi-level chained indexing (invalid params)".to_string(); + (base_str, comment_str) + }, + |params| { + let chain_parts: Vec = params + .path_components + .iter() + .map(|component| match *component { + crate::rvm::instructions::LiteralOrRegister::Literal(idx) => { + program.literals.get(usize::from(idx)).map_or_else( + || format!("[L{}?]", idx), + |literal| match *literal { + crate::Value::String(ref s) => { + format!(".{}", s.as_ref()) + } + ref other => format!( + "[{}]", + serde_json::to_string(other) + .unwrap_or_else(|_| "?".to_string()) + ), + }, + ) } - } - crate::rvm::instructions::LiteralOrRegister::Register(reg) => { - format!("[r{}]", reg) - } - }) - .collect(); + crate::rvm::instructions::LiteralOrRegister::Register(reg) => { + format!("[r{}]", reg) + } + }) + .collect(); - let chain_display = if chain_parts.is_empty() { - String::new() - } else { - format!(" r{}{}", params.root, chain_parts.join("")) - }; + let chain_display = if chain_parts.is_empty() { + String::new() + } else { + format!(" r{}{}", params.root, chain_parts.join("")) + }; - let base_str = format!( - "{}ChainedIndex r{} ← r{}{}", - indent, params.dest, params.root, chain_display - ); - let comment_str = format!( - "Multi-level chained indexing: r{} → r{}", - params.root, params.dest - ); - (base_str, comment_str) - } else { - let base_str = format!("{}ChainedIndex chained_index", indent); - let comment_str = "Multi-level chained indexing (invalid params)".to_string(); - (base_str, comment_str) - }; + let base_str = format!( + "{}ChainedIndex r{} ← r{}{}", + indent, params.dest, params.root, chain_display + ); + let comment_str = format!( + "Multi-level chained indexing: r{} → r{}", + params.root, params.dest + ); + (base_str, comment_str) + }, + ); align_comment(&base, &comment, config.comment_column) } @@ -756,51 +807,59 @@ fn format_instruction_readable( let base = format!("{}Halt halt", indent); align_comment(&base, "Stop execution", config.comment_column) } - Instruction::ComprehensionBegin { params_index } => { - if let Some(params) = instruction_data.get_comprehension_begin_params(params_index) { - let mode_str = match params.mode { - crate::rvm::instructions::ComprehensionMode::Array => "array", - crate::rvm::instructions::ComprehensionMode::Set => "set", - crate::rvm::instructions::ComprehensionMode::Object => "object", - }; - let (source_desc, result_desc) = if params.collection_reg == params.result_reg { - ( - format!("r{}", params.collection_reg), - format!("r{}", params.result_reg), + Instruction::ComprehensionBegin { params_index } => instruction_data + .get_comprehension_begin_params(params_index) + .map_or_else( + || { + let base = format!("{}CompBegin [INVALID P({})] {{", indent, params_index); + align_comment( + &base, + "ERROR: Invalid comprehension parameters", + config.comment_column, ) - } else { - ( - format!("r{} (src)", params.collection_reg), - format!("r{} (dst)", params.result_reg), - ) - }; - let base = format!( - "{}CompBegin {} {} → {} k:{} v:{} {{", - indent, mode_str, source_desc, result_desc, params.key_reg, params.value_reg - ); - let comment = format!( - "{} comprehension in r{}, body: {}-{} (P{})", - mode_str, - params.collection_reg, - params.body_start, - params.comprehension_end, - params_index - ); - align_comment(&base, &comment, config.comment_column) - } else { - let base = format!("{}CompBegin [INVALID P({})] {{", indent, params_index); - align_comment( - &base, - "ERROR: Invalid comprehension parameters", - config.comment_column, - ) - } - } + }, + |params| { + let mode_str = match params.mode { + crate::rvm::instructions::ComprehensionMode::Array => "array", + crate::rvm::instructions::ComprehensionMode::Set => "set", + crate::rvm::instructions::ComprehensionMode::Object => "object", + }; + let (source_desc, result_desc) = if params.collection_reg == params.result_reg { + ( + format!("r{}", params.collection_reg), + format!("r{}", params.result_reg), + ) + } else { + ( + format!("r{} (src)", params.collection_reg), + format!("r{} (dst)", params.result_reg), + ) + }; + let base = format!( + "{}CompBegin {} {} → {} k:{} v:{} {{", + indent, + mode_str, + source_desc, + result_desc, + params.key_reg, + params.value_reg + ); + let comment = format!( + "{} comprehension in r{}, body: {}-{} (P{})", + mode_str, + params.collection_reg, + params.body_start, + params.comprehension_end, + params_index + ); + align_comment(&base, &comment, config.comment_column) + }, + ), Instruction::ComprehensionYield { value_reg, key_reg } => { - let base = match key_reg { - Some(k) => format!("{}CompYield r{} r{}", indent, k, value_reg), - None => format!("{}CompYield r{}", indent, value_reg), - }; + let base = key_reg.map_or_else( + || format!("{}CompYield r{}", indent, value_reg), + |k| format!("{}CompYield r{} r{}", indent, k, value_reg), + ); align_comment(&base, "Yield value to comprehension", config.comment_column) } Instruction::ComprehensionEnd {} => { @@ -919,9 +978,11 @@ const fn get_instruction_name(instruction: &Instruction) -> &'static str { Instruction::Contains { .. } => "CONTAINS", Instruction::Count { .. } => "COUNT", Instruction::AssertEq { .. } => "ASSERT_EQ", - Instruction::AssertNot { .. } => "ASSERT_NOT", - Instruction::AssertCondition { .. } => "ASSERT", - Instruction::AssertNotUndefined { .. } => "ASSERT_NOT_UNDEF", + Instruction::Guard { mode, .. } => match mode { + crate::rvm::instructions::GuardMode::Not => "ASSERT_NOT", + crate::rvm::instructions::GuardMode::Condition => "ASSERT", + crate::rvm::instructions::GuardMode::NotUndefined => "ASSERT_NOT_UNDEF", + }, Instruction::LoopStart { .. } => "LOOP_START", Instruction::LoopNext { .. } => "LOOP_NEXT", Instruction::CallRule { .. } => "CALL_RULE", @@ -974,14 +1035,15 @@ fn format_operation_compact( format!("{}r{} ← r{}[L{}]", indent, dest, container, literal_idx) } Instruction::LoopStart { params_index } => { - if let Some(params) = instruction_data.get_loop_params(params_index) { - format!( - "{}loop r{} in r{} {{", - indent, params.value_reg, params.collection - ) - } else { - format!("{}loop P({}) {{", indent, params_index) - } + instruction_data.get_loop_params(params_index).map_or_else( + || format!("{}loop P({}) {{", indent, params_index), + |params| { + format!( + "{}loop r{} in r{} {{", + indent, params.value_reg, params.collection + ) + }, + ) } Instruction::LoopNext { .. } => { format!("{}}}", indent) diff --git a/src/rvm/tests/instruction_parser.rs b/src/rvm/tests/instruction_parser.rs index 788c5f1..f53cd57 100644 --- a/src/rvm/tests/instruction_parser.rs +++ b/src/rvm/tests/instruction_parser.rs @@ -9,7 +9,7 @@ clippy::pattern_type_mismatch )] // tests unwrap conversions and slice math for brevity -use crate::rvm::instructions::{Instruction, LoopMode}; +use crate::rvm::instructions::{GuardMode, Instruction, LoopMode}; use alloc::string::{String, ToString}; use alloc::vec::Vec; use anyhow::{anyhow, bail, Result}; @@ -63,8 +63,10 @@ pub fn parse_instruction(text: &str) -> Result { "SetAdd" => parse_set_add(params_text), "Contains" => parse_contains(params_text), "Count" => parse_count(params_text), - "AssertCondition" => parse_assert_condition(params_text), - "AssertNotUndefined" => parse_assert_not_undefined(params_text), + "AssertEq" => parse_assert_eq(params_text), + "AssertNot" => parse_guard(params_text, GuardMode::Not), + "AssertCondition" => parse_guard(params_text, GuardMode::Condition), + "AssertNotUndefined" => parse_guard(params_text, GuardMode::NotUndefined), "BuiltinCall" => parse_builtin_call(params_text), "FunctionCall" => parse_function_call(params_text), "CallRule" => parse_call_rule(params_text), @@ -464,20 +466,24 @@ fn parse_count(params_text: &str) -> Result { }) } -fn parse_assert_condition(params_text: &str) -> Result { +fn parse_assert_eq(params_text: &str) -> Result { let params = parse_params(params_text)?; - let condition = get_param_u16(¶ms, "condition")?; - Ok(Instruction::AssertCondition { - condition: condition.try_into().unwrap(), - }) + let left: u8 = get_param_u16(¶ms, "left")?.try_into().unwrap(); + let right: u8 = get_param_u16(¶ms, "right")?.try_into().unwrap(); + Ok(Instruction::AssertEq { left, right }) } -fn parse_assert_not_undefined(params_text: &str) -> Result { +fn parse_guard(params_text: &str, mode: GuardMode) -> Result { let params = parse_params(params_text)?; - let register = get_param_u16(¶ms, "register")?; - Ok(Instruction::AssertNotUndefined { - register: register.try_into().unwrap(), - }) + // Accept the original field name for each mode so YAML tests don't change. + let register: u8 = match mode { + GuardMode::Not => get_param_u16(¶ms, "operand")?, + GuardMode::Condition => get_param_u16(¶ms, "condition")?, + GuardMode::NotUndefined => get_param_u16(¶ms, "register")?, + } + .try_into() + .unwrap(); + Ok(Instruction::Guard { register, mode }) } fn parse_loop_start(params_text: &str) -> Result { diff --git a/src/rvm/vm/arithmetic.rs b/src/rvm/vm/arithmetic.rs index 557fb2b..3d40488 100644 --- a/src/rvm/vm/arithmetic.rs +++ b/src/rvm/vm/arithmetic.rs @@ -1,6 +1,10 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. -#![allow(clippy::pattern_type_mismatch)] + +// `pattern_type_mismatch` requires explicit `&`+`ref` patterns for tuple matches +// on (&Value, &Value), which conflicts with `needless_borrowed_reference`. +// Disable both to keep patterns consistent within this file. +#![allow(clippy::pattern_type_mismatch, clippy::needless_borrowed_reference)] use alloc::collections::BTreeSet; @@ -14,7 +18,7 @@ impl RegoVM { /// Add two values using interpreter's arithmetic logic pub(super) fn add_values(&self, a: &Value, b: &Value) -> Result { match (a, b) { - (Value::Number(x), Value::Number(y)) => Ok(Value::from(x.add(y)?)), + (&Value::Number(ref x), &Value::Number(ref y)) => Ok(Value::from(x.add(y)?)), _ => Err(VmError::InvalidAddition { left: a.clone(), right: b.clone(), @@ -26,8 +30,8 @@ impl RegoVM { /// Subtract two values using interpreter's arithmetic logic pub(super) fn sub_values(&self, a: &Value, b: &Value) -> Result { match (a, b) { - (Value::Number(x), Value::Number(y)) => Ok(Value::from(x.sub(y)?)), - (Value::Set(left), Value::Set(right)) => { + (&Value::Number(ref x), &Value::Number(ref y)) => Ok(Value::from(x.sub(y)?)), + (&Value::Set(ref left), &Value::Set(ref right)) => { let diff: BTreeSet = left.difference(right).cloned().collect(); Ok(Value::from_set(diff)) } @@ -42,7 +46,7 @@ impl RegoVM { /// Multiply two values using interpreter's arithmetic logic pub(super) fn mul_values(&self, a: &Value, b: &Value) -> Result { match (a, b) { - (Value::Number(x), Value::Number(y)) => Ok(Value::from(x.mul(y)?)), + (&Value::Number(ref x), &Value::Number(ref y)) => Ok(Value::from(x.mul(y)?)), _ => Err(VmError::InvalidMultiplication { left: a.clone(), right: b.clone(), @@ -54,7 +58,7 @@ impl RegoVM { /// Divide two values using interpreter's arithmetic logic pub(super) fn div_values(&self, a: &Value, b: &Value) -> Result { match (a, b) { - (Value::Number(x), Value::Number(y)) => { + (&Value::Number(ref x), &Value::Number(ref y)) => { if *y == Number::from(0_u64) { if self.strict_builtin_errors { return Err(VmError::InvalidDivision { @@ -79,7 +83,7 @@ impl RegoVM { /// Modulo two values using interpreter's arithmetic logic pub(super) fn mod_values(&self, a: &Value, b: &Value) -> Result { match (a, b) { - (Value::Number(x), Value::Number(y)) => { + (&Value::Number(ref x), &Value::Number(ref y)) => { if *y == Number::from(0_u64) { if self.strict_builtin_errors { return Err(VmError::InvalidModulo { @@ -110,8 +114,8 @@ impl RegoVM { } pub(super) const fn to_bool(&self, value: &Value) -> Option { - match value { - Value::Bool(b) => Some(*b), + match *value { + Value::Bool(b) => Some(b), Value::Null if !self.strict_builtin_errors => Some(true), _ => None, } diff --git a/src/rvm/vm/comprehension.rs b/src/rvm/vm/comprehension.rs index 9df9074..989d23d 100644 --- a/src/rvm/vm/comprehension.rs +++ b/src/rvm/vm/comprehension.rs @@ -435,6 +435,8 @@ impl RegoVM { } } + self.set_register(result_reg_idx, current_result)?; + let (iteration_state_snapshot, body_start, comprehension_end) = { let frame = self.execution_stack.get_mut(comprehension_index).ok_or( VmError::InvalidIteration { @@ -485,8 +487,6 @@ impl RegoVM { } }; - self.set_register(result_reg_idx, current_result)?; - if let Some(state) = iteration_state_snapshot.as_ref() { let has_next = self.setup_next_iteration(state, key_reg_idx, value_reg_idx)?; diff --git a/src/rvm/vm/dispatch.rs b/src/rvm/vm/dispatch.rs index 78d96c4..317a656 100644 --- a/src/rvm/vm/dispatch.rs +++ b/src/rvm/vm/dispatch.rs @@ -1,7 +1,7 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. -use crate::rvm::instructions::{Instruction, LiteralOrRegister}; +use crate::rvm::instructions::{GuardMode, Instruction, LiteralOrRegister}; use crate::rvm::program::Program; use crate::value::Value; use alloc::collections::BTreeSet; @@ -26,6 +26,7 @@ impl RegoVM { program: &Program, instruction: Instruction, ) -> Result { + self.memory_check()?; self.execute_load_and_move(program, instruction) } @@ -317,9 +318,6 @@ impl RegoVM { } Not { dest, operand } => { let operand_value = self.get_register(operand)?; - - // In Rego, `not expr` succeeds when `expr` is undefined or false, - // and fails for any other defined value (including non-booleans like 42). let negated = match *operand_value { Value::Undefined => true, Value::Bool(b) => !b, @@ -335,35 +333,24 @@ impl RegoVM { self.handle_condition(passed)?; Ok(InstructionOutcome::Continue) } - AssertNot { operand } => { - let value = self.get_register(operand)?; - let passed = match *value { - Value::Undefined => true, - Value::Bool(b) => !b, - _ => false, + Guard { register, mode } => { + let value = self.get_register(register)?; + let passed = match mode { + GuardMode::Not => match *value { + Value::Undefined => true, + Value::Bool(b) => !b, + _ => false, + }, + GuardMode::Condition => match *value { + Value::Bool(b) => b, + Value::Undefined => false, + _ => true, + }, + GuardMode::NotUndefined => !matches!(value, Value::Undefined), }; self.handle_condition(passed)?; Ok(InstructionOutcome::Continue) } - AssertCondition { condition } => { - let value = self.get_register(condition)?; - - let condition_result = match *value { - Value::Bool(b) => b, - Value::Undefined => false, - _ => true, - }; - - self.handle_condition(condition_result)?; - Ok(InstructionOutcome::Continue) - } - AssertNotUndefined { register } => { - let value = self.get_register(register)?; - - let is_undefined = matches!(value, Value::Undefined); - self.handle_condition(!is_undefined)?; - Ok(InstructionOutcome::Continue) - } other => self.execute_call_instruction(program, other), } } @@ -747,6 +734,7 @@ impl RegoVM { available: loop_params_len, })?; let mode = loop_params.mode; + let params = LoopParams { collection: loop_params.collection, key_reg: loop_params.key_reg, diff --git a/src/rvm/vm/execution.rs b/src/rvm/vm/execution.rs index 8b2a049..c4b6699 100644 --- a/src/rvm/vm/execution.rs +++ b/src/rvm/vm/execution.rs @@ -161,7 +161,6 @@ impl RegoVM { self.reset_execution_state(); self.reset_execution_timer_state(); self.execution_state = ExecutionState::Running; - self.enforce_memory_check()?; match self.jump_to(0_u32) { Ok(value) => { self.execution_state = ExecutionState::Completed { @@ -180,7 +179,6 @@ impl RegoVM { self.reset_execution_state(); self.reset_execution_timer_state(); self.execution_state = ExecutionState::Running; - self.enforce_memory_check()?; match self.run_stackless_from(0) { Ok(result) => Ok(result), Err(err) => { @@ -193,7 +191,6 @@ impl RegoVM { fn execute_suspendable_entry(&mut self, entry_point_pc: usize) -> Result { self.execution_state = ExecutionState::Running; self.reset_execution_timer_state(); - self.enforce_memory_check()?; match self.run_stackless_from(entry_point_pc) { Ok(result) => Ok(result), Err(err) => { @@ -204,18 +201,15 @@ impl RegoVM { } pub fn resume(&mut self, resume_value: Option) -> Result { - let old_state = core::mem::replace(&mut self.execution_state, ExecutionState::Running); - let (reason, mut last_result) = match old_state { + let (reason, mut last_result) = match self.execution_state.clone() { ExecutionState::Suspended { reason, last_result, .. } => (reason, last_result), current_state => { - let desc = alloc::format!("{:?}", current_state); - self.execution_state = current_state; return Err(VmError::InvalidResumeState { - state: desc, + state: alloc::format!("{:?}", current_state), pc: self.pc, }); } diff --git a/src/rvm/vm/loops.rs b/src/rvm/vm/loops.rs index 494759f..1984683 100644 --- a/src/rvm/vm/loops.rs +++ b/src/rvm/vm/loops.rs @@ -9,6 +9,16 @@ use super::errors::{Result, VmError}; use super::execution_model::{ExecutionFrame, ExecutionMode, FrameKind}; use super::machine::RegoVM; +/// Result for a loop over a non-iterable value (null, string, number, bool, Undefined). +/// `Every` over empty is vacuously `true`. +#[inline] +const fn non_collection_result(mode: &LoopMode) -> Value { + match *mode { + LoopMode::Every => Value::Bool(true), + LoopMode::Any | LoopMode::ForEach => Value::Bool(false), + } +} + fn compute_body_resume_pc(loop_start_pc: usize, body_start: u16) -> usize { if body_start == 0 { return 0; @@ -72,49 +82,11 @@ impl RegoVM { mode: &LoopMode, params: LoopParams, ) -> Result<()> { - let initial_result = match *mode { - LoopMode::Any | LoopMode::Every | LoopMode::ForEach => Value::Bool(false), - }; - self.set_register(params.result_reg, initial_result.clone())?; - let collection_value = self.get_register(params.collection)?.clone(); + self.set_register(params.result_reg, Value::Bool(false))?; - let iteration_state = match collection_value { - Value::Array(ref items) => { - if items.is_empty() { - self.handle_empty_collection(mode, params.result_reg, params.loop_end)?; - return Ok(()); - } - IterationState::Array { - items: items.clone(), - index: 0, - } - } - Value::Object(ref obj) => { - if obj.is_empty() { - self.handle_empty_collection(mode, params.result_reg, params.loop_end)?; - return Ok(()); - } - IterationState::Object { - obj: obj.clone(), - current_key: None, - first_iteration: true, - } - } - Value::Set(ref set) => { - if set.is_empty() { - self.handle_empty_collection(mode, params.result_reg, params.loop_end)?; - return Ok(()); - } - IterationState::Set { - items: set.clone(), - current_item: None, - first_iteration: true, - } - } - _ => { - self.handle_empty_collection(mode, params.result_reg, params.loop_end)?; - return Ok(()); - } + let iteration_state = match self.resolve_iteration_state(mode, ¶ms)? { + Some(state) => state, + None => return Ok(()), }; let has_next = @@ -169,13 +141,9 @@ impl RegoVM { let action = Self::determine_loop_action(&loop_ctx.mode, iteration_succeeded); match action { - LoopAction::ExitWithSuccess => { - self.set_register(loop_ctx.result_reg, Value::Bool(true))?; - self.pc = usize::from(loop_end_local.saturating_sub(1)); - return Ok(()); - } - LoopAction::ExitWithFailure => { - self.set_register(loop_ctx.result_reg, Value::Bool(false))?; + LoopAction::ExitWithSuccess | LoopAction::ExitWithFailure => { + let result_value = matches!(action, LoopAction::ExitWithSuccess); + self.set_register(loop_ctx.result_reg, Value::Bool(result_value))?; self.pc = usize::from(loop_end_local.saturating_sub(1)); return Ok(()); } @@ -236,50 +204,11 @@ impl RegoVM { mode: &LoopMode, params: LoopParams, ) -> Result<()> { - let initial_result = match *mode { - LoopMode::Any | LoopMode::Every | LoopMode::ForEach => Value::Bool(false), - }; - self.set_register(params.result_reg, initial_result.clone())?; + self.set_register(params.result_reg, Value::Bool(false))?; - let collection_value = self.get_register(params.collection)?.clone(); - - let iteration_state = match collection_value { - Value::Array(ref items) => { - if items.is_empty() { - self.handle_empty_collection(mode, params.result_reg, params.loop_end)?; - return Ok(()); - } - IterationState::Array { - items: items.clone(), - index: 0, - } - } - Value::Object(ref obj) => { - if obj.is_empty() { - self.handle_empty_collection(mode, params.result_reg, params.loop_end)?; - return Ok(()); - } - IterationState::Object { - obj: obj.clone(), - current_key: None, - first_iteration: true, - } - } - Value::Set(ref set) => { - if set.is_empty() { - self.handle_empty_collection(mode, params.result_reg, params.loop_end)?; - return Ok(()); - } - IterationState::Set { - items: set.clone(), - current_item: None, - first_iteration: true, - } - } - _ => { - self.handle_empty_collection(mode, params.result_reg, params.loop_end)?; - return Ok(()); - } + let iteration_state = match self.resolve_iteration_state(mode, ¶ms)? { + Some(state) => state, + None => return Ok(()), }; let has_next = @@ -367,21 +296,9 @@ impl RegoVM { let action = Self::determine_loop_action(&loop_mode, iteration_succeeded); match action { - LoopAction::ExitWithSuccess => { - self.set_register(result_reg, Value::Bool(true))?; - let completed_frame = self - .execution_stack - .pop() - .ok_or(VmError::AssertionFailed { pc: self.pc })?; - if let Some(parent) = self.execution_stack.last_mut() { - parent.pc = resume_pc; - self.frame_pc_overridden = true; - } - drop(completed_frame); - Ok(()) - } - LoopAction::ExitWithFailure => { - self.set_register(result_reg, Value::Bool(false))?; + LoopAction::ExitWithSuccess | LoopAction::ExitWithFailure => { + let result_value = matches!(action, LoopAction::ExitWithSuccess); + self.set_register(result_reg, Value::Bool(result_value))?; let completed_frame = self .execution_stack .pop() @@ -496,6 +413,60 @@ impl RegoVM { } } + /// Resolve the collection value into an `IterationState`, handling empty + /// collections and non-iterable values. + /// + /// Returns `Some(state)` when iteration should proceed, or `None` when the + /// loop was short-circuited (registers and PC already adjusted). + fn resolve_iteration_state( + &mut self, + mode: &LoopMode, + params: &LoopParams, + ) -> Result> { + let collection_value = self.get_register(params.collection)?.clone(); + + match collection_value { + Value::Array(ref items) => { + if items.is_empty() { + self.handle_empty_collection(mode, params.result_reg, params.loop_end)?; + return Ok(None); + } + Ok(Some(IterationState::Array { + items: items.clone(), + index: 0, + })) + } + Value::Object(ref obj) => { + if obj.is_empty() { + self.handle_empty_collection(mode, params.result_reg, params.loop_end)?; + return Ok(None); + } + Ok(Some(IterationState::Object { + obj: obj.clone(), + current_key: None, + first_iteration: true, + })) + } + Value::Set(ref set) => { + if set.is_empty() { + self.handle_empty_collection(mode, params.result_reg, params.loop_end)?; + return Ok(None); + } + Ok(Some(IterationState::Set { + items: set.clone(), + current_item: None, + first_iteration: true, + })) + } + _ => { + let result = non_collection_result(mode); + self.set_register(params.result_reg, result)?; + self.pc = usize::from(params.loop_end).saturating_sub(1); + Ok(None) + } + } + } + fn handle_empty_collection( &mut self, mode: &LoopMode, diff --git a/src/rvm/vm/machine.rs b/src/rvm/vm/machine.rs index 6baf6d2..f075289 100644 --- a/src/rvm/vm/machine.rs +++ b/src/rvm/vm/machine.rs @@ -524,21 +524,11 @@ impl RegoVM { limits::check_memory_limit_if_needed().map_err(|err| self.map_limit_error(err)) } - #[cfg(all(feature = "allocator-memory-limits", not(miri)))] - pub(super) fn enforce_memory_check(&mut self) -> Result<()> { - limits::enforce_memory_limit().map_err(|err| self.map_limit_error(err)) - } - #[cfg(any(miri, not(feature = "allocator-memory-limits")))] pub(super) fn memory_check(&mut self) -> Result<()> { Ok(()) } - #[cfg(any(miri, not(feature = "allocator-memory-limits")))] - pub(super) fn enforce_memory_check(&mut self) -> Result<()> { - Ok(()) - } - /// Get or create the cached dummy span for builtin calls. pub(super) fn get_dummy_span(&mut self) -> Result<&crate::lexer::Span> { if self.dummy_span.is_none() { diff --git a/src/rvm/vm/rules.rs b/src/rvm/vm/rules.rs index 4276609..f53828d 100644 --- a/src/rvm/vm/rules.rs +++ b/src/rvm/vm/rules.rs @@ -158,17 +158,16 @@ impl RegoVM { }); } - // Clone the Arc (cheap atomic increment) so we can borrow &RuleInfo - // without holding an immutable borrow on self. - let program = self.program.clone(); - let rule_info = program + let rule_info = self + .program .rule_infos .get(rule_idx) .ok_or(VmError::RuleInfoMissing { index: rule_index, pc: self.pc, - available: program.rule_infos.len(), - })?; + available: self.program.rule_infos.len(), + })? + .clone(); let is_function_rule = rule_info.function_info.is_some(); @@ -220,7 +219,7 @@ impl RegoVM { }); let (final_result, rule_failed_due_to_inconsistency) = self - .execute_rule_definitions_common(&rule_definitions, rule_info, function_call_params)?; + .execute_rule_definitions_common(&rule_definitions, &rule_info, function_call_params)?; self.set_register(dest, Value::Undefined)?; @@ -236,7 +235,7 @@ impl RegoVM { Value::Undefined }; - self.set_register(dest, result_from_rule)?; + self.set_register(dest, result_from_rule.clone())?; if self.get_register(dest)? == &Value::Undefined && !rule_failed_due_to_inconsistency { match call_context.rule_type { @@ -278,7 +277,7 @@ impl RegoVM { pc: self.pc, available, })?; - *entry = (true, final_value); + *entry = (true, final_value.clone()); } Ok(()) } @@ -307,15 +306,16 @@ impl RegoVM { }); } - let program = self.program.clone(); - let rule_info = program + let rule_info = self + .program .rule_infos .get(rule_idx) .ok_or(VmError::RuleInfoMissing { index: rule_index, pc: self.pc, - available: program.rule_infos.len(), - })?; + available: self.program.rule_infos.len(), + })? + .clone(); let is_function_rule = rule_info.function_info.is_some(); @@ -430,7 +430,7 @@ impl RegoVM { }; let initial_pc = self - .prepare_rule_frame_initial_pc(&mut frame_data, rule_info)? + .prepare_rule_frame_initial_pc(&mut frame_data, &rule_info)? .ok_or(VmError::RuleFrameMissingInitialPc { pc: self.pc })?; let frame = ExecutionFrame::new(initial_pc, FrameKind::Rule(frame_data)); @@ -651,15 +651,16 @@ impl RegoVM { } = frame_data; let rule_idx = usize::from(rule_index); - let program = self.program.clone(); - let rule_info = program + let rule_info = self + .program .rule_infos .get(rule_idx) .ok_or(VmError::RuleInfoMissing { index: rule_index, pc: self.pc, - available: program.rule_infos.len(), - })?; + available: self.program.rule_infos.len(), + })? + .clone(); let result_from_rule = if rule_failed_due_to_inconsistency { Value::Undefined @@ -771,7 +772,6 @@ impl RegoVM { pc: self.pc, available, })?; - // Clone into cache; return the original below. *entry = (true, final_value.clone()); } @@ -790,20 +790,12 @@ impl RegoVM { &mut self, frame_data: &mut RuleFrameData, ) -> Result> { - let program = self.program.clone(); - let rule_info = program - .rule_infos - .get(usize::from(frame_data.rule_index)) - .ok_or(VmError::RuleInfoMissing { - index: frame_data.rule_index, - pc: self.pc, - available: program.rule_infos.len(), - })?; + let rule_info = self.get_rule_info(frame_data.rule_index)?; match frame_data.phase { RuleFramePhase::ExecutingDestructuring => { - self.rule_frame_after_destructuring_success(frame_data, rule_info) + self.rule_frame_after_destructuring_success(frame_data, &rule_info) } - RuleFramePhase::ExecutingBody => self.rule_frame_after_success(frame_data, rule_info), + RuleFramePhase::ExecutingBody => self.rule_frame_after_success(frame_data, &rule_info), RuleFramePhase::Initializing | RuleFramePhase::Finalizing => Ok(None), } } @@ -812,16 +804,21 @@ impl RegoVM { &mut self, frame_data: &mut RuleFrameData, ) -> Result> { - let program = self.program.clone(); - let rule_info = program + let rule_info = self.get_rule_info(frame_data.rule_index)?; + self.rule_frame_after_failure(frame_data, &rule_info) + } + + fn get_rule_info(&self, rule_index: u16) -> Result { + let idx = usize::from(rule_index); + self.program .rule_infos - .get(usize::from(frame_data.rule_index)) + .get(idx) + .cloned() .ok_or(VmError::RuleInfoMissing { - index: frame_data.rule_index, + index: rule_index, pc: self.pc, - available: program.rule_infos.len(), - })?; - self.rule_frame_after_failure(frame_data, rule_info) + available: self.program.rule_infos.len(), + }) } pub(super) fn checked_add_one(&self, value: usize, context: &'static str) -> Result { diff --git a/tests/rvm/compiler.rs b/tests/rvm/compiler.rs index b490cb3..69cf86c 100644 --- a/tests/rvm/compiler.rs +++ b/tests/rvm/compiler.rs @@ -3,6 +3,7 @@ #![cfg(feature = "rvm")] use regorus::languages::rego::compiler::Compiler; +use regorus::rvm::instructions::GuardMode; use regorus::rvm::Instruction; use regorus::{Engine, Rc, Value}; use std::collections::BTreeSet; @@ -128,7 +129,11 @@ fn non_constant_array_is_not_hoisted() { ); } -// --- AssertEq fusion tests --- +// --- Eq + Guard(Condition) tests --- +// +// The compiler emits `Eq { dest, left, right }` followed by +// `Guard { register: dest, mode: Condition }` for equality checks, +// rather than a fused `AssertEq`. /// Count occurrences of a specific instruction pattern in the program. fn count_instructions( @@ -138,56 +143,76 @@ fn count_instructions( program.instructions.iter().filter(|i| pred(i)).count() } +/// Helper: check that the program contains an Eq followed by Guard(Condition). +fn has_eq_guard_condition(program: ®orus::rvm::program::Program) -> bool { + program.instructions.windows(2).any(|w| { + matches!(w[0], Instruction::Eq { .. }) + && matches!( + w[1], + Instruction::Guard { + mode: GuardMode::Condition, + .. + } + ) + }) +} + #[test] -fn equality_check_emits_assert_eq() { - // Assignment `x = 1` followed by `x = 1` triggers EqualityCheck in destructuring. +fn equality_check_emits_eq_guard() { + // Assignment `x = 1` followed by `x = 1` triggers Eq + Guard(Condition). let program = compile_rule( r#" package test p if { x = 1; x = 1 } "#, ); - let assert_eq_count = - count_instructions(&program, |i| matches!(i, Instruction::AssertEq { .. })); assert!( - assert_eq_count > 0, - "expected AssertEq instruction for equality check" + has_eq_guard_condition(&program), + "expected Eq + Guard(Condition) pair for equality check" ); } #[test] -fn destructuring_equality_emits_assert_eq() { +fn destructuring_equality_emits_eq_guard() { let program = compile_rule( r#" package test p if { [1, x] := [1, 2] } "#, ); - let assert_eq_count = - count_instructions(&program, |i| matches!(i, Instruction::AssertEq { .. })); assert!( - assert_eq_count > 0, - "expected AssertEq for destructuring equality" + has_eq_guard_condition(&program), + "expected Eq + Guard(Condition) for destructuring equality" ); } #[test] -fn not_expr_emits_assert_not() { +fn not_expr_emits_not_plus_guard_condition() { let program = compile_rule( r#" package test p if { not false } "#, ); - let assert_not_count = - count_instructions(&program, |i| matches!(i, Instruction::AssertNot { .. })); - assert!( - assert_not_count > 0, - "expected AssertNot for `not` expression" - ); - // The Not+AssertCondition pair should be fused — no separate Not instruction. + // The compiler emits Not { dest, operand } + Guard { register: dest, mode: Condition }. let not_count = count_instructions(&program, |i| matches!(i, Instruction::Not { .. })); - assert_eq!(not_count, 0, "Not should be fused into AssertNot"); + assert!( + not_count > 0, + "expected Not instruction for `not` expression" + ); + let guard_cond_count = count_instructions(&program, |i| { + matches!( + i, + Instruction::Guard { + mode: GuardMode::Condition, + .. + } + ) + }); + assert!( + guard_cond_count > 0, + "expected Guard(Condition) after Not instruction" + ); } // --- B-11: early_exit_on_first_success flag tests ---