fix: global List.add() and Map[]= mutations not persisting#296
fix: global List.add() and Map[]= mutations not persisting#296rafagazani wants to merge 1 commit into
Conversation
Two independent bugs prevented module-level (global) collection mutations from persisting after method calls: ## Bug 1 — List.add() silently ignored (primitives.dart) `BoxList` was wrapping globals with `$List.wrap(<$Value>[...copy])`, creating a new list object disconnected from the one stored in `runtime.globals`. Mutations via `.add()` went to the temporary copy; the original global was never updated. Fix: wrap the existing list in-place — `$List.wrap(existingList)` — so the `$List` and the global share the same underlying List reference. ## Bug 2 — Map[]= crashes with _Map is not a subtype of $Value (reference.dart) `IndexedReference.resolveType()` delegated to `getValue()` for Map types. `getValue()` has the side effect of emitting bytecode (`Unbox + IndexMap`). In `compileAssignmentExpression`, `resolveType()` is called twice for simple `=` assignments. The second call emitted `Unbox` on a slot that was already unboxed by the first call, causing a runtime crash: the raw `Map<$Value,$Value>` cannot be cast to `$Value`. Fix: handle Map in `resolveType()` by reading the value type from the map's type arguments directly — identical to the existing List fast-path — without calling `getValue()`. Adds test/global_list_mutation_test.dart with 11 tests covering: - global List.add() from top-level and instance methods - global Map[]= from top-level and instance methods - print-channel workaround (regression guard) - full ScriptContext simulation (Sulfite use-case)
|
Wow this LLM-generated PR is really hard to comprehend! Including too long comments that simultaneously describe bugs and in theory fix them. |
Therefore, tests were added; were you able to reproduce the problem? The contribution guide is not against the use of LLM. |
|
The Map change is a duplicate fix as in #294 The noncopy list boxing could be correct, I'm not really sure. But I agree with @Zverik this PR is a bit of a mess. I have nothing against LLMs but in order to accept this it'd need to be a slimmer PR with a single, clear explanation of the issue and fix, and one or two targeted tests without random references to "Sulfite" and other things that nobody has heard of. So I'm going to accept #294 and reject this for now. |
3 participants
Problem
Two related bugs in module-level global collection mutation:
Bug 1 —
List.add()silently discards mutationsBoxListinprimitives.dartcalled$List.wrap(<$Value>[...(runtime.frame[reg] as List)]), creating a copy of the inner list. Mutations via.add()/.addAll()etc. were applied to the copy; the original global was never updated.Only explicit reassignment (
_list = [..._list, item]) emittedStoreGlobaland worked correctly.Bug 2 —
Map[]=crashes at runtimeIndexedReference.resolveType()delegated togetValue(), which emits bytecode (Unbox + IndexMap) as a side-effect.compileAssignmentExpressioncallsresolveType()twice for simple=assignments, causingUnboxto be emitted twice on the same slot. The secondUnboxattempted to cast a rawMap<$Value,$Value>as$Value→ runtime crash.Fix
lib/src/eval/runtime/ops/primitives.dart— wrap in-place instead of copying:```dart
// Before
runtime.frame[reg] = $List.wrap(<$Value>[...(runtime.frame[reg] as List)]);
// After
runtime.frame[reg] = $List.wrap(runtime.frame[reg] as List);
```
lib/src/eval/compiler/reference.dart—IndexedReference.resolveType(): handle Map type without callinggetValue()(no bytecode side-effects):```dart
if (_variable.type.isAssignableTo(ctx, CoreTypes.map.ref(ctx), forceAllowDynamic: false)) {
return _variable.type.specifiedTypeArgs.length >= 2
? _variable.type.specifiedTypeArgs[1]
: CoreTypes.dynamic.ref(ctx);
}
```
Tests
Added
test/global_list_mutation_test.dartwith 11 tests covering:List.add()from top-level function, instance method,varandfinalglobalsMap[]=from top-level function and instance method_list = [..._list, item])Full suite: 386/386 passing after both fixes.