Skip to content

Conversation

@YanWQ-monad
Copy link
Contributor

@YanWQ-monad YanWQ-monad commented Aug 17, 2025

} else if let Expression::ArrayExpression(arr) = &self.object {
Some(ConstantValue::Number(arr.elements.len().to_f64().unwrap()))

If there are spread elements in an array, say [...[1, 2, 3]], then arr.elements.len() no longer represents the value of .length property of the array, which will cause mis-optimization.

PoC of the bug

test.js:

let k = [...[1, 2, 3]].length;
console.log(k);
$ node test.js
3

$ cargo run --example minifier -- test.js
    Finished `dev` profile [unoptimized] target(s) in 0.08s
     Running `target/debug/examples/minifier`
console.log(1);

This bug was originally discovered in vitejs/rolldown-vite#375.

Maybe sometimes the .length can 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))) and test_eval("[...a].length", None), but I cannot find a suitable place and found it's not trivial to implement one Ctx for evaluate_value in test files. Please let me know if you have any comments. Thanks.

… elements

Co-authored-by: GZTime <Time.GZ@outlook.com>
Copilot AI review requested due to automatic review settings August 17, 2025 20:32
@graphite-app
Copy link
Contributor

graphite-app bot commented Aug 17, 2025

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • 0-merge - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

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.

@github-actions github-actions bot added the C-bug Category - Bug label Aug 17, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_length function
  • 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-hq
Copy link

codspeed-hq bot commented Aug 17, 2025

CodSpeed Instrumentation Performance Report

Merging #13162 will not alter performance

Comparing YanWQ-monad:fix-1 (305cee8) with main (2674634)

Summary

✅ 34 untouched benchmarks

@Boshen Boshen self-assigned this Aug 18, 2025
GZTimeWalker added a commit to GZTimeWalker/GZCTF that referenced this pull request Aug 19, 2025
@github-actions github-actions bot added the A-minifier Area - Minifier label Aug 20, 2025
@Boshen Boshen merged commit d27a04b into oxc-project:main Aug 20, 2025
25 checks passed
hez2010 pushed a commit to GZTimeWalker/GZCTF that referenced this pull request Oct 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-minifier Area - Minifier C-bug Category - Bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants