diff --git a/src/builtins/json_patch.rs b/src/builtins/json_patch.rs index 3bbb90242..3ab71b3d7 100644 --- a/src/builtins/json_patch.rs +++ b/src/builtins/json_patch.rs @@ -87,7 +87,7 @@ impl EditNode { set.insert(value.render()?); enforce_limit()?; } - Value::Set(crate::Rc::new(set)) + Value::from_set(set) } }) } diff --git a/src/builtins/sets.rs b/src/builtins/sets.rs index 82e34918a..55b6b2a58 100644 --- a/src/builtins/sets.rs +++ b/src/builtins/sets.rs @@ -10,8 +10,6 @@ use crate::lexer::Span; use crate::value::Value; use crate::*; -use alloc::collections::BTreeSet; - use anyhow::{bail, Result}; pub fn register(m: &mut builtins::BuiltinsMap<&'static str, builtins::BuiltinFcn>) { @@ -24,19 +22,19 @@ pub fn register(m: &mut builtins::BuiltinsMap<&'static str, builtins::BuiltinFcn pub fn intersection(expr1: &Expr, expr2: &Expr, v1: Value, v2: Value) -> Result { let s1 = ensure_set("intersection", expr1, v1)?; let s2 = ensure_set("intersection", expr2, v2)?; - Ok(Value::from_set(s1.intersection(&s2).cloned().collect())) + Ok(Value::from(s1.intersection(&s2))) } pub fn union(expr1: &Expr, expr2: &Expr, v1: Value, v2: Value) -> Result { let s1 = ensure_set("union", expr1, v1)?; let s2 = ensure_set("union", expr2, v2)?; - Ok(Value::from_set(s1.union(&s2).cloned().collect())) + Ok(Value::from(s1.union(&s2))) } pub fn difference(expr1: &Expr, expr2: &Expr, v1: Value, v2: Value) -> Result { let s1 = ensure_set("difference", expr1, v1)?; let s2 = ensure_set("difference", expr2, v2)?; - Ok(Value::from_set(s1.difference(&s2).cloned().collect())) + Ok(Value::from(s1.difference(&s2))) } fn binary_set_union( @@ -49,7 +47,7 @@ fn binary_set_union( ensure_args_count(span, name, params, args, 2)?; let left = ensure_set(name, ¶ms[0], args[0].clone())?; let right = ensure_set(name, ¶ms[1], args[1].clone())?; - Ok(Value::from_set(left.union(&right).cloned().collect())) + Ok(Value::from(left.union(&right))) } fn binary_set_intersection( @@ -62,9 +60,7 @@ fn binary_set_intersection( ensure_args_count(span, name, params, args, 2)?; let left = ensure_set(name, ¶ms[0], args[0].clone())?; let right = ensure_set(name, ¶ms[1], args[1].clone())?; - Ok(Value::from_set( - left.intersection(&right).cloned().collect(), - )) + Ok(Value::from(left.intersection(&right))) } fn intersection_of_set_of_sets( @@ -77,7 +73,7 @@ fn intersection_of_set_of_sets( ensure_args_count(span, name, params, args, 1)?; let set = ensure_set(name, ¶ms[0], args[0].clone())?; - let mut res = BTreeSet::new(); + let mut res = crate::value::Set::new(); let mut first = true; for s in set.iter() { @@ -92,11 +88,11 @@ fn intersection_of_set_of_sets( res.clone_from(s); first = false; } else { - res = res.intersection(s).cloned().collect(); + res = res.intersection(s); } } - Ok(Value::from_set(res)) + Ok(Value::from(res)) } fn union_of_set_of_sets( @@ -109,7 +105,7 @@ fn union_of_set_of_sets( ensure_args_count(span, name, params, args, 1)?; let set = ensure_set(name, ¶ms[0], args[0].clone())?; - let mut res = BTreeSet::new(); + let mut res = crate::value::Set::new(); for s in set.iter() { let s = match s { @@ -119,8 +115,8 @@ fn union_of_set_of_sets( ), }; - res = res.union(s).cloned().collect(); + res = res.union(s); } - Ok(Value::from_set(res)) + Ok(Value::from(res)) } diff --git a/src/builtins/utils.rs b/src/builtins/utils.rs index 77f3b8586..bec25fdb7 100644 --- a/src/builtins/utils.rs +++ b/src/builtins/utils.rs @@ -5,13 +5,11 @@ use crate::ast::{Expr, Ref}; use crate::lexer::Span; use crate::number::Number; -use crate::value::Object; +use crate::value::{Object, Set}; use crate::Rc; use crate::Value; use crate::*; -use alloc::collections::BTreeSet; - use anyhow::{bail, Result}; #[inline] @@ -159,7 +157,7 @@ pub fn ensure_array(fcn: &str, arg: &Expr, v: Value) -> Result>> { }) } -pub fn ensure_set(fcn: &str, arg: &Expr, v: Value) -> Result>> { +pub fn ensure_set(fcn: &str, arg: &Expr, v: Value) -> Result> { Ok(match v { Value::Set(s) => s, _ => { diff --git a/src/languages/azure_policy/compiler/metadata.rs b/src/languages/azure_policy/compiler/metadata.rs index 52265390a..e13116f43 100644 --- a/src/languages/azure_policy/compiler/metadata.rs +++ b/src/languages/azure_policy/compiler/metadata.rs @@ -17,7 +17,7 @@ use crate::languages::azure_policy::ast::{ Condition, EffectKind, FieldKind, JsonValue, Lhs, OperatorKind, PolicyDefinition, PolicyRule, ValueOrExpr, }; -use crate::{Rc, Value}; +use crate::Value; use super::core::Compiler; @@ -237,7 +237,7 @@ impl Compiler { .iter() .map(|p| Value::String(p.name.as_str().into())) .collect(); - annot.insert("parameter_names".to_string(), Value::Set(Rc::new(set))); + annot.insert("parameter_names".to_string(), Value::from_set(set)); } // Extra fields: policyType → policy_type, id → policy_id, name → policy_name. @@ -279,6 +279,6 @@ fn insert_string_set_annotation( .iter() .map(|s| Value::String(s.as_str().into())) .collect(); - annot.insert(key.to_string(), Value::Set(Rc::new(set))); + annot.insert(key.to_string(), Value::from_set(set)); } } diff --git a/src/languages/azure_rbac/builtins/lists.rs b/src/languages/azure_rbac/builtins/lists.rs index 96eb3d89d..78b8c586b 100644 --- a/src/languages/azure_rbac/builtins/lists.rs +++ b/src/languages/azure_rbac/builtins/lists.rs @@ -1,9 +1,7 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. -use alloc::collections::BTreeSet; - -use crate::value::Value; +use crate::value::{Set, Value}; use super::evaluator::RbacBuiltinError; @@ -28,7 +26,7 @@ fn list_contains_values(list: &[Value], needle: &Value) -> bool { } // For sets, treat a list/set needle as "all elements are contained". -fn set_contains_values(set: &BTreeSet, needle: &Value) -> bool { +fn set_contains_values(set: &Set, needle: &Value) -> bool { match *needle { // For collection needles, require all elements to be present. Value::Array(ref right_list) => right_list.iter().all(|item| set.contains(item)), diff --git a/src/languages/rego/compiler/expressions/collection_literals.rs b/src/languages/rego/compiler/expressions/collection_literals.rs index 2f3b39f8a..fdb37510e 100644 --- a/src/languages/rego/compiler/expressions/collection_literals.rs +++ b/src/languages/rego/compiler/expressions/collection_literals.rs @@ -40,7 +40,7 @@ pub(in crate::languages::rego::compiler) fn try_eval_const(expr: &Expr) -> Optio .iter() .map(|i| try_eval_const(i.as_ref())) .collect::>>() - .map(|s| Value::Set(Rc::new(s))), + .map(Value::from_set), Expr::Object { fields, .. } => fields .iter() .map(|(_, k, v)| Some((try_eval_const(k.as_ref())?, try_eval_const(v.as_ref())?))) @@ -92,7 +92,7 @@ impl<'a> Compiler<'a> { items.iter().map(|i| try_eval_const(i.as_ref())).collect(); if let Some(values) = all_const { let dest = self.alloc_register(); - let literal_idx = self.add_literal(Value::Set(Rc::new(values))); + let literal_idx = self.add_literal(Value::from_set(values)); self.emit_instruction(Instruction::Load { dest, literal_idx }, span); return Ok(dest); } diff --git a/src/rvm/program/metadata.rs b/src/rvm/program/metadata.rs index ec783e3e0..16d43ed50 100644 --- a/src/rvm/program/metadata.rs +++ b/src/rvm/program/metadata.rs @@ -173,7 +173,7 @@ impl MetadataValue { .collect(), ) } else { - MetadataValue::List(set.iter().map(MetadataValue::from_value).collect()) + MetadataValue::List(set.iter_sorted().map(MetadataValue::from_value).collect()) } } Value::Object(ref obj) => { @@ -202,7 +202,7 @@ impl MetadataValue { for s in set { bset.insert(Value::String(s.as_str().into())); } - Value::Set(Rc::new(bset)) + Value::from_set(bset) } MetadataValue::Bool(b) => Value::Bool(b), MetadataValue::Integer(n) => Value::from(n), @@ -301,7 +301,7 @@ mod tests { let mut set = BTreeSet::new(); set.insert(Value::String("a".into())); set.insert(Value::String("b".into())); - let v = Value::Set(Rc::new(set)); + let v = Value::from_set(set); assert_round_trip(&v, &v); } @@ -328,11 +328,14 @@ mod tests { let mut set = BTreeSet::new(); set.insert(Value::String("a".into())); set.insert(Value::from(1_i64)); - let v = Value::Set(Rc::new(set)); + let v = Value::from_set(set); let mv = MetadataValue::from_value(&v); - assert!( - matches!(mv, MetadataValue::List(_)), - "mixed-type set should produce List, got {mv:?}" + assert_eq!( + mv, + MetadataValue::List(alloc::vec![ + MetadataValue::Integer(1), + MetadataValue::String("a".into()), + ]) ); } diff --git a/src/rvm/program/serialization/value.rs b/src/rvm/program/serialization/value.rs index 0e98a17be..02868a0ce 100644 --- a/src/rvm/program/serialization/value.rs +++ b/src/rvm/program/serialization/value.rs @@ -11,8 +11,8 @@ use serde::ser::{SerializeSeq as _, SerializeTuple as _}; use serde::{Deserialize, Serialize}; use crate::number::Number; -use crate::value::Object; use crate::value::Value; +use crate::value::{Object, Set}; const VARIANT_NULL: u32 = 0; const VARIANT_BOOL: u32 = 1; @@ -118,7 +118,7 @@ impl<'a> Serialize for BinaryValueSlice<'a> { } } -struct BinarySetRef<'a>(&'a BTreeSet); +struct BinarySetRef<'a>(&'a Set); impl<'a> Serialize for BinarySetRef<'a> { fn serialize(&self, serializer: S) -> Result @@ -126,7 +126,7 @@ impl<'a> Serialize for BinarySetRef<'a> { S: serde::Serializer, { let mut seq = serializer.serialize_seq(Some(self.0.len()))?; - for value in self.0.iter() { + for value in self.0.iter_sorted() { seq.serialize_element(&BinaryValueRef(value))?; } seq.end() diff --git a/src/rvm/vm/arithmetic.rs b/src/rvm/vm/arithmetic.rs index 89362d10e..cc3811278 100644 --- a/src/rvm/vm/arithmetic.rs +++ b/src/rvm/vm/arithmetic.rs @@ -30,9 +30,7 @@ impl RegoVM { match (a, b) { (&Value::Number(ref x), &Value::Number(ref y)) => Ok(Value::from(x.sub(y)?)), (&Value::Set(ref left), &Value::Set(ref right)) => { - let diff: alloc::collections::BTreeSet = - left.difference(right).cloned().collect(); - Ok(Value::from(diff)) + Ok(Value::from(left.difference(right))) } _ => Err(VmError::InvalidSubtraction { left: a.clone(), diff --git a/src/rvm/vm/comprehension.rs b/src/rvm/vm/comprehension.rs index ba0dae110..3ccff1997 100644 --- a/src/rvm/vm/comprehension.rs +++ b/src/rvm/vm/comprehension.rs @@ -62,11 +62,8 @@ impl RegoVM { if set.is_empty() { None } else { - Some(IterationState::Set { - items: set, - current_item: None, - first_iteration: true, - }) + let cursor = set.cursor(); + Some(IterationState::Set { items: set, cursor }) } } Value::Undefined => None, @@ -148,11 +145,8 @@ impl RegoVM { if set.is_empty() { None } else { - Some(IterationState::Set { - items: set, - current_item: None, - first_iteration: true, - }) + let cursor = set.cursor(); + Some(IterationState::Set { items: set, cursor }) } } Value::Undefined => None, @@ -251,21 +245,6 @@ impl RegoVM { }; let result_reg = comprehension_context.result_reg; - // Snapshot the iteration value register BEFORE taking the result - // register: if the comprehension compiler ever allocates - // `result_reg == context.value_reg`, the writeback at the bottom - // of this function would clobber the value register, and a - // post-writeback read here would feed the wrong value into - // `IterationState::Set::current_item`. Only Set needs the snapshot - // (Object uses a self-advancing cursor; Array advances by index). - let set_resume_snapshot = if matches!( - comprehension_context.iteration_state, - Some(IterationState::Set { .. }) - ) { - Some(self.get_register(comprehension_context.value_reg)?.clone()) - } else { - None - }; // Take ownership of the result register so Rc refcount stays at 1, // allowing Rc::make_mut to mutate in-place instead of deep-cloning. let mut current_result = self.take_register(result_reg)?; @@ -303,16 +282,6 @@ impl RegoVM { self.set_register(result_reg, current_result)?; if let Some(iter_state) = comprehension_context.iteration_state.as_mut() { - // Set's `Bound::Excluded(current_item)` resume scheme needs the - // pre-mutation snapshot taken at the top of this function. - // Object uses a self-advancing cursor and needs no snapshot. - if let IterationState::Set { - ref mut current_item, - .. - } = *iter_state - { - *current_item = set_resume_snapshot; - } iter_state.advance(); let has_next = self.setup_next_iteration( iter_state, @@ -351,15 +320,7 @@ impl RegoVM { pc: self.pc, })?; - let ( - value_to_add, - key_value, - mode, - result_reg_idx, - key_reg_idx, - value_reg_idx, - iter_is_set, - ) = { + let (value_to_add, key_value, mode, result_reg_idx, key_reg_idx, value_reg_idx) = { let frame = self.execution_stack .get(comprehension_index) @@ -380,9 +341,6 @@ impl RegoVM { let result_reg_idx = context.result_reg; let mode = context.mode.clone(); - let iter_is_set = - matches!(context.iteration_state, Some(IterationState::Set { .. })); - ( value_to_add, key_value, @@ -390,7 +348,6 @@ impl RegoVM { result_reg_idx, context.key_reg, context.value_reg, - iter_is_set, ) } else { return Err(VmError::InvalidIteration { @@ -400,18 +357,6 @@ impl RegoVM { } }; - // Snapshot the iteration value register BEFORE the result writeback: - // if the compiler ever allocates `result_reg == value_reg_idx`, a - // post-writeback read would feed the result accumulator into - // `IterationState::Set::current_item`, breaking the next iteration. - // Only Set needs this (Object cursor self-advances; Array advances - // by index). - let set_resume_snapshot = if iter_is_set { - Some(self.get_register(value_reg_idx)?.clone()) - } else { - None - }; - // Take ownership of the result register so Rc refcount stays at 1, // allowing Rc::make_mut to mutate in-place instead of deep-cloning. let mut current_result = self.take_register(result_reg_idx)?; @@ -459,13 +404,6 @@ impl RegoVM { } = &mut frame.kind { if let Some(iter_state) = context.iteration_state.as_mut() { - if let IterationState::Set { - ref mut current_item, - .. - } = *iter_state - { - *current_item = set_resume_snapshot; - } iter_state.advance(); } @@ -561,16 +499,6 @@ impl RegoVM { context: &mut ComprehensionContext, ) -> Result<()> { if let Some(iter_state) = context.iteration_state.as_mut() { - // Snapshot the current value into Set's `current_item` so the - // next iteration can resume from `Bound::Excluded(current)`. - // Object uses a self-advancing cursor and needs no snapshot here. - if let IterationState::Set { - ref mut current_item, - .. - } = *iter_state - { - *current_item = Some(self.get_register(context.value_reg)?.clone()); - } iter_state.advance(); let has_next = self.setup_next_iteration(iter_state, context.key_reg, context.value_reg)?; diff --git a/src/rvm/vm/context.rs b/src/rvm/vm/context.rs index 56439b9c6..b68e09701 100644 --- a/src/rvm/vm/context.rs +++ b/src/rvm/vm/context.rs @@ -3,9 +3,8 @@ use crate::rvm::instructions::{ComprehensionMode, LoopMode}; use crate::value::Value; -use crate::value::{Object, ObjectCursor}; +use crate::value::{Object, ObjectCursor, Set, SetCursor}; use crate::Rc; -use alloc::collections::BTreeSet; use alloc::vec::Vec; /// Loop execution context for managing iteration state @@ -33,10 +32,7 @@ pub struct LoopContext { /// pre-mutation state. The `ObjectCursor` is opaque and resumes in /// O(log n) for the BTree backend. /// -/// `Set` continues to use the pre-existing snapshot-by-cloned-key -/// approach (`current_item` + `first_iteration`); migration of `Set` -/// to a cursor-based iterator ships with the `Set` storage abstraction -/// in a follow-up PR. +/// `Set` mirrors Object using an opaque cursor over a shared `Rc`. #[derive(Debug, Clone)] pub enum IterationState { Array { @@ -48,9 +44,8 @@ pub enum IterationState { cursor: ObjectCursor, }, Set { - items: Rc>, - current_item: Option, - first_iteration: bool, + items: Rc, + cursor: SetCursor, }, /// Virtual single-element iteration for non-collection values. /// Used by Azure Policy's `[*]` on scalar/null fields: presents a single @@ -79,12 +74,7 @@ impl IterationState { // when it pulls the next item via `Object::next`, so `advance` // is a no-op for the cursor-backed Object variant. Self::Object { .. } => {} - Self::Set { - ref mut first_iteration, - .. - } => { - *first_iteration = false; - } + Self::Set { .. } => {} Self::Single { ref mut consumed, .. } => { diff --git a/src/rvm/vm/dispatch.rs b/src/rvm/vm/dispatch.rs index 3a9379c12..5b91191fe 100644 --- a/src/rvm/vm/dispatch.rs +++ b/src/rvm/vm/dispatch.rs @@ -711,7 +711,7 @@ impl RegoVM { set.insert(self.get_register(reg)?.clone()); } - let set_value = Value::Set(crate::Rc::new(set)); + let set_value = Value::from(set); self.set_register(params.dest, set_value)?; } Ok(InstructionOutcome::Continue) diff --git a/src/rvm/vm/execution.rs b/src/rvm/vm/execution.rs index 47681c30a..22ff6def1 100644 --- a/src/rvm/vm/execution.rs +++ b/src/rvm/vm/execution.rs @@ -120,6 +120,7 @@ impl RegoVM { // Per-instruction sanity check: every iteration of the dispatch // loop must re-enter with the VM in a Running/Ready state and the // working data structures coherent. + #[cfg(debug_assertions)] self.assert_vm_invariants(); self.memory_check()?; if self.executed_instructions >= self.max_instructions { @@ -195,6 +196,7 @@ impl RegoVM { fn execute_suspendable_entry(&mut self, entry_point_pc: usize) -> Result { // Precondition: callers (execute_entry_point_by_{index,name}) reset the // VM before invoking this method, so the VM must be in a clean state. + #[cfg(debug_assertions)] self.debug_assert_state_is_clean(); self.execution_state = ExecutionState::Running; self.reset_execution_timer_state(); @@ -302,6 +304,7 @@ impl RegoVM { while !self.execution_stack.is_empty() { // Per-instruction sanity check: see `assert_vm_invariants` for the // exact contract. Compiled out in release. + #[cfg(debug_assertions)] self.assert_vm_invariants(); self.memory_check()?; self.frame_pc_overridden = false; diff --git a/src/rvm/vm/loops.rs b/src/rvm/vm/loops.rs index fa21ad172..fba2d95ab 100644 --- a/src/rvm/vm/loops.rs +++ b/src/rvm/vm/loops.rs @@ -156,17 +156,6 @@ impl RegoVM { LoopAction::Continue => {} } - // Snapshot the current value for Set so its next iteration can resume - // from `Bound::Excluded(current)`. Object uses a cursor and advances - // inside `setup_next_iteration` itself. - if let &mut IterationState::Set { - ref mut current_item, - .. - } = &mut loop_ctx.iteration_state - { - *current_item = Some(self.get_register(loop_ctx.value_reg)?.clone()); - } - loop_ctx.iteration_state.advance(); let has_next = self.setup_next_iteration( &mut loop_ctx.iteration_state, @@ -337,8 +326,6 @@ impl RegoVM { } }; - let value_value = self.get_register(value_reg)?.clone(); - let frame = self .execution_stack .last_mut() @@ -347,18 +334,6 @@ impl RegoVM { &mut FrameKind::Loop { ref mut context, .. } => { - // Snapshot the current value for Set so its next - // iteration can resume from `Bound::Excluded(current)`. - // Object uses a cursor and advances inside - // `setup_next_iteration` itself. - if let &mut IterationState::Set { - ref mut current_item, - .. - } = &mut context.iteration_state - { - *current_item = Some(value_value); - } - context.iteration_state.advance(); context.current_iteration_failed = false; @@ -483,10 +458,10 @@ impl RegoVM { self.handle_empty_collection(mode, params.result_reg, params.loop_end)?; return Ok(None); } + let cursor = set.cursor(); Ok(Some(IterationState::Set { items: set.clone(), - current_item: None, - first_iteration: true, + cursor, })) } _ => { @@ -576,33 +551,14 @@ impl RegoVM { } IterationState::Set { ref items, - ref current_item, - ref first_iteration, + ref mut cursor, } => { - if *first_iteration { - if let Some(item) = items.iter().next() { - if key_reg != value_reg { - self.set_register(key_reg, item.clone())?; - } - self.set_register(value_reg, item.clone())?; - Ok(true) - } else { - Ok(false) - } - } else if let Some(ref current) = *current_item { - let mut range_iter = items.range(( - core::ops::Bound::Excluded(current), - core::ops::Bound::Unbounded, - )); - if let Some(item) = range_iter.next() { - if key_reg != value_reg { - self.set_register(key_reg, item.clone())?; - } - self.set_register(value_reg, item.clone())?; - Ok(true) - } else { - Ok(false) + if let Some(item) = items.next(cursor) { + if key_reg != value_reg { + self.set_register(key_reg, item.clone())?; } + self.set_register(value_reg, item.clone())?; + Ok(true) } else { Ok(false) } diff --git a/src/rvm/vm/state.rs b/src/rvm/vm/state.rs index 9b16644dc..28fb05b4c 100644 --- a/src/rvm/vm/state.rs +++ b/src/rvm/vm/state.rs @@ -38,6 +38,7 @@ impl RegoVM { // Postcondition: every stack/cache that `reset_execution_state` touches // must be in its documented "clean" shape. This catches accidental // omissions in future edits to this function. + #[cfg(debug_assertions)] self.debug_assert_state_is_clean(); } @@ -47,73 +48,71 @@ impl RegoVM { /// before starting a fresh execution. The body is fully gated by /// `#[cfg(debug_assertions)]` so this is a zero-cost no-op in release. #[inline] + #[cfg(debug_assertions)] pub(super) fn debug_assert_state_is_clean(&self) { - #[cfg(debug_assertions)] - { - // --- Stacks: every per-execution stack must be drained. --- - debug_assert!( - self.execution_stack.is_empty(), - "reset_execution_state postcondition: execution_stack must be empty" - ); - debug_assert!( - self.loop_stack.is_empty(), - "reset_execution_state postcondition: loop_stack must be empty" - ); - debug_assert!( - self.comprehension_stack.is_empty(), - "reset_execution_state postcondition: comprehension_stack must be empty" - ); - debug_assert!( - self.call_rule_stack.is_empty(), - "reset_execution_state postcondition: call_rule_stack must be empty" - ); - debug_assert!( - self.register_stack.is_empty(), - "reset_execution_state postcondition: register_stack must be empty" - ); - - // --- Caches: cleared so a new program/input cannot read stale entries. --- - debug_assert!( - self.builtins_cache.is_empty(), - "reset_execution_state postcondition: builtins_cache must be empty" - ); - - // --- Registers: window resized to the program's base count and zeroed. --- - debug_assert_eq!( - self.registers.len(), - self.base_register_count, - "reset_execution_state postcondition: registers must be sized to base_register_count" - ); - debug_assert!( - self.registers.iter().all(|v| matches!(v, Value::Undefined)), - "reset_execution_state postcondition: all registers must be Undefined" - ); - - // --- Rule cache: sized to the current program and marked uncomputed. --- - debug_assert_eq!( - self.rule_cache.len(), - self.program.rule_infos.len(), - "reset_execution_state postcondition: rule_cache size must match program rule_infos" - ); - debug_assert!( - self.rule_cache.iter().all(|entry| !entry.0), - "reset_execution_state postcondition: rule_cache entries must be uncomputed" - ); - - // --- Counters and execution-state machine: zeroed and back to Ready. --- - debug_assert_eq!( - self.pc, 0, - "reset_execution_state postcondition: pc must be 0" - ); - debug_assert_eq!( - self.executed_instructions, 0, - "reset_execution_state postcondition: executed_instructions must be 0" - ); - debug_assert!( - matches!(self.execution_state, ExecutionState::Ready), - "reset_execution_state postcondition: execution_state must be Ready" - ); - } + // --- Stacks: every per-execution stack must be drained. --- + debug_assert!( + self.execution_stack.is_empty(), + "reset_execution_state postcondition: execution_stack must be empty" + ); + debug_assert!( + self.loop_stack.is_empty(), + "reset_execution_state postcondition: loop_stack must be empty" + ); + debug_assert!( + self.comprehension_stack.is_empty(), + "reset_execution_state postcondition: comprehension_stack must be empty" + ); + debug_assert!( + self.call_rule_stack.is_empty(), + "reset_execution_state postcondition: call_rule_stack must be empty" + ); + debug_assert!( + self.register_stack.is_empty(), + "reset_execution_state postcondition: register_stack must be empty" + ); + + // --- Caches: cleared so a new program/input cannot read stale entries. --- + debug_assert!( + self.builtins_cache.is_empty(), + "reset_execution_state postcondition: builtins_cache must be empty" + ); + + // --- Registers: window resized to the program's base count and zeroed. --- + debug_assert_eq!( + self.registers.len(), + self.base_register_count, + "reset_execution_state postcondition: registers must be sized to base_register_count" + ); + debug_assert!( + self.registers.iter().all(|v| matches!(v, Value::Undefined)), + "reset_execution_state postcondition: all registers must be Undefined" + ); + + // --- Rule cache: sized to the current program and marked uncomputed. --- + debug_assert_eq!( + self.rule_cache.len(), + self.program.rule_infos.len(), + "reset_execution_state postcondition: rule_cache size must match program rule_infos" + ); + debug_assert!( + self.rule_cache.iter().all(|entry| !entry.0), + "reset_execution_state postcondition: rule_cache entries must be uncomputed" + ); + + // --- Counters and execution-state machine: zeroed and back to Ready. --- + debug_assert_eq!( + self.pc, 0, + "reset_execution_state postcondition: pc must be 0" + ); + debug_assert_eq!( + self.executed_instructions, 0, + "reset_execution_state postcondition: executed_instructions must be 0" + ); + debug_assert!( + matches!(self.execution_state, ExecutionState::Ready), + "reset_execution_state postcondition: execution_state must be Ready" + ); } /// Per-opcode VM invariants checked from the inner dispatch loop. @@ -128,45 +127,43 @@ impl RegoVM { /// Fully `#[cfg(debug_assertions)]`-gated so the method body compiles out /// in release. #[inline] + #[cfg(debug_assertions)] pub(super) fn assert_vm_invariants(&self) { - #[cfg(debug_assertions)] - { - // The dispatch loop only runs while execution is live. Once the VM - // has transitioned to a terminal state (Suspended/Completed/Error) - // the loop must have exited. Note `Ready` is also valid here because - // some entry points (e.g. `execute_entry_point_by_index` in - // RunToCompletion mode) drive `jump_to` without flipping the state. - // `execution_state` is mutated only inside the VM and is not - // host-controllable. - debug_assert!( - matches!( - self.execution_state, - ExecutionState::Ready | ExecutionState::Running - ), - "vm invariant: execution_state must be Ready or Running inside the dispatch loop, was {:?}", - self.execution_state - ); - - // Rule cache is sized once at reset (against the currently loaded - // program) and the VM does not resize it mid-execution. Any - // mismatch here would indicate an internal accounting bug rather - // than malformed input. - debug_assert_eq!( - self.rule_cache.len(), - self.program.rule_infos.len(), - "vm invariant: rule_cache size must equal program.rule_infos size" - ); - - // NOTE: `!registers.is_empty()` and an `execution_stack` depth - // ceiling were intentionally *not* asserted here: both can be - // triggered by a host-loaded program (registers via - // `RuleInfo::num_registers == 0`; stack depth via deeply nested - // rules/loops/comprehensions) and would therefore panic in debug - // and poison the engine across FFI. Register access is already - // guarded by `VmError::RegisterIndexOutOfBounds`; runaway recursion - // is bounded in production by `set_max_instructions` and - // `memory_check`. - } + // The dispatch loop only runs while execution is live. Once the VM + // has transitioned to a terminal state (Suspended/Completed/Error) + // the loop must have exited. Note `Ready` is also valid here because + // some entry points (e.g. `execute_entry_point_by_index` in + // RunToCompletion mode) drive `jump_to` without flipping the state. + // `execution_state` is mutated only inside the VM and is not + // host-controllable. + debug_assert!( + matches!( + self.execution_state, + ExecutionState::Ready | ExecutionState::Running + ), + "vm invariant: execution_state must be Ready or Running inside the dispatch loop, was {:?}", + self.execution_state + ); + + // Rule cache is sized once at reset (against the currently loaded + // program) and the VM does not resize it mid-execution. Any + // mismatch here would indicate an internal accounting bug rather + // than malformed input. + debug_assert_eq!( + self.rule_cache.len(), + self.program.rule_infos.len(), + "vm invariant: rule_cache size must equal program.rule_infos size" + ); + + // NOTE: `!registers.is_empty()` and an `execution_stack` depth + // ceiling were intentionally *not* asserted here: both can be + // triggered by a host-loaded program (registers via + // `RuleInfo::num_registers == 0`; stack depth via deeply nested + // rules/loops/comprehensions) and would therefore panic in debug + // and poison the engine across FFI. Register access is already + // guarded by `VmError::RegisterIndexOutOfBounds`; runaway recursion + // is bounded in production by `set_max_instructions` and + // `memory_check`. } /// Return all active objects to their respective pools for reuse diff --git a/src/value/mod.rs b/src/value/mod.rs index 2cf85c601..ff845eb09 100644 --- a/src/value/mod.rs +++ b/src/value/mod.rs @@ -25,7 +25,6 @@ pub use set::Set; #[cfg(feature = "rvm")] #[allow(unused_imports)] // surface for downstream PRs pub use object::ObjectCursor; -#[cfg(feature = "rvm")] #[allow(unused_imports)] // surface for downstream PRs pub use set::SetCursor; @@ -77,7 +76,7 @@ pub enum Value { /// A set of values. /// No JSON equivalent. /// Sets are serialized as arrays in JSON. - Set(Rc>), + Set(Rc), /// An object. /// Unlike JSON, keys can be any value, not just string. @@ -786,7 +785,7 @@ impl From> for Value { /// # Ok(()) /// # } fn from(s: BTreeSet) -> Self { - Value::Set(Rc::new(s)) + Value::Set(Rc::new(Set::from(s))) } } @@ -1244,7 +1243,7 @@ impl Value { } } - /// Cast value to [`& BTreeSet`] if [`Value::Set`]. + /// Cast value to [`&Set`] if [`Value::Set`]. /// ``` /// # use regorus::*; /// # use std::collections::BTreeSet; @@ -1258,14 +1257,14 @@ impl Value { /// assert_eq!(v.as_set()?.first(), Some(&Value::from("Hello"))); /// # Ok(()) /// # } - pub fn as_set(&self) -> Result<&BTreeSet> { + pub fn as_set(&self) -> Result<&Set> { match self { Value::Set(s) => Ok(s), _ => Err(anyhow!("not a set")), } } - /// Cast value to [`&mut BTreeSet`] if [`Value::Set`]. + /// Cast value to [`&mut Set`] if [`Value::Set`]. /// ``` /// # use regorus::*; /// # use std::collections::BTreeSet; @@ -1279,7 +1278,7 @@ impl Value { /// v.as_set_mut()?.insert(Value::from("World")); /// # Ok(()) /// # } - pub fn as_set_mut(&mut self) -> Result<&mut BTreeSet> { + pub fn as_set_mut(&mut self) -> Result<&mut Set> { match self { Value::Set(s) => Ok(Rc::make_mut(s)), _ => Err(anyhow!("not a set")), diff --git a/src/value/set/mod.rs b/src/value/set/mod.rs index d57fba9e2..475295810 100644 --- a/src/value/set/mod.rs +++ b/src/value/set/mod.rs @@ -82,8 +82,10 @@ impl Set { /// deterministic order is required, or [`Set::cursor`] when iteration /// must yield and resume. #[inline] - pub fn iter(&self) -> impl Iterator + '_ { - self.inner.iter() + pub fn iter(&self) -> Iter<'_> { + Iter { + inner: self.inner.iter(), + } } /// Iteration in sorted order (by `Value::Ord`). Non-resumable. @@ -158,7 +160,7 @@ impl Set { /// Wrap into a `Value::Set`. #[inline] pub fn into_value(self) -> Value { - Value::Set(crate::Rc::new(self.inner)) + Value::Set(crate::Rc::new(self)) } /// Create a resumable cursor over elements in implementation-defined diff --git a/tests/rvm/compiler.rs b/tests/rvm/compiler.rs index 69cf86c2d..47bc39831 100644 --- a/tests/rvm/compiler.rs +++ b/tests/rvm/compiler.rs @@ -71,12 +71,12 @@ fn constant_set_is_hoisted() { "#, ); assert_no_collection_create(&program); - let expected_set = Value::Set(Rc::new( + let expected_set = Value::from( [1, 2, 3] .into_iter() .map(Value::from) .collect::>(), - )); + ); assert_literal_exists(&program, &expected_set); }