Skip to content

Prevent arrow functions from being improperly inlined #3757

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

Merged

Conversation

DavidANeil
Copy link
Contributor

Closes #3756.

@google-cla google-cla bot added the cla: yes label Jan 16, 2021
@blickly
Copy link
Contributor

blickly commented Jan 21, 2021

And thanks for the fix as well!

This looks good to me, so I'll work on getting it imported right away

@blickly blickly self-assigned this Jan 21, 2021
@blickly blickly self-requested a review January 21, 2021 01:10
@copybara-service copybara-service bot merged commit 9d1415b into google:master Jan 21, 2021
@blickly
Copy link
Contributor

blickly commented Jan 21, 2021

This PR is now merged. Thanks!

One suggested improvement I heard during review was: only backing off for arrow functions that reference this/super/arguments rather than all arrow functions. This would still avoid the bug of #3756, while at the same time being able to inline arrow functions that don't reference those references from outer scopes (the more common case).

@DavidANeil
Copy link
Contributor Author

My understanding was that isPropertyTree is a recursive function that only returns true if the lhs of the returned property access is this, so I feel like what you've suggested is already the case.

@blickly
Copy link
Contributor

blickly commented Feb 3, 2021

I think this might be best demonstrated with another unit test, similar to the one you created already, but checking that arrow functions can be inlined, something like:

    test(
        lines(
            "class Holder {",
            "    constructor() {",
            "        this.val = {internal: true};",
            "        this.context = {getVal: () => 5}",
            "    }",
            "}",
            "",
            "console.log(new Holder().context.getVal().internal);"),
        lines(
            "class Holder {",
            "    constructor() {",
            "        this.val = {internal: true};",
            "        this.context = {getVal: () => 5}",
            "    }",
            "}",
            "",
            "console.log(new Holder().context.val.internal);"));

If that unit test passes, that would prove that arrow functions can still be inlined by the pass.

@blickly
Copy link
Contributor

blickly commented Feb 3, 2021

I filed #3764 to track this work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

InlineSimpleMethods makes a semantic change in ES2015+
2 participants