fix(codegen): a typed-array store used as an expression evaluated to 0 (#7590) - #7591
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughTyped-array and buffer store expressions now distinguish statement-level discard from nested expression usage. Store values remain available when consumed as operands, while discarded stores retain their existing behavior. Regression coverage validates both cases. ChangesTyped-array store value propagation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test-files/test_typed_array_store_expression_value.ts (1)
1-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend coverage to the Buffer receiver and the dynamic-index proven-view tier.
This test only uses
Uint8Arraywith literal indices. Two other code paths that this PR changes are not exercised:
index_set.rs's proven-view checked-store tier, which fires for a dynamic (non-literal) index on a storage-proven view. A literal index resolves through the earlierlower_typed_array_storetier instead.arrays_finds.rs'sBufferIndexSetdiscard checks, which only apply to aBufferreceiver, notUint8Array.Add a
Buffervariant and a variable-index variant of the consumed-store cases to confirm both tiers return the assigned value instead of0.🧪 Suggested additional test cases
const buf = new Uint8Array(8); +const nodeBuf = Buffer.alloc(4); const out: string[] = []; function check(label: string, got: number, want: number): void { out.push(got === want ? label + ":ok" : label + ":WRONG got=" + got + " want=" + want); } // consumed as a call argument check("arg", (buf[0] = 5), 5); +// consumed on a Buffer receiver +check("buffer", (nodeBuf[0] = 9), 9); +// consumed with a dynamic (non-literal) index, forcing the proven-view checked-store tier +let i = 3; +check("dynamic_index", (buf[i] = 6), 6);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test-files/test_typed_array_store_expression_value.ts` around lines 1 - 35, Extend the typed-array store test to cover both missing paths: add consumed assignment-expression cases using a Buffer receiver to exercise BufferIndexSet discard handling, and use a variable non-literal index on a storage-proven view to exercise index_set.rs’s checked-store tier. Assert each expression evaluates to its assigned value while preserving the existing literal-index Uint8Array and discarded-store coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test-files/test_typed_array_store_expression_value.ts`:
- Around line 1-35: Extend the typed-array store test to cover both missing
paths: add consumed assignment-expression cases using a Buffer receiver to
exercise BufferIndexSet discard handling, and use a variable non-literal index
on a storage-proven view to exercise index_set.rs’s checked-store tier. Assert
each expression evaluates to its assigned value while preserving the existing
literal-index Uint8Array and discarded-store coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9cb3c329-8dc8-471b-8af8-20c1e6ed4c43
📒 Files selected for processing (12)
changelog.d/7591-discard-expr-value-leak.mdcrates/perry-codegen/src/codegen/closure.rscrates/perry-codegen/src/codegen/entry.rscrates/perry-codegen/src/codegen/function.rscrates/perry-codegen/src/codegen/method.rscrates/perry-codegen/src/expr/arrays_finds.rscrates/perry-codegen/src/expr/dispatch.rscrates/perry-codegen/src/expr/index_set.rscrates/perry-codegen/src/expr/mod.rscrates/perry-codegen/src/expr/proxy_reflect.rscrates/perry-codegen/src/stmt/mod.rstest-files/test_typed_array_store_expression_value.ts
Audit — correct and complete. Holding the merge briefly for one reason stated at the end.The mechanism is sound. ctx.discard_this_expr = true;
let result = lower_expr(ctx, e);
ctx.discard_this_expr = false;
ctx.discard_expr_value = prev_discard;
let _ = result?; // deferred, so both flags reset even on the error pathThe explicit It fixes more sites than the body claims. The prose says "four sites"; the The third caller is handled correctly. The surviving reader is correctly scoped out. Ran all four One recommendation, and I think it mattersThere are now two fields one misreading apart, and misreading them is
The names do not encode the distinction. Only the doc comments do, and the (Related, for whoever picks up the deferred Why I have not merged yetThe one check I have not run locally is That is deliberate rather than cautious-by-default: I merged #7579 today after |
`ctx.discard_expr_value` means "this STATEMENT's value is discarded". It is set once per Stmt::Expr and lower_expr never cleared it while recursing, so it was still set while lowering the OPERANDS of `sink(buf[0] = 5);`. Four sites read it as though it meant "this EXPRESSION's value is discarded" and returned 0.0: index_set.rs (typed-array store, proven-view checked store) and arrays_finds.rs (Uint8ArraySet / buffer stores). The stores landed correctly — only the expression's value was wrong, so this was silent. An assignment expression must evaluate to the assigned value (ES2024 13.15.2). Adds FnCtx::discard_this_expr, which dispatch::lower_expr TAKES at the top of every dispatch, so it reaches exactly the statement's own expression and every operand beneath reads false. The handlers receive it as a parameter rather than reading the field: they consult it after lowering their operands, by which point the field has been taken again. Found while gating arr.push(x)'s length computation on the same flag as a perf change (#7511); that produced this bug for sink(a.push(10)), n = a.push(20), a.push(1)+100 and a.push(1)>0?7:9, which exposed the pre-existing one. The optimisation is not included — it needs this fix first. Test consumes a store's value in call-argument, assignment, arithmetic, conditional and nested-store position, plus two discarded stores: the discarded form kept working throughout, so a smoke test passes while the bug is live.
2a7daa6 to
2e45351
Compare
1 participant
Fixes #7590.
The bug
The stores always landed correctly (
buf 5 7 3both before and after) — only the expression's value was wrong. So it was silent: a wrong number, no crash, no diagnostic. An assignment expression must evaluate to the assigned value (ES2024 §13.15.2).Cause
ctx.discard_expr_valuemeans "this STATEMENT's value is discarded". It is set once perStmt::Expr, andlower_exprnever cleared it while recursing — the only reset in the tree islower_call/new_ctor_args.rs, for constructor arguments. So it was still set while lowering the operands ofsink(buf[0] = 5);.Four sites read it as though it meant "this EXPRESSION's value is discarded" and returned
double_literal(0.0):expr/index_set.rs—lower_typed_array_store, proven-view checked storeexpr/arrays_finds.rs—Uint8ArraySet/ buffer storesexpr/dispatch.rsalso reads the flag but only to pick a materialization path, so it is unaffected and left alone.Fix
FnCtx::discard_this_expr, whichdispatch::lower_exprtakes (mem::take) at the top of every dispatch. It therefore reaches exactly one expression — the one the statement is made of — and every operand lowered beneath it readsfalse.The handlers receive it as a parameter rather than reading the field. That is deliberate: they consult the answer after lowering their operands, by which point the field has been taken again, so reading a field there would have reintroduced the same bug in a subtler form.
How it was found
I was gating
arr.push(x)'s length computation on the same flag as a performance change.js_array_lengthis not a field read — it resolves Proxy arrays through thegettrap and probes the registered-Set/Map side tables — and a statement-position push discards its result, so it is 8–13% ofpush_clsspent producing a number nobody reads (see #7511).That optimisation produced exactly this bug for
sink(a.push(10)),n = a.push(20),a.push(1) + 100anda.push(1) > 0 ? 7 : 9— which is what led me to test the pre-existing sites and find they had it already.The optimisation is not in this PR. It needs this fix first, and then the same non-leaking signal; landing them together would have mixed a correctness fix with a perf change.
Testing
test-files/test_typed_array_store_expression_value.tsconsumes a store's value in call-argument, assignment, arithmetic, conditional and nested-store position, and keeps two discarded stores to prove the ordinary path still writes. Output matchesnodeexactly.That combination is the point: the discarded form kept working the entire time the bug was live, so a "does it still run" smoke test passes. Only a test that consumes the value catches it.
test-filesprograms (typed-array/buffer weighted, plus a spread across the corpus) compiled and run under both arms: the only behavioural difference is this new test, which goesassign:WRONG got=0 want=7→assign:ok.perry-codegenfailure set identical toorigin/main(22 pre-existing).cargo fmt --all -- --checkandscripts/check_file_size.shclean.Summary by CodeRabbit
0when their values were used in arguments, assignments, calculations, conditions, or nested expressions.