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

transform-es2015-parameters: arguments deoptimized when shadowed in nested function #5656

Closed
jpommerening opened this Issue Apr 21, 2017 · 3 comments

Comments

Projects
None yet
5 participants
@jpommerening

This is a feature request (I think). Arguments are not optimized if an inner function shadows the name with a parameter (or rest parameters in my case).

Input Code

const log = (...args) => console.log(...args);

function test_opt(...args) {
  log(...args);
}

function test_deopt(...args) {
  const fn = (...args) => log(...args);
  fn(...args);
}

Babel Configuration (.babelrc, package.json, cli command)

{
  "presets": [ "es2015" ]
}

Expected vs. Current Behavior

I'd expect the code to be optimizable to use .apply( thisArg, arguments ) throughout.
However, in test_deopt the outer ...args gets copied just to be passed into the inner fn.
I can verify that the problem disappears if I rename either the ...args of test_deopt or of the fn arrow function.

Environment

software version(s)
Babel 6.24.1
node 6.9.5
npm N/A (yarn 0.20.3)
Operating System macOS
@babel-bot

This comment has been minimized.

Show comment
Hide comment
@babel-bot

babel-bot Apr 21, 2017

Collaborator

Hey @jpommerening! We really appreciate you taking the time to report an issue. The collaborators
on this project attempt to help as many people as possible, but we're a limited number of volunteers,
so it's possible this won't be addressed swiftly.

If you need any help, or just have general Babel or JavaScript questions, we have a vibrant Slack
community that typically always has someone willing to help. You can sign-up here
for an invite.

Collaborator

babel-bot commented Apr 21, 2017

Hey @jpommerening! We really appreciate you taking the time to report an issue. The collaborators
on this project attempt to help as many people as possible, but we're a limited number of volunteers,
so it's possible this won't be addressed swiftly.

If you need any help, or just have general Babel or JavaScript questions, we have a vibrant Slack
community that typically always has someone willing to help. You can sign-up here
for an invite.

@Qantas94Heavy

This comment has been minimized.

Show comment
Hide comment
@Qantas94Heavy

Qantas94Heavy May 9, 2017

Member

I hope I'm not being stupid, but is this really meant to be "expected"?

Test code:

var y = function (foo, ...bar) {
  var x = function z(bar) {
    bar[1] = 5;
  };
};

Current "expected" result:

var y = function (foo) {
  for (var _len2 = arguments.length, bar = Array(_len2 > 1 ? _len2 - 1 : 0), _key2 = 1; _key2 < _len2; _key2++) {
    bar[_key2 - 1] = arguments[_key2];
  }

  var x = function z(bar) {
    bar[1] = 5;
  };
};

I would have thought that given the inner bar shadows the outer rest bar, we wouldn't need to copy the arguments.

Member

Qantas94Heavy commented May 9, 2017

I hope I'm not being stupid, but is this really meant to be "expected"?

Test code:

var y = function (foo, ...bar) {
  var x = function z(bar) {
    bar[1] = 5;
  };
};

Current "expected" result:

var y = function (foo) {
  for (var _len2 = arguments.length, bar = Array(_len2 > 1 ? _len2 - 1 : 0), _key2 = 1; _key2 < _len2; _key2++) {
    bar[_key2 - 1] = arguments[_key2];
  }

  var x = function z(bar) {
    bar[1] = 5;
  };
};

I would have thought that given the inner bar shadows the outer rest bar, we wouldn't need to copy the arguments.

@jpommerening

This comment has been minimized.

Show comment
Hide comment
@jpommerening

jpommerening May 9, 2017

Hey @Qantas94Heavy, thanks for pointing that out!
I was under the impression that this was merely an oversight / missing optimization. I didn't know there was actually a test verifying the (arguably) incorrect behavior. I'm as confused as you are that the transform is expected to copy the arguments in the test case you linked.

Hey @Qantas94Heavy, thanks for pointing that out!
I was under the impression that this was merely an oversight / missing optimization. I didn't know there was actually a test verifying the (arguably) incorrect behavior. I'm as confused as you are that the transform is expected to copy the arguments in the test case you linked.

Qantas94Heavy added a commit to Qantas94Heavy/babel that referenced this issue May 9, 2017

Improve optimisation of shadowed rest parameters
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: babel#5656

Qantas94Heavy added a commit to Qantas94Heavy/babel that referenced this issue May 9, 2017

Improve optimisation of shadowed rest parameters
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: babel#5656

Qantas94Heavy added a commit to Qantas94Heavy/babel that referenced this issue May 9, 2017

Improve optimisation of shadowed rest parameters
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: babel#5656

Qantas94Heavy added a commit to Qantas94Heavy/babel that referenced this issue May 9, 2017

Improve optimisation of shadowed rest parameters
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: babel#5656
Refs: babel#2091

@babel-bot babel-bot added the Has PR label May 9, 2017

Qantas94Heavy added a commit to Qantas94Heavy/babel that referenced this issue May 10, 2017

Fix optimisation of shadowed rest parameters
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: babel#5656
Refs: babel#2091

Qantas94Heavy added a commit to Qantas94Heavy/babel that referenced this issue May 13, 2017

Fix optimisation of shadowed rest parameters
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: babel#5656
Refs: babel#2091

Qantas94Heavy added a commit to Qantas94Heavy/babel that referenced this issue May 16, 2017

Fix optimisation of shadowed rest parameters
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: babel#5656
Refs: babel#2091

@lock lock bot added the outdated label May 4, 2018

@lock lock bot locked as resolved and limited conversation to collaborators May 4, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.