Skip to content

Commit c13bb7c

Browse files
Haofeiclaude
andcommitted
reg-vm: support struct/list/option-typed Map and Set keys
The register VM only accepted Bool/Int/String map keys (VmMapKey was a three-variant enum), so a `derives(Eq, Hash)` struct/sum used as a `Map` key or `Set` element - which the compiled backend keys on directly - failed at runtime with "reg VM Map key does not support ...". That was a real VM/compiled capability divergence (surfaced by the example-corpus differential on uop_interning.rss). Make VmMapKey wrap any hashable VmValue: - equality reuses VmValue's existing structural PartialEq; - Hash is a matching recursive projection (hash_vm_value) so equal keys hash equal - covering scalars, String/Bytes/Char/Unit, the structural List/Deque/Option containers, and struct/variant fields, with Managed unwrapped transparently (consistent with Eq) and +-0.0 normalized. map_key_from_value now accepts the full hashable shape and rejects only the genuinely unhashable values (Float/Map/Json/Closure) with the same clean error; vm_value_from_map_key (used by Map.keys/Set.to_list) just clones the stored value, so keys round-trip exactly with no struct reconstruction. Keys remain Rc-shared rather than deep-copied: Map.insert/Set.insert declare retains(key), so the move checker forbids mutating a key while it is in a map, which is why the (now interior-mutable) VmMapKey is sound - documented at the scoped clippy::mutable_key_type allows. Tests: add backends_agree_on_struct_keyed_collections (struct key with nested List/Option: dedup, lookup, Map.keys round-trip, Set membership - VM == JIT == compiled) and drop the now-unneeded uop_interning skip from the example sweep. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent bee8bf5 commit c13bb7c

6 files changed

Lines changed: 230 additions & 58 deletions

File tree

crates/rsscript/src/reg_vm/mod.rs

Lines changed: 39 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -6617,6 +6617,10 @@ impl RegVm {
66176617
/// the interpreter (`drive`) and the JIT executor (`run_jit`), so the two can
66186618
/// never silently diverge. Jumps update `*ip`; `Return` is handed back to the
66196619
/// caller (which owns frame unwinding); everything else is [`PureStep::NotPure`].
6620+
// `VmMapKey` is interior-mutable (List/struct keys hold `Rc<RefCell<…>>`),
6621+
// but `Map.insert`'s `retains(key)` effect makes mutating a live key
6622+
// unreachable in well-typed RSScript, so the lint's hazard cannot occur.
6623+
#[allow(clippy::mutable_key_type)]
66206624
fn try_exec_pure(
66216625
&mut self,
66226626
instr: &RegInstr,
@@ -8687,6 +8691,9 @@ impl RegVm {
86878691
}
86888692
}
86898693

8694+
// See `try_exec_pure`: interior-mutable `VmMapKey` is safe because
8695+
// `retains(key)` forbids mutating a key while it is in a map.
8696+
#[allow(clippy::mutable_key_type)]
86908697
fn call_intrinsic(
86918698
&mut self,
86928699
unit: &RegUnit,
@@ -13080,23 +13087,43 @@ fn ensure_option_value(value: VmValue) -> Result<VmValue, EvalError> {
1308013087
}
1308113088

1308213089
fn map_key_from_value(value: &VmValue) -> Result<VmMapKey, EvalError> {
13083-
match value {
13084-
VmValue::Bool(value) => Ok(VmMapKey::Bool(*value)),
13085-
VmValue::Int(value) => Ok(VmMapKey::Int(*value)),
13086-
VmValue::String(value) => Ok(VmMapKey::String(Rc::clone(value))),
13087-
other => Err(EvalError::Runtime(format!(
13090+
// The checker guarantees a key's static type is `Hashable`, so this is a
13091+
// defensive gate: it accepts every shape RSScript admits as a key — scalars,
13092+
// strings/bytes, the structural `List`/`Deque`/`Option` containers, and
13093+
// `derives(Eq, Hash)` structs/sums (recursively) — and rejects only the
13094+
// genuinely unhashable values (`Float`, `Map`, raw `Json`, closures) with a
13095+
// clean runtime error rather than a host panic.
13096+
fn is_hashable(value: &VmValue) -> bool {
13097+
match value {
13098+
VmValue::Unit
13099+
| VmValue::Bool(_)
13100+
| VmValue::Int(_)
13101+
| VmValue::Char(_)
13102+
| VmValue::String(_)
13103+
| VmValue::Bytes(_)
13104+
| VmValue::Native(_)
13105+
| VmValue::OptionNone => true,
13106+
VmValue::OptionSome(inner) => is_hashable(inner),
13107+
VmValue::List(items) => items.borrow().iter().all(is_hashable),
13108+
VmValue::Deque(items) => items.borrow().iter().all(is_hashable),
13109+
VmValue::Struct(data) | VmValue::Variant(data) => data.fields.iter().all(is_hashable),
13110+
VmValue::Managed(inner) => is_hashable(&inner.borrow()),
13111+
VmValue::Float(_) | VmValue::Map(_) | VmValue::Json(_) | VmValue::Closure(_) => false,
13112+
}
13113+
}
13114+
13115+
if is_hashable(value) {
13116+
Ok(VmMapKey::new(value.clone()))
13117+
} else {
13118+
Err(EvalError::Runtime(format!(
1308813119
"reg VM Map key does not support `{}`.",
13089-
other.display()
13090-
))),
13120+
value.display()
13121+
)))
1309113122
}
1309213123
}
1309313124

1309413125
fn vm_value_from_map_key(key: &VmMapKey) -> VmValue {
13095-
match key {
13096-
VmMapKey::Bool(value) => VmValue::Bool(*value),
13097-
VmMapKey::Int(value) => VmValue::Int(*value),
13098-
VmMapKey::String(value) => VmValue::String(Rc::clone(value)),
13099-
}
13126+
key.value().clone()
1310013127
}
1310113128

1310213129
fn json_kind(value: &serde_json::Value) -> &'static str {

crates/rsscript/src/reg_vm/runtime_values.rs

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -263,6 +263,12 @@ pub(super) fn json_decode_struct_value(
263263
))))
264264
}
265265

266+
// `VmMapKey` wraps a `VmValue`, which has interior mutability (a `List`/struct
267+
// key holds `Rc<RefCell<…>>`). The `mutable_key_type` hazard — mutating a key
268+
// while it sits in a map — cannot occur in well-typed RSScript: `Map.insert`
269+
// declares `retains(key)`, so the move checker forbids mutating a key after
270+
// insertion. The hash projection reads the (immutable-by-contract) contents.
271+
#[allow(clippy::mutable_key_type)]
266272
pub(super) fn json_decode_field_value(
267273
unit: &RegUnit,
268274
type_name: &str,
@@ -334,7 +340,7 @@ pub(super) fn json_decode_field_value(
334340
let mut decoded = ValueMap::with_capacity_and_hasher(object.len(), Default::default());
335341
for (key, value) in object {
336342
decoded.insert(
337-
VmMapKey::String(Rc::new(key.clone())),
343+
VmMapKey::from_string(key.clone()),
338344
json_decode_field_value(unit, args[1], value)?,
339345
);
340346
}

crates/rsscript/src/reg_vm/value_convert.rs

Lines changed: 16 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -285,7 +285,7 @@ pub(super) fn vm_value_to_json_literal(value: &VmValue) -> Result<serde_json::Va
285285
VmValue::Map(entries) => {
286286
let mut object = serde_json::Map::new();
287287
for (key, value) in entries.borrow().iter() {
288-
let VmMapKey::String(key) = key else {
288+
let Some(key) = key.as_str() else {
289289
return Err(EvalError::Runtime(format!(
290290
"cannot convert map key `{}` to a JSON literal.",
291291
key.display()
@@ -460,6 +460,10 @@ pub(super) fn unmanage_vm_value(value: VmValue) -> VmValue {
460460
/// reference type, so it keeps its handle (mirroring the backend, which shares
461461
/// `Managed` and treats plain collections as value types). Immutable handles
462462
/// (`String`/`Bytes`/`Json`) and opaque values are cloned shallowly.
463+
// See the note in `runtime_values::json_decode_field_value`: a `VmMapKey` is
464+
// interior-mutable but the `retains(key)` effect makes mutating a live key
465+
// unreachable in well-typed programs.
466+
#[allow(clippy::mutable_key_type)]
463467
pub(super) fn deep_copy_value(value: &VmValue) -> VmValue {
464468
match value {
465469
VmValue::List(items) => {
@@ -647,10 +651,10 @@ pub(super) fn vm_value_from_native_value(value: NativeValue) -> VmValue {
647651

648652
pub(super) fn vm_map_key_from_native_value(value: NativeValue) -> VmMapKey {
649653
match value {
650-
NativeValue::Bool(value) => VmMapKey::Bool(value),
651-
NativeValue::Int(value) => VmMapKey::Int(value),
652-
NativeValue::String(value) => VmMapKey::String(Rc::new(value)),
653-
other => VmMapKey::String(Rc::new(format!("{other:?}"))),
654+
NativeValue::Bool(value) => VmMapKey::new(VmValue::Bool(value)),
655+
NativeValue::Int(value) => VmMapKey::new(VmValue::Int(value)),
656+
NativeValue::String(value) => VmMapKey::from_string(value),
657+
other => VmMapKey::from_string(format!("{other:?}")),
654658
}
655659
}
656660

@@ -708,7 +712,7 @@ mod tests {
708712
);
709713

710714
let mut map: ValueMap = ValueMap::default();
711-
map.insert(VmMapKey::Int(1), VmValue::string("one"));
715+
map.insert(VmMapKey::new(VmValue::Int(1)), VmValue::string("one"));
712716
assert_eq!(
713717
to_native(VmValue::Map(Rc::new(RefCell::new(map)))),
714718
NativeValue::Map(vec![(
@@ -861,20 +865,20 @@ mod tests {
861865
fn map_key_from_native_falls_back_to_string() {
862866
assert_eq!(
863867
vm_map_key_from_native_value(NativeValue::Bool(true)),
864-
VmMapKey::Bool(true)
868+
VmMapKey::new(VmValue::Bool(true))
865869
);
866870
assert_eq!(
867871
vm_map_key_from_native_value(NativeValue::Int(2)),
868-
VmMapKey::Int(2)
872+
VmMapKey::new(VmValue::Int(2))
869873
);
870874
assert_eq!(
871875
vm_map_key_from_native_value(NativeValue::String("k".to_string())),
872-
VmMapKey::String(Rc::new("k".to_string()))
876+
VmMapKey::from_string("k")
873877
);
874878
// A non-scalar key has no VM key form, so it falls back to its debug string.
875-
match vm_map_key_from_native_value(NativeValue::Float(1.0)) {
876-
VmMapKey::String(s) => assert!(s.contains("Float"), "{s}"),
877-
other => panic!("expected string fallback, got {other:?}"),
879+
match vm_map_key_from_native_value(NativeValue::Float(1.0)).as_str() {
880+
Some(s) => assert!(s.contains("Float"), "{s}"),
881+
None => panic!("expected string fallback"),
878882
}
879883
}
880884
}

crates/rsscript/src/vm_value.rs

Lines changed: 120 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -168,28 +168,133 @@ pub(crate) struct VmNative {
168168
pub(crate) id: i64,
169169
}
170170

171-
#[derive(Debug, Clone, PartialEq, Eq, Hash)]
172-
pub(crate) enum VmMapKey {
173-
Bool(bool),
174-
Int(i64),
175-
String(Rc<String>),
176-
}
171+
/// A `Map`/`Set` key: any hashable `VmValue`. Keys are not restricted to scalars
172+
/// — RSScript's `Hashable` bound also admits `derives(Eq, Hash)` structs/sums and
173+
/// the structural `List`/`Option` containers over hashable types, all of which
174+
/// the compiled backend keys on directly. Equality reuses `VmValue`'s structural
175+
/// `PartialEq`; `Hash` is a matching recursive projection (see [`hash_vm_value`]),
176+
/// so equal keys hash equal and the VM stays in lockstep with the derived
177+
/// `Hash`/`Eq` of the lowered Rust.
178+
#[derive(Debug, Clone)]
179+
pub(crate) struct VmMapKey(VmValue);
177180

178181
impl VmMapKey {
179-
pub(crate) fn display(&self) -> String {
180-
match self {
181-
Self::Bool(value) => value.to_string(),
182-
Self::Int(value) => value.to_string(),
183-
Self::String(value) => value.to_string(),
182+
pub(crate) fn new(value: VmValue) -> Self {
183+
VmMapKey(value)
184+
}
185+
186+
pub(crate) fn from_string(value: impl Into<String>) -> Self {
187+
VmMapKey(VmValue::string(value))
188+
}
189+
190+
/// The underlying value, returned as-is by `Map.keys()` / `Set.to_list()`.
191+
pub(crate) fn value(&self) -> &VmValue {
192+
&self.0
193+
}
194+
195+
/// The key's string contents, when it is a `String` key (e.g. JSON object
196+
/// keys must be strings).
197+
pub(crate) fn as_str(&self) -> Option<&str> {
198+
match &self.0 {
199+
VmValue::String(value) => Some(value),
200+
_ => None,
184201
}
185202
}
186203

204+
pub(crate) fn display(&self) -> String {
205+
self.0.display()
206+
}
207+
187208
pub(crate) fn native_value(&self) -> NativeValue {
188-
match self {
189-
Self::Bool(value) => NativeValue::Bool(*value),
190-
Self::Int(value) => NativeValue::Int(*value),
191-
Self::String(value) => NativeValue::String(value.to_string()),
209+
// Every hashable key has a native form; the string fallback only covers
210+
// shapes the host ABI lacks a slot for (e.g. an `Option` key), keeping
211+
// this total rather than panicking on the boundary.
212+
self.0
213+
.native_value()
214+
.unwrap_or_else(|| NativeValue::String(self.0.display()))
215+
}
216+
}
217+
218+
impl PartialEq for VmMapKey {
219+
fn eq(&self, other: &Self) -> bool {
220+
self.0 == other.0
221+
}
222+
}
223+
224+
impl Eq for VmMapKey {}
225+
226+
impl std::hash::Hash for VmMapKey {
227+
fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
228+
hash_vm_value(&self.0, state);
229+
}
230+
}
231+
232+
/// Recursively hash a `VmValue` consistently with [`VmValue`]'s `PartialEq`:
233+
/// equal values must hash equal. `Managed` is transparent in equality, so it is
234+
/// unwrapped here *before* the discriminant is mixed in (a `Managed(Int(1))`
235+
/// must hash like `Int(1)`). `Float` keys cannot occur (not `Hashable`), but the
236+
/// ±0.0 case is normalized anyway so the function is a correct `Hash` for any
237+
/// `VmValue`.
238+
fn hash_vm_value<H: std::hash::Hasher>(value: &VmValue, state: &mut H) {
239+
use std::hash::Hash;
240+
241+
if let VmValue::Managed(inner) = value {
242+
hash_vm_value(&inner.borrow(), state);
243+
return;
244+
}
245+
246+
std::mem::discriminant(value).hash(state);
247+
match value {
248+
VmValue::Unit | VmValue::OptionNone => {}
249+
VmValue::Bool(value) => value.hash(state),
250+
VmValue::Int(value) => value.hash(state),
251+
VmValue::Char(value) => value.hash(state),
252+
VmValue::String(value) => value.hash(state),
253+
VmValue::Bytes(value) => value.hash(state),
254+
VmValue::Float(value) => {
255+
let bits = if *value == 0.0 { 0 } else { value.to_bits() };
256+
bits.hash(state);
257+
}
258+
VmValue::Json(value) => value.to_string().hash(state),
259+
VmValue::List(items) => {
260+
let items = items.borrow();
261+
items.len().hash(state);
262+
for item in items.iter() {
263+
hash_vm_value(item, state);
264+
}
265+
}
266+
VmValue::Deque(items) => {
267+
let items = items.borrow();
268+
items.len().hash(state);
269+
for item in items.iter() {
270+
hash_vm_value(item, state);
271+
}
272+
}
273+
VmValue::OptionSome(inner) => hash_vm_value(inner, state),
274+
VmValue::Struct(data) | VmValue::Variant(data) => {
275+
data.name.hash(state);
276+
for field in &data.fields {
277+
hash_vm_value(field, state);
278+
}
279+
}
280+
VmValue::Native(data) => {
281+
data.type_name.hash(state);
282+
data.id.hash(state);
283+
}
284+
// Not a hashable key shape (the checker rejects `Map`/closure keys), but
285+
// stay total: an order-independent fold so equal maps hash equally.
286+
VmValue::Map(entries) => {
287+
let mut acc: u64 = 0;
288+
for (key, value) in entries.borrow().iter() {
289+
let mut hasher = FnvHasher::default();
290+
key.hash(&mut hasher);
291+
hash_vm_value(value, &mut hasher);
292+
acc = acc.wrapping_add(std::hash::Hasher::finish(&hasher));
293+
}
294+
acc.hash(state);
192295
}
296+
VmValue::Closure(closure) => (Rc::as_ptr(closure) as usize).hash(state),
297+
VmValue::Managed(_) => unreachable!("Managed handled above"),
193298
}
194299
}
195300

crates/rsscript/tests/backend_differential.rs

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -419,6 +419,53 @@ fn main() -> Unit {
419419
common::differential::assert_backends_agree("struct-field-writes.rss", source, &[]);
420420
}
421421

422+
/// A `derives(Eq, Hash)` struct (with nested `List`/`Option` fields) used as a
423+
/// `Map` key and `Set` element. The VM keys on a structural projection of the
424+
/// value while the compiled backend keys on the derived `Hash`/`Eq`; both must
425+
/// agree on dedup (a structurally-equal, separately-constructed key overwrites),
426+
/// membership, and the round-trip through `Map.keys()` (key value reconstruction).
427+
/// Output is reduced to counts/booleans so hash *iteration order* — which legitimately
428+
/// differs between the backends — is not under test. interp == jit == compiled.
429+
#[test]
430+
fn backends_agree_on_struct_keyed_collections() {
431+
let source = "\
432+
struct Point derives(Clone, Eq, Hash) {
433+
x: Int,
434+
y: Int,
435+
tags: List<Int>
436+
}
437+
438+
fn main() -> Unit {
439+
let counts = Map.new<Point, Int>()
440+
let p1 = Point(x: 1, y: 2, tags: List.new<Int>())
441+
let p2 = Point(x: 3, y: 4, tags: List.new<Int>())
442+
// Structurally identical to `p1` but a distinct allocation.
443+
let p1_again = Point(x: 1, y: 2, tags: List.new<Int>())
444+
445+
Map.insert(map: mut counts, key: read p1, value: read 10)
446+
Map.insert(map: mut counts, key: read p2, value: read 20)
447+
// Same structural key: overwrites rather than adds.
448+
Map.insert(map: mut counts, key: read p1_again, value: read 99)
449+
450+
Log.write(message: read String.from_int(value: Map.len(map: read counts)))
451+
Log.write(message: read String.from_bool(value: Map.contains_key(map: read counts, key: read p1_again)))
452+
let absent = Point(x: 9, y: 9, tags: List.new<Int>())
453+
Log.write(message: read String.from_bool(value: Map.contains_key(map: read counts, key: read absent)))
454+
// Round-trips keys back to `Point` values (key reconstruction).
455+
Log.write(message: read String.from_int(value: List.len(list: read Map.keys(map: read counts))))
456+
457+
let seen = Set.new<Point>()
458+
Set.insert(set: mut seen, value: read p1)
459+
Set.insert(set: mut seen, value: read p2)
460+
Set.insert(set: mut seen, value: read p1_again)
461+
Log.write(message: read String.from_int(value: Set.len(set: read seen)))
462+
Log.write(message: read String.from_bool(value: Set.contains(set: read seen, value: read p1_again)))
463+
return Unit
464+
}
465+
";
466+
common::differential::assert_backends_agree("struct-keyed-collections.rss", source, &[]);
467+
}
468+
422469
/// Deque / Set / SortedSet / SortedMap mutations with out-of-order inserts,
423470
/// duplicates, and removes, then sorted/ordered dumps. This gates the collection
424471
/// backing/representation optimizations: the VM (`Vec`-backed) and the compiled

0 commit comments

Comments
 (0)