Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2112163002:
Block-scoped functions in evals are now only conditionally hoisted out. (Closed)
|
Created:
4 years, 5 months ago by bakkot Modified:
4 years, 4 months ago Reviewers:
Dan Ehrenberg CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionBlock-scoped functions in evals are now only conditionally hoisted out.
Annex B.3.3 of the spec requires that sloppy-mode block-scoped functions
declared by "eval" are hoisted unless doing so would cause an early
error (which is to say, conflict with a lexical declaration). This patch
amends the check for conflicting declarations to include those outside
of the eval itself.
BUG=v8:4468, v8:4479
Committed: https://crrev.com/f6c6ae903463327af2a0a8766a15d882a9eb3e93
Cr-Commit-Position: refs/heads/master@{#37783}
Patch Set 1 #
Total comments: 1
Patch Set 2 : fix test #
Total comments: 1
Patch Set 3 : more tests #
Messages
Total messages: 28 (15 generated)
bakkot@google.com changed reviewers: + littledan@chromium.org
Description was changed from ========== Block-scoped functions in evals are now only conditionally hoisted out. Annex B.3.3 of the spec requires that sloppy-mode block-scoped functions declared by "eval" are hoisted unless doing so would cause an early error (which is to say, conflict with a lexical declaration). This patch amends the check for conflicting declarations to include those outside of the eval itself. BUG=v8:4468,v8:4452 ========== to ========== Block-scoped functions in evals are now only conditionally hoisted out. Annex B.3.3 of the spec requires that sloppy-mode block-scoped functions declared by "eval" are hoisted unless doing so would cause an early error (which is to say, conflict with a lexical declaration). This patch amends the check for conflicting declarations to include those outside of the eval itself. There should probably be more tests before this goes in. This appears to fix v8:4452 as well, but to be honest I'm not sure why. BUG=v8:4468,v8:4452 ==========
Description was changed from ========== Block-scoped functions in evals are now only conditionally hoisted out. Annex B.3.3 of the spec requires that sloppy-mode block-scoped functions declared by "eval" are hoisted unless doing so would cause an early error (which is to say, conflict with a lexical declaration). This patch amends the check for conflicting declarations to include those outside of the eval itself. There should probably be more tests before this goes in. This appears to fix v8:4452 as well, but to be honest I'm not sure why. BUG=v8:4468,v8:4452 ========== to ========== Block-scoped functions in evals are now only conditionally hoisted out. Annex B.3.3 of the spec requires that sloppy-mode block-scoped functions declared by "eval" are hoisted unless doing so would cause an early error (which is to say, conflict with a lexical declaration). This patch amends the check for conflicting declarations to include those outside of the eval itself. This appears to fix v8:4452 as well, but to be honest I'm not sure why. BUG=v8:4468,v8:4452 ==========
There should probably be more tests before this goes in, but it seems correct.
The CQ bit was checked by littledan@chromium.org 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.
Need to think more about the main change (seems right at a first glance), but I figured out your miraculous frozen global fix https://codereview.chromium.org/2112163002/diff/1/test/mjsunit/es6/block-slop... File test/mjsunit/es6/block-sloppy-function.js (right): https://codereview.chromium.org/2112163002/diff/1/test/mjsunit/es6/block-slop... test/mjsunit/es6/block-sloppy-function.js:559: assertFalse(throws); This one's funny. Because the global object is already frozen, the change in the previous test from true to false means that it's changed down here too. Because we're in sloppy mode, the write on line 551 doesn't take. You didn't fix this particular bug :)
Description was changed from ========== Block-scoped functions in evals are now only conditionally hoisted out. Annex B.3.3 of the spec requires that sloppy-mode block-scoped functions declared by "eval" are hoisted unless doing so would cause an early error (which is to say, conflict with a lexical declaration). This patch amends the check for conflicting declarations to include those outside of the eval itself. This appears to fix v8:4452 as well, but to be honest I'm not sure why. BUG=v8:4468,v8:4452 ========== to ========== Block-scoped functions in evals are now only conditionally hoisted out. Annex B.3.3 of the spec requires that sloppy-mode block-scoped functions declared by "eval" are hoisted unless doing so would cause an early error (which is to say, conflict with a lexical declaration). This patch amends the check for conflicting declarations to include those outside of the eval itself. BUG=v8:4468 ==========
Corrected the incorrectly-passing test.
On 2016/07/01 19:49:59, bakkot wrote: > Corrected the incorrectly-passing test.
https://codereview.chromium.org/2112163002/diff/20001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2112163002/diff/20001/src/parsing/parser.cc#n... src/parsing/parser.cc:5118: decl_scope = decl_scope->outer_scope()->DeclarationScope(); It feels like this will search a little too far. Do you mean decl_scope->DeclarationScope()->outer_scope()?
On 2016/07/01 at 23:55:53, littledan wrote: > https://codereview.chromium.org/2112163002/diff/20001/src/parsing/parser.cc > File src/parsing/parser.cc (right): > > https://codereview.chromium.org/2112163002/diff/20001/src/parsing/parser.cc#n... > src/parsing/parser.cc:5118: decl_scope = decl_scope->outer_scope()->DeclarationScope(); > It feels like this will search a little too far. Do you mean decl_scope->DeclarationScope()->outer_scope()? Eval scopes are declaration scopes (from what I can tell). The intent is to walk up to the innermost declaration scope which is not an eval scope: so, step outside of the eval scope, then walk up to the nearest declaration scope.
lgtm OK, thought about it more, this seems right.
Is anything holding up committing this patch?
Description was changed from ========== Block-scoped functions in evals are now only conditionally hoisted out. Annex B.3.3 of the spec requires that sloppy-mode block-scoped functions declared by "eval" are hoisted unless doing so would cause an early error (which is to say, conflict with a lexical declaration). This patch amends the check for conflicting declarations to include those outside of the eval itself. BUG=v8:4468 ========== to ========== Block-scoped functions in evals are now only conditionally hoisted out. Annex B.3.3 of the spec requires that sloppy-mode block-scoped functions declared by "eval" are hoisted unless doing so would cause an early error (which is to say, conflict with a lexical declaration). This patch amends the check for conflicting declarations to include those outside of the eval itself. BUG=v8:4468, v8:4479 ==========
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.
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 Link to the patchset: https://codereview.chromium.org/2112163002/#ps40001 (title: "more tests")
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 #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Block-scoped functions in evals are now only conditionally hoisted out. Annex B.3.3 of the spec requires that sloppy-mode block-scoped functions declared by "eval" are hoisted unless doing so would cause an early error (which is to say, conflict with a lexical declaration). This patch amends the check for conflicting declarations to include those outside of the eval itself. BUG=v8:4468, v8:4479 ========== to ========== Block-scoped functions in evals are now only conditionally hoisted out. Annex B.3.3 of the spec requires that sloppy-mode block-scoped functions declared by "eval" are hoisted unless doing so would cause an early error (which is to say, conflict with a lexical declaration). This patch amends the check for conflicting declarations to include those outside of the eval itself. BUG=v8:4468, v8:4479 Committed: https://crrev.com/f6c6ae903463327af2a0a8766a15d882a9eb3e93 Cr-Commit-Position: refs/heads/master@{#37783} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/f6c6ae903463327af2a0a8766a15d882a9eb3e93 Cr-Commit-Position: refs/heads/master@{#37783}
Message was sent while issue was closed.
Patchset #4 (id:60001) has been deleted |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
