Storage abstraction for Value::Object and Value::Set - #55
Closed
anakrish wants to merge 6 commits into
Closed
Conversation
Introduce the v6 storage-agnostic abstraction layer for Value::Object and Value::Set per DESIGN.md. - New crate::collections module with Object/Set opaque public types wrapping pub(crate) ObjectStorage/SetStorage enums (BTree-only variant in this PR). - ObjectRef/ObjectRefMut, SetRef/SetRefMut borrow views (no unsafe; ObjectRefMut duplicates reads instead of Deref). - MapEntry/OccupiedMapEntry/VacantMapEntry entry API. - Named iterator types with ExactSizeIterator/FusedIterator/Clone. - InsertError::NonFiniteKey for uniform key validation. - Property tests vs BTreeMap/BTreeSet oracle at sizes 0..1024. - New Value methods: object_ref/_mut, set_ref/_mut, new_object/_set, object_from_iter/set_from_iter, object_len/set_len, object_get, object_get_str, object_contains_key, set_contains. - Legacy as_object/_mut, as_set/_mut continue to return real &BTreeMap/&BTreeSet via DerefMut on the wrapper, no allocation copy. - RVM IterationState uses Rc<[Value]> snapshot pattern (DESIGN §7.3); loops.rs range resumption replaced by index-cursor advance. - ensure_set/ensure_object signatures return Rc<Set>/Rc<Object>. - Transitional doc-hidden Deref/DerefMut Object->BTreeMap and Set->BTreeSet enables existing call sites to continue operating through auto-deref while the abstraction lands. - Renamed crate-local Set->StdHashOrBTreeSet, MapEntry->StdMapEntry to free the new public names. Tests: cargo test (all targets) — 302 tests pass. Clippy: clean under cargo clippy --all-targets --all-features. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ll callers to ObjectRef/SetRef API The prior commit (ddb4536) introduced Object and Set opaque newtypes embedded in Value::Object and Value::Set, but shipped them with transitional Deref<Target=BTreeMap<Value,Value>> / Deref<Target=BTreeSet<Value>> impls. Those impls let every existing call site reach the inner BTreeMap/BTreeSet unchanged, which defeated the storage abstraction's primary goal: any future non-BTree storage variant (two-tier inline+hash objects, lazy/streaming objects, etc. — see DESIGN.md §10) would have broken hundreds of call sites in one shot. This commit removes the bridge and migrates ~200 call sites across ~38 files to the new wrapper API: collections: - Delete impl Deref/DerefMut for Object and Set. - Rename Object::try_insert -> Object::insert and Set::try_insert -> Set::insert (the try_ prefix existed only because plain insert would have been shadowed via DerefMut). - Extend ObjectRef / ObjectRefMut / SetRef / SetRefMut with the additional methods needed by callers (e.g. SetRef::first as iter().next() shorthand, additional convenience getters). value.rs: - #[deprecated] Value::as_object / as_object_mut / as_set / as_set_mut with a note pointing callers at object_ref / object_ref_mut / set_ref / set_ref_mut. The shim bodies dereference through Object::storage / Set::storage explicitly (rather than the now- removed Deref) and carry #[allow(deprecated)] only on their own bodies. - Migrate value.rs internal callers (merge, Index impl, Serialize, make_or_get_value_mut, etc.) off the deprecated shims. call-site migration (recipes in DESIGN.md §7.1): - 'rc.iter() / get / contains_key / contains' on Rc<Object>/Rc<Set> pattern bindings -> 'rc.as_ref().iter()' etc. - 'Rc::make_mut(rc).insert(k, v)' -> '.as_mut().insert(k, v)?' (the ? propagates InsertError::NonFiniteKey through anyhow / thiserror callers, never actually fires for BTree storage but the contract is uniform). - 'Rc::make_mut(rc).{remove,entry,extend,retain,clear,iter_mut, values_mut,append}' likewise route through .as_mut(). - 'v.as_object()?.X' -> 'v.object_ref()?.X' (or v.object_get(k) / v.object_contains_key(k) when there is a fast-path on Value). - Set algebra: BTreeSet::{intersection,union,difference} returned iterators; SetRef::{intersection,union,difference} return owned Set values per DESIGN. Callers that previously did '.intersection(...).cloned().collect()' now use '.intersection(...).into_value()' or iterate the returned Set. migrated modules: src/{value,interpreter,engine,test_utils,lib,tests/common}.rs src/builtins/{aggregates,encoding,graph,objects,sets,strings, utils,uuid,azure_policy/*}.rs src/rvm/{vm/{arithmetic,comprehension,dispatch,virtual_data}, program/{metadata,serialization/value}}.rs src/languages/azure_policy/{aliases/*,compiler/*}.rs src/languages/azure_rbac/builtins/lists.rs src/schema/{validate,tests/suite}.rs tests/{azure_policy,azure_policy_builtins,opa,parser,value, ensure_no_std}/... examples/regorus/azure_policy.rs Verification: - cargo xtask ci-debug: GREEN - grep 'try_insert' src/ tests/: 0 matches - grep '\.as_object\b|\.as_set\b|\.as_object_mut\b|\.as_set_mut\b' src/: 0 matches - #[allow(deprecated)] in src/: only on the 4 shim bodies in value.rs - Deref in src/collections/: 0 matches - object_ref/set_ref adoption in src/: 49 call sites - #![forbid(unsafe_code)] preserved No Value enum variants changed. Memory-limit semantics unchanged (enforce_limit_anyhow still called by the caller after each insert, just as before — the ? added by the migration runs before any limit check, which is the desired ordering: validate the key, then check the new memory footprint). Both execution paths (interpreter and RVM) still produce identical results for the same input (verified by the RVM yaml suite and OPA conformance suite in ci-debug). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The transitional ObjectStorage::BTree(Rc<BTreeMap>) and SetStorage::BTree(Rc<BTreeSet>) introduced a redundant Rc layer on top of the outer Rc<Object>/Rc<Set> at the Value level, costing one extra heap allocation (~24 B refcount cell + payload) and one extra pointer hop per Object/Set instance. The outer Rc<Object>/Rc<Set> already provides COW semantics via Rc::make_mut, so the inner Rc is removed: ObjectStorage::BTree(BTreeMap<Value, Value>) SetStorage::BTree(BTreeSet<Value>) Layout now matches the pre-abstraction Value::Object(Rc<BTreeMap>): one heap allocation per Object/Set, no extra pointer hop on access. The dead from_rc_btreemap / from_rc_btreeset constructors on ObjectStorage / Object / SetStorage / Set are removed (verified zero external callers via grep). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Allows ergonomic iteration via 'for (k, v) in &obj' and 'for v in &set' without an explicit .iter() call. Pure additive ergonomic helpers; the abstraction surface area is unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…entation-defined, iter_sorted() is explicit Following std's convention distinction (HashMap::iter() is unordered; BTreeMap::iter() is sorted-by-construction), the abstraction's public iter() / SetRef::iter() now carries an implementation-defined-order contract. iter_sorted() / SetRef::iter_sorted() is the explicit opt-in for callers that require deterministic order. Rationale: the whole point of abstracting away the storage type is to free future storage variants (hash tier, lazy, schema-shared, etc.) from amortized-sort-cache complexity. Sites that require sorted order declare it explicitly via iter_sorted(); sites that don't get the variant's native iteration order for free. Today's observable behavior is unchanged because the default storage is still BTreeMap-backed — both iter() and iter_sorted() return entries sorted because BTreeMap::iter() is sorted. The contract is weaker for iter(), allowing future variants to deliver implementation- defined order without per-caller migration. Renames: - Iter type (sorted) → IterSorted - IterUnordered (delegates to sorted today) → Iter - SetIter (sorted) → SetIterSorted - SetIterUnordered → SetIter - method iter() → iter_sorted() (sorted-required callers) - method iter_unordered() → iter() (default) Migrated one internal caller (src/builtins/graph.rs:130) from .iter().rev() to .iter_sorted().rev() because the unsorted SetIter does not implement DoubleEndedIterator. DESIGN.md §4 (Iteration order contract) updated. OPA conformance: 2861/0 (unchanged from previous commit). cargo xtask ci-debug: green. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Owner
Author
|
@copilot Review this PR using all the different review skills in this repo. Use a separate agent for each skill. |
There was a problem hiding this comment.
Pull request overview
This PR introduces a storage abstraction for Value::Object and Value::Set, replacing direct BTreeMap/BTreeSet usage with public opaque collection types and borrow views. It also migrates interpreter, RVM, builtins, tests, bindings, and Azure Policy helpers to the new API.
Changes:
- Adds
src/collectionswithObject,Set, refs, iterators, entry API, storage wrappers, and validation errors. - Updates
Valuevariants and APIs, with deprecated compatibility shims foras_object/_mutandas_set/_mut. - Refactors RVM/interpreter/builtins/tests to use the abstraction and snapshot-based RVM collection iteration.
Reviewed changes
Copilot reviewed 56 out of 56 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
src/collections/mod.rs |
Adds public collection module exports. |
src/collections/object.rs |
Defines Object and object borrow views. |
src/collections/set.rs |
Defines Set and set borrow views. |
src/collections/object_storage.rs |
Adds internal object storage and key validation. |
src/collections/set_storage.rs |
Adds internal set storage. |
src/collections/entry.rs |
Adds object entry API wrappers. |
src/collections/error.rs |
Adds InsertError. |
src/collections/iter.rs |
Adds named iterator wrappers. |
src/collections/tests.rs |
Adds oracle-style collection tests. |
src/value.rs |
Switches object/set variants and adds new APIs. |
src/interpreter.rs |
Migrates interpreter object/set access and mutation. |
src/rvm/vm/context.rs |
Changes RVM iteration state to snapshots. |
src/rvm/vm/loops.rs |
Builds RVM object/set iteration snapshots. |
src/rvm/vm/comprehension.rs |
Updates comprehension iteration/result handling. |
src/rvm/vm/dispatch.rs |
Updates object/set bytecode instructions. |
src/rvm/vm/virtual_data.rs |
Migrates nested object updates/lookups. |
src/rvm/vm/arithmetic.rs |
Uses new set difference helper. |
src/rvm/program/serialization/value.rs |
Serializes wrapped object/set storage. |
src/rvm/program/metadata.rs |
Constructs metadata values with new wrappers. |
src/lib.rs |
Exposes collections module and renames internal aliases. |
src/compiled_policy.rs |
Uses renamed internal set alias. |
src/engine.rs |
Uses object_ref for data validation. |
src/builtins/utils.rs |
Returns wrapped object/set types. |
src/builtins/sets.rs |
Migrates set builtins. |
src/builtins/objects.rs |
Migrates object builtins. |
src/builtins/graph.rs |
Uses new object/set APIs and sorted iteration where needed. |
src/builtins/aggregates.rs |
Migrates set iteration in aggregate builtins. |
src/builtins/uuid.rs |
Updates UUID timestamp API usage. |
src/builtins/azure_policy/template_functions_misc.rs |
Migrates Azure Policy object item creation. |
src/builtins/azure_policy/template_functions_collection.rs |
Migrates Azure Policy collection functions. |
src/languages/rego/compiler/expressions/collection_literals.rs |
Emits wrapped object/set literals. |
src/languages/azure_rbac/builtins/lists.rs |
Accepts wrapped set type. |
src/languages/azure_policy/compiler/utils.rs |
Migrates JSON-to-runtime object building. |
src/languages/azure_policy/compiler/mod.rs |
Migrates parameter defaults object building. |
src/languages/azure_policy/compiler/metadata.rs |
Wraps metadata set annotations. |
src/languages/azure_policy/compiler/effects.rs |
Migrates default lookup and template object construction. |
src/languages/azure_policy/aliases/obj_map.rs |
Migrates Azure Policy alias object manipulation. |
src/languages/azure_policy/aliases/normalizer/mod.rs |
Uses object views in normalization. |
src/languages/azure_policy/aliases/normalizer/flatten.rs |
Uses object views while flattening. |
src/languages/azure_policy/aliases/normalizer/element_remap.rs |
Migrates nested object remapping. |
src/languages/azure_policy/aliases/normalizer/alias_resolution.rs |
Migrates alias path navigation. |
src/languages/azure_policy/aliases/denormalizer/mod.rs |
Migrates denormalization object merging. |
src/languages/azure_policy/aliases/denormalizer/sub_resource.rs |
Migrates sub-resource object handling. |
src/schema/validate.rs |
Accepts Object in schema validation helpers. |
src/schema/tests/suite.rs |
Updates expected empty object construction. |
src/test_utils.rs |
Migrates test value processing. |
src/tests/common.rs |
Migrates shared test helper object/set processing. |
tests/value/mod.rs |
Updates value API tests. |
tests/rvm/compiler.rs |
Updates expected set literal construction. |
tests/parser/mod.rs |
Uses object_ref. |
tests/opa.rs |
Uses object views for bindings. |
tests/ensure_no_std/src/main.rs |
Adjusts hosted vs bare-metal no-std test entrypoints. |
tests/azure_policy/mod.rs |
Migrates Azure Policy test object construction. |
tests/azure_policy_builtins/mod.rs |
Migrates YAML mapping conversion. |
examples/regorus/azure_policy.rs |
Updates example object mutations. |
bindings/python/src/lib.rs |
Converts wrapped object/set values to Python objects. |
Comment on lines
+23
to
26
| let set = set_value.set_ref_mut()?; | ||
| for item in v["set!"].as_array()? { | ||
| set.insert(process_value(item)?); | ||
| } |
Comment on lines
+36
to
39
| let object = object_value.object_ref_mut()?; | ||
| for item in v["object!"].as_array()? { | ||
| object.insert(process_value(&item["key"])?, process_value(&item["value"])?); | ||
| } |
Comment on lines
+56
to
59
| let object = object_value.object_ref_mut()?; | ||
| for (key, value) in fields.iter() { | ||
| object.insert(process_value(key)?, process_value(value)?); | ||
| } |
Comment on lines
+264
to
+272
| impl Serialize for Object { | ||
| fn serialize<S: Serializer>(&self, serializer: S) -> Result<S::Ok, S::Error> { | ||
| let map = self.storage.as_btreemap(); | ||
| let mut s = serializer.serialize_map(Some(map.len()))?; | ||
| for (k, v) in map { | ||
| s.serialize_entry(k, v)?; | ||
| } | ||
| s.end() | ||
| } |
Comment on lines
716
to
+721
| let mut set = BTreeSet::new(); | ||
| for ® in params.element_registers() { | ||
| set.insert(self.get_register(reg)?.clone()); | ||
| } | ||
|
|
||
| let set_value = Value::Set(crate::Rc::new(set)); | ||
| let set_value = Value::Set(crate::Rc::new(set.into())); |
Comment on lines
+455
to
+458
| let values: alloc::vec::Vec<Value> = crate::collections::Set::as_ref(set) | ||
| .iter() | ||
| .cloned() | ||
| .collect(); |
Comment on lines
+56
to
+59
| let keys: alloc::vec::Vec<Value> = crate::collections::Object::as_ref(&obj) | ||
| .keys() | ||
| .cloned() | ||
| .collect(); |
Comment on lines
+71
to
+74
| let values: alloc::vec::Vec<Value> = crate::collections::Set::as_ref(&set) | ||
| .iter() | ||
| .cloned() | ||
| .collect(); |
Comment on lines
+153
to
+156
| let keys: alloc::vec::Vec<Value> = crate::collections::Object::as_ref(&obj) | ||
| .keys() | ||
| .cloned() | ||
| .collect(); |
Comment on lines
+168
to
+171
| let values: alloc::vec::Vec<Value> = crate::collections::Set::as_ref(&set) | ||
| .iter() | ||
| .cloned() | ||
| .collect(); |
Completed both review skills with separate agents:
Deep review report saved at |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Introduces a storage-agnostic abstraction layer for
Value::Objectand
Value::Set. ~430 call sites that previously namedBTreeMap<Value, Value>/BTreeSet<Value>directly now go throughpublic opaque types (
Object,Set) and borrow views (ObjectRef,ObjectRefMut,SetRef,SetRefMut). The internal storage(
ObjectStorage/SetStorage) ispub(crate)and currently has asingle
BTreevariant — zero behavioral change at landing time.Once this lands, future storage representations (two-tier
inline+hash, LazyObject/streaming, immutable input-origin variants,
schema-shared layouts, projection-aware backends, etc.) become
internal-only changes that do not require another ~430-site
migration. See
memory-pressure/DESIGN.md§10 in thecompanion design doc for the enumeration of future variants this
abstraction enables.
Commits
feat(collections): introduce Object/Set storage abstraction— adds the
src/collections/module with all public types,storage layer, property tests vs
BTreeMap/BTreeSetoracle,and wires
Value::Object(Rc<Object>)/Value::Set(Rc<Set>).refactor(collections): remove transitional Deref bridge and migrate all callers to ObjectRef/SetRef API— deletes thetransitional
Deref<Target=BTreeMap>leak; mechanicallymigrates ~234 call sites that were silently relying on it;
applies
#[deprecated]toas_object/_mut/as_set/_mut.perf(collections): eliminate inner Rc; restore single-allocation layout— removes a redundantRc<BTreeMap>layer insideObjectStorage::BTree. OuterRc<Object>already provides COW;empty-Object footprint returns to 56 B (parity with today's
Value::Object(Rc<BTreeMap>)).feat(collections): impl IntoIterator for &Object/&mut Object/&Set— ergonomic helpers so
for (k, v) in &objworks.feat(collections): invert iteration-order contract — iter() is implementation-defined, iter_sorted() is explicit— followsstd's distinction (
HashMap::iteris unordered;BTreeMap::iteris sorted-by-construction). Frees futurenon-sorted storage variants from amortized-sort-cache
complexity. Today's BTree storage still iterates sorted so
observable behavior is unchanged.
What ships
Object,Set,ObjectRef,ObjectRefMut,SetRef,SetRefMut,MapEntry(+OccupiedMapEntry/VacantMapEntry),InsertError, named iterator types(
Iter,IterSorted,IterMut,Keys,Values,ValuesMut,SetIter,SetIterSorted,SetIntoIter,IntoIter).Valuemethods:object_ref,object_ref_mut,set_ref,set_ref_mut,new_object,new_set,object_from_iter,set_from_iter,object_get,object_get_str,object_contains_key,object_len,set_contains,set_len.Value::as_object,Value::as_object_mut,Value::as_set,Value::as_set_mut.Shims work but the mutable forms are intentionally slow
(documented in the deprecation note).
IterationStaterefactored toRc<[Value]>snapshotpattern to support the new abstraction; preserves resumable
comprehension semantics.
What does NOT ship
#[non_exhaustive]onValue(a separate follow-onbreaking-change release).
as_object/_mut/as_set/_mut(samefollow-on release).
Valuevariants.BTree— the abstraction is thedeliverable; future variants are separate workstreams.
Hash for Value/Hash for Number(deferred to whicheverfuture variant first needs them; design spec'd in DESIGN.md §6).
Validation
cargo xtask ci-debug— GREEN (fmt, clippy, build × multiplefeature combos, workspace tests, OPA conformance, no_std build).
baseline on `main`).
cargo xtask test-no-std— GREEN (`thumbv7m-none-eabi`).cargo xtask test-all-bindings— GREEN (Python skipped due tolocal env Python 3.9; bindings require 3.10+ — not a code issue).
Benchmarks (Darwin, ACI workload)
Compared `main` vs `storage-abstraction-v6` on `bench aci_benchmark`
and `bench rvm_benchmark hot/aci` (45 measurements):
All within ±5% acceptance gate; most within Criterion's typical
run-to-run noise band.
Layout / size
`size_of::()`: 24 bytes (unchanged from main).
Empty Object heap+stack footprint:
Migration
match e { BTreeMapEntry::* => ... }match e? { MapEntry::* => ... }(entry() now fallible)iter_sorted()iter()Design doc with full per-pattern recipe table:
memory-pressure/DESIGN.md(companion repo).Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com
EOF
)