fix(ecmascript): skip array length evaluation if there are any spread elements#13162
fix(ecmascript): skip array length evaluation if there are any spread elements#13162Boshen merged 2 commits intooxc-project:mainfrom
Conversation
… elements Co-authored-by: GZTime <Time.GZ@outlook.com>
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug in ECMAScript constant evaluation where array length was incorrectly calculated when arrays contained spread elements. Arrays with spread syntax like [...a] were incorrectly evaluated using the number of elements rather than recognizing that the actual length cannot be statically determined.
- Refactored length evaluation logic into a separate
evaluate_value_lengthfunction - Added check to skip evaluation when spread elements are present in arrays
- Fixed both static and computed member expression handling for the "length" property
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
CodSpeed Instrumentation Performance ReportMerging #13162 will not alter performanceComparing Summary
|
- waiting for oxc-project/oxc#13162
- waiting for oxc-project/oxc#13162
oxc/crates/oxc_ecmascript/src/constant_evaluation/mod.rs
Lines 501 to 502 in 4d0a05e
If there are spread elements in an array, say
[...[1, 2, 3]], thenarr.elements.len()no longer represents the value of.lengthproperty of the array, which will cause mis-optimization.PoC of the bug
test.js:This bug was originally discovered in vitejs/rolldown-vite#375.
Maybe sometimes the
.lengthcan be evaluated recursively, but here we just skip the evaluation as a quick fix.What's more, I think it would be better to add some tests like
test_eval("[1, 2, 3].length", Some(Number(3.0)))andtest_eval("[...a].length", None), but I cannot find a suitable place and found it's not trivial to implement oneCtxforevaluate_valuein test files. Please let me know if you have any comments. Thanks.