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

Fix unicode handling in generated function names #14047

Merged
merged 4 commits into from Dec 20, 2021

Conversation

The-x-Theorist
Copy link
Contributor

@The-x-Theorist The-x-Theorist commented Dec 13, 2021

Q                       A
Fixed Issues? Fixes #13983
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

transform-unicode-escapes won't throw if a function name contain unicode encoding.
transform-function-name won't inject name if it's unicode encoding with helper-function-name.

@babel-bot
Copy link
Collaborator

@babel-bot babel-bot commented Dec 13, 2021

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/50423/

@@ -223,6 +223,8 @@ export default function (
name = id.name;
}

if (isFunction(node) && /[\u{10000}-\u{10ffff}]/u.test(name)) return;
Copy link
Contributor

@lightmare lightmare Dec 13, 2021

Choose a reason for hiding this comment

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

Just curious: won't this get transpiled into an unnecessarily complex regex without unicode flag?
I mean, /[\uD800-\uDFFF]/.test(name) would work as well, right?

Copy link
Contributor Author

@The-x-Theorist The-x-Theorist Dec 13, 2021

Choose a reason for hiding this comment

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

@nicolo-ribaudo suggested me to use this #14047 in this issue.

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo Dec 13, 2021

Choose a reason for hiding this comment

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

We transpile our code targeting Node.js 6, so we don't transpile the u flag. However yes, /[\uD800-\uDFFF]/.test(name) would work too.

Copy link
Contributor

@JLHwung JLHwung Dec 13, 2021

Choose a reason for hiding this comment

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

BTW /[\uD800-\uDFFF]/ are not equivalent to /[\u{10000}-\u{10ffff}]/u because lone surrogate can be a valid string literal. 🤦

var o = {
  "\uD800"() {}
}

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo Dec 13, 2021

Choose a reason for hiding this comment

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

Ok so /[\uD800-\uDFFF]/ is the correct one in this case.

Copy link
Contributor Author

@The-x-Theorist The-x-Theorist Dec 14, 2021

Choose a reason for hiding this comment

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

Okay, I will change it to /[\uD800-\uDFFF]/.

@@ -0,0 +1,4 @@
{
"sourceType": "unambiguous",
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo Dec 13, 2021

Choose a reason for hiding this comment

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

No need to specify it; the default (script) is ok to reproduce the bug.

Copy link
Contributor Author

@The-x-Theorist The-x-Theorist Dec 14, 2021

Choose a reason for hiding this comment

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

Okay, I will change it.

@@ -0,0 +1,4 @@
{
"sourceType": "unambiguous",
"presets": ["env"]
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo Dec 13, 2021

Choose a reason for hiding this comment

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

The plugin enabled by preset env will not always be the same. Can you enable just this plugin instead?

Copy link
Contributor Author

@The-x-Theorist The-x-Theorist Dec 14, 2021

Choose a reason for hiding this comment

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

Okay, I will enable only the plugin.

Copy link
Contributor Author

@The-x-Theorist The-x-Theorist Dec 14, 2021

Choose a reason for hiding this comment

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

If we don't use preset "env" it doesn't throw the error.

Copy link
Contributor

@JLHwung JLHwung Dec 15, 2021

Choose a reason for hiding this comment

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

I believe the plugin refers to the transform-function-name here. There is an options.json already, so it suffices to remove presets: ["env"].

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

@The-x-Theorist Since many engines support unicode in identifiers, we can approach this problem as @JLHwung suggested in #13983 (comment):

  1. The default function of @babel/helper-function-name takes an optional supportsUnicodeId parmeter, which defaults to false for better compatibility.
  2. In @babel/plugin-transform-function-name, in the plugin factory function (the exported one) we add
    const supportsUnicodeId = !isRequired("transform-unicode-escapes", api.targets());
    where isRequired is imported from @babel/helper-compilation-targets
  3. When calling nameFunction, we pass the supportsUnicodeId flag.

You then must add one test specifying targets: { "firefox": 50 } and one with targets: { "firefox": 60 } in options.json.

@nicolo-ribaudo nicolo-ribaudo added the PR: Bug Fix 🐛 label Dec 14, 2021
@The-x-Theorist
Copy link
Contributor Author

@The-x-Theorist The-x-Theorist commented Dec 14, 2021

@The-x-Theorist Since many engines support unicode in identifiers, we can approach this problem as @JLHwung suggested in #13983 (comment):

  1. The default function of @babel/helper-function-name takes an optional supportsUnicodeId parmeter, which defaults to false for better compatibility.

  2. In @babel/plugin-transform-function-name, in the plugin factory function (the exported one) we add

    const supportsUnicodeId = !isRequired("transform-unicode-escapes", api.targets());

    where isRequired is imported from @babel/helper-compilation-targets

  3. When calling nameFunction, we pass the supportsUnicodeId flag.

You then must add one test specifying targets: { "firefox": 50 } and one with targets: { "firefox": 60 } in options.json.
@nicolo-ribaudo I will do the same by adding supportUnicodeId in factory function in transform-function-name and adding a new test for a change.

@The-x-Theorist
Copy link
Contributor Author

@The-x-Theorist The-x-Theorist commented Dec 14, 2021

  1. In @babel/plugin-transform-function-name, in the plugin factory function (the exported one) we add
    const supportsUnicodeId = !isRequired("transform-unicode-escapes", api.targets());
    where isRequired is imported from @babel/helper-compilation-targets

If we add supportUnicodeId const in callback of declare in transform-function-name its value never change when we pass it in nameFunction of helper-function-name, thus it gives same output for different targets.

@nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Dec 14, 2021

If we add supportUnicodeId const in callback of declare in transform-function-name its value never change when we pass it in nameFunction of helper-function-name, thus it gives same output for different targets.

That's unexpected 🤔 Could you push your current code so that I can see why it's not working?

@The-x-Theorist
Copy link
Contributor Author

@The-x-Theorist The-x-Theorist commented Dec 15, 2021

If we add supportUnicodeId const in callback of declare in transform-function-name its value never change when we pass it in nameFunction of helper-function-name, thus it gives same output for different targets.

That's unexpected 🤔 Could you push your current code so that I can see why it's not working?

@nicolo-ribaudo I pushed the code, If preset env is not added to the earlier test it doesn't throw the error and if targets are added in options it still doesn't throw the error even with preset env in options, I don't the problem behind this.

@@ -0,0 +1,4 @@
{
"plugins": ["transform-unicode-escapes"],
"targets": { "firefox": 60 }
Copy link
Contributor

@JLHwung JLHwung Dec 15, 2021

Choose a reason for hiding this comment

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

This test case is testing the behaviour of transform-function-name when unicode id is not supported, however, Firefox 60 does support unicode escapes. In fact it has supported since 53. So we should specify Firefox 52 or other versions here.

Copy link
Contributor Author

@The-x-Theorist The-x-Theorist Dec 16, 2021

Choose a reason for hiding this comment

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

Okay, I will use Firefox 52 for not supported unicode escapes.

Copy link
Contributor Author

@The-x-Theorist The-x-Theorist Dec 16, 2021

Choose a reason for hiding this comment

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

After changing the targets it still gives the same result for different targets, I don't know what's happening?

Copy link
Contributor Author

@The-x-Theorist The-x-Theorist Dec 16, 2021

Choose a reason for hiding this comment

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

supportUnicodeId is same for all the tests.

@@ -0,0 +1,3 @@
var o = {
"\uD835\uDC9C"() {}
Copy link
Contributor

@JLHwung JLHwung Dec 15, 2021

Choose a reason for hiding this comment

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

The new test cases use shorthand-properties which is not supported by function-name. We can either plug in transform-shorthand-properties or just use ObjectProperty instead.

var o = {
  "\uD835\uDC9C": function () {}
}

When unicode escapes are supported, the output should be

var o = {
  "\uD835\uDC9C": function 𝒜() {}
};

otherwise, the output should be

var o = {
  "\uD835\uDC9C": function () {}
};

In this case the plugin does nothing.

@@ -223,6 +225,10 @@ export default function (
name = id.name;
}

if (!supportUnicodeId && isFunction(node) && /[\uD800-\uDFFF]/.test(name)) {
return;
}
Copy link
Contributor

@JLHwung JLHwung Dec 15, 2021

Choose a reason for hiding this comment

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

nit: This test is more expensive, consider place it after the if (name === undefined) check.

Copy link
Contributor Author

@The-x-Theorist The-x-Theorist Dec 16, 2021

Choose a reason for hiding this comment

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

Okay, I will move it down.

Copy link
Contributor

@JLHwung JLHwung Dec 17, 2021

Choose a reason for hiding this comment

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

Did you commit the changes?

@@ -18,7 +18,8 @@
],
"dependencies": {
"@babel/helper-function-name": "workspace:^",
"@babel/helper-plugin-utils": "workspace:^"
"@babel/helper-plugin-utils": "workspace:^",
"@babel/helper-compilation-targets": "workspace:^"
Copy link
Contributor

@JLHwung JLHwung Dec 15, 2021

Choose a reason for hiding this comment

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

Please run yarn install when new dependencies are added and commit the changed yarn lock file.

Copy link
Contributor Author

@The-x-Theorist The-x-Theorist Dec 16, 2021

Choose a reason for hiding this comment

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

Okay, I will run yarn install and commit the changes in yarn lock file.

@@ -0,0 +1,4 @@
{
"plugins": ["transform-unicode-escapes"],
Copy link
Contributor

@JLHwung JLHwung Dec 16, 2021

Choose a reason for hiding this comment

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

Suggested change
"plugins": ["transform-unicode-escapes"],
"plugins": ["transform-function-name", "transform-shorthand-properties"],

See also https://github.com/babel/babel/pull/14047/files/673c623ec94a8daf6bce79b87b5f1f6c8f5bca3d..af6dcf7895b497080d54557977792fe7d0d26571#r770063967 and #14047 (comment)

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Don't forget about #14047 (review)!

@nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Dec 20, 2021

Thanks!

@nicolo-ribaudo nicolo-ribaudo changed the title Fix: unicode encoding function name handling Fix unicode handling in generated function names Dec 20, 2021
@nicolo-ribaudo nicolo-ribaudo merged commit d6ff91d into babel:main Dec 20, 2021
26 of 28 checks passed
@github-actions github-actions bot added the outdated label Apr 5, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR: Bug Fix 🐛
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants