Repository navigation
fix(engine): correct computed class key evaluation timing - #5641
shreyashsri79 wants to merge 2 commits into
Conversation
github-actions
Bot
added
Waiting On Review
Test262 conformance changes
Fixed tests (1):Tested main commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5641 +/- ##
===========================================
+ Coverage 47.24% 63.13% +15.88%
===========================================
Files 476 536 +60
Lines 46892 60539 +13647
===========================================
+ Hits 22154 38221 +16067
+ Misses 24738 22318 -2420 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| // ClassDefinitionEvaluation initializes the class binding after evaluating | ||
| // all element names, but before executing static initializers. | ||
| // https://tc39.es/ecma262/#sec-runtime-semantics-classdefinitionevaluation | ||
| if let Some(scope) = class.name_scope { | ||
| let binding = scope.get_identifier_reference(class_name.clone()); | ||
| let index = self.insert_binding(binding); | ||
| self.emit_binding_access( | ||
| BindingAccessOpcode::PutLexicalValue, | ||
| &index, | ||
| &class_register, | ||
| ); | ||
| } |
There was a problem hiding this comment.
question: shouldn't this be after static elements as well?
There was a problem hiding this comment.
This initializes the inner class-name binding. In ClassDefinitionEvaluation, step 28 initializes it before step 32 executes static fields and blocks. The outer declaration binding is initialized afterward by BindingClassDeclarationEvaluation step 5.
Moving this below the static loop would make class C { static self = C; static { C; } } throw. The existing class_name_is_initialized_before_static_initializers regression covers both accesses. Commit 22e1da4 makes the two bindings and their spec steps explicit in the comments.
| self.bytecode.emit_to_property_key( | ||
| name_register.variable(), | ||
| name_register.variable(), | ||
| ); |
There was a problem hiding this comment.
suggestion: consider adding spec steps in this function, like we have for built-in objects.
There was a problem hiding this comment.
Added references to the relevant spec steps in 22e1da4: element evaluation, computed-name conversion, inner binding initialization, static initialization, and the outer declaration binding. The derived-constructor reference also now uses the current step number.
This revision changes comments only. cargo fmt --all --check and git diff --check pass; non-comment lines and Cargo.lock are unchanged. The engine rerun was not started because local disk space fell below the safety floor. The PR description keeps the earlier runtime results attributed to ace614a and records this revision's narrower checks.
Fixes #5635.
Computed static field names were converted to property keys during static initialization, after later instance field keys and earlier static initializers. The class-name binding was also initialized before computed names were evaluated.
This change converts each static computed name to a key at its source position and initializes the class binding after the element loop, before static initializers run, following ClassDefinitionEvaluation. The later redundant key conversion for anonymous static field initializers is removed.
Five tests cover mixed method/static/instance key order, key conversion before initializers, abrupt key conversion, the class-name TDZ in declarations and named expressions, and class-name access from static and instance initializers.
Validation at
ace614acc17106f8d8633b0b1071991506583433, based on329e89748ed34e218fe593b4209b1dbe1d6eb64d, with Rust/Cargo 1.99.1:C is not defineddiagnostic andReferenceErrorkind.cargo test --locked --profile ci -p boa_engine --lib -- --nocapture --test-threads=1: 1,144 passed, no failures or ignored tests.cargo test --locked --profile ci -p insta-bytecode -- --nocapture --test-threads=1: bytecode snapshots passed without updates.cargo fmt --all --check: passed.cargo clippy --locked --profile ci -p boa_engine --all-features --all-targets -- -D warnings: passed.cargo run --locked --profile ci --bin boa_tester -- run -vv -d --test262-path <pinned-checkout> -s <suite> -o <results>fortest/language/statements/classandtest/language/expressions/class: 8,402 passed, 24 ignored, zero failures/panics on both baseline and candidate, with no case-level outcome changes. Test262 revision:d86b2294eb0a17eaa281ff12c73c473ec864c72f. These existing suites also pass on the buggy baseline; the new unit tests establish the regressions.Full workspace/all-feature test suites and full Test262 were not run locally.
Cargo.lockis unchanged.Prepared with OpenAI Codex assistance.
Review follow-up at
22e1da48260bb290214921ec59d6b03201b582dfadds specification-step comments only.cargo fmt --all --checkandgit diff --checkpassed; non-comment lines andCargo.lockare unchanged fromace614ac. The engine rerun was not started because free disk space fell below the local 8 GiB safety floor. Bytecode, Clippy and Test262 were not rerun for this comments-only revision; the runtime results above are fromace614ac.