Add errors for declarations which conflict with catch parameters.

Add errors for declarations which conflict with catch parameters.

Catch parameters are largely treated as lexical declarations in the
block which contains their body for the purposes of early syntax errors,
with some exceptions outlined in B.3.5. This patch introduces most of
those errors, except those from `eval('for (var e of ...);')` inside of
a catch with a simple parameter named 'e'.

Note that annex B.3.5 allows var declarations to conflict with simple
catch parameters, except when the variable declaration is the init of a
for-of statement.

BUG=v8:5112, v8:4231
Committed: https://crrev.com/2907c726b2bb5cf20b2bec639ca9e6a521585406
Cr-Commit-Position: refs/heads/master@{#37462}

[bakkot, 2016-06-28 22:09:40]

Description was changed from

==========
Add errors for declarations which conflict with catch parameters.

Catch parameters are largely treated as lexical declarations in the
block which contains their body for the purposes of early syntax errors,
with some exceptions outlined in B.3.3. This patch introduces most of
those errors, except those from `eval('for (var e of ...);')` inside of
a catch with a simple parameter named 'e'.

BUG=v8:5112,v8:4231
==========

to

==========
Add errors for declarations which conflict with catch parameters.

Catch parameters are largely treated as lexical declarations in the
block which contains their body for the purposes of early syntax errors,
with some exceptions outlined in B.3.5. This patch introduces most of
those errors, except those from `eval('for (var e of ...);')` inside of
a catch with a simple parameter named 'e'.

Note that annex B.3.5 allows var declarations to conflict with simple
catch parameters, except when the variable declaration is the init of a
for-of statement.

BUG=v8:5112,v8:4231
==========

[bakkot, 2016-06-28 22:09:40]

bakkot@google.com changed reviewers:
+ littledan@chromium.org

[bakkot, 2016-06-29 18:31:25]

The CQ bit was checked by bakkot@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-29 18:31:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-29 18:34:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-29 18:34:26]

Dry run: Try jobs failed on following builders:
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/8274)
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/8251)

[bakkot, 2016-06-29 19:17:43]

The CQ bit was checked by bakkot@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-29 19:17:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-29 19:35:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-29 19:35:53]

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...)

[Dan Ehrenberg, 2016-06-29 20:24:32]

https://codereview.chromium.org/2109733003/diff/1/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/2109733003/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:3643: // special case for legacy for (var/const x =....
in)
It's only var at this point, right? Could you change the unrelated comment which
happens to be adjacent to your change?

https://codereview.chromium.org/2109733003/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:3723: parsing_result.descriptor.mode == VariableMode::VAR)
Minor cleanup suggestion: since it occurs multiple times, make a boolean
variable for this special case.

[bakkot, 2016-06-29 21:10:14]

https://codereview.chromium.org/2109733003/diff/1/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/2109733003/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:3643: // special case for legacy for (var/const x =....
in)
On 2016/06/29 at 20:24:32, Dan Ehrenberg wrote:
> It's only var at this point, right? Could you change the unrelated comment
which happens to be adjacent to your change?

Done, and in a few other places.

https://codereview.chromium.org/2109733003/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:3723: parsing_result.descriptor.mode == VariableMode::VAR)
On 2016/06/29 at 20:24:32, Dan Ehrenberg wrote:
> Minor cleanup suggestion: since it occurs multiple times, make a boolean
variable for this special case.

Done.

[bakkot, 2016-06-29 21:10:21]

The CQ bit was checked by bakkot@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-29 21:10:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bakkot, 2016-06-29 21:12:35]

This probably shouldn't be committed until after
https://codereview.chromium.org/2110193002/, which touches some of the same code
and is higher priority.

[Dan Ehrenberg, 2016-06-29 21:37:48]

lgtm

Thanks for the cleanup; I trust you to do the rebase when appropriate.

[commit-bot: I haz the power, 2016-06-29 21:37:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-29 21:37:56]

Dry run: This issue passed the CQ dry run.

[bakkot, 2016-06-30 17:19:51]

The CQ bit was checked by bakkot@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-30 17:20:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-30 17:39:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-30 17:39:37]

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_avx2_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/4305)
  v8_mac_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng_triggered/bui...)

[bakkot, 2016-06-30 20:52:20]

The CQ bit was checked by bakkot@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-30 20:52:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-30 21:17:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-30 21:17:43]

Dry run: This issue passed the CQ dry run.

[bakkot, 2016-06-30 21:32:00]

Nontrivial changes since last lgtm (Adam's patch changed some semantics); PTAL.

[Dan Ehrenberg, 2016-06-30 22:11:18]

lgtm

Unfortunate, but makes sense. I wonder if it would've been OK to just use the
location of the catch params rather than all the logic to find the declaration,
but it's not too much and seems like a better error message.

[Dan Ehrenberg, 2016-06-30 22:12:09]

By the way, any idea what's behind the failing Blink test
http/tests/media/video-seek-to-middle.html ? I didn't see anything obvious.
Should probably be figured out before submitting.

[Dan Ehrenberg, 2016-06-30 23:10:14]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-30 23:10:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-30 23:12:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-30 23:12:33]

Dry run: This issue passed the CQ dry run.

[bakkot, 2016-06-30 23:58:14]

The CQ bit was checked by bakkot@google.com

[commit-bot: I haz the power, 2016-06-30 23:58:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-01 00:00:48]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-07-01 00:01:42]

Description was changed from

==========
Add errors for declarations which conflict with catch parameters.

Catch parameters are largely treated as lexical declarations in the
block which contains their body for the purposes of early syntax errors,
with some exceptions outlined in B.3.5. This patch introduces most of
those errors, except those from `eval('for (var e of ...);')` inside of
a catch with a simple parameter named 'e'.

Note that annex B.3.5 allows var declarations to conflict with simple
catch parameters, except when the variable declaration is the init of a
for-of statement.

BUG=v8:5112,v8:4231
==========

to

==========
Add errors for declarations which conflict with catch parameters.

Catch parameters are largely treated as lexical declarations in the
block which contains their body for the purposes of early syntax errors,
with some exceptions outlined in B.3.5. This patch introduces most of
those errors, except those from `eval('for (var e of ...);')` inside of
a catch with a simple parameter named 'e'.

Note that annex B.3.5 allows var declarations to conflict with simple
catch parameters, except when the variable declaration is the init of a
for-of statement.

BUG=v8:5112,v8:4231

Committed: https://crrev.com/2907c726b2bb5cf20b2bec639ca9e6a521585406
Cr-Commit-Position: refs/heads/master@{#37462}
==========

[commit-bot: I haz the power, 2016-07-01 00:01:43]

Patchset 6 (id:??) landed as
https://crrev.com/2907c726b2bb5cf20b2bec639ca9e6a521585406
Cr-Commit-Position: refs/heads/master@{#37462}

[bakkot, 2016-07-01 04:21:34]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/2112223002/ by bakkot@google.com.

The reason for reverting is: Fuzzer claims it causes a crash..

[bakkot, 2016-07-01 18:58:52]

Fixed the bug found by the fuzzer (was using the wrong index in a nested loop).

[adamk, 2016-07-01 19:14:03]

adamk@chromium.org changed reviewers:
+ adamk@chromium.org

[adamk, 2016-07-01 19:14:06]

Thanks for the fix, but I've got some comments on other parts of the CL (sorry I
didn't take another pass on the original version).

https://codereview.chromium.org/2109733003/diff/120001/src/ast/scopes.cc
File src/ast/scopes.cc (right):

https://codereview.chromium.org/2109733003/diff/120001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:613: Declaration* Scope::CheckLexDeclarationsConflictingWith(
Is this only called on block scopes? Can you add a DCHECK to that effect if it's
true?

https://codereview.chromium.org/2109733003/diff/120001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:615: int length = names->length();
Nit: should be no need to hoist this.

https://codereview.chromium.org/2109733003/diff/120001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:618: if (var != nullptr && IsLexicalVariableMode(var->mode()))
{
If my DCHECK suggestion above is correct, then can IsLexicalVariableMode ever be
false here?

https://codereview.chromium.org/2109733003/diff/120001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:621: int decls_length = decls_.length();
Again, no need to hoist this out of the loop.

https://codereview.chromium.org/2109733003/diff/120001/src/ast/scopes.h
File src/ast/scopes.h (right):

https://codereview.chromium.org/2109733003/diff/120001/src/ast/scopes.h#newco...
src/ast/scopes.h:245: ZoneList<const AstRawString*>* names);
Style: given that this arg isn't mutated by the callee, it ought to be a
 const reference (that is, "const ZoneList<const AstRawString*>&").

https://codereview.chromium.org/2109733003/diff/120001/src/ast/scopes.h#newco...
src/ast/scopes.h:502: AstRawString* catch_variable_name() const {
this should return a "const AstRawString*", non-const AstRawStrings really
shouldn't exist (and only do because our hashmap is not typed enough).

https://codereview.chromium.org/2109733003/diff/120001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/2109733003/diff/120001/src/parsing/parser.cc#...
src/parsing/parser.cc:3068: position == kNoSourcePosition
When does this hold?