feat(rvm): add context/metadata support, new instructions, and serial…#659
feat(rvm): add context/metadata support, new instructions, and serial…#659anakrish wants to merge 1 commit intomicrosoft:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR extends the RVM to support additional runtime/environment inputs (evaluation context + program metadata) and introduces several Azure Policy–oriented instruction/semantic additions (guard-style early return, undefined→null coalescing, skip-undefined array push, and scalar/non-collection iteration behavior), along with expanded VM YAML test coverage and tooling support (parser + assembly listing).
Changes:
- Add
LoadContext/LoadMetadatainstructions and VM plumbing to supply host context and cached program metadata at runtime. - Add new instructions:
ArrayPushDefined,ReturnUndefinedIfNotTrue, andCoalesceUndefinedToNull, plusIterationState::Singlefor scalar/non-collection loop iteration. - Extend VM test harness + add new YAML suites covering new instructions and context/metadata behavior; update instruction parser and program listing formatting.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/rvm/vm/suites/type_errors.yaml | Updates a NOT type-error test description for clarified semantics. |
| tests/rvm/vm/suites/return_undefined_if_not_true.yaml | New YAML suite validating ReturnUndefinedIfNotTrue (including HostAwait interactions). |
| tests/rvm/vm/suites/load_context_metadata.yaml | New YAML suite for LoadContext/LoadMetadata behaviors and interactions with Input/Data. |
| tests/rvm/vm/suites/coalesce_undefined_to_null.yaml | New YAML suite validating undefined→null coalescing and interactions with guards/HostAwait. |
| tests/rvm/vm/suites/array_push_defined.yaml | New YAML suite validating skip-undefined array push semantics and error behavior. |
| src/rvm/vm/machine.rs | Adds VM context, metadata caching, Azure Policy loop semantic flag; refactors timer + memory-check paths. |
| src/rvm/vm/loops.rs | Adds Azure Policy “virtual element” iteration path via IterationState::Single. |
| src/rvm/vm/dispatch.rs | Implements new instruction dispatch for context/metadata loads and the new Azure Policy instructions. |
| src/rvm/vm/context.rs | Adds IterationState::Single and advance behavior. |
| src/rvm/vm/comprehension.rs | Updates comprehension iteration handling to tolerate IterationState::Single. |
| src/rvm/tests/vm.rs | Extends YAML harness to accept context + metadata inputs and recompute derived program flags. |
| src/rvm/tests/instruction_parser.rs | Adds parser support for newly introduced instructions. |
| src/rvm/program/listing.rs | Adds assembly listing support for new instructions and names. |
| src/rvm/instructions/mod.rs | Adds new instruction variants to the core instruction enum. |
| src/rvm/instructions/display.rs | Adds Display formatting for newly introduced instructions. |
Comments suppressed due to low confidence (1)
src/rvm/vm/machine.rs:426
execution_timer_tick()now callsmonotonic_now()whenever a timer config is present, which likely means a clock read per instruction (this function is invoked each dispatch step). That defeats the check-interval amortization thatExecutionTimer::accumulatewas designed for. Consider accumulating work units first and only reading the clock when a check is due (or provide a helper that preserves the interval behavior without requiringnowevery call).
if self.execution_timer.limit().is_none() {
return Ok(());
}
let Some(now) = monotonic_now() else {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - "Load { dest: 4, literal_idx: 1 }" | ||
| - "Index { dest: 5, container: 3, key: 4 }" | ||
| # Check context value | ||
| - "Load { dest: 6, literal_idx: 0 }" # reuse the "context_key" literal for comparison | ||
| - "Return { value: 2 }" |
There was a problem hiding this comment.
In load_context_independent_of_input, the steps labeled “reuse … for comparison” never actually perform a comparison, and the loaded input value (r5) is never asserted/returned. This makes the test pass even if LoadInput/LoadContext interaction were broken, and also leaves an unused Load (dest: 6). Consider returning both values (e.g., as an array/object) or adding an Eq/And chain so the test truly validates independence.
9247770 to
96a13a2
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
96a13a2 to
baeca8d
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
baeca8d to
7fabd51
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/rvm/vm/machine.rs
Outdated
| // Azure Policy: `Every` over non-collection → false (not vacuous true) | ||
| self.virtual_element_on_non_collection = program.metadata.language == "azure_policy"; | ||
|
|
||
| // Cache the metadata as a Value for LoadMetadata instructions | ||
| self.metadata_value = program.metadata.to_value(); |
There was a problem hiding this comment.
The comment above virtual_element_on_non_collection initialization is misleading: enabling this flag doesn’t make Every over a non-collection always evaluate to false; it changes Every to iterate once over a virtual Null element (so the result depends on the loop body, and is just no longer vacuously true). Consider rewording the comment to describe the virtual-element behavior rather than a fixed boolean outcome.
…ization v6 Extend the RVM infrastructure with evaluation context, program metadata, and several new instructions needed by language frontends beyond Rego. VM & instructions: - Add LoadContext / LoadMetadata instructions for accessing evaluation context and program metadata from within RVM programs - Add ArrayPushDefined (skip-undefined variant of ArrayPush) for wildcard alias collection where absent properties should be excluded - Add ReturnUndefinedIfNotTrue for early-return semantics without triggering VM assertion failures - Add CoalesceUndefinedToNull for languages where missing fields are null rather than undefined - Support IterationState::Single in loops and comprehensions for iterating over scalar values alongside collections Program metadata: - Extend ProgramMetadata with language identifier, typed annotations (BTreeMap<String, MetadataValue>), and to_value() conversion - Add MetadataValue enum (String, Bool, Integer, Float, Array, Object) with full serde support - Add has_host_await flag to Program with recompute_host_await_presence() Serialization: - Bump binary format version to 6 - Add JSON serialization for ProgramMetadata including annotations Compiler: - Track has_host_await during compilation - Minor import cleanups Test infrastructure: - Extend VM test harness with context, metadata_language, and metadata_annotations fields - Add instruction parser support for new instructions - Add assembly listing support (readable, tabular, compact formats) - Add 3 new YAML test suites: load_context_metadata, coalesce_undefined_to_null, return_undefined_if_not_true
7fabd51 to
65622bd
Compare
…ization v6
Extend the RVM infrastructure with evaluation context, program metadata, and several new instructions needed by language frontends beyond Rego.
VM & instructions:
Program metadata:
Serialization:
Compiler:
Test infrastructure: