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 optimisation of shadowed rest parameters #5721

Merged
merged 1 commit into from Jun 13, 2017

Conversation

@Qantas94Heavy
Copy link
Member

Qantas94Heavy commented May 9, 2017

Q A
Patch: Bug Fix? yes
Major: Breaking Change? no
Minor: New Feature? no
Deprecations? no
Spec Compliancy? no
Tests Added/Pass? yes
Fixed Tickets #5656
License MIT
Doc PR no
Dependency Changes no

The arguments of a function would be unnecessarily copied if there was
a nested function that had a parameter with the same identifier as the
rest parameter for the outer function. This checks the scope of the
parameter is correct before deoptimising.

Possibly has something to do with #2091 but not sure.

@mention-bot

This comment has been minimized.

Copy link

mention-bot commented May 9, 2017

@Qantas94Heavy, thanks for your PR! By analyzing the history of the files in this pull request, we identified @jridgewell, @fabiomcosta and @neVERberleRfellerER to be potential reviewers.

@codecov

This comment has been minimized.

Copy link

codecov bot commented May 9, 2017

Codecov Report

Merging #5721 into 7.0 will decrease coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##              7.0   #5721      +/-   ##
=========================================
- Coverage   84.63%   84.6%   -0.04%     
=========================================
  Files         282     282              
  Lines        9861    9865       +4     
  Branches     2766    2767       +1     
=========================================
  Hits         8346    8346              
- Misses        997    1002       +5     
+ Partials      518     517       -1
Impacted Files Coverage Δ
...bel-plugin-transform-es2015-parameters/src/rest.js 100% <100%> (+1.85%) ⬆️
packages/babel-helper-call-delegate/src/index.js 64% <0%> (-4%) ⬇️
packages/babel-traverse/src/path/context.js 84.48% <0%> (-1.73%) ⬇️
packages/babel-traverse/src/path/modification.js 73.78% <0%> (-0.98%) ⬇️
...bel-plugin-transform-es2015-classes/src/vanilla.js 90.12% <0%> (-0.86%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ce6f672...0667160. Read the comment docs.

const ident = path.scope.getBindingIdentifier(state.name);
if (!ident || ident === state.outerBinding) {
return true;
}

This comment has been minimized.

Copy link
@xtuc

xtuc May 10, 2017

Member

Thanks for your PR, it's looking good 👍

Nit, referencesRest does not consistently return a boolean, do you mind adding a return false here?

This comment has been minimized.

Copy link
@Qantas94Heavy

Qantas94Heavy May 10, 2017

Author Member

I thought this was covered by the return false below. Do you still want me to add one here?

This comment has been minimized.

Copy link
@xtuc

xtuc May 10, 2017

Member

Yes that's correct, i'm fine with that.

@Qantas94Heavy Qantas94Heavy force-pushed the Qantas94Heavy:rest-parameters-5656 branch from 5a66484 to b316205 May 10, 2017
@Qantas94Heavy Qantas94Heavy changed the title Improve optimisation of shadowed rest parameters Fix optimisation of shadowed rest parameters May 10, 2017
@Qantas94Heavy

This comment has been minimized.

Copy link
Member Author

Qantas94Heavy commented May 10, 2017

@xtuc I've updated the PR to use a pre-existing function. It seems odd that there is almost the same code within Scope though -- is there some better way of doing this?

@xtuc

This comment has been minimized.

Copy link
Member

xtuc commented May 10, 2017

Yes both are basically doing the same, but conditions are not exactly the same. It looks fine to me. You could still create a wrapper function for bindingIdentifierEquals since arguments are always the sames.

@Qantas94Heavy Qantas94Heavy force-pushed the Qantas94Heavy:rest-parameters-5656 branch from b316205 to 7b92254 May 13, 2017
@Qantas94Heavy

This comment has been minimized.

Copy link
Member Author

Qantas94Heavy commented May 13, 2017

On further inspection, it seems like the condition inside the Scope visitor is never triggered. Let me see why that is...

@Qantas94Heavy Qantas94Heavy force-pushed the Qantas94Heavy:rest-parameters-5656 branch from 7b92254 to 5da9430 May 16, 2017
@Qantas94Heavy

This comment has been minimized.

Copy link
Member Author

Qantas94Heavy commented May 16, 2017

That was silly - I ended up finding out that Scope does not cover function-created scopes, hence the need for handling it within the identifier visitors.

The arguments of a function would be unnecessarily copied if there was
a nested function that had a parameter with the same identifier as the
rest parameter for the outer function. This checks the scope of the
parameter is correct before deoptimising.

Fixes: #5656
Refs: #2091
@Qantas94Heavy Qantas94Heavy force-pushed the Qantas94Heavy:rest-parameters-5656 branch from 5da9430 to 0667160 May 16, 2017
@hzoo
hzoo approved these changes May 19, 2017
@xtuc
xtuc approved these changes Jun 6, 2017
Copy link
Member

xtuc left a comment

Nice job 👍

@jridgewell jridgewell merged commit 5387d9f into babel:7.0 Jun 13, 2017
3 checks passed
3 checks passed
ci/circleci Your tests passed on CircleCI!
Details
codecov/project 84.6% (target 80%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@lock lock bot added the outdated label Oct 6, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants
You can’t perform that action at this time.