|
| 1 | +# Collection Rebind Delayed-Move Lowering Implementation Plan |
| 2 | + |
| 3 | +> **For Hermes:** Use `subagent-driven-development` to implement this plan task-by-task, with `test-driven-development` for each behavior change. |
| 4 | +
|
| 5 | +**Goal:** Lower same-local collection rebinds as an existing-bytecode ownership transfer so `set` / `array_push` receives the unique collection without introducing opcodes, runtime pattern matching, or deep COW copies. |
| 6 | + |
| 7 | +**Architecture:** Keep the target local readable while evaluating the key and RHS. Immediately before the existing builtin `Call`, emit the existing move-clear sequence `ldc null; stloc target`; the collection handle already on the stack then becomes the owned call argument. The existing post-call `stloc target` writes the result back. Alias legality remains owned by the current lifetime pass. |
| 8 | + |
| 9 | +**Tech Stack:** Rust, RustScript compiler IR/codegen, existing `Ldloc`/`Ldc`/`Stloc`/`Call`, Arc COW collections, interpreter/JIT/AOT/no-std compatibility tests. |
| 10 | + |
| 11 | +--- |
| 12 | + |
| 13 | +## Root cause |
| 14 | + |
| 15 | +The current pipeline loses move intent at lowering: |
| 16 | + |
| 17 | +1. `parse_index_assign_with_terminator` builds `set(Expr::Var(target), key, value)` and wraps it in `Stmt::Assign { index: target, ... }`. |
| 18 | +2. The lifetime pass recognizes `Set` / `ArrayPush` collection mutation and calls `require_collection_mutation_permitted`, so aliases are already rejected. |
| 19 | +3. `rewrite_local_source_move_on_rebind` only rewrites a whole RHS shaped as `Expr::Var(source)` and explicitly skips `source == target`; it never rewrites the nested call’s first argument. |
| 20 | +4. Codegen compiles that first `Expr::Var(target)` through `emit_copy_ldloc`. |
| 21 | +5. The target local remains populated at `Arc::make_mut`, so the stack handle and local handle force a deep COW detach despite lifetime analysis having proved the mutation legal. |
| 22 | + |
| 23 | +Correct existing-bytecode lowering: |
| 24 | + |
| 25 | +```text |
| 26 | +ldloc target # temporary Arc-handle copy; target remains readable |
| 27 | +<evaluate key> |
| 28 | +<evaluate rhs> |
| 29 | +ldc null |
| 30 | +stloc target # delayed move: stack handle now owns the collection |
| 31 | +call set/3 |
| 32 | +stloc target # rebind result |
| 33 | +``` |
| 34 | + |
| 35 | +For `ArrayPush`, omit the key. No collection contents are copied. Delaying the clear until after argument evaluation preserves `a[i] = a[j]` and side-effecting key/RHS behavior. |
| 36 | + |
| 37 | +## Constraints |
| 38 | + |
| 39 | +- Do not modify `OpCode`, opcode values, assembler/disassembler syntax, VMBC, AOT IR, native bridge op tables, or no-std opcode tables. |
| 40 | +- Do not add runtime call-pattern recognition or Arc-pointer matching. |
| 41 | +- Do not add hidden locals or stack-reordering instructions. |
| 42 | +- Do not alter parser evaluation order or lifetime alias policy. |
| 43 | +- Activate only when local move semantics are enabled, the assignment destination equals the collection source, and the call is built-in `Set` or `ArrayPush` with exact arity. |
| 44 | +- Keep compatibility-language/plugin compilation on the existing generic path when local move semantics are disabled. |
| 45 | +- The transient `Null` is a runtime ownership transfer; do not change compiler `type_state` before the final rebind. |
| 46 | + |
| 47 | +--- |
| 48 | + |
| 49 | +### Task 1: Add a RED bytecode-shape regression test |
| 50 | + |
| 51 | +**Objective:** Prove current codegen leaves the target local populated at the collection builtin call. |
| 52 | + |
| 53 | +**Files:** |
| 54 | +- Modify: `tests/compiler/compiler_common_tests.rs` |
| 55 | + |
| 56 | +**Step 1: Extend the test decoder to expose `Call` operands** |
| 57 | + |
| 58 | +Update `DecodedInstr` / `decode_instructions` to record the existing call index and argc without changing production code. |
| 59 | + |
| 60 | +**Step 2: Add `same_local_collection_set_clears_target_immediately_before_call`** |
| 61 | + |
| 62 | +Compile: |
| 63 | + |
| 64 | +```rust |
| 65 | +let mut a = [10, 20]; |
| 66 | +a[0] = a[1]; |
| 67 | +a[0]; |
| 68 | +``` |
| 69 | + |
| 70 | +Resolve `a` through debug info and find `BuiltinFunction::Set.call_index()`. Assert the instruction window ending at the assignment call is: |
| 71 | + |
| 72 | +```text |
| 73 | +Ldc <Null> |
| 74 | +Stloc <a> |
| 75 | +Call <Set, argc=3> |
| 76 | +Stloc <a> |
| 77 | +``` |
| 78 | + |
| 79 | +Also assert an `Ldloc <a>` occurs before key/RHS evaluation, so the move remains delayed rather than clearing `a` before `a[1]` is read. |
| 80 | + |
| 81 | +**Step 3: Run the RED test** |
| 82 | + |
| 83 | +```bash |
| 84 | +cargo test --locked --test compiler_tests \ |
| 85 | + same_local_collection_set_clears_target_immediately_before_call -- --nocapture |
| 86 | +``` |
| 87 | + |
| 88 | +Expected RED: current output contains `Call(Set)` followed by `Stloc(a)` but lacks the immediate `Ldc Null; Stloc(a)` pair before the call. |
| 89 | + |
| 90 | +**Step 4: Commit the RED test only after confirming the expected failure** |
| 91 | + |
| 92 | +Do not modify production code in this step. |
| 93 | + |
| 94 | +--- |
| 95 | + |
| 96 | +### Task 2: Implement delayed-move lowering in codegen |
| 97 | + |
| 98 | +**Objective:** Transfer the target local’s ownership immediately before the existing builtin call. |
| 99 | + |
| 100 | +**Files:** |
| 101 | +- Modify: `src/compiler/codegen.rs` |
| 102 | +- Test: `tests/compiler/compiler_common_tests.rs` |
| 103 | + |
| 104 | +**Step 1: Factor direct-call emission from argument compilation** |
| 105 | + |
| 106 | +Split the current `compile_direct_call` responsibilities: |
| 107 | + |
| 108 | +```rust |
| 109 | +fn compile_direct_call(&mut self, index: u16, args: &[Expr]) -> Result<(), CompileError> { |
| 110 | + for arg in args { |
| 111 | + self.compile_scalar_expr(arg)?; |
| 112 | + } |
| 113 | + self.emit_direct_call(index, args) |
| 114 | +} |
| 115 | + |
| 116 | +fn emit_direct_call(&mut self, index: u16, args: &[Expr]) -> Result<(), CompileError> { |
| 117 | + let argc = u8::try_from(args.len()).map_err(|_| CompileError::CallArityOverflow)?; |
| 118 | + if let Some(builtin) = BuiltinFunction::from_call_index(index) { |
| 119 | + debug_assert!(builtin.accepts_arity(argc)); |
| 120 | + self.record_builtin_call_operand_types(args); |
| 121 | + self.assembler.call(index, argc); |
| 122 | + return Ok(()); |
| 123 | + } |
| 124 | + let remapped_index = self.call_index_remap.get(&index).copied().unwrap_or(index); |
| 125 | + self.assembler.call(remapped_index, argc); |
| 126 | + Ok(()) |
| 127 | +} |
| 128 | +``` |
| 129 | + |
| 130 | +Keep all existing call-index remapping and operand-type recording behavior. |
| 131 | + |
| 132 | +**Step 2: Add exact same-local collection matching** |
| 133 | + |
| 134 | +Add a codegen helper: |
| 135 | + |
| 136 | +```rust |
| 137 | +fn try_compile_same_local_collection_rebind( |
| 138 | + &mut self, |
| 139 | + target: LocalSlot, |
| 140 | + expr: &Expr, |
| 141 | +) -> Result<bool, CompileError> |
| 142 | +``` |
| 143 | + |
| 144 | +Return `false` unless all conditions hold: |
| 145 | + |
| 146 | +- `self.enable_local_move_semantics`; |
| 147 | +- `expr` is `Expr::Call(index, _, args)`; |
| 148 | +- builtin is `Set` with three args or `ArrayPush` with two args; |
| 149 | +- `args[0]` is exactly `Expr::Var(target)`. |
| 150 | + |
| 151 | +Matching exact parser IR is intentional. Do not broaden to borrow wrappers or unrelated host calls. |
| 152 | + |
| 153 | +**Step 3: Emit delayed ownership transfer** |
| 154 | + |
| 155 | +For a match: |
| 156 | + |
| 157 | +```rust |
| 158 | +for arg in args { |
| 159 | + self.compile_scalar_expr(arg)?; |
| 160 | +} |
| 161 | +self.assembler.push_const(Value::Null); |
| 162 | +self.emit_stloc(target)?; |
| 163 | +self.emit_direct_call(*index, args)?; |
| 164 | +Ok(true) |
| 165 | +``` |
| 166 | + |
| 167 | +This order keeps `target` available throughout key/RHS evaluation. Do not call `emit_move_ldloc`, since it clears immediately after loading the first argument. |
| 168 | + |
| 169 | +**Step 4: Integrate before the generic RHS path** |
| 170 | + |
| 171 | +In `assign_expr_to_slot`, replace the unconditional expression compilation with: |
| 172 | + |
| 173 | +```rust |
| 174 | +if !self.try_compile_same_local_collection_rebind(slot, expr)? { |
| 175 | + self.compile_scalar_expr(expr)?; |
| 176 | +} |
| 177 | +self.emit_stloc(slot)?; |
| 178 | +``` |
| 179 | + |
| 180 | +Keep all callable/type inference and final `type_state` updates unchanged. The existing final `emit_stloc(slot)` performs the result rebind. |
| 181 | + |
| 182 | +**Step 5: Run GREEN tests** |
| 183 | + |
| 184 | +```bash |
| 185 | +cargo test --locked --test compiler_tests \ |
| 186 | + same_local_collection_set_clears_target_immediately_before_call -- --nocapture |
| 187 | +cargo test --locked --test compiler_tests rustscript_local_move -- --nocapture |
| 188 | +``` |
| 189 | + |
| 190 | +Expected: delayed-clear shape passes; existing local-move tests remain green. |
| 191 | + |
| 192 | +**Step 6: Commit** |
| 193 | + |
| 194 | +```bash |
| 195 | +git add src/compiler/codegen.rs tests/compiler/compiler_common_tests.rs |
| 196 | +git commit -m "perf: lower collection rebinds as delayed moves" |
| 197 | +``` |
| 198 | + |
| 199 | +--- |
| 200 | + |
| 201 | +### Task 3: Cover self-reference, aliases, array push, and compatibility mode |
| 202 | + |
| 203 | +**Objective:** Lock down language semantics and fallback boundaries. |
| 204 | + |
| 205 | +**Files:** |
| 206 | +- Modify: `tests/compiler/compiler_common_tests.rs` |
| 207 | +- Modify: `tests/compiler/compiler_rustscript_tests.rs` |
| 208 | +- Modify only if a failing test exposes an implementation defect: `src/compiler/codegen.rs` |
| 209 | + |
| 210 | +**Step 1: Add self-reference behavior cases** |
| 211 | + |
| 212 | +Add runtime cases for: |
| 213 | + |
| 214 | +```rust |
| 215 | +let mut a = [10, 20]; |
| 216 | +a[0] = a[1]; |
| 217 | +a[0]; |
| 218 | +``` |
| 219 | + |
| 220 | +and a key/RHS expression that records evaluation order through ordinary RustScript functions/counters. Assert key evaluates before RHS and both can read `a` before the delayed move. |
| 221 | + |
| 222 | +**Step 2: Add map and append cases** |
| 223 | + |
| 224 | +Cover: |
| 225 | + |
| 226 | +- map field overwrite and null deletion; |
| 227 | +- array `index == len` append through `Set`; |
| 228 | +- explicit same-local `a = array_push(a, value)` if accepted by current frontend. |
| 229 | + |
| 230 | +Assert each same-local call has `Ldc Null; Stloc target` immediately before `Call`, followed by the normal result `Stloc target`. |
| 231 | + |
| 232 | +**Step 3: Preserve alias rejection** |
| 233 | + |
| 234 | +Retain and narrow-run the existing cases where `let b = a; a[0] = ...` or mutable-borrow aliases cause compile errors. The delayed move relies on the lifetime pass’s existing `require_collection_mutation_permitted` proof. |
| 235 | + |
| 236 | +```bash |
| 237 | +cargo test --locked --test compiler_tests collection_alias -- --nocapture |
| 238 | +``` |
| 239 | + |
| 240 | +**Step 4: Verify fallback boundaries** |
| 241 | + |
| 242 | +Add tests proving no delayed clear is emitted when: |
| 243 | + |
| 244 | +- move semantics are disabled by a compatibility frontend; |
| 245 | +- the assignment destination differs from the collection source; |
| 246 | +- the call is not built-in `Set` / `ArrayPush`; |
| 247 | +- arity or IR shape differs. |
| 248 | + |
| 249 | +**Step 5: Run targeted suite and commit** |
| 250 | + |
| 251 | +```bash |
| 252 | +cargo test --locked --test compiler_tests collection -- --nocapture |
| 253 | +cargo test --locked --test compiler_tests indexed_assignment -- --nocapture |
| 254 | +``` |
| 255 | + |
| 256 | +```bash |
| 257 | +git add tests/compiler/compiler_common_tests.rs \ |
| 258 | + tests/compiler/compiler_rustscript_tests.rs src/compiler/codegen.rs |
| 259 | +git commit -m "test: cover delayed collection move semantics" |
| 260 | +``` |
| 261 | + |
| 262 | +--- |
| 263 | + |
| 264 | +### Task 4: Verify engine compatibility and absence of deep COW lowering |
| 265 | + |
| 266 | +**Objective:** Confirm the compiler-only change works on every execution path with unchanged bytecode ABI. |
| 267 | + |
| 268 | +**Files:** |
| 269 | +- Modify: `tests/vm/functional_parity_tests.rs` |
| 270 | +- Modify: `examples/mini_bench.rs` |
| 271 | +- Do not modify: VM opcode/wire/runtime files |
| 272 | + |
| 273 | +**Step 1: Add interpreter/JIT parity** |
| 274 | + |
| 275 | +Run repeated same-local array/map mutation with JIT disabled and enabled. Compare final stack and locals. |
| 276 | + |
| 277 | +```bash |
| 278 | +cargo test --locked --test vm_tests \ |
| 279 | + interpreter_and_jit_match_for_delayed_collection_move -- --nocapture |
| 280 | +``` |
| 281 | + |
| 282 | +**Step 2: Add a focused release workload** |
| 283 | + |
| 284 | +Extend `examples/mini_bench.rs` with a collection-rebind loop using ordinary RustScript indexed assignment. Keep timing assertions out of CI. |
| 285 | + |
| 286 | +Compare `fe93300` and the implementation commit with identical release settings and at least seven trials. Report medians separately for interpreter and JIT. |
| 287 | + |
| 288 | +**Step 3: Verify unchanged ABI/runtime scope** |
| 289 | + |
| 290 | +```bash |
| 291 | +git diff --exit-code fe93300 -- \ |
| 292 | + src/bytecode.rs src/assembler.rs src/vmbc.rs \ |
| 293 | + src/vm pd-vm-nostd/src/program.rs pd-vm-nostd/src/vm.rs |
| 294 | +``` |
| 295 | + |
| 296 | +Expected: no production diff in opcode, VMBC, interpreter, JIT/native bridge, or no-std runtime files. |
| 297 | + |
| 298 | +**Step 4: Run full verification** |
| 299 | + |
| 300 | +```bash |
| 301 | +cargo fmt --all -- --check |
| 302 | +cargo test --locked --lib |
| 303 | +cargo test --locked --test compiler_tests |
| 304 | +cargo test --locked --test vm_tests |
| 305 | +cargo test --locked --test jit_tests |
| 306 | +cargo test --locked -p pd-vm-nostd |
| 307 | +cargo test --locked --all-targets |
| 308 | +cargo clippy --locked --all-targets --all-features -- -D warnings |
| 309 | +``` |
| 310 | + |
| 311 | +**Step 5: Commit benchmark/parity tests** |
| 312 | + |
| 313 | +```bash |
| 314 | +git add tests/vm/functional_parity_tests.rs examples/mini_bench.rs |
| 315 | +git commit -m "bench: measure delayed collection moves" |
| 316 | +``` |
| 317 | + |
| 318 | +--- |
| 319 | + |
| 320 | +## Acceptance criteria |
| 321 | + |
| 322 | +- Same-local `Set` / `ArrayPush` uses only existing opcodes. |
| 323 | +- Target remains readable during key/RHS evaluation. |
| 324 | +- `Ldc Null; Stloc target` occurs immediately before the builtin call. |
| 325 | +- At `Arc::make_mut`, the target local no longer retains the stack collection handle; alias-free source therefore avoids deep COW detachment. |
| 326 | +- The ordinary post-call `Stloc target` restores the updated collection. |
| 327 | +- Existing alias rejection remains the ownership proof. |
| 328 | +- Compatibility frontends with move semantics disabled retain their current lowering. |
| 329 | +- Parser, lifetime model, runtime dispatch, opcode tables, VMBC, AOT/native lowering, and no-std runtime remain unchanged. |
0 commit comments