Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2099623003:
Annex B.3.3 semantics (Closed)
DescriptionSloppy-mode function declarations in blocks are now hoisted appropriately.
In ES2016, function declarations nested in blocks are formally allowed. This was
never a part of ECMAScript, but was a common extension. Unfortunately
implementations differed in the exact semantics. Annex B.3.3 in the spec tries
to standardize the parts which are common to different implementations, but does
so with some fairly complicated semantics.
This CL addresses three issues related to annex B.3.3:
* When the outer function had a complex parameter list, no hoisting whatsoever was
being performed.
* Hoisting was not blocked by parameters of the same name.
* Hoisting was not blocked by nested lexical declarations of the same name.
We had tests which checked for the second, but they were incorrectly passing due to
the first. This CL adds more complete tests.
BUG=v8:5151, v8:5111
Committed: https://crrev.com/9bbba1441a920ac6036e726da49f037e107835aa
Cr-Commit-Position: refs/heads/master@{#37405}
Patch Set 1 #Patch Set 2 : remove test which should not yet have been passing #Patch Set 3 : more b33 fixes #
Total comments: 4
Patch Set 4 : typo and test #Patch Set 5 : more documentation #
Total comments: 10
Patch Set 6 : cleanup per adam #Patch Set 7 : param->params #
Total comments: 2
Patch Set 8 : typos #
Messages
Total messages: 38 (18 generated)
bakkot@google.com changed reviewers: + littledan@chromium.org
The failure was because functions with complex parameters create a nested Scope, and the sloppy_block_function_map of this was not getting passed to the function scope. As such, the outer function saw no sloppy block functions. It's not enough to just copy the map to the function scope, because the var bindings actually do need to get created in the inner scope (since they must not be visible to expressions in the parameter list). As such, the call to InsertSloppyBlockFunctionVarBindings has to be moved inside ParseEagerFunctionBody, which is the only place where the inner scope actually exists.
The CQ bit was checked by bakkot@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...) v8_win_nosnap_shared_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)
The CQ bit was checked by bakkot@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm Good catch, and the new test seems a lot better (what was I thinking?). I think it's fine to land this patch as long as the other fix makes in before the branch; otherwise we might want to revert it if it doesn't get in in time.
Description was changed from ========== Functions in blocks in functions with complex parameters are now hoisted Annex B.3.3 requires hoisting nested function declarations in non-strict code under many circumstances. That this was not happening when the outer function had complex parameters was a bug. An existing test should have caught this case, but was passing by accident. BUG=v8:5151 ========== to ========== Functions in blocks in functions with complex parameters are now hoisted Annex B.3.3 requires hoisting nested function declarations in non-strict code under many circumstances. That this was not happening when the outer function had complex parameters was a bug. An existing test should have caught this case, but was passing by accident. BUG=v8:5151, v8:5111 ==========
Description was changed from ========== Functions in blocks in functions with complex parameters are now hoisted Annex B.3.3 requires hoisting nested function declarations in non-strict code under many circumstances. That this was not happening when the outer function had complex parameters was a bug. An existing test should have caught this case, but was passing by accident. BUG=v8:5151, v8:5111 ========== to ========== Sloppy-mode function declarations in blocks are now hoisted appropriately. This CL addresses three issues related to annex B.3.3: * When the outer function had a complex parameter list, no hoisting whatsoever was being performed. * Hoisting was not blocked by parameters of the same name. * Hoisting was not blocked by nested lexical declarations of the same name. We had tests which checked for the second, but they were incorrectly passing due to the first. This CL adds more complete tests. BUG=v8:5151, v8:5111 ==========
The CQ bit was checked by bakkot@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ptal
Description was changed from ========== Sloppy-mode function declarations in blocks are now hoisted appropriately. This CL addresses three issues related to annex B.3.3: * When the outer function had a complex parameter list, no hoisting whatsoever was being performed. * Hoisting was not blocked by parameters of the same name. * Hoisting was not blocked by nested lexical declarations of the same name. We had tests which checked for the second, but they were incorrectly passing due to the first. This CL adds more complete tests. BUG=v8:5151, v8:5111 ========== to ========== Sloppy-mode function declarations in blocks are now hoisted appropriately. In ES2016, function declarations nested in blocks are formally allowed. This was never a part of ECMAScript, but was a common extension. Unfortunately implementations differed in the exact semantics. Annex B.3.3 in the spec tries to standardize the parts which are common to different implementations, but does so with some fairly complicated semantics. This CL addresses three issues related to annex B.3.3: * When the outer function had a complex parameter list, no hoisting whatsoever was being performed. * Hoisting was not blocked by parameters of the same name. * Hoisting was not blocked by nested lexical declarations of the same name. We had tests which checked for the second, but they were incorrectly passing due to the first. This CL adds more complete tests. BUG=v8:5151, v8:5111 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2099623003/diff/40001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2099623003/diff/40001/src/parsing/parser.cc#n... src/parsing/parser.cc:5140: if (outer_is_param_scope && outer_scope->LookupLocal(name) != nullptr) { Why would this not prohibit hoisting in cases like this (which should be allowed)? function f(...x) { var g; { function g() {} } } https://codereview.chromium.org/2099623003/diff/40001/src/parsing/parser.h File src/parsing/parser.h (right): https://codereview.chromium.org/2099623003/diff/40001/src/parsing/parser.h#ne... src/parsing/parser.h:1022: void InsertSloppyBlockFunctionVarBindings(Scope* scope, bool check_outer, Nit: call this outer_is_param_scope here too?
Addressed comments; ptal https://codereview.chromium.org/2099623003/diff/40001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2099623003/diff/40001/src/parsing/parser.cc#n... src/parsing/parser.cc:5140: if (outer_is_param_scope && outer_scope->LookupLocal(name) != nullptr) { On 2016/06/28 at 01:07:00, Dan Ehrenberg wrote: > Why would this not prohibit hoisting in cases like this (which should be allowed)? > > function f(...x) { > var g; > { function g() {} } > } The relevant "outer scope" is that of the scope in which the var binding would be created (i.e., it is that of the body of the outer function), not the one in which the inner function is declared. outer_is_param_scope holds iff outer_scope is the new scope created for non-simple parameters. https://codereview.chromium.org/2099623003/diff/40001/src/parsing/parser.h File src/parsing/parser.h (right): https://codereview.chromium.org/2099623003/diff/40001/src/parsing/parser.h#ne... src/parsing/parser.h:1022: void InsertSloppyBlockFunctionVarBindings(Scope* scope, bool check_outer, On 2016/06/28 at 01:07:00, Dan Ehrenberg wrote: > Nit: call this outer_is_param_scope here too? Whoops, done.
adamk@chromium.org changed reviewers: + adamk@chromium.org
Some comments (your conversation this morning piqued my interest :) https://codereview.chromium.org/2099623003/diff/80001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2099623003/diff/80001/src/parsing/parser.cc#n... src/parsing/parser.cc:5139: /* Check if there's a conflict with a complex parameter. Style nit: please use '//' style comments for consistency. https://codereview.chromium.org/2099623003/diff/80001/src/parsing/parser.cc#n... src/parsing/parser.cc:5140: * This depends on the fact that functions always create a new scope to This comment seems a bit backwards, and it confused me on first reading. We create a new scope to hold the declarations in the function body, and the parameters themselves live in the scope that's left outside. I suppose that amounts to the same thing, but as I said this was confusing as a reader of this comment. The use of the name "param_scope" in "outer_scope_is_param_scope" is further confusing since "param_scope" is the name given to scopes created when a sloppy eval is used in a complex parameter. Please consider rewriting in terms that match the usage elsewhere in this file. https://codereview.chromium.org/2099623003/diff/80001/src/parsing/parser.cc#n... src/parsing/parser.cc:5142: * precisely the names of the parameters. IsDeclaredParameter(name) does Does IsDeclaredParameter ever hold in this case? I don't think it does, because complex parameters are never named. https://codereview.chromium.org/2099623003/diff/80001/src/parsing/parser.cc#n... src/parsing/parser.cc:5153: Scope* query_scope = delegate->scope()->outer_scope(); Can't everything above here live outside the delegate loop? It doesn't seem to depend on the delegate at all. https://codereview.chromium.org/2099623003/diff/80001/src/parsing/parser.cc#n... src/parsing/parser.cc:5156: do { Even this loop seems like it should be hoistable with a little help (we presumably query the same scopes multiple times...). Maybe add a TODO?
The CQ bit was checked by bakkot@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by bakkot@google.com
The CQ bit was checked by bakkot@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Addressed Adam's comments. https://codereview.chromium.org/2099623003/diff/80001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2099623003/diff/80001/src/parsing/parser.cc#n... src/parsing/parser.cc:5139: /* Check if there's a conflict with a complex parameter. On 2016/06/28 at 20:55:11, adamk wrote: > Style nit: please use '//' style comments for consistency. Done. https://codereview.chromium.org/2099623003/diff/80001/src/parsing/parser.cc#n... src/parsing/parser.cc:5140: * This depends on the fact that functions always create a new scope to On 2016/06/28 at 20:55:11, adamk wrote: > This comment seems a bit backwards, and it confused me on first reading. We create a new scope to hold the declarations in the function body, and the parameters themselves live in the scope that's left outside. I suppose that amounts to the same thing, but as I said this was confusing as a reader of this comment. > > The use of the name "param_scope" in "outer_scope_is_param_scope" is further confusing since "param_scope" is the name given to scopes created when a sloppy eval is used in a complex parameter. > > Please consider rewriting in terms that match the usage elsewhere in this file. Amended the comment slightly, and now we have a complex_params_scope passed in instead of that flag. https://codereview.chromium.org/2099623003/diff/80001/src/parsing/parser.cc#n... src/parsing/parser.cc:5142: * precisely the names of the parameters. IsDeclaredParameter(name) does On 2016/06/28 at 20:55:10, adamk wrote: > Does IsDeclaredParameter ever hold in this case? I don't think it does, because complex parameters are never named. You're correct; refactored that out. (I thought it might hold for the simple parameters in a non-simple parameter list, but it does not.) https://codereview.chromium.org/2099623003/diff/80001/src/parsing/parser.cc#n... src/parsing/parser.cc:5153: Scope* query_scope = delegate->scope()->outer_scope(); On 2016/06/28 at 20:55:10, adamk wrote: > Can't everything above here live outside the delegate loop? It doesn't seem to depend on the delegate at all. Done. https://codereview.chromium.org/2099623003/diff/80001/src/parsing/parser.cc#n... src/parsing/parser.cc:5156: do { On 2016/06/28 at 20:55:10, adamk wrote: > Even this loop seems like it should be hoistable with a little help (we presumably query the same scopes multiple times...). Maybe add a TODO? Not exactly, but I added a note.
lgtm https://codereview.chromium.org/2099623003/diff/120001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2099623003/diff/120001/src/parsing/parser.cc#... src/parsing/parser.cc:5138: if (complex_params_scope) { Might as well do a != nullptr here too (that's most common in v8) https://codereview.chromium.org/2099623003/diff/120001/src/parsing/parser.cc#... src/parsing/parser.cc:5163: // `{ let e; try {} catch (f) { function f(){} } }` s/e/f/ in this comment
The CQ bit was checked by bakkot@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from littledan@chromium.org, adamk@chromium.org Link to the patchset: https://codereview.chromium.org/2099623003/#ps140001 (title: "typos")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Sloppy-mode function declarations in blocks are now hoisted appropriately. In ES2016, function declarations nested in blocks are formally allowed. This was never a part of ECMAScript, but was a common extension. Unfortunately implementations differed in the exact semantics. Annex B.3.3 in the spec tries to standardize the parts which are common to different implementations, but does so with some fairly complicated semantics. This CL addresses three issues related to annex B.3.3: * When the outer function had a complex parameter list, no hoisting whatsoever was being performed. * Hoisting was not blocked by parameters of the same name. * Hoisting was not blocked by nested lexical declarations of the same name. We had tests which checked for the second, but they were incorrectly passing due to the first. This CL adds more complete tests. BUG=v8:5151, v8:5111 ========== to ========== Sloppy-mode function declarations in blocks are now hoisted appropriately. In ES2016, function declarations nested in blocks are formally allowed. This was never a part of ECMAScript, but was a common extension. Unfortunately implementations differed in the exact semantics. Annex B.3.3 in the spec tries to standardize the parts which are common to different implementations, but does so with some fairly complicated semantics. This CL addresses three issues related to annex B.3.3: * When the outer function had a complex parameter list, no hoisting whatsoever was being performed. * Hoisting was not blocked by parameters of the same name. * Hoisting was not blocked by nested lexical declarations of the same name. We had tests which checked for the second, but they were incorrectly passing due to the first. This CL adds more complete tests. BUG=v8:5151, v8:5111 Committed: https://crrev.com/9bbba1441a920ac6036e726da49f037e107835aa Cr-Commit-Position: refs/heads/master@{#37405} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/9bbba1441a920ac6036e726da49f037e107835aa Cr-Commit-Position: refs/heads/master@{#37405} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
