Skip to content

fix(es/minifier): Handle toExponential(undefined)#11583

Open
cuyl wants to merge 2 commits intoswc-project:mainfrom
cuyl:handle-undefined-args-for-toprecision
Open

fix(es/minifier): Handle toExponential(undefined)#11583
cuyl wants to merge 2 commits intoswc-project:mainfrom
cuyl:handle-undefined-args-for-toprecision

Conversation

@cuyl
Copy link
Contributor

@cuyl cuyl commented Feb 24, 2026

  • Evaluate when parameters is empty or undefined.

Closes #11572

@cuyl cuyl requested a review from a team as a code owner February 24, 2026 08:32
@changeset-bot
Copy link

changeset-bot bot commented Feb 24, 2026

⚠️ No Changeset found

Latest commit: c72e302

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions
Copy link
Contributor

github-actions bot commented Feb 24, 2026

Binary Sizes

File Size
swc.linux-x64-gnu.node 28M (28744264 bytes)

Commit: 035bdb0

@codspeed-hq
Copy link

codspeed-hq bot commented Feb 24, 2026

Merging this PR will not alter performance

✅ 190 untouched benchmarks


Comparing cuyl:handle-undefined-args-for-toprecision (c72e302) with main (f11fd70)

Open in CodSpeed

@cuyl cuyl force-pushed the handle-undefined-args-for-toprecision branch from b737d27 to 2b02292 Compare February 26, 2026 03:33
@cuyl cuyl marked this pull request as draft February 26, 2026 03:49
@cuyl
Copy link
Contributor Author

cuyl commented Feb 26, 2026

Opps, let me see if I can find a way to remove clone.

@cuyl cuyl force-pushed the handle-undefined-args-for-toprecision branch from 2b02292 to 0e614e3 Compare February 26, 2026 10:21
@cuyl cuyl marked this pull request as ready for review February 26, 2026 11:08
@cuyl cuyl requested a review from kdy1 February 26, 2026 11:09
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0e614e347d

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

@cuyl
Copy link
Contributor Author

cuyl commented Feb 26, 2026

P1 Badge Do not reject all spread arguments in number call folding
This guard returns immediately whenever the first argument is spread, which makes get_first_arg unreachable for cases like (1.23).toExponential(...[]) and (1.23).toExponential(...[undefined]). Those calls are equivalent to passing no precision and are exactly the new cases added in tests/terser/compress/evaluate/number_method_call/input.js, so the current condition prevents the intended optimization from ever running.
Useful? React with 👍 / 👎.

We already have eval_array_spread, eval_spread_array_in_args and eval_spread_array_in_array runs before eval_number_method_call I belive most of cases like this are handled.

@cuyl cuyl force-pushed the handle-undefined-args-for-toprecision branch from 0e614e3 to c72e302 Compare February 28, 2026 03:04
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c72e302ce2

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

@cuyl
Copy link
Contributor Author

cuyl commented Mar 14, 2026

Does anything block this PR?
@kdy1

Copy link
Member

@kdy1 kdy1 left a comment

Choose a reason for hiding this comment

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

AI coding is fine, but well... the diff in this PR does not look fine

/// evaluate it.
///
/// `None` means that there is no argument.
fn get_first_arg<'a>(
Copy link
Member

Choose a reason for hiding this comment

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

This function is very wrong

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opps, Should I revert it? I just want to remove the clone as you requested.
At first time I tried to add lifetime to prevent the clone, but I got an error.

error: reached the recursion limit while instantiating `get_first_arg::<'_, std::iter::Map<std::slice::Iter<'_, std::option::Option<swc_ecma_ast::ExprOrSpread>>, {closure@...}>>`

The code is here.

fn get_first_arg<'a, I>(args: I, expr_ctx: &ExprCtx) -> Option<Option<f64>>

where

    I: IntoIterator<Item = Option<&'a ExprOrSpread>>,

{

    args.into_iter()

        .find_map(|arg| {

            if let Some(arg) = arg {

                if arg.spread.is_some() {

                    return match arg.expr.as_array() {

                        Some(args) => {

                            if args.elems.is_empty() {

                                // next argument is used if the first spread argument is empty

                                // so we can't evaluate the call.

                                None

                            } else {

                                Some(get_first_arg(

                                    args.elems.iter().map(|e| e.as_ref()),

                                    expr_ctx,

                                ))

                            }

                        }

                        // If the spread argument is not an array, we can't evaluate the call.

                        None => Some(None),

                    };

                } else if !arg.expr.is_undefined(*expr_ctx) {

                    return Some(Some(eval_as_number(*expr_ctx, &arg.expr)));

                }

            }

            Some(None)

        })

        .unwrap_or(None)

}

To resolve the error, I've using AI to refactor it to the non-recursion version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But AI can't make it right, so I changed some logic to make it right.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Should treat undefined the same as empty args when calling toExponential and toPrecision

2 participants