Assignment to rest param element triggers error T6932 - #3249

The expected here is probably wrong, but either way this actual currently errors and doesn't transform to anything. Updated.

Current coverage is 85.07%

Merging #3249 into master will decrease coverage by -0.20% as of 1993757

@@            master   #3249   diff @@
======================================
  Files          215     215       
  Stmts        15712   15714     +2
  Branches      3361    3362     +1
  Methods          0       0       
======================================
- Hit          13398   13368    -30
- Partial        672     702    +30
- Missed        1642    1644     +2

Review entire Coverage Diff as of 1993757

Powered by Codecov. Updated on successful CI builds.

In 6.3.21, before this bug was introduced, it transformed to:

function x() {
  arguments[0] = 0;
}

But that could generate out of bounds argument access, which it looks like the commits that introduced this bug (#3165) were designed to adddress.

This fix bails out of the arguments optimizations if an element of a rest parameter is assigned to and results in the helper code that generates a new array being inserted (in contrast to the < 6.3.25 output). If it's worth it for some reason I suppose it'd be possible to transform to something like this instead:

if (arguments.length > 10) arguments[10] = 0;

looks good to fix it and we can revisit it later

What does this currently get transformed to?

In phab: it errors with TypeError: unknown: Property left of AssignmentExpression expected node to be of a type ["LVal"] but instead got "ConditionalExpression"

do you know what the generated code looks like?

@hzoo @amasad Thanks for reviewing.

@amasad To be clear, it errors at compile time, so there is no transformation / generated code. I think you're asking what state is it in when the error occurs? I think it's trying to construct an AST representing something like:

(arguments.length <= 0 ? undefined : arguments[0]) = "0";

@hzoo

looks good to fix it and we can revisit it later

That's what I was thinking -- just fix it and revisit optimization later, because that's been tricky to get right and I'm not sure if there's a clear win there.

Actually I don't see this as an option:

If it's worth it for some reason I suppose it'd be possible to transform to something like this instead:

  if (arguments.length > 10) arguments[10] = 0;

For example, this should work:

(function (...items) {
  items[9] = 0;
  assert(items.length === 10);
})();

The old transformation before this bug wouldn't have worked right there either.

On another note, is assigning to arguments.length "safe" in the sense of not interfering with engine optimization? /cc @vhf

is assigning to arguments.length "safe" in the sense of not interfering with engine optimization?

@jmm Nope. Crankshaft will bailout with a "Bad value context for arguments value".

@vhf Thanks! I'd checked the optimization killers article and it wasn't clear to me from there if "use" arguments.length meant assign to it or just read it, so I guess that'd be a good addition. So this is doing 2 things wrong with regard to that: causing the function to be deoptimized and producing incorrect results. E.g.:

Input:

(function (...rest) {
  rest.length = 2;
  assert(rest[2] === undefined);
})(1, 2, 3);

Output:

(function () {
  arguments.length = 2;
  assert(arguments[2] === undefined);
})(1, 2, 3);

I'm working on additional changes to this transform, including regarding that.

I pushed new commits here with a better home for the fixtures (in an existing file) based on my improved understanding of the transform. The test case here is just a situation where the transform should bail out on optimizing references to rest. As a byproduct it catches this bug that causes transformation to fail altogether.

So this is doing 2 things wrong with regard to that: [...] and producing incorrect results.

Actually, that was my mistake -- I accidentally checked that with 6.3.21. With 6.4.2 it transforms to this:

(function () {
  arguments.length = 2;
  assert((arguments.length <= 2 ? undefined : arguments[2]) === undefined);
})(1, 2, 3);

So the first part is still an issue -- it "optimizes" it so that it assigns to arguments.length.

Hitting this in Meteor's command-line tool code while trying to upgrade to Babel 6: meteor/meteor#6141

Do you need help with this PR or can I do anything to help @jmm?

Added some new tests that fail even with @jmm's fixes. If he merges my PR, this PR should update, and the new tests should fail here… in theory!

EDIT: oops, wound up with wrong at-mention for some reason (sorry iamolivinius => vhf).
@vhf

Do you need help with this PR or can I do anything to help @jmm?

Thanks! I was just hoping to get a couple 👍 from other collaborators. At this point, since it's an obvious bug and there's been no objection, I'd consider merging and pushing a release, but there's a bunch of unreleased stuff already on master to sort out. I think others were considering pushing that stuff out in a pre-release version.

@benjamn

Added some new tests that fail even with @jmm's fixes. If he merges my PR, this PR should update, and the new tests should fail here… in theory!

Thanks! I need to take a closer look at those -- I'm sure we should bring them in, I just want to see if they should be added to this PR or become a new issue / PR.

I wonder if optimizing these references to arguments sometimes is worth the complexity....

Please pull them in, I already got a fix for them. (The expected fixtures
added by @benjamn are not perfect but they're fixed in my incoming PR to
your branch.)

On Tue, Feb 2, 2016 at 11:19 PM, Jesse McCarthy [email protected]
wrote:

@iamolivinius https://github.com/iamolivinius
I'm

Do you need help with this PR or can I do anything to help @jmm
https://github.com/jmm?

Thanks! I was just hoping to get a couple [image: 👍] from other
collaborators. At this point, since it's an obvious bug and there's been no
objection, I'd consider merging and pushing a release, but there's a bunch
of unreleased stuff already on master to sort out. I think others were
considering pushing that stuff out in a pre-release version.

@benjamn https://github.com/benjamn

Added some new tests that fail even with @jmm https://github.com/jmm's
fixes. If he merges my PR, this PR should update, and the new tests should
fail here… in theory!

Thanks! I need to take a closer look at those -- I'm sure we should bring
them in, I just want to see if they should be added to this PR or become a
new issue / PR.

I wonder if optimizing these references to arguments sometimes is worth
the complexity....

—
Reply to this email directly or view it on GitHub
#3249 (comment).

I don't think we're doing the pre-release anymore - probably a release this week though. 👍 for the current stuff

The additional tests are definitely an instance of the same general problem, so they very much belong in this PR.

@vhf @benjamn Yeah, just given that this PR hasn't gotten merged yet and it's been the same for 19 days, I would consider PR'ing other fixes separately in an attempt to incrementally get some of the problems fixed sooner than all of them. Definitely appreciate your efforts in identifying and fixing other cases though!!

Thanks @hzoo.

@vhf The path.isLVal() method feels closer to the truth, but I believe any MemberExpression counts as an LVal according to that method, even if it would be safe to replace it, which might ruin some legitimate optimization opportunities.

I wonder if optimizing these references to arguments sometimes is worth the complexity…

I would support a solution that disables the optimization completely. Simple, general, foolproof, reversible in the future if anyone cares to put in the time to do this right, etc. etc.

@benjamn

I would support a solution that disables the optimization completely. Simple, general, foolproof, reversible in the future if anyone cares to put in the time to do this right, etc. etc.

I'd say let's proceed if it seems like we're close to being able to make it robust, since a lot of work has already gone into it. (I also have performance optimizations of the optimization code and other fixes sitting on local branches waiting on my open PRs to be merged.) If it seems like a bottomless pit of difficult to detect problem cases, I'm open to just removing the optimization. Would like to hear what other collaborators think about that, and would need some agreement on it anyway. Thank you for all of the additional test cases! I'm going to take a look at those.

What's our status on this one?

@hzoo I'm working on fixing other problem cases that @benjamn identified, but this should be merged for now. We should keep an eye on it in the future to see if problems keep arising and if the optmizations are worth the complexity.

In the meantime, babel-preset-meteor is using a compiled version of the parameters transform that disables the optimizations. I am still worried this bug may turn out to have many heads, and that the fixes may be more complicated than the optimization was worth, but I trust you to make that call!

Why don't we make an option for it, state.opts.optimise?

@benjamn

I am still worried this bug may turn out to have many heads, and that the fixes may be more complicated than the optimization was worth

I'm with you on all of that. I have fixes coming soon for the failing test cases you provided, but even so that doesn't make me confident that there aren't other problem cases that we haven't thought of lurking. I think for now we should merge those fixes and keep an eye on it to see what happens.

but I trust you to make that call!

Thanks. I, for one, am interested in feedback from the community on this -- not just bug reports, but whether people even think the optimizations are important, are worth people spending limited time maintaining, etc.

@hzoo

Why don't we make an option for it, state.opts.optimise?

Yeah, we could do that. @benjamn Would you use that right now if it were available? @hzoo Also, something to keep in mind is that there are actually multiple optimizations (referencing arguments instead of making a real array, where to place the loop when you do make an array, etc.)

@benjamn I see that in https://github.com/meteor/babel-preset-meteor you have a:

// TODO Somehow expose a hash of these plugin options?

Note sure exactly what that means, but you might be interested in the discussion in #3349.

feedback from the community on this -- not just bug reports, but whether people even think the optimizations are important, are worth people spending limited time maintaining, etc.

I'm obviously completely biased since I'm the one who thought avoiding v8 deoptimization in this module was a good move but here's my take anyway.

There are two options: either Babel only compiles from ESx to ESy or Babel compiles and optimizes the code. The second one makes more sense to me.

Before v8 got rest and default params, Babel compiled them into what v8 would deoptimize. Babel seemed to care a lot about performances at this time so these optimization (which are more de-deoptimizations) were introduced.

Now that v8 has rest and default params, people could just stop using this module/transform when targeting node/chrome... if v8 wasn't deoptimizing them! I really think it makes sense to not only "transpile" but also optimize.

That said, I'm all in favor of an option. It would allow people targeting various browsers to have "safe" (but sometimes slow) code, and people targeting node to have faster code.

I'm not sure I see how v8 deoptimization comes into play here. There's nothing optimization-killing about looping over arguments to create a new array and then using that array instead of arguments. I can understand trying to avoid the cost of the copy, but that's not what makes the difference between a function being optimized by v8 or deoptimized. Using ...rest parameters (transpiled or native) instead of arguments is a great way to avoid deoptimizations related to arguments, and occasionally optimizing back to arguments is what creates the risk. That's my mental model, at least.

More importantly, safe but slow code is better than buggy but fast code every day of the week. It's not a weighted balance; safety is an absolute requirement. I'm reminded of a professor who gave zero credit on a homework assignment if any of our compiler optimizations were unsound. I got a zero on that assignment, which is maybe why I remember it so well, but she had a point!

Why don't we make an option for it, state.opts.optimise?

Yeah, we could do that. @benjamn Would you use that right now if it were available?

I would use that option!

@vhf Thanks for the feedback.

EDIT: FYI, I didn't see @benjamn's latest comments before posting. Reading now.

Let me just recap a little to make sure we're all on the same page here. In it's transformation of rest parameters, Babel sometimes loops over arguments to create a real array and in some situations skips that and just references arguments directly. I will call that the Babel arguments optimization. There there's the question of whether the output of Babel's arguments optimization will cause V8 to deoptimize the function. The latter is what @vhf's enhancements have been designed to avoid.

There are two options: either Babel only compiles from ESx to ESy or Babel compiles and optimizes the code. The second one makes more sense to me.

Ok, so when you say that you think Babel should optimize the code, you mean that you think it should perform the Babel arguments optimization?

Now that v8 has rest and default params, people could just stop using this module/transform when targeting node/chrome... if v8 wasn't deoptimizing them!

Do you mean that V8 deoptimizes functions with rest parameters?

The thing that I'm determined to fix one way or another are the errors in the code that performs the Babel arguments optimization that construct an invalid AST and cause transformation to fail altogether.

@benjamn Thanks for the feedback.

and occasionally optimizing back to arguments is what creates the risk. That's my mental model, at least.

Yes...and doing that optimization in a way that avoids V8 deoptimization is what has created some of the complexity that has resulted in the current issues where compilation crashes. There are 3 basic options:

1. Always create a real array. Least complicated to implement. Not sure how significant the impact on performance would tend to be.
2. Sometimes skip creating an array and replace references to rest with simple references to arguments. May cause V8 to deoptimize.
3. Sometimes skip creating an array and attempt to replace references to rest with references to arguments that will not cause V8 to deoptimize. Most complicated to implement. Presumably yields the best runtime performance.

I would use that option!

Ok, thanks.

Ok, so when you say that you think Babel should optimize the code, you mean that you think it should perform the Babel arguments optimization?

Yes. I think Babel should avoid copying the arguments object to an array as much as possible. @mraleph told me the following (and I tend to trust him on all V8-related topics): Allocating array (and hope it will get handled by some optimization pass in the V8) is a bad idea.

Do you mean that V8 deoptimizes functions with rest parameters?

Yes. I mentioned it here: it currently triggers a bailout: "Rest parameters". This will change in the future as TurboFan will most probably be able to optimize all uses of the arguments object. (Cf. the commit introducing this bailout and the current state of v8/crankshaft master.)

Regarding the three options you proposed I suggest to have 1. when state.opts.optimise === false and 3. when state.opts.optimise === true. I would avoid 2.. My thinking is that I'd rather not use ES6 default or rest parameters and have fast code than knowing Babel will make my code run slow because of a bailout. On the other hand, I don't mind if the worst case is sometimes having Babel copy the arguments object, but only if it's really the worst case and not the way it's always handled.

@vhf Thanks for the feedback.

Yes. I think Babel should avoid copying the arguments object to an array as much as possible. @mraleph told me the following (and I tend to trust him on all V8-related topics): Allocating array (and hope it will get handled by some optimization pass in the V8) is a bad idea.

Ok, so I've skimmed your bailout reasons document a few times previously and thought that's what it said, but don't really understand it. (It's not explained very thoroughly -- like what exactly it means for it to be a "bad idea" -- not that I'd necessarily be able to understand it any better if it were.) Prior to that I thought that was the conventional wisdom on how to handle it, and that's what the optimization killers article advises to do to avoid leaking arguments. So I don't know what to make of this. Who's right? (I really do not relish dealing with these esoteric engine optimization issues.)

Yes. I mentioned it here: it currently triggers a bailout: "Rest parameters". This will change in the future as TurboFan will most probably be able to optimize all uses of the arguments object. (Cf. the commit introducing this bailout and the current state of v8/crankshaft master.)

Ok, thanks.

Regarding the three options you proposed I suggest to have 1. when state.opts.optimise === false and 3. when state.opts.optimise === true. I would avoid 2.. My thinking is that I'd rather not use ES6 default or rest parameters and have fast code than knowing Babel will make my code run slow because of a bailout. On the other hand, I don't mind if the worst case is sometimes having Babel copy the arguments object, but only if it's really the worst case and not the way it's always handled.

Ok, thanks. I really wish we could nail down definitively the repercussions of looping over to create a real array.

On the other hand, I don't mind if the worst case is sometimes having Babel copy the arguments object, but only if it's really the worst case and not the way it's always handled.

So is the premise here that looping over to create a real array is just as bad as directly leaking arguments by e.g. passing it out of / returning it from the function, or assigning to any prop of arguments? That would change the status quo of what the transform "decides" to optimize I think. If that's true then what are the cases for creating a real array?:

• When you assign to any indexed property or length of rest (because of the behavior of length, not V8 deopt).
• When you read any prop of the rest param other than an indexed property or length.
• I have to look at how array destructuring works and if an arguments object as the rval would perform like a real array (although in that case V8 would deopt it anyway, right?) A real quick glance makes me think it would perform the same.

Maybe I'll think of more later.