[tests] add tests for "yield" as function name

[tests] add tests for "yield" as function name

BUG=v8:3983
LOG=N
R=wingo@igalia.com, littledan@chromium.org

Committed: https://crrev.com/cacc6108d919a7ad21fa95c62e010e84a42b3b9d
Cr-Commit-Position: refs/heads/master@{#32682}

[caitp (gmail), 2015-11-12 01:15:36]

caitpotter88@gmail.com changed reviewers:
+ littledan@chromium.org, wingo@igalia.com

[caitp (gmail), 2015-11-12 01:15:36]

Fixups for function name stuff, fixes a few more TC39 tests.

There were comments in the source about let-bound NFEs (which, in code, refers
to bindings within the NFE's own body) being used for certain things --- but as
is, it's never possible to use "yield" in a case where it would cause problems
here ("yield" can't be used as the name of a GeneratorExpression, and can't be
used as the name of a function within a Generator --- so no harm here, afaict).

[caitp (gmail), 2015-11-12 01:15:53]

Description was changed from

==========
[parser] allow "yield" for BindingIdentifier in FunctionExpression

This does not allow "yield" as a BindingIdentifier in GeneratorExpression,
and does not allow this in strict mode, as this is explicitly forbidden in the
spec.

BUG=v8:3983
LOG=N
R=
==========

to

==========
[parser] allow "yield" for BindingIdentifier in FunctionExpression

This does not allow "yield" as a BindingIdentifier in GeneratorExpression,
and does not allow this in strict mode, as this is explicitly forbidden in the
spec.

BUG=v8:3983
LOG=N
R=wingo@igalia.com, littledan@chromium.org
==========

[Dan Ehrenberg, 2015-11-12 22:22:45]

Code looks basically good, but this could use some more testing. Could you add
tests that yield creates errors when used as an identifier within a generator in
both strict and sloppy mode (rather than just delete tests which are now
invalid)? A few messages tests would also be nice (corresponding to the various
codepaths, and confirming the error message), as well as new cctest/test-parsing
tests (confirming that the parser and preparser both get it right).

Looks like this implementation will make a kUnexpectedStrictReserved error for
yield. This would be a little counterintuitive for developers in sloppy mode.
How hard would it be to make a better message? When organizing the code, think
forward to how the same pattern will be used for a misplaced 'await' in future
ES versions.

[caitp (gmail), 2015-11-12 22:24:07]

On 2015/11/12 22:22:45, Dan Ehrenberg wrote:
> Code looks basically good, but this could use some more testing. Could you add
> tests that yield creates errors when used as an identifier within a generator
in
> both strict and sloppy mode (rather than just delete tests which are now
> invalid)? A few messages tests would also be nice (corresponding to the
various
> codepaths, and confirming the error message), as well as new
cctest/test-parsing
> tests (confirming that the parser and preparser both get it right).
> 
> Looks like this implementation will make a kUnexpectedStrictReserved error for
> yield. This would be a little counterintuitive for developers in sloppy mode.
> How hard would it be to make a better message? When organizing the code, think
> forward to how the same pattern will be used for a misplaced 'await' in future
> ES versions.

It makes kUnexpectedStrictReserved word in strict mode, it's
kUnexpectedIdentifier in sloppy mode. I'll add some message tests

[caitp (gmail), 2015-11-12 22:25:09]

On 2015/11/12 22:24:07, caitp wrote:
> On 2015/11/12 22:22:45, Dan Ehrenberg wrote:
> > Code looks basically good, but this could use some more testing. Could you
add
> > tests that yield creates errors when used as an identifier within a
generator
> in
> > both strict and sloppy mode (rather than just delete tests which are now
> > invalid)? A few messages tests would also be nice (corresponding to the
> various
> > codepaths, and confirming the error message), as well as new
> cctest/test-parsing
> > tests (confirming that the parser and preparser both get it right).
> > 
> > Looks like this implementation will make a kUnexpectedStrictReserved error
for
> > yield. This would be a little counterintuitive for developers in sloppy
mode.
> > How hard would it be to make a better message? When organizing the code,
think
> > forward to how the same pattern will be used for a misplaced 'await' in
future
> > ES versions.
> 
> It makes kUnexpectedStrictReserved word in strict mode, it's
> kUnexpectedIdentifier in sloppy mode. I'll add some message tests

kUnexpectedTokenIdentifier*

[caitp (gmail), 2015-12-04 18:30:10]

On 2015/11/12 22:25:09, caitp wrote:
> On 2015/11/12 22:24:07, caitp wrote:
> > On 2015/11/12 22:22:45, Dan Ehrenberg wrote:
> > > Code looks basically good, but this could use some more testing. Could you
> add
> > > tests that yield creates errors when used as an identifier within a
> generator
> > in
> > > both strict and sloppy mode (rather than just delete tests which are now
> > > invalid)? A few messages tests would also be nice (corresponding to the
> > various
> > > codepaths, and confirming the error message), as well as new
> > cctest/test-parsing
> > > tests (confirming that the parser and preparser both get it right).
> > > 
> > > Looks like this implementation will make a kUnexpectedStrictReserved error
> for
> > > yield. This would be a little counterintuitive for developers in sloppy
> mode.
> > > How hard would it be to make a better message? When organizing the code,
> think
> > > forward to how the same pattern will be used for a misplaced 'await' in
> future
> > > ES versions.
> > 
> > It makes kUnexpectedStrictReserved word in strict mode, it's
> > kUnexpectedIdentifier in sloppy mode. I'll add some message tests
> 
> kUnexpectedTokenIdentifier*

Dan, is there anything else needed here?

[caitp (gmail), 2015-12-04 18:33:20]

On 2015/12/04 18:30:10, caitp wrote:
> On 2015/11/12 22:25:09, caitp wrote:
> > On 2015/11/12 22:24:07, caitp wrote:
> > > On 2015/11/12 22:22:45, Dan Ehrenberg wrote:
> > > > Code looks basically good, but this could use some more testing. Could
you
> > add
> > > > tests that yield creates errors when used as an identifier within a
> > generator
> > > in
> > > > both strict and sloppy mode (rather than just delete tests which are now
> > > > invalid)? A few messages tests would also be nice (corresponding to the
> > > various
> > > > codepaths, and confirming the error message), as well as new
> > > cctest/test-parsing
> > > > tests (confirming that the parser and preparser both get it right).
> > > > 
> > > > Looks like this implementation will make a kUnexpectedStrictReserved
error
> > for
> > > > yield. This would be a little counterintuitive for developers in sloppy
> > mode.
> > > > How hard would it be to make a better message? When organizing the code,
> > think
> > > > forward to how the same pattern will be used for a misplaced 'await' in
> > future
> > > > ES versions.
> > > 
> > > It makes kUnexpectedStrictReserved word in strict mode, it's
> > > kUnexpectedIdentifier in sloppy mode. I'll add some message tests
> > 
> > kUnexpectedTokenIdentifier*
> 
> Dan, is there anything else needed here?

It looks like an alternative version of this CL had already landed and I never
heard about it, so that's nice

[Dan Ehrenberg, 2015-12-04 18:35:25]

On 2015/12/04 at 18:30:10, caitpotter88 wrote:
> On 2015/11/12 22:25:09, caitp wrote:
> > On 2015/11/12 22:24:07, caitp wrote:
> > > On 2015/11/12 22:22:45, Dan Ehrenberg wrote:
> > > > Code looks basically good, but this could use some more testing. Could
you
> > add
> > > > tests that yield creates errors when used as an identifier within a
> > generator
> > > in
> > > > both strict and sloppy mode (rather than just delete tests which are now
> > > > invalid)? A few messages tests would also be nice (corresponding to the
> > > various
> > > > codepaths, and confirming the error message), as well as new
> > > cctest/test-parsing
> > > > tests (confirming that the parser and preparser both get it right).
> > > > 
> > > > Looks like this implementation will make a kUnexpectedStrictReserved
error
> > for
> > > > yield. This would be a little counterintuitive for developers in sloppy
> > mode.
> > > > How hard would it be to make a better message? When organizing the code,
> > think
> > > > forward to how the same pattern will be used for a misplaced 'await' in
> > future
> > > > ES versions.
> > > 
> > > It makes kUnexpectedStrictReserved word in strict mode, it's
> > > kUnexpectedIdentifier in sloppy mode. I'll add some message tests
> > 
> > kUnexpectedTokenIdentifier*
> 
> Dan, is there anything else needed here?

Oh, I'm sorry, I didn't see the newly updated version. Next time, so that an
email so sent out, could you make a reply in codereview.chromium.org? It doesn't
send an email when you upload a version, only when you make comments.

Adam has separately fixed this bug, but the tests should still be useful. If you
want to put the tests into a separate patch, that could be submitted. Or is
there any change in behavior between your version and his?

[caitp (gmail), 2015-12-04 18:36:24]

On 2015/12/04 18:35:25, Dan Ehrenberg wrote:
> On 2015/12/04 at 18:30:10, caitpotter88 wrote:
> > On 2015/11/12 22:25:09, caitp wrote:
> > > On 2015/11/12 22:24:07, caitp wrote:
> > > > On 2015/11/12 22:22:45, Dan Ehrenberg wrote:
> > > > > Code looks basically good, but this could use some more testing. Could
> you
> > > add
> > > > > tests that yield creates errors when used as an identifier within a
> > > generator
> > > > in
> > > > > both strict and sloppy mode (rather than just delete tests which are
now
> > > > > invalid)? A few messages tests would also be nice (corresponding to
the
> > > > various
> > > > > codepaths, and confirming the error message), as well as new
> > > > cctest/test-parsing
> > > > > tests (confirming that the parser and preparser both get it right).
> > > > > 
> > > > > Looks like this implementation will make a kUnexpectedStrictReserved
> error
> > > for
> > > > > yield. This would be a little counterintuitive for developers in
sloppy
> > > mode.
> > > > > How hard would it be to make a better message? When organizing the
code,
> > > think
> > > > > forward to how the same pattern will be used for a misplaced 'await'
in
> > > future
> > > > > ES versions.
> > > > 
> > > > It makes kUnexpectedStrictReserved word in strict mode, it's
> > > > kUnexpectedIdentifier in sloppy mode. I'll add some message tests
> > > 
> > > kUnexpectedTokenIdentifier*
> > 
> > Dan, is there anything else needed here?
> 
> Oh, I'm sorry, I didn't see the newly updated version. Next time, so that an
> email so sent out, could you make a reply in codereview.chromium.org? It
doesn't
> send an email when you upload a version, only when you make comments.
> 
> Adam has separately fixed this bug, but the tests should still be useful. If
you
> want to put the tests into a separate patch, that could be submitted. Or is
> there any change in behavior between your version and his?

A bit more looking makes it look like the separate fix is not actually correct,
so I think there is still more here

[Dan Ehrenberg, 2015-12-04 18:40:52]

On 2015/12/04 at 18:36:24, caitpotter88 wrote:
> On 2015/12/04 18:35:25, Dan Ehrenberg wrote:
> > On 2015/12/04 at 18:30:10, caitpotter88 wrote:
> > > On 2015/11/12 22:25:09, caitp wrote:
> > > > On 2015/11/12 22:24:07, caitp wrote:
> > > > > On 2015/11/12 22:22:45, Dan Ehrenberg wrote:
> > > > > > Code looks basically good, but this could use some more testing.
Could
> > you
> > > > add
> > > > > > tests that yield creates errors when used as an identifier within a
> > > > generator
> > > > > in
> > > > > > both strict and sloppy mode (rather than just delete tests which are
now
> > > > > > invalid)? A few messages tests would also be nice (corresponding to
the
> > > > > various
> > > > > > codepaths, and confirming the error message), as well as new
> > > > > cctest/test-parsing
> > > > > > tests (confirming that the parser and preparser both get it right).
> > > > > > 
> > > > > > Looks like this implementation will make a kUnexpectedStrictReserved
> > error
> > > > for
> > > > > > yield. This would be a little counterintuitive for developers in
sloppy
> > > > mode.
> > > > > > How hard would it be to make a better message? When organizing the
code,
> > > > think
> > > > > > forward to how the same pattern will be used for a misplaced 'await'
in
> > > > future
> > > > > > ES versions.
> > > > > 
> > > > > It makes kUnexpectedStrictReserved word in strict mode, it's
> > > > > kUnexpectedIdentifier in sloppy mode. I'll add some message tests
> > > > 
> > > > kUnexpectedTokenIdentifier*
> > > 
> > > Dan, is there anything else needed here?
> > 
> > Oh, I'm sorry, I didn't see the newly updated version. Next time, so that an
> > email so sent out, could you make a reply in codereview.chromium.org? It
doesn't
> > send an email when you upload a version, only when you make comments.
> > 
> > Adam has separately fixed this bug, but the tests should still be useful. If
you
> > want to put the tests into a separate patch, that could be submitted. Or is
> > there any change in behavior between your version and his?
> 
> A bit more looking makes it look like the separate fix is not actually
correct, so I think there is still more here

Oh OK, nice catch, I'd be happy to review this patch rebased on HEAD.

[caitp (gmail), 2015-12-04 18:59:31]

Rebased --- so the only "error" is that the other fix didn't disallow "yield" in
strict mode in non-generators, which does not quite match the spec (apparently).
I can't find where this logic is in the grammar, but per the note in
http://tc39.github.io/ecma262/#sec-identifiers-runtime-semantics-evaluation,
strict code can never have "yield" as an identifier.

Whether that's something V8 wants to forbid or not, that's up to you guys. If
only the tests are wanted, I'll remove those changes and the tests that verify
them

[Dan Ehrenberg, 2015-12-04 19:24:33]

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

[caitp (gmail), 2015-12-04 20:50:28]

On 2015/12/04 18:59:31, caitp wrote:
> Rebased --- so the only "error" is that the other fix didn't disallow "yield"
in
> strict mode in non-generators, which does not quite match the spec
(apparently).
> I can't find where this logic is in the grammar, but per the note in
> http://tc39.github.io/ecma262/#sec-identifiers-runtime-semantics-evaluation,
> strict code can never have "yield" as an identifier.
> 
> Whether that's something V8 wants to forbid or not, that's up to you guys. If
> only the tests are wanted, I'll remove those changes and the tests that verify
> them

Oh, the spec section detailing this is
http://tc39.github.io/ecma262/#sec-identifiers-static-semantics-early-errors

[adamk, 2015-12-08 01:51:09]

I applied this patch locally, removed the changes to the parser, and ran your
tests. They all pass.

The conditional in ParseIdentifierOrStrictReservedWord is:

   ...
 } else if (... || (next == Token::YIELD && !is_generator)) {
    *is_strict_reserved = true;
 } else {
    ReportUnexpectedToken();
 }

So when the token is YIELD, it's either disallowed outright (by hitting the else
clause when is_generator is true) or marked as a strict reserved word (if
is_generator is false). If we're in strict mode, then the strict reserved word
check kicks in later in CheckFunctionName(). I think all this patch does is move
the error checking a little earlier for an NFE in strict mode.

Am I missing something?

[caitp (gmail), 2015-12-08 01:52:11]

On 2015/12/08 01:51:09, adamk wrote:
> I applied this patch locally, removed the changes to the parser, and ran your
> tests. They all pass.
> 
> The conditional in ParseIdentifierOrStrictReservedWord is:
> 
>    ...
>  } else if (... || (next == Token::YIELD && !is_generator)) {
>     *is_strict_reserved = true;
>  } else {
>     ReportUnexpectedToken();
>  }
> 
> So when the token is YIELD, it's either disallowed outright (by hitting the
else
> clause when is_generator is true) or marked as a strict reserved word (if
> is_generator is false). If we're in strict mode, then the strict reserved word
> check kicks in later in CheckFunctionName(). I think all this patch does is
move
> the error checking a little earlier for an NFE in strict mode.
> 
> Am I missing something?

If tests pass, then cool

[adamk, 2015-12-08 02:00:49]

On 2015/12/08 01:52:11, caitp wrote:
> On 2015/12/08 01:51:09, adamk wrote:
> > I applied this patch locally, removed the changes to the parser, and ran
your
> > tests. They all pass.
> > 
> > The conditional in ParseIdentifierOrStrictReservedWord is:
> > 
> >    ...
> >  } else if (... || (next == Token::YIELD && !is_generator)) {
> >     *is_strict_reserved = true;
> >  } else {
> >     ReportUnexpectedToken();
> >  }
> > 
> > So when the token is YIELD, it's either disallowed outright (by hitting the
> else
> > clause when is_generator is true) or marked as a strict reserved word (if
> > is_generator is false). If we're in strict mode, then the strict reserved
word
> > check kicks in later in CheckFunctionName(). I think all this patch does is
> move
> > the error checking a little earlier for an NFE in strict mode.
> > 
> > Am I missing something?
> 
> If tests pass, then cool

Okay, sounds good. And sorry I didn't see this patch when putting my own fix
together; I saw that the bug was "Available" so I picked it up.

[caitp (gmail), 2015-12-08 02:07:48]

Description was changed from

==========
[parser] allow "yield" for BindingIdentifier in FunctionExpression

This does not allow "yield" as a BindingIdentifier in GeneratorExpression,
and does not allow this in strict mode, as this is explicitly forbidden in the
spec.

BUG=v8:3983
LOG=N
R=wingo@igalia.com, littledan@chromium.org
==========

to

==========
[tests] add tests for "yield" as function name

BUG=v8:3983
LOG=N
R=wingo@igalia.com, littledan@chromium.org
==========

[caitp (gmail), 2015-12-08 02:10:57]

On 2015/12/08 02:00:49, adamk wrote:
> On 2015/12/08 01:52:11, caitp wrote:
> > On 2015/12/08 01:51:09, adamk wrote:
> > > I applied this patch locally, removed the changes to the parser, and ran
> your
> > > tests. They all pass.
> > > 
> > > The conditional in ParseIdentifierOrStrictReservedWord is:
> > > 
> > >    ...
> > >  } else if (... || (next == Token::YIELD && !is_generator)) {
> > >     *is_strict_reserved = true;
> > >  } else {
> > >     ReportUnexpectedToken();
> > >  }
> > > 
> > > So when the token is YIELD, it's either disallowed outright (by hitting
the
> > else
> > > clause when is_generator is true) or marked as a strict reserved word (if
> > > is_generator is false). If we're in strict mode, then the strict reserved
> word
> > > check kicks in later in CheckFunctionName(). I think all this patch does
is
> > move
> > > the error checking a little earlier for an NFE in strict mode.
> > > 
> > > Am I missing something?
> > 
> > If tests pass, then cool
> 
> Okay, sounds good. And sorry I didn't see this patch when putting my own fix
> together; I saw that the bug was "Available" so I picked it up.

its alright --- anyways, if the message tests are wanted, PS is now just those.
otherwise i'll close the CL

[adamk, 2015-12-08 02:26:11]

lgtm

Happy to have more message test coverage

[caitp (gmail), 2015-12-08 14:16:31]

The CQ bit was checked by caitpotter88@gmail.com

[commit-bot: I haz the power, 2015-12-08 14:16:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1437803004/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1437803004/80001

[commit-bot: I haz the power, 2015-12-08 15:04:19]

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

[commit-bot: I haz the power, 2015-12-08 15:04:22]

Try jobs failed on following builders:
  v8_linux_arm_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel/builds/11153)
  v8_linux_dbg on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg/builds/11764)

[caitp (gmail), 2015-12-08 15:42:02]

On 2015/12/08 15:04:22, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   v8_linux_arm_rel on tryserver.v8 (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel/builds/11153)
>   v8_linux_dbg on tryserver.v8 (JOB_FAILED,
> http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg/builds/11764)

hmm, related to the swarming bots thing?

[caitp (gmail), 2015-12-08 19:09:30]

The CQ bit was checked by caitpotter88@gmail.com

[commit-bot: I haz the power, 2015-12-08 19:09:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1437803004/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1437803004/80001

[commit-bot: I haz the power, 2015-12-08 20:40:42]

Description was changed from

==========
[tests] add tests for "yield" as function name

BUG=v8:3983
LOG=N
R=wingo@igalia.com, littledan@chromium.org
==========

to

==========
[tests] add tests for "yield" as function name

BUG=v8:3983
LOG=N
R=wingo@igalia.com, littledan@chromium.org
==========

[commit-bot: I haz the power, 2015-12-08 20:40:43]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-12-08 20:41:38]

Description was changed from

==========
[tests] add tests for "yield" as function name

BUG=v8:3983
LOG=N
R=wingo@igalia.com, littledan@chromium.org
==========

to

==========
[tests] add tests for "yield" as function name

BUG=v8:3983
LOG=N
R=wingo@igalia.com, littledan@chromium.org

Committed: https://crrev.com/cacc6108d919a7ad21fa95c62e010e84a42b3b9d
Cr-Commit-Position: refs/heads/master@{#32682}
==========

[commit-bot: I haz the power, 2015-12-08 20:41:38]

Patchset 5 (id:??) landed as
https://crrev.com/cacc6108d919a7ad21fa95c62e010e84a42b3b9d
Cr-Commit-Position: refs/heads/master@{#32682}