fix(transform): keep statement-position inlines from returning out of the caller - #10530
proggeramlug wants to merge 2 commits into
Conversation
… the caller A call used as a statement (`f(x);`) whose inlined callee ended in an `if` containing `return` had that return spliced into the caller: the statement arm of inline_calls_in_stmts only looked for nested returns in take(len - 1) and only rewrote a bare trailing Return. The caller then returned the callee's value (decimal.js pow() returned true), and at module top level the stray ret produced invalid LLVM IR. The statement arm now removes every return structurally (discard_inlined_returns): `return e` becomes `e;`, and statements after an `if` that returns move into the branch that falls through to them. No loop wrapper is added. It declines (keeps the call) instead of duplicating or dropping statements, or when a return sits under a loop, switch, try or label. Two siblings of the same blind spot are fixed as well: the declined-inline fallback replaced the call statement with the setup hoisted out of its arguments (deleting the call), and the void-method expression inliner spliced statements that follow `return;`.
proggeramlug
added
the
package-audit
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe inliner now rewrites discarded returns safely, preserves calls for unsupported control flow, stops void-method expansion after bare returns, and adds unit, integration, and end-to-end regression coverage. ChangesDiscarded-result inlining
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The discarded-result rewrite preserves nested closure returns, and no actionable merge risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 51.85% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 54 functions across 4 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
|
baseline 7661bc05fe |
this branch | |
|---|---|---|
PERRY_SKIP_BUILD=1 ./run_parity_tests.sh --filter test_gap_cron_cronjob |
12/12 PASS | 12/12 PASS |
| compiled test binary run directly, 24 concurrent instances | 24/24 identical correct output | 24/24 identical correct output |
| same binary pinned to one core against 3 busy loops, 5 runs | 5/5 correct | 5/5 correct |
The test also passed in my full local gap sweep on this branch (test-parity/reports/…_103801.json: {"id": "test_gap_cron_cronjob", "status": "pass"}, 820 tests, GAP_EXIT=0, the only 6 failures being the same 6 the baseline sweep has).
3. CI history says the same. Across the last 40 test.yml runs I found 20 gap-suite jobs that ran this test. It passed in 19 and failed in exactly one: job 105204344569, the one on this PR. Passing jobs take 11–15 s for this test, and the failing one took 14.2 s, so the duration does not distinguish them either.
Why it can flake. The test asserts booleans that are gated on wall-clock ticks: it waits up to 10 s for a * * * * * * job to fire twice, then prints manual ticked at least twice: <bool> and auto ticked at least twice: <bool>. Node and Perry each run under their own deadline, so a slow moment on either side flips a boolean on that side only and the byte comparison fails. That matches the reported symptom — first line (constructed, ticks now: 0) equal, a later line differing, both exit 0. The harness truncated the diff, so I cannot say which side flipped; nothing in the run points at Perry rather than at the Node oracle.
I have not changed the test: it is not this PR's, and tightening it would be a change to an unrelated fixture. It may be worth a separate issue to make the timing assertions deterministic (or to give the deadline more headroom) — happy to file one if you want it.
Lint, for the record. scripts/run_lint_gates.sh (full, including the compile tier) is 82 of 83 green on this branch, with 2 CI-only steps skipped. The single red is Public benchmark evidence freshness (benchmarks/ci_public_baseline_check.py: "public artifact benchmark inputs changed"), which fails identically on a clean 7661bc05fe checkout — the known-red step that is left red on main.
|
Landed via merge train #10610 (v0.5.1594). All source commits preserve authorship; merged main matches the validated train exactly. |
1 participant
Summary
A small helper called as a statement (
f(x);, result discarded) whose inlined body ended in anifcontainingreturnhad thatreturnspliced into the caller by the HIR inliner. The caller then returned the helper's value: decimal.jsintPowreturnedtruncate'strue, sonew Decimal(x).pow(n)wastrue. At module top level the strayret doublefailed LLVM IR validation againstmain'si32result.Root cause
inline_calls_in_stmts'sStmt::Exprarm (crates/perry-transform/src/inline/call_inliner.rs:380-447on 7661bc0) checked for nested returns only ininlined_stmts.iter().take(len - 1), so it never looked inside the last statement, and then rewrote the last statement only when it was a bareStmt::Return. A trailingifcontaining returns passed both checks, and itsReturns were spliced into the caller unchanged. Thelet r = f()arm (:547-592) already handles this shape with ado { … } while (false)wrapper plusconvert_returns_in_stmts.Auditing the same file for the same blind spot (a
returnhandled as if it sat at the tail) found two siblings, both confirmed on 7661bc0:inline_calls_in_expron the call and then replaced the call statement with whatever setup it hoisted out of the arguments (new_stmts = Some(hoisted)).f(g(i));, wheregis aconst z = …; return z + 1helper andfhas an early return, printed nothing: the call tofwas gone.try_inline_simple_call's void-method path (:1910-1926) skippedStmt::Return(None)and kept going, som() { return; this.x = 1; }used as a value (console.log(o.m())) ranthis.x = 1.The
let r = f()arm, thereturn f()/throw f()arm and the expression-position arms (try_inline_simple_callpatterns 1/2) are not affected.Fix
inline/discarded_result.rs:discard_inlined_returnsremoves every return from an inlined body whose result is discarded, structurally rather than with a loop wrapper.return ebecomese;(evaluation kept, value dropped),return;becomes nothing, and the statements after anifthat returns move into the branch that can still fall through to them (if (c) { a; return v; } restbecomesif (c) { a; v; } else { rest }). It declines, which keeps the call, instead of duplicating or dropping statements: both branches can fall through to a non-empty continuation, statements follow areturn(or anifwhose branches all exit), or areturnsits under a loop,switch,tryor label. It adds no loop or label, so abreak/continuein the callee keeps its target, and it does not rely onhas_simple_control_flowfor any of that.Stmt::Exprarm uses it. When it declines, the arm uses the ordinary hoist path, which keeps the call statement after the hoisted argument setup (sibling 1).return;(sibling 2).Why not the
do { } while (false)wrapper, and why not refuse: refusing loses inlining for exactly the guard helpers this affects, and a real call is 3.4 to 3.8x the instructions in a hot loop (table below). The wrapper puts a loop in the caller's hot path, with a GC back-edge poll, exact-receiver facts cleared after it, and a loop-shape change for any loop the call sits in. The structural rewrite measured equal to or slightly better than the wrapper written out by hand (1%). A side effect: bodies with an early return before more statements (if (x < 0) return; acc[0] += x;) were previously never inlined at a statement call site. They now are, which matches thelet r = f()arm.Tests
test-files/test_gap_10416_inliner_stmt_return.ts: call statements infor,while,do-whileandfor (;;)loops (non-constant bounds, so they stay loops), in a function with more than 10 statements, in class methods with and without a loop, in arrows (module-level and nested), at module top level, and in a method-call callee on an exact receiver. Callee shapes:if (c) { return v; }only/last,const y = …; if (y) return v;, if/else returning in both branches, early return then more work, else-branch return, a nested conditional return (declined, call kept, argument helper still inlined),return;,vardeclared before the return, a return with a side effect, nested helpers, andswitch/try/finally/generator/async callers. Controls: value uses,return wafter theif,throwafter theif, and a call site where theifis false at runtime.error: value doesn't match function result type 'i32'). With the top-level block removed, 13 of 15 checked lines differ from Node (for exampleinFor:1,2becomestrue,nested:…loses the deleted call's output, anddead:0becomesdead:1). The two that match are the controls line and them()value line.PERRY_SKIP_BUILD=1 ./run_parity_tests.sh --filter test_gap_10416).crates/perry-transform/src/inline/discarded_result.rs: 7 rewrite unit tests, plus 3 through the inliner: the Inliner copies a callee'sreturninto the caller when the call is a statement and the callee ends inif (...) { return ... }#10416 repro viainline_functions(function body and module init), the kept call after hoisted argument setup, and the void method with statements afterreturn;. The 3 inliner tests fail with the oldcall_inliner.rs(verified by swapping the file back); all 10 pass on this branch.Validation (perrybuilder, Linux x64)
cargo test --release -p perry-transform --testscargo test --release -p perry-codegen --testscargo test --release -p perry --testsperrybin unit tests: 1128 of 1129 passed. The one failure,geisterhand::tests::warm_archives_are_rebuilt_as_one_runtime_graph, is aNotFoundspawning a PATH-resolved tool and passes when run alone; this crate's tests mutate the process-globalPATH, and the test is unrelated to this change. Integration suites: running all 380 serially was not practical on the shared host, so I ran the 23 that exercise inlining, IR shape or GC rooting (issue_inline_seq_arg_binding_hoist,method_shape_inline_guard,minsize_inline_policy,issue_5437_comma_seq_ctor_fields,issue_9131_prototype_method_replacement,issue_8897_field_push_writeback,issue_class_instance_wide_set,issue_5195_property_method_chain,issue_6040_generic_class_dispatch,issue_8693_imported_this_specialization,guarded_numeric_arith,issue_8774_argument_shape_clones,loop_property_array_hoist,issue_9342_u8_inline_read,packed_loop_offset_read_accumulator,strided_tagged_fill,module_global_typed_array_read,issue_9253_affine_range_index,issue_9259_length_bound_offset_reads,gc_array_iter_rooted,gc_write_barrier_stress,issue_10300_cross_module_native_base,standalone_regression): 63 passed, 0 failed, 2 ignored.cargo fmt --all -- --checkscripts/run_lint_gates.sh(full, incl. compile tier)Public benchmark evidence freshness(ci_public_baseline_check.py: "public artifact benchmark inputs changed"), which is pre-existing: it fails the same way on a 7661bc0 checkout. The compile tier (-D warningscheck and clippy, product and host-compatible, API docs regen and drift) is green.scripts/run_gap_tests.sh) vs 7661bc0 baseline runGAP_EXIT=0, gap snapshot OK. 814 of 820 pass. The 6 non-passing tests (2159_defineproperty_class_prototype,2514_settracesigint,json_lazy_defineproperty_index,perfhooks_3088_3008_3010_3011,prop_plan_cache_invalidation,v8_2_3680plus) are the same 6 that fail in the 7661bc0 baseline run. No new failures. The new test passes.Performance (
perf stat -e instructions, 3 runs each, 50M iterations; Node 26.5.1 wall for context)truncate(arr, 10)(trailing if-return,ifnever true)if (v < 0) return; acc[0] += v;(was never inlined)D:0: the caller returned on the first iteration)benchmarks/suite/09_method_calls.tstimes/plus/div/sqrt)Hand-written loop bodies on this branch put the structural form against the
do { … } while (false)wrapper: 10.864e9 vs 10.865e9 for if/else, and 9.963e9 vs 10.064e9 for a guard plus work.Binary size: decimal.js
main_w31,554,768 → 31,554,792 bytes (+24).Package check (decimal.js 10.6.0, audit
main_w.ts)pownow matches Node (["1.267650600228229401496703205376e+30","2.718145926825224864","0.125"]; it threw(boolean).toSignificantDigits is not a functionbefore). The remaining differences are identical on 7661bc0 and are separate blockers:chain,toFixedandbig moneygiveNaN,toExponential/toPrecisiongiveNaN,new D2(1) instanceof Decimalfor aDecimal.clone()constructor isfalse, andtoHexadecimal/toBinarygive0x0/0b0.Not verified
PERRY_SKIP_BUILD=1, no auto-optimize) on Linux x64 only.do { } while (false)alternative was compared only as hand-written TypeScript loop bodies, not as a compiler variant.perryintegration suites (the sweep and full CI tiers run them).inline_calls_in_stmtsarm.Fixes #10416
Summary by CodeRabbit
Bug Fixes
return;from affecting generated code.Tests