Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Minifier should not drop new for RegExp #6941

Closed
kdy1 opened this issue Feb 14, 2023 · 2 comments · Fixed by #7091
Closed

Minifier should not drop new for RegExp #6941

kdy1 opened this issue Feb 14, 2023 · 2 comments · Fixed by #7091
Labels
Milestone

Comments

@kdy1
Copy link
Member

kdy1 commented Feb 14, 2023

Describe the bug

I'm not sure if this is important, though.

Reported by vercel/next.js#30237 (comment)

Input code

const pattern = /[abc]+/g;

const instance1 = RegExp(pattern);
const instance2 = new RegExp(pattern);

instance1 === pattern // true
instance 2 === pattern // false

Config

{
  "jsc": {
    "parser": {
      "syntax": "typescript",
      "tsx": true
    },
    "target": "es2020",
    "loose": false,
    "minify": {
      "compress": false,
      "mangle": false
    }
  },
  "module": {
    "type": "es6"
  },
  "minify": false,
  "isModule": true
}

Playground link

https://play.swc.rs/?version=1.3.35&code=H4sIAAAAAAAAA0vOzysuUShILClJLcpTsFXQj05MSo7V1k%2B35uJKBstlAonEvORUQ6BsUGq6a0WBBlS5pjWaEiOgkrzUckxlXEiG2NrCrdPXVygpKk2FyyoYoUunJeYUpwIAjgX8hqQAAAA%3D&config=H4sIAAAAAAAAA0WOSwrDMAxE76J1FiGLLnyHHsK4SnDxD0mBGuO7VzYJ2Q0zo6dp8GUHpkGxxEhDcU1if2BAakF25IvAAsJqCZ3YVVs6ULSBvK3bqmnImRHMbgPjAtEnv9fBcjkWQuYnsukId7MrK%2BbPOYw2303mC%2FrDuO48v6%2Fi3PAHuM3Ro7cAAAA%3D

Expected behavior

It should pass

Actual behavior

No response

Version

1.3.35

Additional context

No response

@kdy1 kdy1 added the C-bug label Feb 14, 2023
@kdy1 kdy1 added this to the Planned milestone Feb 14, 2023
@kdy1 kdy1 self-assigned this Feb 14, 2023
@kdy1 kdy1 removed their assignment Mar 10, 2023
@andersk
Copy link
Contributor

andersk commented Mar 15, 2023

Based on the comment at 5940894#diff-671927f9f8892091ded45850d4642521fa87cd1b0ee1aba677ca7739aed217e9R252-R253, this optimization seems to have been motivated by the following sentence in the ECMAScript specification:

Thus the function call RegExp(…) is equivalent to the object creation expression new RegExp(…) with the same arguments.

However, I think that sentence is wrong. It contradicts the detailed steps in the following section of the specification, as well as the empirical behavior of JavaScriptCore, SpiderMonkey, and V8:

> r = /a/;
/a/
> RegExp(r) === r
true
> new RegExp(r) === r
false

I’ve opened a thread at https://es.discourse.group/t/is-regexp-really-equivalent-to-new-regexp/1637 to ask about the spec contradiction, but I think this optimization for new RegExp(…) needs to be removed.

Cc @magic-akari (#4932).

andersk added a commit to andersk/swc that referenced this issue Mar 16, 2023
In swc-project#4932, an optimization was added that transforms `new RegExp(…)` to
`RegExp(…)`.  It seems to have been motivated by this sentence in the
ECMAScript specification:

“Thus the function call RegExp(…) is equivalent to the object creation
expression new RegExp(…) with the same arguments.”

But that sentence turns out to be wrong.  It contradicts the detailed
steps in the following section of the specification, as well as the
empirical behavior of JavaScriptCore, SpiderMonkey, and V8:

    > r = /a/;
    /a/
    > RegExp(r) === r
    true
    > new RegExp(r) === r
    false

Restrict this optimization to the case where we can prove one of the
first two arguments is a string.

Fixes swc-project#6941.

Signed-off-by: Anders Kaseorg <andersk@mit.edu>
andersk added a commit to andersk/swc that referenced this issue Mar 16, 2023
In swc-project#4932, an optimization was added that transforms `new RegExp(…)` to
`RegExp(…)`.  It seems to have been motivated by this sentence in the
ECMAScript specification:

“Thus the function call RegExp(…) is equivalent to the object creation
expression new RegExp(…) with the same arguments.”

But that sentence turns out to be wrong.  It contradicts the detailed
steps in the following section of the specification, as well as the
empirical behavior of JavaScriptCore, SpiderMonkey, and V8:

    > r = /a/;
    /a/
    > RegExp(r) === r
    true
    > new RegExp(r) === r
    false

Restrict this optimization to the case where we can prove one of the
first two arguments is a string.

Fixes swc-project#6941.

Signed-off-by: Anders Kaseorg <andersk@mit.edu>
@kdy1 kdy1 modified the milestones: Planned, v1.3.41 Mar 17, 2023
@swc-bot
Copy link
Collaborator

swc-bot commented Apr 16, 2023

This closed issue has been automatically locked because it had no new activity for a month. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@swc-project swc-project locked as resolved and limited conversation to collaborators Apr 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging a pull request may close this issue.

3 participants