Storage abstraction for Value::Object and Value::Set (v7 — simplified)

[anakrish]

Summary

Introduces a storage-agnostic abstraction layer for Value::Object and Value::Set. Call sites that previously named BTreeMap<Value, Value> / BTreeSet<Value> directly now go through two public opaque types (Object, Set) whose internal storage is private. Future storage representations (two-tier inline+hash, lazy/streaming, schema-shared, projection-aware, etc.) become internal changes to those types without further call-site migration.

This is a simplified restart of an earlier attempt (PR #55, preserved at branch storage-abstraction-v6-backup and tag v6-snapshot-pre-restart). After review converged on the earlier shape being materially overbuilt, this branch starts fresh from main with a much thinner design modeled on serde_json::Map's pattern.

Headline numbers

Metric	v6 (PR #55)	v7 (this PR)	Reduction
src/collections/ LoC	1828	794	−57%
Total diff lines	+2622 / −546	+1088 / −414	−59%
Files modified	56	35	−38%
Public types	Object, Set, ObjectRef, ObjectRefMut, SetRef, SetRefMut, MapEntry, Occupied/VacantMapEntry, InsertError + ~10 named iterator newtypes	Object, Set	massive
Escape hatches (as_btreemap* etc.)	leaked via #[doc(hidden)] pub	0	—
cargo xtask ci-debug	green	green	—
OPA conformance	2861 / 0	2861 / 0	unchanged

Commits (7)

• 4cacb18 refactor(tests): migrate to Object/Set API
• 2097f75 refactor(languages,schema): migrate to Object/Set API
• 4734bf3 refactor(builtins): migrate to Object/Set API
• c2201f1 refactor(rvm): migrate to Object/Set API and snapshot IterationState
• 0bc88f7 refactor(interpreter): migrate to Object/Set API
• 89c0f8c refactor(value): swap Object/Set payloads through new accessors
• 7969c0e feat(collections): introduce Object/Set storage abstraction (v7)

Each commit corresponds to a per-area logical chunk (collections module → Value wiring → interpreter → RVM → builtins → languages → tests). Intermediate commits were pushed with --no-verify because the pre-commit hook (full fmt/clippy/build) cannot pass on a tree where only some call sites are migrated; the final tree is independently verified via cargo xtask ci-debug and the pre-push hook (which ran OPA conformance) accepted the push.

Public API

// Two opaque public types in src/collections/
pub struct Object { /* private: BTreeMap<Value, Value> */ }
pub struct Set    { /* private: BTreeSet<Value> */ }

// Embedded in Value
pub enum Value {
    // ...
    Set(Rc<Set>),
    Object(Rc<Object>),
    // ...
}

// Value accessors (old names kept; return types updated)
impl Value {
    pub fn as_object(&self) -> Result<&Object>;
    pub fn as_object_mut(&mut self) -> Result<&mut Object>;  // internal Rc::make_mut
    pub fn as_set(&self) -> Result<&Set>;
    pub fn as_set_mut(&mut self) -> Result<&mut Set>;

    pub fn new_object() -> Value;
    pub fn new_set() -> Value;
    pub fn object_from_iter<I: IntoIterator<Item=(Value, Value)>>(it: I) -> Value;
    pub fn set_from_iter<I: IntoIterator<Item=Value>>(it: I) -> Value;

    pub fn object_get(&self, k: &Value) -> Option<&Value>;
    pub fn object_get_str(&self, k: &str) -> Option<&Value>;
    pub fn object_contains_key(&self, k: &Value) -> bool;
    pub fn object_len(&self) -> Option<usize>;
    pub fn set_contains(&self, v: &Value) -> bool;
    pub fn set_len(&self) -> Option<usize>;
}

Object mirrors BTreeMap<Value, Value>'s shape with infallible insert -> Option<Value> and the std-collection methods (get, get_mut, remove, retain, clear, extend, iter, iter_mut, keys, values, etc.). Trait impls: Default, Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize, Extend, FromIterator, IntoIterator for owned/&/&mut, From<BTreeMap>, From<Object> for Value.

Set is symmetric, plus set algebra (intersection/union/difference/is_subset/is_disjoint) returning fresh Set.

Iteration order contract

Following std's distinction (HashMap::iter is implementation-defined; BTreeMap::iter is sorted-by-construction):

• Object::iter() / Set::iter() are implementation-defined order. Today they happen to iterate sorted (storage is BTreeMap-backed), but callers MUST NOT depend on that.
• Object::iter_sorted() / Set::iter_sorted() are sorted by Value::Ord. Use when deterministic order is required.

Internal regorus sites that require sorted output use iter_sorted() explicitly: Serialize (canonical JSON), Debug (stable error messages), Ord/PartialOrd, object.keys / object.values builtins, print() builtin output, RVM IterationState snapshot (preserving RVM↔interpreter comprehension parity).

The contract frees future non-sorted storage variants from amortized-sort-cache complexity; callers that need order have already opted in.

What is NOT in this PR (deliberate)

• No ObjectRef / ObjectRefMut / SetRef / SetRefMut borrow-view types — &Object / &mut Object cover the same use cases. No surveyed JSON library uses this layer.
• No MapEntry / OccupiedMapEntry / VacantMapEntry — the 4 audit-confirmed entry-pattern sites migrate to contains_key + insert.
• No named iterator newtypes — methods return impl Iterator + '_. Saves ~370 LoC of boilerplate.
• No ObjectStorage / SetStorage separate one-variant enum layer — Object wraps BTreeMap directly. When a future non-BTree variant ships, Object's private field becomes an enum without affecting call sites.
• No InsertError / try_insert — insert returns Option<Value>, identical to BTreeMap::insert. If a future storage variant requires key validation, that becomes a separate concern handled at construction (Number::from(f64)) or at the backend boundary.
• No as_btreemap* / as_btreeset* public escape hatches — any caller needing a BTreeMap materializes one via obj.iter().collect(). Zero such hatches exist in src/.
• No #[non_exhaustive] on Value — deferred to a separate breaking-change PR.
• No Hash for Value / Hash for Number — Value doesn't implement Hash today; deferred until a hash-backed storage variant actually needs it.

What CHANGED (the breaking part)

• Value::Object and Value::Set payload types changed:
  • Value::Object(Rc<BTreeMap<Value, Value>>) → Value::Object(Rc<Object>)
  • Value::Set(Rc<BTreeSet<Value>>) → Value::Set(Rc<Set>)
• Value::as_object, as_object_mut, as_set, as_set_mut return types changed from &BTreeMap / &BTreeSet (etc.) to &Object / &Set (etc.). Method NAMES are unchanged — the rename is to the return type only. Most call sites compile unchanged because Object / Set carry the same method shape as BTreeMap / BTreeSet (get, insert, iter, len, contains_key/contains, etc.).

External Rust consumers will see a compile error if they pattern-match Value::Object(ref m) expecting &Rc<BTreeMap<...>>, or if they pass an &BTreeMap from as_object() to another crate. Migration is mechanical.

RVM IterationState

src/rvm/vm/loops.rs previously used BTreeMap::range((Excluded(prev_key), Unbounded)) to resume iteration past a yielded position. v7 replaces this with a Rc<[Value]> snapshot pattern:

pub enum IterationState {
    // ...
    Object { obj: Rc<Object>, keys: Rc<[Value]>, pos: usize },
    Set    { values: Rc<[Value]>, pos: usize },
}

The snapshot is built via iter_sorted() so RVM and interpreter produce identical comprehension outputs (the dual-path invariant). Memory-limit checks are interleaved into the snapshot build (not just before/after the collect) to maintain allocator-limit behavior under large inputs.

Validation

• cargo xtask ci-debug — GREEN (fmt, clippy, build × multiple feature combos, workspace tests, OPA conformance, no-std build).
• OPA conformance: 2861 / 0 (unchanged from main).
• cargo xtask test-no-std — GREEN (thumbv7m-none-eabi).
• cargo xtask test-all-bindings — GREEN (Python may skip due to local env Python 3.9 vs 3.10+ requirement — not a code issue).
• #![forbid(unsafe_code)] preserved.
• Zero new dependencies in Cargo.toml.

Layout / size analysis

size_of::<Value>(): 24 bytes (unchanged from main).

Empty Value::Object heap+stack footprint:

• main: 56 B (1 Rc cell containing empty BTreeMap)
• v7: 56 B (1 Rc cell containing Object { inner: BTreeMap } — Object is a zero-overhead wrapper around BTreeMap)

No regression vs main. When a future non-BTree variant ships (e.g., two-tier inline+hash for compact small objects), the cost is borne by that variant's footprint, not by the abstraction layer.

Future flexibility (same as v6 PR #55)

• Future storage variants (two-tier inline+hash, LazyObject, immutable boxed-slice, schema-shared, projection-aware) are enabled by changing Object's private inner field from BTreeMap to an enum. Zero call-site migration required.
• Per-instance variant choice (e.g., a SARIF parser picking Inline for small objects, BTree for large) is supported via future Object::with_capacity / Object::new_inline / Object::new_lazy(provider) constructors returning opaque Object.
• #[non_exhaustive] on Value and removal of legacy as_object / as_set overloads (if needed) are independent future PRs.

Comparison vs PR #55

Reviewers of PR #55 flagged 23 issues. This PR side-steps essentially all of them by removing the layers that made them possible: no ObjectRef/Mut (eliminates iteration-order-contract gaps between Object and ObjectRef), no InsertError (eliminates the validation-bypass class — F1/F21 in the v6 review), no as_btreemap* escape hatches (eliminates F4), no MapEntry (eliminates F19), etc. The IterationState memory-check gap (F22) is addressed by per-push checks in the snapshot loop.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

[anakrish]

@copilot Review this PR using all the revie skills in this repository and report the results in detail. Use a separate agent for each skill.

[Copilot]

Pull request overview

Introduces opaque Object and Set collection wrappers for Value::Object and Value::Set, enabling future storage changes without another broad call-site migration. The PR also updates interpreter, RVM, builtins, schema, language-specific code, and tests to use the new abstraction.

Changes:

• Adds src/collections with public Object/Set APIs, serde/debug/order semantics, and oracle tests.
• Rewires Value, RVM iteration snapshots, compiler literals, and builtins to use Object/Set.
• Updates Azure Policy/RBAC, schema validation, metadata serialization, and tests for the new payload types.

Reviewed changes

Copilot reviewed 35 out of 35 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
src/collections/mod.rs	Adds collection abstraction module and iteration-order contract.
src/collections/object.rs	Implements opaque Object wrapper.
src/collections/set.rs	Implements opaque Set wrapper.
src/collections/tests.rs	Adds BTreeMap/BTreeSet oracle coverage.
src/value.rs	Changes Value::Object/Set payloads and accessors.
src/lib.rs	Exposes Object/Set and renames internal set alias.
src/interpreter.rs	Migrates rule output/object updates to Object APIs.
src/rvm/vm/context.rs	Changes iteration state to key/value snapshots.
src/rvm/vm/loops.rs	Builds sorted snapshots for RVM loop iteration.
src/rvm/vm/comprehension.rs	Applies snapshot iteration to comprehensions.
src/rvm/vm/dispatch.rs	Uses new set constructors in VM dispatch.
src/rvm/vm/arithmetic.rs	Uses Set::difference.
src/rvm/program/serialization/value.rs	Serializes/deserializes binary values through Object/Set.
src/rvm/program/metadata.rs	Builds metadata values with Object/Set.
src/languages/rego/compiler/expressions/collection_literals.rs	Hoists constant collections as Object/Set.
src/languages/azure_policy/compiler/effects.rs	Builds object templates with Object.
src/languages/azure_policy/compiler/metadata.rs	Stores metadata set values using Set.
src/languages/azure_policy/aliases/obj_map.rs	Converts alias maps to/from Object.
src/languages/azure_policy/aliases/normalizer/mod.rs	Accepts Object during normalization.
src/languages/azure_policy/aliases/normalizer/element_remap.rs	Uses Object in remap helpers.
src/languages/azure_policy/aliases/denormalizer/mod.rs	Merges properties via Object helper API.
src/languages/azure_policy/aliases/denormalizer/sub_resource.rs	Uses Object in denormalization helpers.
src/languages/azure_rbac/builtins/lists.rs	Accepts Set in RBAC list containment.
src/schema/validate.rs	Validates schema objects via Object.
src/schema/tests/suite.rs	Updates default object assertion.
src/scheduler.rs	Qualifies enum variants to avoid Object name clash.
src/compiled_policy.rs	Uses renamed internal MapSet alias.
src/builtins/utils.rs	Returns Rc<Object>/Rc<Set> from helpers.
src/builtins/sets.rs	Uses new Set algebra methods.
src/builtins/objects.rs	Uses Object::get_or_insert_with.
src/builtins/graph.rs	Uses Object and explicit sorted set iteration.
src/builtins/azure_policy/template_functions_collection.rs	Builds/merges objects via Object.
src/builtins/azure_policy/template_functions_misc.rs	Builds item objects via Object.
tests/rvm/compiler.rs	Updates set literal-hoisting expectation.
tests/azure_policy/mod.rs	Updates request context insertion helpers.

[Copilot]

Good catch — fixed. template is wrapped directly in Rc::new(template) now, avoiding the redundant drain+rebuild.

[Copilot]

@copilot Review this PR using all the revie skills in this repository and report the results in detail. Use a separate agent for each skill.

PR Review Results — code-review + deep-review Skills

Both review skills completed. Results synthesised below. The two skills agreed on all material findings; the deep-review adversarial verifier dismissed 4 false-positive candidates. Zero panics, zero wrong policy results, and zero security issues were found. Full detail per finding follows.

---

🔴 HIGH — Intentional but Breaking Public API Change

Location: src/value.rs:63–67, src/value.rs:1255, src/value.rs:1300
Agents: code-review + deep-review (CONFIRMED)

Value::Set(Rc<BTreeSet<Value>>) → Value::Set(Rc<Set>) and Value::Object(Rc<BTreeMap<…>>) → Value::Object(Rc<Object>), plus as_set()/as_object() return-type changes. Any downstream Rust crate that pattern-matches on Value::Object(ref m) expecting a BTreeMap, or receives a &BTreeMap from as_object(), will fail to compile. The PR description explicitly calls this out as the breaking part. The finding is valid and intentional, but worth flagging explicitly for any downstream consumers:

• The 9 FFI bindings (Python, C, Wasm, …) need explicit compilation verification.
• External Rust consumers that previously pattern-matched Value::Object(ref m) require mechanical migration: use as_object() instead of destructuring.

No fix required if a major semver bump accompanies the release; otherwise provide a migration guide or transition shims.

---

🟡 MEDIUM — Binary Serialization Uses iter() not iter_sorted()

Location: src/rvm/program/serialization/value.rs:128 (BinarySetRef) and :143 (BinaryObjectRef)
Agents: code-review + deep-review (CONFIRMED)

// BinarySetRef::serialize — line 128
for value in self.0.iter() {          // ← should be iter_sorted()

// BinaryObjectRef::serialize — line 143
for (key, value) in self.0.iter() {   // ← should be iter_sorted()

The PR's own stated contract: "callers MUST NOT depend on [iter()] order." The binary serialization format is used to persist and reload RVM program literals (from_bytes::<Vec<BinaryValue>>). If a future non-BTree storage variant changes the iteration order of iter(), two programs with identical semantics will produce different bytes — breaking content-addressed caching, binary equality checks, and round-trip reproducibility.

Today the backing is BTreeMap/BTreeSet, so iter() and iter_sorted() produce the same bytes — no regression yet. But the PR's whole point is that storage will change, and this is a time-bomb.

Fix (two lines):

// line 128
for value in self.0.iter_sorted() {

// line 143
for (key, value) in self.0.iter_sorted() {

---

🟡 MEDIUM — IntoIterator Impls Leak Concrete btree_map::IntoIter Types

Location: src/collections/object.rs:212, 221, 230 (and symmetric in set.rs)
Agents: code-review + deep-review (CONFIRMED)

impl IntoIterator for Object {
    type IntoIter = alloc::collections::btree_map::IntoIter<Value, Value>;  // leaks impl
    fn into_iter(self) -> Self::IntoIter { self.inner.into_iter() }
}
impl<'a> IntoIterator for &'a Object {
    type IntoIter = alloc::collections::btree_map::Iter<'a, Value, Value>;  // leaks impl
    ...
}

The type IntoIter = … associated type is part of the public API contract. Any caller that writes let it: btree_map::IntoIter<_, _> = obj.into_iter() (or captures the iterator type in a struct/generic bound) will break if the storage changes. This directly contradicts the PR's stated goal: "Future storage representations become internal changes to those types without further call-site migration."

Fix: Introduce private newtype iterator wrappers:

pub struct ObjectIntoIter(alloc::collections::btree_map::IntoIter<Value, Value>);
impl Iterator for ObjectIntoIter { … }

impl IntoIterator for Object {
    type IntoIter = ObjectIntoIter;
    fn into_iter(self) -> ObjectIntoIter { ObjectIntoIter(self.inner.into_iter()) }
}

The same pattern applies to &Object, &mut Object, and the Set equivalents. This is ~40 lines of boilerplate but the only way to preserve the storage-agnostic guarantee for owned iteration.

---

🟡 MEDIUM — Serde Round-Trip Lossiness for Non-String Object Keys

Location: src/collections/object.rs:264
Agent: code-review

let key_str = serde_json::to_string(k).map_err(Error::custom)?;
// e.g. Value::Number(1) serializes as key "1"
// Value::Bool(true) serializes as key "true"

JSON only allows string map keys; this is unavoidable at the JSON boundary. However, a round-trip Object → JSON → Object silently changes Value::Number(1) keys to Value::String("1") keys and Value::Bool(true) to Value::String("true"). Any Rego policy that uses non-string keys in an object and round-trips through serde_json will see different key types on the other side.

This is a pre-existing limitation of the JSON encoding (not introduced by this PR), but the new Deserialize impl in collections/object.rs makes it easier for external callers to hit it. Worth documenting explicitly in the Serialize/Deserialize impls.

Fix: Add a doc comment on impl Serialize for Object noting that non-string keys are stringified and that JSON → Object deserialization cannot reconstruct the original key type. No code change needed if this behavior is intentional.

---

🟢 LOW — Index Impl Panic Reachable via Public API

Location: src/collections/object.rs:245–251
Agent: deep-review

impl core::ops::Index<&Value> for Object {
    #[allow(clippy::indexing_slicing)] // BTreeMap::Index panics on missing — matches std contract
    fn index(&self, key: &Value) -> &Value { &self.inner[key] }
}

Index panics on a missing key, consistent with std's contract. This is low severity because: (a) obj[key] syntax makes the panic nature visible to callers; (b) regorus internals that previously used btreemap[key] already had this panic. However, making it pub on an Object that callers can now hold means any external code calling obj[&Value::String("missing")] will panic — and a panic across an FFI boundary poisons the process.

Fix: Consider pub(crate) scope or documenting the panic explicitly with # Panics in the doc comment. The existing get() method is the safe alternative.

---

🟢 LOW — iter_sorted() vs iter() Naming Could Mislead Future Maintainers

Location: src/collections/object.rs:96–97, src/collections/set.rs:83–84

pub fn iter_sorted(&self) -> impl DoubleEndedIterator<…> + '_ {
    self.inner.iter()  // today: always sorted because BTree
}

iter_sorted() currently delegates directly to self.inner.iter() without any explicit sort step. If a future storage variant is added where inner.iter() is not pre-sorted, a maintainer adding a new inner variant must remember to sort here — with no compile-time enforcement. Consider a debug assertion or a comment making this explicit.

---

✅ Correctly Implemented (adversarial verifier confirmed)

• IterationState snapshot/advance/resume — Object { obj, keys, pos } correctly builds a sorted key snapshot and resumes iteration from pos. Memory-limit checks interleaved in the snapshot loop.
• Serialize for Object uses iter_sorted() — canonical JSON serialization is deterministic.
• Serialize for Set uses iter_sorted() (serde collect_seq) — correct.
• Deserialize for Object/Set — memory limits enforced per-entry via enforce_limit().
• Ord/PartialOrd for Object/Set — correctly delegated to inner BTree ordering, consistent with prior semantics.
• From<BTreeMap<Value,Value>> for Object and From<BTreeSet<Value>> for Set — correct zero-cost wrappers.
• Vacuous truth for empty every — unchanged and correct.
• Value::as_object_mut / as_set_mut — correct Rc::make_mut pattern.

---

Summary

5 findings: 1 High, 3 Medium, 2 Low. No Critical.

The abstraction is structurally sound and the OPA conformance/test suite is unaffected. The two actionable items are: (1) binary serialization should use iter_sorted() to honour the abstraction contract before storage changes, and (2) IntoIterator associated types should be newtype-wrapped to preserve storage flexibility. The High finding (semver break) is intentional per the PR design — it just needs a major-version bump or migration guide on release.