Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1127063003:
[es6] implement default parameters via desugaring (Closed)
|
Created:
5 years, 7 months ago by caitp (gmail) Modified:
5 years, 3 months ago Reviewers:
Dmitry Lomov (no reviews), rossberg, Michael Starzinger, wingo, arv (Not doing code reviews), adamk CC:
v8-dev, adamk Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[es6] implement default parameters via desugaring
Stage 1 implementation:
- Parameters can't be referenced before initialized (from left-to-right)
- SingleNameBindings only, no support for BindingPatterns
Known issues:
- Incorrect scoping (parameter expressions may reference variables declared in function body)
- Function arity is untouched
- Hole-checking needs work
- Rest parameters are broken when mixed with optional arguments
BUG=v8:2160
LOG=N
R=arv@chromium.org, rossberg@chromium.org
Committed: https://crrev.com/892c85485881f8be2f17bd83238980f858126576
Cr-Commit-Position: refs/heads/master@{#28739}
Patch Set 1 #
Total comments: 13
Patch Set 2 : Put it behind a flag #Patch Set 3 : Rebase it #Patch Set 4 : arv's comments #
Total comments: 15
Patch Set 5 : Add a TODO #Patch Set 6 : some nits #
Total comments: 19
Patch Set 7 : Put function body in a Block statement #Patch Set 8 : Rename "optional arguments" to "default parameters" #Patch Set 9 : Add new test cases #Patch Set 10 : Rebase and fix tests #Patch Set 11 : Rebase + add lazy parsing test + WIP review comments #
Total comments: 1
Patch Set 12 : Fix default-parameters-lazy.js #Patch Set 13 : It works! ish #Patch Set 14 : Rebase #
Total comments: 6
Patch Set 15 : Add more context-allocation tests #
Total comments: 19
Patch Set 16 : Simplify lazy-parsing test #Patch Set 17 : Adam's comments addressed #Patch Set 18 : Don't AllocateParameter() if hasParameterExpressions is true #
Total comments: 7
Patch Set 19 : Rebase + rossberg nits #
Messages
Total messages: 58 (9 generated)
PTAL --- third prototype, after #1104223002 This one is limited in order to be incrementally updated, as discussed in the meeting.
caitpotter88@gmail.com changed reviewers: + arv@chromium.org, dslomov@chromium.org, rossberg@chromium.org, wingo@igalia.com
This actually breaks rest parameters when used in conjunction with default arguments, due to the ShadowParametersForInitializers() thing. I think that's okay, because it will be fixed subsequently when rest params are implemented with desugaring
Great incremental first step https://codereview.chromium.org/1127063003/diff/1/src/parser.cc File src/parser.cc (right): https://codereview.chromium.org/1127063003/diff/1/src/parser.cc#newcode3538 src/parser.cc:3538: How about adding a comment block explaining how the desugaring is done? https://codereview.chromium.org/1127063003/diff/1/src/parser.cc#newcode3561 src/parser.cc:3561: Runtime::FunctionForId(Runtime::kInlineArguments); I think Andreas wanted: function f(x = expr) {} to be desugared to: function f(temp) { let x = IS_UNDEFINED(temp) ? expr : temp; } If you use %_Arguments(i), do you also need to check the number of arguments so you do not index out of bounds? https://codereview.chromium.org/1127063003/diff/1/src/parser.cc#newcode3612 src/parser.cc:3612: } else { no else after return https://codereview.chromium.org/1127063003/diff/1/src/parser.h File src/parser.h (right): https://codereview.chromium.org/1127063003/diff/1/src/parser.h#newcode752 src/parser.h:752: scope->DeclareParameter(name, VAR, pos, is_rest, &is_duplicate); keep pos at end? https://codereview.chromium.org/1127063003/diff/1/src/preparser.h File src/preparser.h (right): https://codereview.chromium.org/1127063003/diff/1/src/preparser.h#newcode3535 src/preparser.h:3535: Scope* param_scope = NewScope(scope_, BLOCK_SCOPE); Is BLOCK_SCOPE sufficient? Does it need this new VAR_DECLARATION_SCOPE? function f(x = eval('var z = 2'), y = z) { return y; } f() === 2 // ? is that eval a strict eval? So confusing... https://codereview.chromium.org/1127063003/diff/1/src/scopes.cc File src/scopes.cc (right): https://codereview.chromium.org/1127063003/diff/1/src/scopes.cc#newcode513 src/scopes.cc:513: void Scope::ShadowParametersForExpressions() { This could use a comment https://codereview.chromium.org/1127063003/diff/1/src/scopes.h File src/scopes.h (right): https://codereview.chromium.org/1127063003/diff/1/src/scopes.h#newcode129 src/scopes.h:129: int pos, bool is_rest, bool* is_duplicate); keep pos at end for consistency https://codereview.chromium.org/1127063003/diff/1/test/mjsunit/harmony/option... File test/mjsunit/harmony/optional-arguments.js (right): https://codereview.chromium.org/1127063003/diff/1/test/mjsunit/harmony/option... test/mjsunit/harmony/optional-arguments.js:17: // TODO(caitp): infer function name correctly https://code.google.com/p/v8/issues/detail?id=3699
https://codereview.chromium.org/1127063003/diff/1/src/parser.cc File src/parser.cc (right): https://codereview.chromium.org/1127063003/diff/1/src/parser.cc#newcode3561 src/parser.cc:3561: Runtime::FunctionForId(Runtime::kInlineArguments); On 2015/05/06 20:32:38, arv wrote: > I think Andreas wanted: > > function f(x = expr) {} > > to be desugared to: > > function f(temp) { > let x = IS_UNDEFINED(temp) ? expr : temp; > } > > If you use %_Arguments(i), do you also need to check the number of arguments so > you do not index out of bounds? I don't think bounds-checking is needed --- at least in testing, `(function(x = 1, y = 2) { return x + y; })();` works just fine. Maybe that becomes an issue with the adapter frame issue https://codereview.chromium.org/1127063003/diff/1/src/preparser.h File src/preparser.h (right): https://codereview.chromium.org/1127063003/diff/1/src/preparser.h#newcode3535 src/preparser.h:3535: Scope* param_scope = NewScope(scope_, BLOCK_SCOPE); On 2015/05/06 20:32:38, arv wrote: > Is BLOCK_SCOPE sufficient? Does it need this new VAR_DECLARATION_SCOPE? > To get scoping right, we do need a new declaration scope --- but I think we decided that would be the next step in a followup CL more focused on scoping > function f(x = eval('var z = 2'), y = z) { > return y; > } > f() === 2 // ? > > is that eval a strict eval? So confusing... Currently, ``` d8> function f(a = eval("var z = 2"), b = z) { return z; } undefined d8> f() 2 d8> z (d8):1: ReferenceError: z is not defined z ^ ReferenceError: z is not defined at (d8):1:1 ``` You end up with that... I'm not sure if that's what we want, it probably isn't. But it makes sense that `z` doesn't end up in the global scope https://codereview.chromium.org/1127063003/diff/1/src/scopes.cc File src/scopes.cc (right): https://codereview.chromium.org/1127063003/diff/1/src/scopes.cc#newcode513 src/scopes.cc:513: void Scope::ShadowParametersForExpressions() { On 2015/05/06 20:32:38, arv wrote: > This could use a comment I believe there's a comment in parser.h for the function that calls it, but yeah will do https://codereview.chromium.org/1127063003/diff/1/src/scopes.h File src/scopes.h (right): https://codereview.chromium.org/1127063003/diff/1/src/scopes.h#newcode129 src/scopes.h:129: int pos, bool is_rest, bool* is_duplicate); On 2015/05/06 20:32:38, arv wrote: > keep pos at end for consistency Acknowledged. https://codereview.chromium.org/1127063003/diff/1/test/mjsunit/harmony/option... File test/mjsunit/harmony/optional-arguments.js (right): https://codereview.chromium.org/1127063003/diff/1/test/mjsunit/harmony/option... test/mjsunit/harmony/optional-arguments.js:17: // TODO(caitp): infer function name correctly On 2015/05/06 20:32:38, arv wrote: > https://code.google.com/p/v8/issues/detail?id=3699 Acknowledged.
https://codereview.chromium.org/1127063003/diff/60001/src/preparser.h File src/preparser.h (right): https://codereview.chromium.org/1127063003/diff/60001/src/preparser.h#newcode... src/preparser.h:3541: DCHECK(scope_->is_function_scope()); Apparently this is a problem for lazy-parsed arrow functions :( There has got to be a better way
https://codereview.chromium.org/1127063003/diff/60001/src/preparser.h File src/preparser.h (right): https://codereview.chromium.org/1127063003/diff/60001/src/preparser.h#newcode... src/preparser.h:3492: if (initializer && allow_harmony_optional_params() && Check(Token::ASSIGN)) { This looks strange. Why are we checking initializer here? https://codereview.chromium.org/1127063003/diff/60001/src/preparser.h#newcode... src/preparser.h:3541: DCHECK(scope_->is_function_scope()); On 2015/05/09 20:57:22, caitp wrote: > Apparently this is a problem for lazy-parsed arrow functions :( There has got to > be a better way Andy, could you take a look too?
https://codereview.chromium.org/1127063003/diff/60001/src/preparser.h File src/preparser.h (right): https://codereview.chromium.org/1127063003/diff/60001/src/preparser.h#newcode... src/preparser.h:3492: if (initializer && allow_harmony_optional_params() && Check(Token::ASSIGN)) { On 2015/05/11 14:10:54, arv wrote: > This looks strange. Why are we checking initializer here? below, *initializer is assigned, and we probably don't want to do that if `initializer` is null (which, at the moment, it is for arrow function parsing) --- that's kind of a temporary thing though
https://codereview.chromium.org/1127063003/diff/60001/src/preparser.h File src/preparser.h (right): https://codereview.chromium.org/1127063003/diff/60001/src/preparser.h#newcode... src/preparser.h:3492: if (initializer && allow_harmony_optional_params() && Check(Token::ASSIGN)) { On 2015/05/11 14:34:57, caitp wrote: > On 2015/05/11 14:10:54, arv wrote: > > This looks strange. Why are we checking initializer here? > > below, *initializer is assigned, and we probably don't want to do that if > `initializer` is null (which, at the moment, it is for arrow function parsing) > --- that's kind of a temporary thing though What initializer are you passing in before the initializer has been parsed? I guess you are using Empty for the preparser. Can you add a comment/todo?
https://codereview.chromium.org/1127063003/diff/60001/src/parser.cc File src/parser.cc (right): https://codereview.chromium.org/1127063003/diff/60001/src/parser.cc#newcode1153 src/parser.cc:1153: ParseFormalParameter(scope, &error_locs, nullptr, nullptr, has_rest, nit: would be easier to read with named variables for these nullptrs https://codereview.chromium.org/1127063003/diff/60001/src/parser.cc#newcode3546 src/parser.cc:3546: // If hasParameterExpressions for the function is true, each parameter is has_initializers I guess? I know hasParameterExpressions is the spec language; is has_initializers the same? https://codereview.chromium.org/1127063003/diff/60001/src/parser.cc#newcode3574 src/parser.cc:3574: DCHECK(ok); Seems that Declare can only fail for a var redeclaration, which is impossible here, OK. Still, a comment would save the reader some time :) https://codereview.chromium.org/1127063003/diff/60001/src/preparser.h File src/preparser.h (right): https://codereview.chromium.org/1127063003/diff/60001/src/preparser.h#newcode... src/preparser.h:3541: DCHECK(scope_->is_function_scope()); On 2015/05/09 20:57:22, caitp wrote: > Apparently this is a problem for lazy-parsed arrow functions :( There has got to > be a better way So only lazy-parsed arrow functions will have a chance to have initializer expressions AFAIU -- is that right? I mean, otherwise we don't know that it's an initializer and you haven't modified ParseArrowFunctionFormalParameters to handle initializers. That is going to be horrific by the way -- you'll be lifting expressions parsed in one function to another. There's still some state being allocated in the parser (literal counts, handler indices, expected property counts) that will need to be rewritten for initializers lifted to inner functions. In my mind the right way to do that is to move to a post-pass; for that reason it seems that optional argument initializers are going to be quite a challenge. Dunno. So that said it should be impossible to get to a lazily parsed arrow function with default initializers with the current code. The preparser should have already discounted the possibility of the "formals" of the arrow function being valid.
One other little thought https://codereview.chromium.org/1127063003/diff/60001/src/scopes.cc File src/scopes.cc (right): https://codereview.chromium.org/1127063003/diff/60001/src/scopes.cc#newcode468 src/scopes.cc:468: param_positions_.Add(pos, zone()); Don't we require already that parameters are declared left-to-right? In that case the "pos" argument is unneeded. Just a thought.
https://codereview.chromium.org/1127063003/diff/60001/src/parser.cc File src/parser.cc (right): https://codereview.chromium.org/1127063003/diff/60001/src/parser.cc#newcode1153 src/parser.cc:1153: ParseFormalParameter(scope, &error_locs, nullptr, nullptr, has_rest, On 2015/05/11 15:00:48, wingo wrote: > nit: would be easier to read with named variables for these nullptrs Acknowledged. https://codereview.chromium.org/1127063003/diff/60001/src/parser.cc#newcode3546 src/parser.cc:3546: // If hasParameterExpressions for the function is true, each parameter is On 2015/05/11 15:00:48, wingo wrote: > has_initializers I guess? I know hasParameterExpressions is the spec language; > is has_initializers the same? HasParameterExpressions is also true for Object binding patterns with computed property keys, so the language will have to change eventually https://codereview.chromium.org/1127063003/diff/60001/src/parser.cc#newcode3574 src/parser.cc:3574: DCHECK(ok); On 2015/05/11 15:00:48, wingo wrote: > Seems that Declare can only fail for a var redeclaration, which is impossible > here, OK. Still, a comment would save the reader some time :) Acknowledged. https://codereview.chromium.org/1127063003/diff/60001/src/preparser.h File src/preparser.h (right): https://codereview.chromium.org/1127063003/diff/60001/src/preparser.h#newcode... src/preparser.h:3541: DCHECK(scope_->is_function_scope()); On 2015/05/11 15:00:48, wingo wrote: > On 2015/05/09 20:57:22, caitp wrote: > > Apparently this is a problem for lazy-parsed arrow functions :( There has got > to > > be a better way > > So only lazy-parsed arrow functions will have a chance to have initializer > expressions AFAIU -- is that right? I mean, otherwise we don't know that it's > an initializer and you haven't modified ParseArrowFunctionFormalParameters to > handle initializers. That is going to be horrific by the way -- you'll be > lifting expressions parsed in one function to another. There's still some state > being allocated in the parser (literal counts, handler indices, expected > property counts) that will need to be rewritten for initializers lifted to inner > functions. In my mind the right way to do that is to move to a post-pass; for > that reason it seems that optional argument initializers are going to be quite a > challenge. Dunno. > > So that said it should be impossible to get to a lazily parsed arrow function > with default initializers with the current code. The preparser should have > already discounted the possibility of the "formals" of the arrow function being > valid. The issue with lazy-parsing is that the scope doesn't seem to be a function/arrow scope during lazy parsing --- so the DCHECK fails, and in release builds we're potentially declaring things in the wrong scope. Right now, arrow functions never have valid destructuring of any kind, and that will take more work to support. But we want this DCHECK not to fail, so we might need to encode some extra info when lazily parsing these. https://codereview.chromium.org/1127063003/diff/60001/src/scopes.cc File src/scopes.cc (right): https://codereview.chromium.org/1127063003/diff/60001/src/scopes.cc#newcode468 src/scopes.cc:468: param_positions_.Add(pos, zone()); On 2015/05/11 15:02:51, wingo wrote: > Don't we require already that parameters are declared left-to-right? In that > case the "pos" argument is unneeded. Just a thought. It's needed because they end up as lexical declarations, which require a position. The hole-checking logic uses it too, so it needs to be accurate
On 2015/05/11 15:34:25, caitp wrote: > https://codereview.chromium.org/1127063003/diff/60001/src/preparser.h > File src/preparser.h (right): > > https://codereview.chromium.org/1127063003/diff/60001/src/preparser.h#newcode... > src/preparser.h:3541: DCHECK(scope_->is_function_scope()); > On 2015/05/11 15:00:48, wingo wrote: > > On 2015/05/09 20:57:22, caitp wrote: > > > Apparently this is a problem for lazy-parsed arrow functions :( There has > got > > to > > > be a better way > > > > So only lazy-parsed arrow functions will have a chance to have initializer > > expressions AFAIU -- is that right? I mean, otherwise we don't know that it's > > an initializer and you haven't modified ParseArrowFunctionFormalParameters to > > handle initializers. That is going to be horrific by the way -- you'll be > > lifting expressions parsed in one function to another. There's still some > state > > being allocated in the parser (literal counts, handler indices, expected > > property counts) that will need to be rewritten for initializers lifted to > inner > > functions. In my mind the right way to do that is to move to a post-pass; for > > that reason it seems that optional argument initializers are going to be quite > a > > challenge. Dunno. > > > > So that said it should be impossible to get to a lazily parsed arrow function > > with default initializers with the current code. The preparser should have > > already discounted the possibility of the "formals" of the arrow function > being > > valid. > > The issue with lazy-parsing is that the scope doesn't seem to be a > function/arrow scope during lazy parsing --- so the DCHECK fails, and in release > builds we're potentially declaring things in the wrong scope. Excuse my density, I still don't understand. By "lazy parsing" do you mean that you get errors with the preparser or later when the full parser runs? I was thinking that it would actually be impossible to get to the point where the full parser runs lazily because the preparser should have generated an early error. There are two ways to parse an arrow function: to reinterpret a comma expression as formals and proceed from there, either with the preparser or the full parser, or lazily with the full parser, which knows that it is looking for a formal parameter list because the preparser has already identified the expression as an arrow function. But to get to that point you have to have already checked that the arrow function parameters are valid with the preparser, which should not be the case if there are initializers. Another question would be how this relates to arrow functions. If arrow functions are staged first, would it be necessary to support optional arguments of arrow function parameters in order to stage optional arguments? Note that this might be quite a pain for the reasons I described in the previous message. I will see what I can do to enable rest arguments with arrow functions, as Dmitry's work should make that possible now.
On 2015/05/11 16:53:21, wingo wrote: > On 2015/05/11 15:34:25, caitp wrote: > > https://codereview.chromium.org/1127063003/diff/60001/src/preparser.h > > File src/preparser.h (right): > > > > > https://codereview.chromium.org/1127063003/diff/60001/src/preparser.h#newcode... > > src/preparser.h:3541: DCHECK(scope_->is_function_scope()); > > On 2015/05/11 15:00:48, wingo wrote: > > > On 2015/05/09 20:57:22, caitp wrote: > > > > Apparently this is a problem for lazy-parsed arrow functions :( There has > > got > > > to > > > > be a better way > > > > > > So only lazy-parsed arrow functions will have a chance to have initializer > > > expressions AFAIU -- is that right? I mean, otherwise we don't know that > it's > > > an initializer and you haven't modified ParseArrowFunctionFormalParameters > to > > > handle initializers. That is going to be horrific by the way -- you'll be > > > lifting expressions parsed in one function to another. There's still some > > state > > > being allocated in the parser (literal counts, handler indices, expected > > > property counts) that will need to be rewritten for initializers lifted to > > inner > > > functions. In my mind the right way to do that is to move to a post-pass; > for > > > that reason it seems that optional argument initializers are going to be > quite > > a > > > challenge. Dunno. > > > > > > So that said it should be impossible to get to a lazily parsed arrow > function > > > with default initializers with the current code. The preparser should have > > > already discounted the possibility of the "formals" of the arrow function > > being > > > valid. > > > > The issue with lazy-parsing is that the scope doesn't seem to be a > > function/arrow scope during lazy parsing --- so the DCHECK fails, and in > release > > builds we're potentially declaring things in the wrong scope. > > Excuse my density, I still don't understand. By "lazy parsing" do you mean that > you get errors with the preparser or later when the full parser runs? I was > thinking that it would actually be impossible to get to the point where the full > parser runs lazily because the preparser should have generated an early error. > > There are two ways to parse an arrow function: to reinterpret a comma expression > as formals and proceed from there, either with the preparser or the full parser, > or lazily with the full parser, which knows that it is looking for a formal > parameter list because the preparser has already identified the expression as an > arrow function. But to get to that point you have to have already checked that > the arrow function parameters are valid with the preparser, which should not be > the case if there are initializers. > > Another question would be how this relates to arrow functions. If arrow > functions are staged first, would it be necessary to support optional arguments > of arrow function parameters in order to stage optional arguments? Note that > this might be quite a pain for the reasons I described in the previous message. > > I will see what I can do to enable rest arguments with arrow functions, as > Dmitry's work should make that possible now. The preparser logs locations to be lazily parsed, we then parse (lazily) with the full parser, with a really weird (and incorrect) scope tree, at which point ParseFormalParameterList is used on a scope which is not a function scope. That's what I'm talking about when I'm saying lazy parsing. I agree that it's going to be a pain to get optional arguments and destructuring working in pre-parsed arrow functions without performance regressions, but what can you do?
On 2015/05/11 16:58:19, caitp wrote: > The preparser logs locations to be lazily parsed, we then parse (lazily) with > the full parser, with a really weird (and incorrect) scope tree, at which point > ParseFormalParameterList is used on a scope which is not a function scope. > That's what I'm talking about when I'm saying lazy parsing. If the full parse is running with the wrong scope tree, then that's a bug. Could it simply be that the scope is not made current? That's probably it. I'll see what I can do this morning.
On 2015/05/12 07:32:11, wingo wrote: > On 2015/05/11 16:58:19, caitp wrote: > > The preparser logs locations to be lazily parsed, we then parse (lazily) with > > the full parser, with a really weird (and incorrect) scope tree, at which > point > > ParseFormalParameterList is used on a scope which is not a function scope. > > That's what I'm talking about when I'm saying lazy parsing. > > If the full parse is running with the wrong scope tree, then that's a bug. > Could it simply be that the scope is not made current? That's probably it. > I'll see what I can do this morning. Humm, it's not quite that simple. Really the arguments should be parsed with the FunctionState of the function in question, so that the initializers can get the right e.g. materialized literal indexes. I'm going to punt on this for now as "super" in arrow functions probably has higher priority. Given that in the future "scope_" should be the same as the "scope" passed to ParseFormalParameterList(), why not just change the DCHECK to assert on "scope" and not "scope_", and add a TODO?
https://codereview.chromium.org/1127063003/diff/100001/src/bootstrapper.cc File src/bootstrapper.cc (right): https://codereview.chromium.org/1127063003/diff/100001/src/bootstrapper.cc#ne... src/bootstrapper.cc:1730: EMPTY_NATIVE_FUNCTIONS_FOR_FEATURE(harmony_optional_params) Nit: harmony_default_parameters https://codereview.chromium.org/1127063003/diff/100001/src/flag-definitions.h File src/flag-definitions.h (right): https://codereview.chromium.org/1127063003/diff/100001/src/flag-definitions.h... src/flag-definitions.h:197: V(harmony_optional_params, "harmony optional parameters") Nit: s/optional/default/ https://codereview.chromium.org/1127063003/diff/100001/src/parser.cc File src/parser.cc (right): https://codereview.chromium.org/1127063003/diff/100001/src/parser.cc#newcode3575 src/parser.cc:3575: proxy->var()->set_maybe_assigned(); Why is this set? https://codereview.chromium.org/1127063003/diff/100001/src/parser.cc#newcode3585 src/parser.cc:3585: // TODO(caitp): ensure proper TDZ behaviour --- need hole-check for Nit: this comment seems obsolete https://codereview.chromium.org/1127063003/diff/100001/src/parser.cc#newcode4301 src/parser.cc:4301: body->AddAll(*inner_body, zone()); Shouldn't this create a block statement instead? https://codereview.chromium.org/1127063003/diff/100001/src/scopes.cc File src/scopes.cc (right): https://codereview.chromium.org/1127063003/diff/100001/src/scopes.cc#newcode513 src/scopes.cc:513: void Scope::ShadowParametersForExpressions() { Instead of spreading this out into a different function, can't we do that during desugaring, right before invoking re-Declare on the respective variable in DesugarInitializedPArameters? https://codereview.chromium.org/1127063003/diff/100001/test/mjsunit/harmony/o... File test/mjsunit/harmony/optional-arguments.js (right): https://codereview.chromium.org/1127063003/diff/100001/test/mjsunit/harmony/o... test/mjsunit/harmony/optional-arguments.js:55: More tests, e.g.: - (function(x = this) { return x }).call(o) === o - (function(x = () => this) { return x() }).call(o) === o - (function f(x, y = f) { return x ? y(false) + 1 : 0 })(true) === 1 - (function(x, y = function() { return x }) { return y() })(7) === 7 - (function(x = function() { return y }, y) { return x() })(8) === 8 - (function(x, y = eval("x")) { let x; return y })(7) === 7 - (function(x = eval("y"), y = 0) {})() throws - (function(x, y = () => eval("x")) { let x; return y() })(7) === 7 - (function(x = () => eval("y"), y = 9) { let y; return x() })() === 9 - (function(x = {a: 4}) { return x.a })() === 4 - (function(x, y = {a: x}) { return y.a })(5) === 5 - a function large enough to trigger lazy parsing
Thanks https://codereview.chromium.org/1127063003/diff/100001/src/parser.cc File src/parser.cc (right): https://codereview.chromium.org/1127063003/diff/100001/src/parser.cc#newcode4301 src/parser.cc:4301: body->AddAll(*inner_body, zone()); On 2015/05/12 14:04:53, rossberg wrote: > Shouldn't this create a block statement instead? Is this for the extra bailout id? I have mostly been thinking about scoping, so I'm not sure what effect wrapping it in a block statement has https://codereview.chromium.org/1127063003/diff/100001/src/scopes.cc File src/scopes.cc (right): https://codereview.chromium.org/1127063003/diff/100001/src/scopes.cc#newcode513 src/scopes.cc:513: void Scope::ShadowParametersForExpressions() { On 2015/05/12 14:04:53, rossberg wrote: > Instead of spreading this out into a different function, can't we do that during > desugaring, right before invoking re-Declare on the respective variable in > DesugarInitializedPArameters? well the scope VariableMap is private, so it's hard for the parser to do itself --- do you mean just get rid of the parser version of the function?
https://codereview.chromium.org/1127063003/diff/100001/test/mjsunit/harmony/o... File test/mjsunit/harmony/optional-arguments.js (right): https://codereview.chromium.org/1127063003/diff/100001/test/mjsunit/harmony/o... test/mjsunit/harmony/optional-arguments.js:55: On 2015/05/12 14:04:53, rossberg wrote: > More tests, e.g.: > > - (function(x = this) { return x }).call(o) === o > - (function(x = () => this) { return x() }).call(o) === o > - (function f(x, y = f) { return x ? y(false) + 1 : 0 })(true) === 1 > - (function(x, y = function() { return x }) { return y() })(7) === 7 > - (function(x = function() { return y }, y) { return x() })(8) === 8 > - (function(x, y = eval("x")) { let x; return y })(7) === 7 > - (function(x = eval("y"), y = 0) {})() throws > - (function(x, y = () => eval("x")) { let x; return y() })(7) === 7 > - (function(x = () => eval("y"), y = 9) { let y; return x() })() === 9 > - (function(x = {a: 4}) { return x.a })() === 4 > - (function(x, y = {a: x}) { return y.a })(5) === 5 > - a function large enough to trigger lazy parsing Added eval and arrow variants to a bunch of these tests --- something is very broken with the TDZ behaviour when "eval" is used, I'm not sure if this affects normal lexical variables or not. If it does I'll file a bug
https://codereview.chromium.org/1127063003/diff/100001/src/parser.cc File src/parser.cc (right): https://codereview.chromium.org/1127063003/diff/100001/src/parser.cc#newcode3575 src/parser.cc:3575: proxy->var()->set_maybe_assigned(); On 2015/05/12 14:04:53, rossberg wrote: > Why is this set? Ping https://codereview.chromium.org/1127063003/diff/100001/src/parser.cc#newcode4301 src/parser.cc:4301: body->AddAll(*inner_body, zone()); On 2015/05/12 14:34:17, caitp wrote: > On 2015/05/12 14:04:53, rossberg wrote: > > Shouldn't this create a block statement instead? > > Is this for the extra bailout id? I have mostly been thinking about scoping, so > I'm not sure what effect wrapping it in a block statement has The scope chain needs to be in sync with the context chain we create at runtime (up to some optimisations). I'm not sure how it can work without, at least not if something like eval or with appears in the body, referring to respective variables (which reminds me that we should probably have tests for those cases :) ). https://codereview.chromium.org/1127063003/diff/100001/src/scopes.cc File src/scopes.cc (right): https://codereview.chromium.org/1127063003/diff/100001/src/scopes.cc#newcode513 src/scopes.cc:513: void Scope::ShadowParametersForExpressions() { On 2015/05/12 14:34:17, caitp wrote: > On 2015/05/12 14:04:53, rossberg wrote: > > Instead of spreading this out into a different function, can't we do that > during > > desugaring, right before invoking re-Declare on the respective variable in > > DesugarInitializedPArameters? > > well the scope VariableMap is private, so it's hard for the parser to do itself > --- do you mean just get rid of the parser version of the function? Well, this whole removing variables from scope thing still worries me. Do you think it would be possible to reuse the same variables in the desugaring instead of removing and re-Declare-ing them? I figure you'd still need to do some adjustments (e.g. modifying their VariableMode), but those may be less drastic. https://codereview.chromium.org/1127063003/diff/100001/test/mjsunit/harmony/o... File test/mjsunit/harmony/optional-arguments.js (right): https://codereview.chromium.org/1127063003/diff/100001/test/mjsunit/harmony/o... test/mjsunit/harmony/optional-arguments.js:55: On 2015/05/12 18:35:35, caitp wrote: > On 2015/05/12 14:04:53, rossberg wrote: > > More tests, e.g.: > > > > - (function(x = this) { return x }).call(o) === o > > - (function(x = () => this) { return x() }).call(o) === o > > - (function f(x, y = f) { return x ? y(false) + 1 : 0 })(true) === 1 > > - (function(x, y = function() { return x }) { return y() })(7) === 7 > > - (function(x = function() { return y }, y) { return x() })(8) === 8 > > - (function(x, y = eval("x")) { let x; return y })(7) === 7 > > - (function(x = eval("y"), y = 0) {})() throws > > - (function(x, y = () => eval("x")) { let x; return y() })(7) === 7 > > - (function(x = () => eval("y"), y = 9) { let y; return x() })() === 9 > > - (function(x = {a: 4}) { return x.a })() === 4 > > - (function(x, y = {a: x}) { return y.a })(5) === 5 > > - a function large enough to trigger lazy parsing Please add a couple of lazy parsing tests as well (you need to write global functions with >2K characters size; long dummy comments are good enough to achieve that). > Added eval and arrow variants to a bunch of these tests --- something is very > broken with the TDZ behaviour when "eval" is used, I'm not sure if this affects > normal lexical variables or not. If it does I'll file a bug How do these cases fail? If it is due to a deeper bug then it should be possible to create a JS test in desugared form that exhibits it. If not, then s.th likely is wrong with the scope handling in this CL, and probably should be fixed before landing.
https://codereview.chromium.org/1127063003/diff/100001/src/parser.cc File src/parser.cc (right): https://codereview.chromium.org/1127063003/diff/100001/src/parser.cc#newcode3575 src/parser.cc:3575: proxy->var()->set_maybe_assigned(); On 2015/05/20 07:41:47, rossberg wrote: > On 2015/05/12 14:04:53, rossberg wrote: > > Why is this set? > > Ping Maybe it's not needed, but it reads like it does the right thing, since the variable gets assigned immediately https://codereview.chromium.org/1127063003/diff/100001/src/scopes.cc File src/scopes.cc (right): https://codereview.chromium.org/1127063003/diff/100001/src/scopes.cc#newcode513 src/scopes.cc:513: void Scope::ShadowParametersForExpressions() { On 2015/05/20 07:41:47, rossberg wrote: > On 2015/05/12 14:34:17, caitp wrote: > > On 2015/05/12 14:04:53, rossberg wrote: > > > Instead of spreading this out into a different function, can't we do that > > during > > > desugaring, right before invoking re-Declare on the respective variable in > > > DesugarInitializedPArameters? > > > > well the scope VariableMap is private, so it's hard for the parser to do > itself > > --- do you mean just get rid of the parser version of the function? > > Well, this whole removing variables from scope thing still worries me. Do you > think it would be possible to reuse the same variables in the desugaring instead > of removing and re-Declare-ing them? I figure you'd still need to do some > adjustments (e.g. modifying their VariableMode), but those may be less drastic. I can try it https://codereview.chromium.org/1127063003/diff/100001/test/mjsunit/harmony/o... File test/mjsunit/harmony/optional-arguments.js (right): https://codereview.chromium.org/1127063003/diff/100001/test/mjsunit/harmony/o... test/mjsunit/harmony/optional-arguments.js:55: On 2015/05/20 07:41:47, rossberg wrote: > On 2015/05/12 18:35:35, caitp wrote: > > On 2015/05/12 14:04:53, rossberg wrote: > > > More tests, e.g.: > > > > > > - (function(x = this) { return x }).call(o) === o > > > - (function(x = () => this) { return x() }).call(o) === o > > > - (function f(x, y = f) { return x ? y(false) + 1 : 0 })(true) === 1 > > > - (function(x, y = function() { return x }) { return y() })(7) === 7 > > > - (function(x = function() { return y }, y) { return x() })(8) === 8 > > > - (function(x, y = eval("x")) { let x; return y })(7) === 7 > > > - (function(x = eval("y"), y = 0) {})() throws > > > - (function(x, y = () => eval("x")) { let x; return y() })(7) === 7 > > > - (function(x = () => eval("y"), y = 9) { let y; return x() })() === 9 > > > - (function(x = {a: 4}) { return x.a })() === 4 > > > - (function(x, y = {a: x}) { return y.a })(5) === 5 > > > - a function large enough to trigger lazy parsing > > Please add a couple of lazy parsing tests as well (you need to write global > functions with >2K characters size; long dummy comments are good enough to > achieve that). > > > Added eval and arrow variants to a bunch of these tests --- something is very > > broken with the TDZ behaviour when "eval" is used, I'm not sure if this > affects > > normal lexical variables or not. If it does I'll file a bug > > How do these cases fail? If it is due to a deeper bug then it should be > possible to create a JS test in desugared form that exhibits it. If not, then > s.th likely is wrong with the scope handling in this CL, and probably should be > fixed before landing. I wasn't able to reproduce it with the regular desugaring, so it does seem to be a scoping bug. I can't remember exactly what the failures were, but eval("param name") returned something weird (I think it was returning the global object, which it probably got from the receiver on the stack). After rebasing I'll check
https://codereview.chromium.org/1127063003/diff/100001/src/parser.cc File src/parser.cc (right): https://codereview.chromium.org/1127063003/diff/100001/src/parser.cc#newcode3575 src/parser.cc:3575: proxy->var()->set_maybe_assigned(); On 2015/05/20 11:29:38, caitp wrote: > On 2015/05/20 07:41:47, rossberg wrote: > > On 2015/05/12 14:04:53, rossberg wrote: > > > Why is this set? > > > > Ping > > Maybe it's not needed, but it reads like it does the right thing, since the > variable gets assigned immediately Ah, note that these are initialisations, not assignments. This flag tries to conservatively indicate whether a variable is ever assigned, i.e., can change its value observably, or is quasi const (which is relevant for optimisations later). For lexical declarations, which have a proper TDZ, initialisation does not observably change their value (though it does for 'var').
https://codereview.chromium.org/1127063003/diff/100001/src/scopes.cc File src/scopes.cc (right): https://codereview.chromium.org/1127063003/diff/100001/src/scopes.cc#newcode513 src/scopes.cc:513: void Scope::ShadowParametersForExpressions() { On 2015/05/20 11:29:38, caitp wrote: > On 2015/05/20 07:41:47, rossberg wrote: > > On 2015/05/12 14:34:17, caitp wrote: > > > On 2015/05/12 14:04:53, rossberg wrote: > > > > Instead of spreading this out into a different function, can't we do that > > > during > > > > desugaring, right before invoking re-Declare on the respective variable in > > > > DesugarInitializedPArameters? > > > > > > well the scope VariableMap is private, so it's hard for the parser to do > > itself > > > --- do you mean just get rid of the parser version of the function? > > > > Well, this whole removing variables from scope thing still worries me. Do you > > think it would be possible to reuse the same variables in the desugaring > instead > > of removing and re-Declare-ing them? I figure you'd still need to do some > > adjustments (e.g. modifying their VariableMode), but those may be less > drastic. > > I can try it I'm not totally sure what needs to change. Right now, I'm changing the parameter to be a lexical declaration, and making sure it is allocated with AllocateLocal. However, this seems to break the TDZ behaviour. It worked a lot better when the var was completely re-declared
https://codereview.chromium.org/1127063003/diff/200001/src/scopes.cc File src/scopes.cc (right): https://codereview.chromium.org/1127063003/diff/200001/src/scopes.cc#newcode1433 src/scopes.cc:1433: if (IsLexicalVariableMode(var->mode())) { So this approach doesn't work --- it causes the `DCHECK(stack_locals[i]->index() == first_slot_index + i);` in scopeinfo.cc to fail, and it doesn't really fix anything
On 2015/05/20 17:05:16, caitp wrote: > https://codereview.chromium.org/1127063003/diff/200001/src/scopes.cc > File src/scopes.cc (right): > > https://codereview.chromium.org/1127063003/diff/200001/src/scopes.cc#newcode1433 > src/scopes.cc:1433: if (IsLexicalVariableMode(var->mode())) { > So this approach doesn't work --- it causes the `DCHECK(stack_locals[i]->index() > == first_slot_index + i);` in scopeinfo.cc to fail, and it doesn't really fix > anything So, I didn't have much luck just changing the original variable. But, I've fixed up the bugs identified previously with eval(), and it works beautifully. It's probably not written in a way that will make support for binding patterns in parameters easy, though.
Patchset #14 (id:260001) has been deleted
https://codereview.chromium.org/1127063003/diff/280001/src/scopes.cc File src/scopes.cc (right): https://codereview.chromium.org/1127063003/diff/280001/src/scopes.cc#newcode1442 src/scopes.cc:1442: var = variables_.Lookup(var->raw_name()); This is a hack to make sure the re-declared variables are properly context allocated. It's finicky, and I don't think it's quite right, but I'm not 100% sure how to verify it. So this is really the area I'm most unsure about atm
LGTM This has some issues but lets revisit those in follow up CLs. https://codereview.chromium.org/1127063003/diff/280001/test/mjsunit/harmony/d... File test/mjsunit/harmony/default-parameters-lazy.js (right): https://codereview.chromium.org/1127063003/diff/280001/test/mjsunit/harmony/d... test/mjsunit/harmony/default-parameters-lazy.js:9: /* ............................................................................ I think there is a command line option that allows you to always use lazy parsing.
https://codereview.chromium.org/1127063003/diff/280001/test/mjsunit/harmony/d... File test/mjsunit/harmony/default-parameters-lazy.js (right): https://codereview.chromium.org/1127063003/diff/280001/test/mjsunit/harmony/d... test/mjsunit/harmony/default-parameters-lazy.js:9: /* ............................................................................ On 2015/05/26 19:13:26, arv wrote: > I think there is a command line option that allows you to always use lazy > parsing. It looks like you can disable lazy parsing/compilation, but you can't force it.
adamk@chromium.org changed reviewers: + adamk@chromium.org
Various style nits follow. I think I'm about as uneasy as rossberg is about undeclaring variables. https://codereview.chromium.org/1127063003/diff/300001/src/parser.cc File src/parser.cc (right): https://codereview.chromium.org/1127063003/diff/300001/src/parser.cc#newcode1146 src/parser.cc:1146: bool hasParameterExpressions = false; Same style nit, use_underscores. I think this shows up a bunch of other places, should be an easy search/replace. https://codereview.chromium.org/1127063003/diff/300001/src/parser.cc#newcode1147 src/parser.cc:1147: ZoneList<Expression*>* initializers = Please add a TODO here, since you're just dropping this on the floor. https://codereview.chromium.org/1127063003/diff/300001/src/parser.cc#newcode3424 src/parser.cc:3424: ZoneList<Statement*>* body = new (zone()) ZoneList<Statement*>(0, zone()); Don't you know how big a list you need here (initializers->length())? https://codereview.chromium.org/1127063003/diff/300001/src/parser.cc#newcode3431 src/parser.cc:3431: static const int capacity = 1; More style nits: kCapacity https://codereview.chromium.org/1127063003/diff/300001/src/parser.cc#newcode3432 src/parser.cc:3432: static const bool is_initializer_block = true; // ? kIsInitializerBlock. Also the "// ?" should either be gone or better explained. https://codereview.chromium.org/1127063003/diff/300001/src/parser.cc#newcode3436 src/parser.cc:3436: static const VariableMode mode = LET; kMode (though I'm not sure this adds much) https://codereview.chromium.org/1127063003/diff/300001/src/parser.cc#newcode3452 src/parser.cc:3452: new (zone()) ZoneList<Expression*>(0, zone()); s/0/1/ for the capacity arg here. https://codereview.chromium.org/1127063003/diff/300001/src/parser.cc#newcode3456 src/parser.cc:3456: if (initializer) { if (initializer != nullptr) https://codereview.chromium.org/1127063003/diff/300001/src/parser.cc#newcode3459 src/parser.cc:3459: new (zone()) ZoneList<Expression*>(0, zone()); s/0/1/ again. https://codereview.chromium.org/1127063003/diff/300001/src/parser.h File src/parser.h (right): https://codereview.chromium.org/1127063003/diff/300001/src/parser.h#newcode1076 src/parser.h:1076: Scope* scope, bool hasParameterExpressions, Style nit: has_parameter_expressions
https://codereview.chromium.org/1127063003/diff/280001/src/scopes.cc File src/scopes.cc (right): https://codereview.chromium.org/1127063003/diff/280001/src/scopes.cc#newcode1442 src/scopes.cc:1442: var = variables_.Lookup(var->raw_name()); On 2015/05/26 17:14:07, caitp wrote: > This is a hack to make sure the re-declared variables are properly context > allocated. It's finicky, and I don't think it's quite right, but I'm not 100% > sure how to verify it. > > So this is really the area I'm most unsure about atm Can we just skip the loop over params_ in AllocateParameterLocals() if has_parameter_expressions_ is set? It seems like all the Variables in there are bogus at this point if that bit is set (since they got "undeclared").
https://codereview.chromium.org/1127063003/diff/280001/src/scopes.cc File src/scopes.cc (right): https://codereview.chromium.org/1127063003/diff/280001/src/scopes.cc#newcode1442 src/scopes.cc:1442: var = variables_.Lookup(var->raw_name()); On 2015/05/26 20:03:20, adamk wrote: > On 2015/05/26 17:14:07, caitp wrote: > > This is a hack to make sure the re-declared variables are properly context > > allocated. It's finicky, and I don't think it's quite right, but I'm not 100% > > sure how to verify it. > > > > So this is really the area I'm most unsure about atm > > Can we just skip the loop over params_ in AllocateParameterLocals() if > has_parameter_expressions_ is set? It seems like all the Variables in there are > bogus at this point if that bit is set (since they got "undeclared"). Maybe, but there are a few things that might go wrong - The redeclared variable might need to be context allocated, which probably won't happen without this check - Stack and heap slot counts need to be what they're expected to be so that scopeinfo ends up usable and sane I'll try that after the next PS and see if it works reasonably well https://codereview.chromium.org/1127063003/diff/300001/src/parser.cc File src/parser.cc (right): https://codereview.chromium.org/1127063003/diff/300001/src/parser.cc#newcode1146 src/parser.cc:1146: bool hasParameterExpressions = false; On 2015/05/26 19:58:10, adamk wrote: > Same style nit, use_underscores. I think this shows up a bunch of other places, > should be an easy search/replace. Done. https://codereview.chromium.org/1127063003/diff/300001/src/parser.cc#newcode1147 src/parser.cc:1147: ZoneList<Expression*>* initializers = On 2015/05/26 19:58:10, adamk wrote: > Please add a TODO here, since you're just dropping this on the floor. Done. https://codereview.chromium.org/1127063003/diff/300001/src/parser.cc#newcode3424 src/parser.cc:3424: ZoneList<Statement*>* body = new (zone()) ZoneList<Statement*>(0, zone()); On 2015/05/26 19:58:10, adamk wrote: > Don't you know how big a list you need here (initializers->length())? Done. https://codereview.chromium.org/1127063003/diff/300001/src/parser.cc#newcode3431 src/parser.cc:3431: static const int capacity = 1; On 2015/05/26 19:58:10, adamk wrote: > More style nits: kCapacity Done. https://codereview.chromium.org/1127063003/diff/300001/src/parser.cc#newcode3432 src/parser.cc:3432: static const bool is_initializer_block = true; // ? On 2015/05/26 19:58:10, adamk wrote: > kIsInitializerBlock. > > Also the "// ?" should either be gone or better explained. Done. The "?" comment is a reminder to ask what the flag actually means https://codereview.chromium.org/1127063003/diff/300001/src/parser.cc#newcode3436 src/parser.cc:3436: static const VariableMode mode = LET; On 2015/05/26 19:58:10, adamk wrote: > kMode (though I'm not sure this adds much) Done. https://codereview.chromium.org/1127063003/diff/300001/src/parser.cc#newcode3452 src/parser.cc:3452: new (zone()) ZoneList<Expression*>(0, zone()); On 2015/05/26 19:58:10, adamk wrote: > s/0/1/ for the capacity arg here. Done. https://codereview.chromium.org/1127063003/diff/300001/src/parser.cc#newcode3456 src/parser.cc:3456: if (initializer) { On 2015/05/26 19:58:10, adamk wrote: > if (initializer != nullptr) Done. https://codereview.chromium.org/1127063003/diff/300001/src/parser.cc#newcode3459 src/parser.cc:3459: new (zone()) ZoneList<Expression*>(0, zone()); On 2015/05/26 19:58:10, adamk wrote: > s/0/1/ again. Done.
https://codereview.chromium.org/1127063003/diff/280001/src/scopes.cc File src/scopes.cc (right): https://codereview.chromium.org/1127063003/diff/280001/src/scopes.cc#newcode1442 src/scopes.cc:1442: var = variables_.Lookup(var->raw_name()); On 2015/05/26 20:24:44, caitp wrote: > On 2015/05/26 20:03:20, adamk wrote: > > On 2015/05/26 17:14:07, caitp wrote: > > > This is a hack to make sure the re-declared variables are properly context > > > allocated. It's finicky, and I don't think it's quite right, but I'm not > 100% > > > sure how to verify it. > > > > > > So this is really the area I'm most unsure about atm > > > > Can we just skip the loop over params_ in AllocateParameterLocals() if > > has_parameter_expressions_ is set? It seems like all the Variables in there > are > > bogus at this point if that bit is set (since they got "undeclared"). > > Maybe, but there are a few things that might go wrong > > - The redeclared variable might need to be context allocated, which probably > won't happen without this check > - Stack and heap slot counts need to be what they're expected to be so that > scopeinfo ends up usable and sane > > I'll try that after the next PS and see if it works reasonably well Actually, it passes tests at the moment, so I guess this is fine
https://codereview.chromium.org/1127063003/diff/360001/src/scopes.h File src/scopes.h (right): https://codereview.chromium.org/1127063003/diff/360001/src/scopes.h#newcode375 src/scopes.h:375: Variable* parameter(int index) const { Is this called anywhere (besides your new caller in scopes.cc)? It scares me a bit that callers might pull a Variable out of this now that these are bogus.
https://codereview.chromium.org/1127063003/diff/360001/src/scopes.h File src/scopes.h (right): https://codereview.chromium.org/1127063003/diff/360001/src/scopes.h#newcode375 src/scopes.h:375: Variable* parameter(int index) const { On 2015/05/26 20:34:02, adamk wrote: > Is this called anywhere (besides your new caller in scopes.cc)? It scares me a > bit that callers might pull a Variable out of this now that these are bogus. Yes, it's called in function prologues to move parameters pushed on the stack into context. It should never actually have to "do" that since the "original" param variables are never allocated right now, so I think it's safe-ish.
https://codereview.chromium.org/1127063003/diff/360001/src/scopes.h File src/scopes.h (right): https://codereview.chromium.org/1127063003/diff/360001/src/scopes.h#newcode375 src/scopes.h:375: Variable* parameter(int index) const { On 2015/05/26 20:59:11, caitp wrote: > On 2015/05/26 20:34:02, adamk wrote: > > Is this called anywhere (besides your new caller in scopes.cc)? It scares me a > > bit that callers might pull a Variable out of this now that these are bogus. > > Yes, it's called in function prologues to move parameters pushed on the stack > into context. It should never actually have to "do" that since the "original" > param variables are never allocated right now, so I think it's safe-ish. Thanks for the pointer, I see how my git grep missed that now. I agree that it's "safe-ish" :)
So, just to recap discussions from IRC: This patch is the (as far as I can tell) simplest approach to implementing a subset of the default parameters feature. This subset includes: - SingleNameBindings only, no BindingPattern support yet - ReferenceError when parameters are referenced before initialized (in order, left to right) by an initializer or by the value passed on the stack There is a list of things TODO in followup CLs: - Properly scope the function body in a separate declaration scope - Refactor FormalParameter parsing to not declare parameters (remove need to re-declare them) --- This ties in with BindingPattern support - Fix function arity for rest and optional parameters (This CL does not touch function arity) - Re-implement rest parameters using desugaring (this will most likely address some of the performance pains, and at least match the performance of lodash's _.restParams() method =) --- Not directly related, but since optional params get desugared, it makes sense to desugar those too. The current CL breaks rest params when mixed with optional arguments, so some kind of fix is needed anyways. ---- So, given that there is a plan to move this forward, is it cool if I land this and finish the work started? =)
lgtm https://codereview.chromium.org/1127063003/diff/360001/src/parser.cc File src/parser.cc (right): https://codereview.chromium.org/1127063003/diff/360001/src/parser.cc#newcode3440 src/parser.cc:3440: static const VariableMode kMode = LET; Nit: drop this auxiliary https://codereview.chromium.org/1127063003/diff/360001/src/parser.cc#newcode3460 src/parser.cc:3460: if (initializer != nullptr) { Nit: switch branches (simple first)
https://codereview.chromium.org/1127063003/diff/360001/src/parser.cc File src/parser.cc (right): https://codereview.chromium.org/1127063003/diff/360001/src/parser.cc#newcode3440 src/parser.cc:3440: static const VariableMode kMode = LET; On 2015/06/01 15:46:39, rossberg wrote: > Nit: drop this auxiliary Done. https://codereview.chromium.org/1127063003/diff/360001/src/parser.cc#newcode3460 src/parser.cc:3460: if (initializer != nullptr) { On 2015/06/01 15:46:39, rossberg wrote: > Nit: switch branches (simple first) Done.
The CQ bit was checked by caitpotter88@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from arv@chromium.org, rossberg@chromium.org Link to the patchset: https://codereview.chromium.org/1127063003/#ps380001 (title: "Rebase + rossberg nits")
The CQ bit was unchecked by caitpotter88@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1127063003/380001
The CQ bit was unchecked by caitpotter88@gmail.com
The CQ bit was checked by caitpotter88@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1127063003/380001
Message was sent while issue was closed.
Committed patchset #19 (id:380001)
Message was sent while issue was closed.
Patchset 19 (id:??) landed as https://crrev.com/892c85485881f8be2f17bd83238980f858126576 Cr-Commit-Position: refs/heads/master@{#28739}
Message was sent while issue was closed.
A revert of this CL (patchset #19 id:380001) has been created in https://codereview.chromium.org/1163853002/ by caitpotter88@gmail.com. The reason for reverting is: Broken on arm64.
The output from --trace-turbo-graph looks generally okay (at least the portions that I understand). So I'm not sure what's causing the arm64 sim test to fail. https://gist.githubusercontent.com/caitp/20f4ba77fc0d03033552/raw/log for a (big) log with --trace-turbo and --trace-turbo-graph.
caitpotter88@gmail.com changed reviewers: + mstarzinger@chromium.org
On 2015/06/02 12:47:23, caitp wrote: > The output from --trace-turbo-graph looks generally okay (at least the portions > that I understand). So I'm not sure what's causing the arm64 sim test to fail. > https://gist.githubusercontent.com/caitp/20f4ba77fc0d03033552/raw/log for a > (big) log with --trace-turbo and --trace-turbo-graph. mstarzinger@ I was wondering if you (or someone else familiar with TF) might be able to shed some light on where this is going wrong on ARM64, or how I could determine it. The --trace-turbo-graph output for the function looks correct, but we still end up with a kLetBindingReInitialization bailout, only on ARM64, and only with turbofan.
On 2015/06/02 14:55:18, caitp wrote: > On 2015/06/02 12:47:23, caitp wrote: > > The output from --trace-turbo-graph looks generally okay (at least the > portions > > that I understand). So I'm not sure what's causing the arm64 sim test to fail. > > https://gist.githubusercontent.com/caitp/20f4ba77fc0d03033552/raw/log for a > > (big) log with --trace-turbo and --trace-turbo-graph. > > mstarzinger@ I was wondering if you (or someone else familiar with TF) might be > able to shed some light on where this is going wrong on ARM64, or how I could > determine it. The --trace-turbo-graph output for the function looks correct, but > we still end up with a kLetBindingReInitialization bailout, only on ARM64, and > only with turbofan. From IRC: [12:03] <caitp> you know what [12:04] <caitp> replacing `if (FLAG_debug_code && op == Token::INIT_LET) {` with `if (generate_debug_code_ && op == Token::INIT_LET) {` in full-codegen-arm64.cc fixes it [12:04] <caitp> and matches the other arches better [12:04] <caitp> derp so... I assume that's just a bug in full-codegen-arm64 unless this was written this way on purpose?
On 2015/06/02 16:05:00, caitp wrote: > On 2015/06/02 14:55:18, caitp wrote: > > On 2015/06/02 12:47:23, caitp wrote: > > > The output from --trace-turbo-graph looks generally okay (at least the > > portions > > > that I understand). So I'm not sure what's causing the arm64 sim test to > fail. > > > https://gist.githubusercontent.com/caitp/20f4ba77fc0d03033552/raw/log for a > > > (big) log with --trace-turbo and --trace-turbo-graph. > > > > mstarzinger@ I was wondering if you (or someone else familiar with TF) might > be > > able to shed some light on where this is going wrong on ARM64, or how I could > > determine it. The --trace-turbo-graph output for the function looks correct, > but > > we still end up with a kLetBindingReInitialization bailout, only on ARM64, and > > only with turbofan. > > From IRC: > > [12:03] <caitp> you know what > [12:04] <caitp> replacing `if (FLAG_debug_code && op == Token::INIT_LET) {` > with `if (generate_debug_code_ && op == Token::INIT_LET) {` in > full-codegen-arm64.cc fixes it > [12:04] <caitp> and matches the other arches better > [12:04] <caitp> derp > > so... I assume that's just a bug in full-codegen-arm64 unless this was written > this way on purpose? Anyways, looking into this more, I don't think it's strictly a TF issue but maybe more related to deoptimization, and probably the desugaring itself is slightly suspect since it's doing this bad of a job of it. For whatever reason, after de-opt, let bindings are re-assigned and re-initialized. Maybe during deoptimization or OSR, these variable initializations should be skipped somehow? I'm not sure the approach to take with this, but I guess this is still not ready to land
On 2015/06/03 03:53:48, caitp wrote: > On 2015/06/02 16:05:00, caitp wrote: > > On 2015/06/02 14:55:18, caitp wrote: > > > On 2015/06/02 12:47:23, caitp wrote: > > > > The output from --trace-turbo-graph looks generally okay (at least the > > > portions > > > > that I understand). So I'm not sure what's causing the arm64 sim test to > > fail. > > > > https://gist.githubusercontent.com/caitp/20f4ba77fc0d03033552/raw/log for > a > > > > (big) log with --trace-turbo and --trace-turbo-graph. > > > > > > mstarzinger@ I was wondering if you (or someone else familiar with TF) might > > be > > > able to shed some light on where this is going wrong on ARM64, or how I > could > > > determine it. The --trace-turbo-graph output for the function looks correct, > > but > > > we still end up with a kLetBindingReInitialization bailout, only on ARM64, > and > > > only with turbofan. > > > > From IRC: > > > > [12:03] <caitp> you know what > > [12:04] <caitp> replacing `if (FLAG_debug_code && op == Token::INIT_LET) {` > > with `if (generate_debug_code_ && op == Token::INIT_LET) {` in > > full-codegen-arm64.cc fixes it > > [12:04] <caitp> and matches the other arches better > > [12:04] <caitp> derp > > > > so... I assume that's just a bug in full-codegen-arm64 unless this was written > > this way on purpose? > > Anyways, looking into this more, I don't think it's strictly a TF issue but > maybe more related to deoptimization, and probably the desugaring itself is > slightly suspect since it's doing this bad of a job of it. > > For whatever reason, after de-opt, let bindings are re-assigned and > re-initialized. Maybe during deoptimization or OSR, these variable > initializations should be skipped somehow? I'm not sure the approach to take > with this, but I guess this is still not ready to land My thinking today is that this is a bug with the "re-declaration" of parameters, so I guess that's not going to fly. I've started working on removing that, but it breaks a lot of stuff right now. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
