Skip to content

Commit 17196a8

Browse files
olwangclaude
andcommitted
reg VM: make Managed transparent on read; don't wrap immutable scalars
The reg VM models the 'manage' keyword as VmValue::Managed(Rc<RefCell<..>>), a shared mutable cell for retaining a value in a collection/field. But this wrapper leaked into reads: a value stored via 'manage' (e.g. List.push(manage x) or Map.insert(value: read (manage x))) came back wrapped, and the typed accessors rejected it ("reg VM expected List/String, got ..."). Two complementary fixes, matching the rest of the value model where Managed is already transparent (display/native_value/equality all unwrap it): 1. Owned/Copy/Rc-returning accessors (expect_int/float/char/bool/list/deque/ map/closure_ref) now peel Managed by recursing, so a Managed collection or scalar reaching an op is handled from any source. 2. The borrow-returning accessors (String/Bytes/Json -> &str/&[u8]/&Value) cannot peel through a RefCell. Fix at the source: Manage now skips wrapping immutable scalars (Unit/Int/Float/Bool/Char/Bytes/String/Json), since they have value semantics and no in-place mutation -- wrapping them was a no-op that only leaked an opaque Managed. Collections/structs still wrap (aliasing). Unblocks running real packages on the dev VM (tinygrad port): the device allocator/buffer store and state-dict name lists round-trip managed values through Map/List. Full rsscript suite green (only the unrelated pre-existing Channel.message parity-fixture failure remains). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 5ac8157 commit 17196a8

2 files changed

Lines changed: 42 additions & 1 deletion

File tree

crates/rsscript/src/reg_vm/mod.rs

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6677,7 +6677,18 @@ impl RegVm {
66776677
}
66786678
RegInstr::Manage { dst, src } => {
66796679
let value = self.reg(base + *src).clone();
6680-
self.set_reg(base + *dst, VmValue::Managed(Rc::new(RefCell::new(value))));
6680+
// `manage` wraps a value in a shared mutable cell so it can be
6681+
// retained (stored in a collection/field) and mutated in place.
6682+
// Immutable scalars cannot be mutated in place and have value (not
6683+
// reference) semantics, so wrapping them is a no-op that only leaks
6684+
// an opaque `Managed` into reads — borrow-returning accessors
6685+
// (`String`/`Bytes`/`Json`) can't peel it. Store them directly.
6686+
let managed = if value.is_immutable_scalar() {
6687+
value
6688+
} else {
6689+
VmValue::Managed(Rc::new(RefCell::new(value)))
6690+
};
6691+
self.set_reg(base + *dst, managed);
66816692
}
66826693
RegInstr::GetField {
66836694
dst,
@@ -12634,6 +12645,10 @@ fn eval_numeric_compare(
1263412645
fn expect_int_ref(value: &VmValue) -> Result<i64, EvalError> {
1263512646
match value {
1263612647
VmValue::Int(value) => Ok(*value),
12648+
// `Managed` is transparent (see vm_value: display/native_value/equality
12649+
// all unwrap it). A value retained into storage via `manage` and read
12650+
// back arrives wrapped; see through it like the rest of the value model.
12651+
VmValue::Managed(inner) => expect_int_ref(&inner.borrow()),
1263712652
other => Err(EvalError::Runtime(format!(
1263812653
"reg VM expected Int, got `{}`.",
1263912654
other.display()
@@ -12644,6 +12659,7 @@ fn expect_int_ref(value: &VmValue) -> Result<i64, EvalError> {
1264412659
fn expect_float_ref(value: &VmValue) -> Result<f64, EvalError> {
1264512660
match value {
1264612661
VmValue::Float(value) => Ok(*value),
12662+
VmValue::Managed(inner) => expect_float_ref(&inner.borrow()),
1264712663
other => Err(EvalError::Runtime(format!(
1264812664
"reg VM expected Float, got `{}`.",
1264912665
other.display()
@@ -12654,6 +12670,7 @@ fn expect_float_ref(value: &VmValue) -> Result<f64, EvalError> {
1265412670
fn expect_char_ref(value: &VmValue) -> Result<char, EvalError> {
1265512671
match value {
1265612672
VmValue::Char(value) => Ok(*value),
12673+
VmValue::Managed(inner) => expect_char_ref(&inner.borrow()),
1265712674
other => Err(EvalError::Runtime(format!(
1265812675
"reg VM expected Char, got `{}`.",
1265912676
other.display()
@@ -13094,6 +13111,7 @@ fn expect_string_list_ref(value: &VmValue) -> Result<Vec<String>, EvalError> {
1309413111
fn expect_bool_ref(value: &VmValue) -> Result<bool, EvalError> {
1309513112
match value {
1309613113
VmValue::Bool(value) => Ok(*value),
13114+
VmValue::Managed(inner) => expect_bool_ref(&inner.borrow()),
1309713115
other => Err(EvalError::Runtime(format!(
1309813116
"reg VM expected Bool, got `{}`.",
1309913117
other.display()
@@ -13104,6 +13122,7 @@ fn expect_bool_ref(value: &VmValue) -> Result<bool, EvalError> {
1310413122
fn expect_list_ref(value: &VmValue) -> Result<Rc<RefCell<Vec<VmValue>>>, EvalError> {
1310513123
match value {
1310613124
VmValue::List(value) => Ok(Rc::clone(value)),
13125+
VmValue::Managed(inner) => expect_list_ref(&inner.borrow()),
1310713126
other => Err(EvalError::Runtime(format!(
1310813127
"reg VM expected List, got `{}`.",
1310913128
other.display()
@@ -13116,6 +13135,7 @@ fn expect_deque_ref(
1311613135
) -> Result<Rc<RefCell<std::collections::VecDeque<VmValue>>>, EvalError> {
1311713136
match value {
1311813137
VmValue::Deque(value) => Ok(Rc::clone(value)),
13138+
VmValue::Managed(inner) => expect_deque_ref(&inner.borrow()),
1311913139
other => Err(EvalError::Runtime(format!(
1312013140
"reg VM expected Deque, got `{}`.",
1312113141
other.display()
@@ -13139,6 +13159,7 @@ fn list_item_at(
1313913159
fn expect_map_ref(value: &VmValue) -> Result<Rc<RefCell<ValueMap>>, EvalError> {
1314013160
match value {
1314113161
VmValue::Map(value) => Ok(Rc::clone(value)),
13162+
VmValue::Managed(inner) => expect_map_ref(&inner.borrow()),
1314213163
other => Err(EvalError::Runtime(format!(
1314313164
"reg VM expected Map, got `{}`.",
1314413165
other.display()
@@ -13159,6 +13180,7 @@ fn expect_json_ref(value: &VmValue) -> Result<&serde_json::Value, EvalError> {
1315913180
fn expect_closure_rc(value: &VmValue) -> Result<Rc<VmClosure>, EvalError> {
1316013181
match value {
1316113182
VmValue::Closure(value) => Ok(Rc::clone(value)),
13183+
VmValue::Managed(inner) => expect_closure_rc(&inner.borrow()),
1316213184
other => Err(EvalError::Runtime(format!(
1316313185
"reg VM expected Closure, got `{}`.",
1316413186
other.display()

crates/rsscript/src/vm_value.rs

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -303,6 +303,25 @@ impl VmValue {
303303
Self::String(Rc::new(value.into()))
304304
}
305305

306+
/// Whether this value has value (not reference) semantics and no in-place
307+
/// mutation — so wrapping it in a `Managed` shared cell would be a semantic
308+
/// no-op. Used by the `manage` op to avoid leaking an opaque `Managed`
309+
/// around immutable scalars (`String`/`Bytes`/`Json` are `Rc`-shared and
310+
/// immutable; the rest are `Copy`).
311+
pub(crate) fn is_immutable_scalar(&self) -> bool {
312+
matches!(
313+
self,
314+
Self::Unit
315+
| Self::Int(_)
316+
| Self::Float(_)
317+
| Self::Bool(_)
318+
| Self::Char(_)
319+
| Self::Bytes(_)
320+
| Self::String(_)
321+
| Self::Json(_)
322+
)
323+
}
324+
306325
pub(crate) fn display(&self) -> String {
307326
match self {
308327
Self::Unit => "Unit".to_string(),

0 commit comments

Comments
 (0)