fix(value/array): tighten memory-limit & API-surface review findings

Addresses follow-up review on PR #63 plus our own strict re-review:

- Drop the process-aborting Extend/FromIterator impls. They now match the
  Object convention: delegate directly to the inner container without
  consulting the cooperative memory limit (rely on the allocator to fail
  on true OOM). Limit-aware bulk insertion has explicit fallible methods.
- Add Array::try_from_vec and Array::try_from_iter as the limit-aware
  alternatives to the From<Vec<Value>> / FromIterator pairs.
- with_capacity now also consults check_memory_limit_if_needed after
  the inner try_reserve so capacity reservations cannot silently push
  usage past the cooperative ceiling.
- insert now returns Result<()>: out-of-bounds and limit failures both
  flow through the same anyhow error channel. The limit case rolls back
  the inserted element so the array length is unchanged.
- push and extend_from_slice roll back the pushed element(s) on a limit
  failure rather than leaving the array in a half-mutated state.
- Make the array module itself private (was `pub mod array`); only
  `Array` (always) and `ArrayCursor` (rvm feature) are re-exported from
  `crate::value`. This matches the Object/ObjectCursor pattern and keeps
  the module layout out of the public API surface.
- Rename the rvm-only Cursor type to ArrayCursor for symmetry with
  ObjectCursor and to disambiguate at the value-module level.
- Update docs/value/array.md to reflect the private module, the
  ArrayCursor name, and the limit-enforcement contract.
- Extend the test suite with try_from_vec, try_from_iter, and an
  insert out-of-bounds error case. Update existing insert call sites
  to the Result API.

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

Author: anakrish

docs/value/array.md

@@ -7,17 +7,22 @@ shared design philosophy.
 ## Design
 
 `Array` wraps a `Vec<Value>` today, but the inner vector is private: callers
-cannot pattern-match it or take ownership of the backing store. The public API is
-the curated surface in `src/value/array/mod.rs`, grouped roughly as:
+cannot pattern-match it or take ownership of the backing store. The module
+itself is private (`mod array;` in `value/mod.rs`); `Array` (and the
+`rvm`-gated `ArrayCursor`) are the only types re-exported, so the module
+layout is not part of the public API. The curated surface in
+`src/value/array/mod.rs` groups roughly as:
 
-- construction/conversion: `new`, `with_capacity`, `From<Vec<Value>>`,
-  `FromIterator<Value>`, `IntoIterator`, `From<Array> for Value`, `into_value`
+- construction/conversion: `new`, `with_capacity`, `try_from_vec`,
+  `try_from_iter`, `From<Vec<Value>>`, `FromIterator<Value>`, `IntoIterator`,
+  `From<Array> for Value`, `into_value`
 - read access: `len`, `is_empty`, `get`, `first`, `last`, `as_slice`, indexing
 - mutation: `get_mut`, `push`, `pop`, `clear`, `insert`, `remove`, `truncate`,
   `extend_from_slice`, `iter_mut`, `sort`, `sort_by`, `dedup`, `reverse`
 - iteration: `iter`, borrowed/mutable/owned `IntoIterator`, serde
 - RVM-only resumable traversal: `cursor` and `next`, both compiled only with
-  `#[cfg(feature = "rvm")]`
+  `#[cfg(feature = "rvm")]`; the cursor type is `ArrayCursor`, mirroring
+  `ObjectCursor`.
 
 Iteration follows sequence order. Cursor types support incremental traversal
 needed by the RVM iteration state without exposing iterator internals.
@@ -29,6 +34,20 @@ by materializing a contiguous view internally.
 `Ord` is hand-written against the sequence iterator rather than derived from the
 storage, so future backends compare exactly like today's `Vec<Value>` payload.
 
+## Memory-limit enforcement
+
+Growth-path methods that return `Result` (`push`, `extend_from_slice`,
+`insert`, `with_capacity`, `try_from_vec`, `try_from_iter`) consult the
+cooperative memory limit and report `Err` when it would be exceeded; the
+array is left in a consistent state — any in-progress mutation that would
+cross the limit is rolled back before the error is returned.
+
+Infallible trait impls (`Extend<Value>`, `FromIterator<Value>`,
+`From<Vec<Value>>`) match the [`Object`](object.md) convention and **do not**
+consult the cooperative limit; they exist for ergonomic and migration use and
+rely on the allocator to fail on true OOM. Limit-aware callers must use the
+explicit fallible methods above.
+
 ## Scenarios enabled
 
 - **Inline-small storage** — store short arrays inline and spill to heap only for

src/value/array/iter.rs

@@ -127,7 +127,7 @@ impl<'a> FusedIterator for IterMut<'a> {}
 /// of a long-lived state struct (e.g. an RVM iteration frame).
 #[cfg(feature = "rvm")]
 #[derive(Debug, Clone, Default)]
-pub struct Cursor {
+pub struct ArrayCursor {
     pub(super) next: usize,
 }
 

src/value/array/mod.rs

@@ -11,13 +11,13 @@ use core::cmp::Ordering;
 use core::fmt;
 use core::ops;
 
-use anyhow::Result;
+use anyhow::{bail, Result};
 
 use crate::value::Value;
 
 #[cfg(feature = "rvm")]
 #[allow(unused_imports)] // surface for downstream PRs
-pub use iter::Cursor;
+pub use iter::ArrayCursor;
 #[allow(unused_imports)] // surface for downstream PRs
 pub use iter::{IntoIter, Iter, IterMut};
 
@@ -32,6 +32,21 @@ pub use iter::{IntoIter, Iter, IterMut};
 /// - [`Array::iter`] — sequence order; non-resumable.
 /// - [`Array::cursor`] / [`Array::next`] — sequence order, resumable. Used by
 ///   interpreter/RVM when iteration must yield mid-flight.
+///
+/// # Memory-limit enforcement
+///
+/// Growth-path methods that take a fallible `Result` return value
+/// ([`Array::push`], [`Array::extend_from_slice`], [`Array::insert`],
+/// [`Array::with_capacity`], [`Array::try_from_vec`], [`Array::try_from_iter`])
+/// consult the configured cooperative memory limit and report
+/// `Err` when it is exceeded; the array is left in a consistent state (any
+/// in-progress mutation that would have crossed the limit is rolled back).
+///
+/// Infallible trait impls ([`Extend`], [`FromIterator`], [`From<Vec<Value>>`])
+/// match the [`crate::value::Object`] convention and **do not** consult the
+/// cooperative limit — they exist for ergonomic / migration use and rely on the
+/// allocator to fail on true OOM. Limit-aware callers must use the explicit
+/// fallible methods above.
 #[derive(Default, Clone, Eq, PartialEq)]
 pub struct Array {
     inner: Vec<Value>,
@@ -46,12 +61,14 @@ impl Array {
 
     /// Create an empty `Array` with space for at least `capacity` elements.
     ///
-    /// Returns `None` if the requested capacity is too large or cannot be
-    /// reserved by the allocator.
+    /// Returns `None` if the requested capacity is too large for the
+    /// allocator, or if reserving it would push memory usage past the
+    /// configured cooperative limit.
     #[inline]
     pub fn with_capacity(capacity: usize) -> Option<Self> {
         let mut inner = Vec::new();
         inner.try_reserve(capacity).ok()?;
+        crate::utils::limits::check_memory_limit_if_needed().ok()?;
         Some(Self { inner })
     }
 
@@ -106,12 +123,18 @@ impl Array {
 
     /// Appends `value` to the end of the array.
     ///
-    /// Returns an error if the configured memory limit is exceeded after the
-    /// growth operation.
+    /// Returns `Err` if the cooperative memory limit would be exceeded by the
+    /// appended element; in that case the value is rolled back (popped) before
+    /// returning, so the array length is unchanged.
     #[inline]
     pub fn push(&mut self, value: Value) -> Result<()> {
         self.inner.push(value);
-        crate::utils::limits::check_memory_limit_if_needed().map_err(anyhow::Error::new)
+        if let Err(err) = crate::utils::limits::check_memory_limit_if_needed() {
+            // Roll back the mutation so the caller observes a consistent state.
+            self.inner.pop();
+            return Err(anyhow::Error::new(err));
+        }
+        Ok(())
     }
 
     #[inline]
@@ -126,15 +149,22 @@ impl Array {
 
     /// Inserts `value` at `index`, shifting existing elements right.
     ///
-    /// Returns `Some(())` on success, or `None` if `index > self.len()`
-    /// (rather than panicking — this is a host-reachable API surface).
+    /// Returns `Err` if `index > self.len()` (rather than panicking — this is
+    /// a host-reachable API surface) or if the cooperative memory limit would
+    /// be exceeded by the appended element; in the limit case the insertion is
+    /// rolled back so the array length is unchanged.
     #[inline]
-    pub fn insert(&mut self, index: usize, value: Value) -> Option<()> {
-        if index > self.inner.len() {
-            return None;
+    pub fn insert(&mut self, index: usize, value: Value) -> Result<()> {
+        let len = self.inner.len();
+        if index > len {
+            bail!("Array::insert: index {index} out of bounds (len={len})");
         }
         self.inner.insert(index, value);
-        Some(())
+        if let Err(err) = crate::utils::limits::check_memory_limit_if_needed() {
+            self.inner.remove(index);
+            return Err(anyhow::Error::new(err));
+        }
+        Ok(())
     }
 
     /// Removes and returns the element at `index`, shifting subsequent
@@ -175,11 +205,17 @@ impl Array {
 
     /// Extends the array by cloning values from `other`.
     ///
-    /// Checks the configured memory limit after each appended element.
-    #[inline]
+    /// Checks the cooperative memory limit after each appended element; on
+    /// failure, all elements appended by this call (including the offending
+    /// one) are rolled back so the array length is unchanged.
     pub fn extend_from_slice(&mut self, other: &[Value]) -> Result<()> {
+        let original_len = self.inner.len();
         for value in other {
-            self.push(value.clone())?;
+            self.inner.push(value.clone());
+            if let Err(err) = crate::utils::limits::check_memory_limit_if_needed() {
+                self.inner.truncate(original_len);
+                return Err(anyhow::Error::new(err));
+            }
         }
         Ok(())
     }
@@ -189,6 +225,27 @@ impl Array {
         self.inner.reverse();
     }
 
+    /// Build an `Array` from an existing `Vec<Value>` with memory-limit
+    /// enforcement. Returns `Err` if the cooperative limit is exceeded after
+    /// taking ownership of `values`.
+    #[inline]
+    pub fn try_from_vec(values: Vec<Value>) -> Result<Self> {
+        let array = Self { inner: values };
+        crate::utils::limits::check_memory_limit_if_needed().map_err(anyhow::Error::new)?;
+        Ok(array)
+    }
+
+    /// Collect an iterator of `Value`s into an `Array` with memory-limit
+    /// enforcement per-element. Rolls back any partially-collected state on
+    /// limit failure (the returned `Err` leaves nothing behind).
+    pub fn try_from_iter<I: IntoIterator<Item = Value>>(iter: I) -> Result<Self> {
+        let mut array = Self::new();
+        for value in iter {
+            array.push(value)?;
+        }
+        Ok(array)
+    }
+
     /// Wrap into a `Value::Array`.
     #[inline]
     pub fn into_value(self) -> Value {
@@ -205,13 +262,13 @@ impl Array {
     /// current contents at each stored index.
     #[cfg(feature = "rvm")]
     #[inline]
-    pub const fn cursor(&self) -> Cursor {
-        Cursor { next: 0 }
+    pub const fn cursor(&self) -> ArrayCursor {
+        ArrayCursor { next: 0 }
     }
 
     /// Advance `cursor` and yield the next `(index, value)` pair. O(1).
     #[cfg(feature = "rvm")]
-    pub fn next<'a>(&'a self, cursor: &mut Cursor) -> Option<(usize, &'a Value)> {
+    pub fn next<'a>(&'a self, cursor: &mut ArrayCursor) -> Option<(usize, &'a Value)> {
         let index = cursor.next;
         let value = self.inner.get(index)?;
         cursor.next = index.saturating_add(1);
@@ -256,25 +313,25 @@ impl ops::Index<usize> for Array {
     }
 }
 
-fn abort_on_growth_error(result: Result<()>) {
-    if result.is_err() {
-        alloc::alloc::handle_alloc_error(core::alloc::Layout::new::<Value>());
-    }
-}
+// Bulk-insertion trait impls mirror the [`crate::value::Object`] convention:
+// they delegate directly to the inner container and **do not** consult the
+// cooperative memory limit. Limit-aware callers must use [`Array::push`],
+// [`Array::extend_from_slice`], [`Array::try_from_vec`], or
+// [`Array::try_from_iter`].
 
 impl Extend<Value> for Array {
+    #[inline]
     fn extend<I: IntoIterator<Item = Value>>(&mut self, iter: I) {
-        for value in iter {
-            abort_on_growth_error(self.push(value));
-        }
+        self.inner.extend(iter);
     }
 }
 
 impl FromIterator<Value> for Array {
+    #[inline]
     fn from_iter<I: IntoIterator<Item = Value>>(iter: I) -> Self {
-        let mut array = Self::new();
-        array.extend(iter);
-        array
+        Self {
+            inner: Vec::from_iter(iter),
+        }
     }
 }
 

src/value/mod.rs

@@ -11,7 +11,7 @@
     clippy::as_conversions
 )] // value helpers index paths directly for performance
 
-pub mod array;
+mod array;
 mod object;
 
 #[cfg(test)]
@@ -22,6 +22,9 @@ pub use array::Array;
 #[allow(unused_imports)] // surface for downstream PRs
 pub use object::{IntoIter, Iter, IterMut, Object};
 
+#[cfg(feature = "rvm")]
+#[allow(unused_imports)] // surface for downstream PRs
+pub use array::ArrayCursor;
 #[cfg(feature = "rvm")]
 #[allow(unused_imports)] // surface for downstream PRs
 pub use object::ObjectCursor;

src/value/tests.rs

@@ -609,7 +609,7 @@ fn array_mutators_and_accessors() {
     let mut array = Array::new();
     array.push(val(1)).expect("push should fit");
     array.push(val(3)).expect("push should fit");
-    assert_eq!(array.insert(1, val(2)), Some(()));
+    array.insert(1, val(2)).expect("insert should succeed");
 
     assert_eq!(array.len(), 3);
     assert_eq!(array.get(0), Some(&val(1)));
@@ -641,16 +641,18 @@ fn array_out_of_range_access_is_safe() {
     assert_eq!(array[2], Value::Undefined);
     assert_eq!(array[usize::MAX], Value::Undefined);
 
-    // insert past the end returns None instead of panicking.
-    assert_eq!(array.insert(99, val(0)), None);
+    // insert past the end errors instead of panicking.
+    assert!(array.insert(99, val(0)).is_err());
     assert_eq!(array.len(), 2);
 
     // remove past the end returns None instead of panicking.
     assert_eq!(array.remove(99), None);
     assert_eq!(array.len(), 2);
 
     // insert at exactly len is valid (append-like).
-    assert_eq!(array.insert(2, val(3)), Some(()));
+    array
+        .insert(2, val(3))
+        .expect("insert at len should succeed");
     assert_eq!(array.as_slice(), &[val(1), val(2), val(3)]);
 }
 
@@ -774,3 +776,28 @@ fn array_iterators_are_double_ended_exact_and_fused() {
     assert_eq!(owned.next_back(), Some(val(3)));
     assert_eq!(owned.collect::<Vec<_>>(), vec![val(1), val(2)]);
 }
+
+#[test]
+fn array_try_from_vec_and_try_from_iter() {
+    let from_vec = Array::try_from_vec(vec![val(1), val(2), val(3)])
+        .expect("try_from_vec under no limit succeeds");
+    assert_eq!(from_vec.as_slice(), &[val(1), val(2), val(3)]);
+
+    let from_iter = Array::try_from_iter([val(1), val(2), val(3)])
+        .expect("try_from_iter under no limit succeeds");
+    assert_eq!(from_iter, from_vec);
+}
+
+#[test]
+fn array_insert_out_of_bounds_returns_err() {
+    let mut array = Array::from_iter([val(1), val(2)]);
+    let err = array
+        .insert(99, val(0))
+        .expect_err("out-of-bounds insert should error");
+    let msg = alloc::format!("{err}");
+    assert!(
+        msg.contains("out of bounds"),
+        "error should mention out of bounds: {msg}"
+    );
+    assert_eq!(array.as_slice(), &[val(1), val(2)]);
+}