Skip to content

Transform arrow function in variable declaration to named function #16001

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

Closed
wants to merge 2 commits into from
Closed

Transform arrow function in variable declaration to named function #16001

wants to merge 2 commits into from

Conversation

mpodlasin
Copy link

This is a work-in-progress for #6433.

I wanted to start with the simplest case of transforming

const x = () => {}

into

var x = function x() {}; // currently it emits "var x = function() {}" (note lack of function name)

I was able to get desired result in baselines, but many tests fail with

Error: Debug Failure. False expression: position cannot precede the beginning of the file

I imagine this is a problem with position of synthesized identifier for function name, but I have no idea how should I fix it. Many transforms create nodes (for example object literal transform creates many assignments) and yet they do not have to manipulate positions of synthesized nodes.

I am super confused. Help! I need some pointers how to proceed. @rbuckton @mhegazy

Thanks in advance!

@mhegazy
Copy link
Contributor

mhegazy commented May 22, 2017

@rbuckton can you take a look.

@mpodlasin
Copy link
Author

Ok. It seems I have figured it out. Tests should pass now.
I however still need confirmation that general direction of changes is ok. :)

@kpreisser
Copy link
Contributor

kpreisser commented Jun 9, 2017

Hi,

I think there needs to be a special treatment for unnamed functions that reference the variable in which they are stored:

var x = recursive => {
    if (!recursive)
        return x(true);
    else
        return 1;
}
var y = x;

x = () => {
    return 2;
};

console.log(y(false));

Expected output (when running the code directly in Node, or running the transpiled ES5 code without this PR in Node):

2

Actual output with this PR, when running the transpiled ES5 code in Node:

1

Maybe in this case the x variable needs to be renamed (like it is done when having multiple let declarations with the same name).

var x_1 = function x(recursive) {
    if (!recursive)
        return x_1(true);
    else
        return 1;
};
var y = x_1;
x_1 = function () {
    return 2;
};
console.log(y(false));

Thanks!

@microsoft microsoft deleted a comment from msftclas Sep 26, 2017
@microsoft microsoft deleted a comment from msftclas Sep 26, 2017
@mhegazy
Copy link
Contributor

mhegazy commented Nov 7, 2017

I am afraid the fix here is not as simple as just adding a name. As @kpreisser noted, the name could be changed outside the function.. moreover, adding the name may shadow another variable that was visible otherwise.. e.g. var a; var o = { a: () => { a } }.

Looking at this again, i am not sure this is the right solution. The other solution is setting the name using object.defineProperty, but that too, not sure we want to litter the generated code with such calls.. It may be better to just do nothing on this issue in the time being.

@mhegazy
Copy link
Contributor

mhegazy commented Nov 7, 2017

closing for now, as this has grown stale, and it is an incomplete fix in the first place.

@mhegazy mhegazy closed this Nov 7, 2017
@mhegazy mhegazy mentioned this pull request Nov 8, 2017
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants