fix: initialize sloppy block functions at block entry - #10232
proggeramlug wants to merge 2 commits into
Conversation
|
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 (10)
Included review availability: Your plan provides up to 8 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughBlock-function lowering now initializes and reuses block-local bindings in sloppy and strict code. Regression tests cover script and module contexts, loop captures, Annex B updates, parameter shadowing, and multiple optimization levels. CI builds the required runtime package for the suite. ChangesBlock Function Hoisting
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant TestSuite
participant Node
participant Perry
participant Executable
TestSuite->>Node: run fixture in CommonJS and module contexts
TestSuite->>Perry: compile at O0, Os, and Oz
Perry->>Executable: generate executable
TestSuite->>Executable: run compiled fixture
TestSuite->>TestSuite: compare output with Node
Merge Risk: ⚪ Minimal · up to The implementation and regression coverage are consistent with the intended block-function behavior and are ready to merge after normal checks. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 35.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 20 functions across 8 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
1 participant
Summary
Sloppy-script block functions used before their declarations were still undefined. Initialize their lexical bindings at block entry, preserving the separate Annex B outer-variable copy at the textual declaration. The reported retained-callback loop now prints
2,2,2, matching Node in both script and ES module packages.Changes
var, fresh lexical captures, parameter shadowing, outer-binding updates, and mutual recursion.Related issue
Fixes #10079.
Test plan
Validated on macOS arm64, LLVM 22, Node 26.5.1 (the repository pin), with a compiler and runtime/stdlib/net archives built together from this checkout. Local builds use the development profile, as required by the repository instructions.
cargo build -p perry -p perry-runtime-static -p perry-stdlib-static -p perry-ext-net -j3— passed.cargo test -p perry-hir --libpassed 413 tests, with 1 ignored.cargo test -p perry --test issue_10079_script_block_function_hoisting— passed all 6 native comparisons: commonjs/module × O0/Os/Oz,--platform bun --no-cache --no-auto-optimize --no-codegen. Each package context has its own successful Node oracle; the script fixture has no strict directive or ESM marker.PERRY_NO_AUTO_OPTIMIZE=1 RUST_TEST_THREADS=1 cargo test -p perry --test issue_nested_block_forward_capture --test issue_5848_annexb_global_init_reflection --test issue_5869_labeled_block_capture_boxing— 10 passed.node --test scripts/test-require-runtime.test.mjs— 8 passed.scripts/pre-tag-check.sh --quick— all checks passed except the existing public benchmark evidence-freshness failure. The artifact, verifier, and every source/harness fingerprint input are unchanged from base6000a00dfe.actionlint .github/workflows/test.ymlreports the same three ShellCheck findings as the base workflow (SC2086, SC2012, SC2129), all outside the modified setup.git diff --check— passed.Full workspace, release, and platform-cross checks were not run locally. The initial native test attempt exposed the missing Bun startup archive; the recorded passing run followed a coherent rebuild including
perry-ext-net.Checklist
Cargo.toml,Cargo.lock,CLAUDE.md, andCHANGELOG.mdunchanged.fix:prefix.Summary by CodeRabbit
Bug Fixes
Tests
CI follow-up
The
zizmoraudit fails on the existingworkflow_runtrigger in.github/workflows/gate-failure-watch.yml(dangerous-triggers); that workflow is unchanged by this PR. The TLS checker also fails on the existing raw thread-local inruntime/regex/perex_owner.rs. Neither failure comes from the scoped integration dependency setup. Other jobs are still running.CI follow-up: the completed run is red. All eight gap failures also occur on the independent #10229 and #10230 branches, which do not contain this hoisting change:
iterator_prototype_next_patch(Node oracle exit 1),2899_2779_2777_static_helpers,disposablestack_2875,gc_http2_pending_event_callback_rooting(port 443 permission denial),2159_defineproperty_class_prototype(Perry exit 13),json_lazy_defineproperty_index(descriptor-read assertion),perfhooks_3088_3008_3010_3011, andv8_2_3680plus(Perry exit 1). The HTTP/2 fixture also fails all seven GC-stress arms. This supports a shared base/environment cause; it is not an exact-main A/B rerun.