fix(hir): #5579 — publish module-top global eval's top-level var/function as configurable global bindings#5725
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR changes indirect eval lowering to skip completion-IIFE folding for eval bodies containing class declarations, and it rewrites top-level ChangesIndirect eval lowering
Sequence Diagram(s)sequenceDiagram
participant try_indirect_eval_general
participant apply_global_eval_hoist
participant build_eval_completion_iife
try_indirect_eval_general->>apply_global_eval_hoist: rewrite sloppy global eval body
try_indirect_eval_general->>build_eval_completion_iife: fold only when class-free
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@crates/perry-hir/src/lower/const_fold_fn.rs`:
- Around line 791-805: The eval lowering in
const_fold_fn::build_eval_completion_iife is treating every None from
apply_global_eval_hoist as “safe to fold,” but that also includes unsafe cases
like var-scoped globals, with, destructuring var, and globalThis rebinding.
Change apply_global_eval_hoist to distinguish hoisted, safe-no-hoist, and
unsafe-bail outcomes, then only call build_eval_completion_iife for strict eval
or bodies confirmed not to need global publication; otherwise keep the runtime
thunk path. Update the relevant eval tests in the const_fold_fn test area so for
(var i = ...) asserts the correct global publication/runtime behavior.
In `@crates/perry-hir/src/lower/global_eval_hoist.rs`:
- Around line 646-655: The hoisting logic in global_eval_hoist::fold is treating
nested var declarations as IIFE-local, but sloppy global eval requires all
var-declared names to be published globally even when they appear inside blocks,
cases, or loops. Update the handling around
ast::Stmt::Decl(ast::Decl::Var(var_decl)) so eval-scope var declarations are
rewritten/hoisted regardless of nesting, or emit an unsafe-bail signal that
prevents raw IIFE folding when the rewrite cannot preserve global
VarDeclaredNames semantics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1c6ac2ac-4228-4fbc-91be-0001bb276e26
📒 Files selected for processing (2)
crates/perry-hir/src/lower/const_fold_fn.rscrates/perry-hir/src/lower/global_eval_hoist.rs
…tion as configurable global bindings
EvalDeclarationInstantiation: a sloppy *global* (direct or indirect) eval whose
body declares a top-level `var` or `function` must create a configurable own
binding on the global object (CreateGlobalVarBinding / CreateGlobalFunctionBinding)
that survives after the eval returns. Perry folds an eval body into a
scope-capturing arrow IIFE, which trapped top-level `var`/`function` as
arrow-locals, so `eval('var x; …')` / `eval('function f(){}')` never published
`x`/`f` to globalThis (test262 reason: "x/f should be an own property"). #5636
already handled *nested* block functions; this extends the same hoist to the eval
body's own top level.
- global_eval_hoist: a top-level `function` is renamed to a hidden binding and
published with its value at instantiation via `void (f = <hidden>)` (the `void`
keeps the declaration's empty completion value); a top-level `var x = init`
becomes a create-if-absent global slot plus `void (x = init)`. A non-simple
(destructuring) declarator bails the rewrite.
- const_fold_fn: a declaration-bearing *indirect* eval body at module-top global
with nothing to hoist now still folds to the completion IIFE (running its side
effects + yielding the completion value) instead of deferring to the runtime
eval thunk, which returns `undefined` without executing the body. Guarded by
`eval_body_iife_foldable`, which keeps a class-declaring body on the runtime
thunk (Perry would otherwise leak the class to module scope —
language/eval-code/indirect/lex-env-distinct-cls).
test262 language/eval-code + annexB/language/eval-code: 753 → 764 passing
(parity 93.2% → 94.6%), zero regressions. Fixes var-env-{var,func}-init-global-new
(direct+indirect), var-env-func-init-multi (direct+indirect),
var-env-func-init-global-update-non-configurable, and the
non-definable-function-with-{function,variable} negatives.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
proggeramlug
force-pushed
the
worktree-fix-5579-eval-residual
branch
from
ee18f99 to
4f0a66b
Compare
…nctions (descriptor-correct global publish)
Follow-up to the top-level var/function eval publishing: a top-level function in
a sloppy global eval is now published with the spec's CreateGlobalFunctionBinding
descriptor rules instead of a bare `void (f = <hidden>)` assignment. An absent or
configurable global binding is (re)defined as a writable, enumerable, configurable
data property; a non-configurable one keeps its attributes and only takes the new
value. This makes the published descriptor correct when the eval redefines an
existing *configurable* global whose attributes differ (e.g. a non-enumerable,
non-writable `f`).
`apply_global_eval_hoist` now also bails (→ unmodified fold) when the eval body
rebinds `Object` at function scope, since the publish reads `Object.defineProperty`
/ `Object.getOwnPropertyDescriptor`.
test262 language/eval-code + annexB/language/eval-code: 764 → 766 passing
(parity 94.6% → 94.8%), zero regressions. Fixes
var-env-func-init-global-update-configurable (direct + indirect).
(The non-definable-global-{function,generator} negatives remain: Perry does not
model `NaN`/`undefined` as non-configurable own properties of globalThis, so the
illegal redefinition does not throw — a separate globalThis-modeling gap.)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jun 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Residual of the #5579 regression tracker: the
var-env-*eval-code cluster ("x/fshould be an own property"). A sloppy global (direct or indirect)evalwhose body declares a top-levelvarorfunctionmust create a configurable own binding on the global object (CreateGlobalVarBinding / CreateGlobalFunctionBinding) that survives after the eval returns.Perry folds an eval body into a scope-capturing arrow IIFE, which trapped top-level
var/functionas arrow-locals, soeval('var x; …')/eval('function f(){}')never publishedx/ftoglobalThis. #5636 already handled nested block functions; this extends the same hoist to the eval body's own top level.Changes
global_eval_hoist: a top-levelfunctionis renamed to a hidden binding and published at instantiation via a CreateGlobalFunctionBinding block (Object.defineProperty(globalThis, …)— writable/enumerable/configurable for an absent/configurable binding; value-only for a non-configurable one); a top-levelvar x = initbecomes a create-if-absent global slot plusvoid (x = init)(thevoidpreserves the declaration's empty completion value). Non-simple (destructuring) declarators, or a body that rebindsglobalThis/Object, bail to the unmodified fold.const_fold_fn: a declaration-bearing indirect eval body at module-top global scope that has nothing to hoist now still folds to the completion IIFE (running its side effects + yielding the completion value) instead of deferring to the runtime eval thunk, which returnsundefinedwithout executing the body. Guarded byeval_body_iife_foldable, which keeps a class-declaring body on the runtime thunk (Perry would otherwise leak the class to module scope —lex-env-distinct-cls).Validation
test262
language/eval-code+annexB/language/eval-code(pinned sha, Node v26.3.0vm.runInThisContextoracle):Zero regressions (verified by diffing the full failure path-sets). Newly passing (13):
var-env-{var,func}-init-global-new(direct+indirect),var-env-func-init-multi(direct+indirect),var-env-func-init-global-update-{non-,}configurable(indirect / direct+indirect), and thenon-definable-function-with-{function,variable}negatives (direct+indirect).New unit tests cover top-level var/function publishing, the bare-
varslot-only path, the CreateGlobalFunctionBinding block, andeval_body_iife_foldable(class vs non-class bodies).The remaining eval-code residual (descriptor-preserving var updates, function-scope B.3.3.3 / #5657,
non-definable-global-*which needsNaN/undefinedmodeled as non-configurable globalThis own props, and theif(x) function f(){}Annex-B parser gap) is out of scope here.Refs #5579.
🤖 Generated with Claude Code
Summary by CodeRabbit
evalfolding to avoid unsafe transformations when the eval body containsclassdeclarations.evalso hoisted top-levelfunction/varbehavior matches correct completion semantics.functionand simple identifiervardeclarations into the global during optimizedeval.classdeclarations and for global publishing/slot-creation behavior for top-levelfunctionandvar.