[parsing]: eval/arguments parameter names are ok in sloppy mode

[parsing]: eval/arguments parameter names are ok in sloppy mode

BUG=v8:3891
LOG=N
R=arv@chromium.org, marja@chromium.org

Committed: https://crrev.com/663a9eeae401f8d915b3f39d6c184519d46a1fd7
Cr-Commit-Position: refs/heads/master@{#26673}

[caitp (gmail), 2015-02-15 18:53:56]

PTAL

There are a lot of inconsistencies in the spec WRT certain things being errors
under some conditions (especially strict mode), or when the parameter list looks
a certain way.

If IsSimpleParameterList is true, or if FormalParameters are
StrictFormalParameters, then duplicate parameters are not allowed (but, in both
of these cases, in sloppy mode, 'eval' or 'arguments' parameter names are
allowed).

Maybe it would be more sensible to have the rules made simpler in the spec?

[caitp (gmail), 2015-02-15 18:54:36]

On 2015/02/15 18:53:56, caitp wrote:
> If IsSimpleParameterList is true, ....

Er, 'false' not 'true'

[arv (Not doing code reviews), 2015-02-15 21:38:34]

Even though this CL looks good I don't think we want this behavior.

The intent was that if the formal parameters are NOT simple, they should behave
like strict formal parameters. That means, no duplicates, no eval/arguments and
no strict future keywords.

With the current CL the following is valid

function f(...arguments) {
  return arguments;
}

I'm pretty sure that we will have trouble with this as is.

https://codereview.chromium.org/924403002/diff/1/test/cctest/test-parsing.cc
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/924403002/diff/1/test/cctest/test-parsing.cc#...
test/cctest/test-parsing.cc:4945: "...arguments",
Wow, that is terrible. I think this is a spec bug. We should treat
StrictFormalParameters the same as FormalParameters in strict functions.

[caitp (gmail), 2015-02-15 21:43:39]

On 2015/02/15 21:38:34, arv wrote:
> Even though this CL looks good I don't think we want this behavior.
> 
> The intent was that if the formal parameters are NOT simple, they should
behave
> like strict formal parameters. That means, no duplicates, no eval/arguments
and
> no strict future keywords.
> 
> With the current CL the following is valid
> 
> function f(...arguments) {
>   return arguments;
> }
> 
> I'm pretty sure that we will have trouble with this as is.
> 
> https://codereview.chromium.org/924403002/diff/1/test/cctest/test-parsing.cc
> File test/cctest/test-parsing.cc (right):
> 
>
https://codereview.chromium.org/924403002/diff/1/test/cctest/test-parsing.cc#...
> test/cctest/test-parsing.cc:4945: "...arguments",
> Wow, that is terrible. I think this is a spec bug. We should treat
> StrictFormalParameters the same as FormalParameters in strict functions.

Yeah, I think it's fine if this is just raised as a spec bug

[marja, 2015-02-16 09:12:02]

Otherwise lg, but maybe wait until the spec problem is resolved before landing?

https://codereview.chromium.org/924403002/diff/1/src/scopes.h
File src/scopes.h (right):

https://codereview.chromium.org/924403002/diff/1/src/scopes.h#newcode475
src/scopes.h:475: return var != NULL && params_.Contains(var);
Why is it not enough to check if params_.Contains(var)? (Scope::DeclareParameter
puts the variable in params_ and declares it in variables_ too, so these seem to
go together anyway.)

https://codereview.chromium.org/924403002/diff/1/test/cctest/test-parsing.cc
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/924403002/diff/1/test/cctest/test-parsing.cc#...
test/cctest/test-parsing.cc:3978: 
Hmm, do we have tests for duplicate params in test-parsing.cc I couldn't find
any, could you add a test that asserts that duplicate parameters is an error in
the strict mode?

[arv (Not doing code reviews), 2015-02-16 16:24:23]

LGTM with Marja's comments resolved.

Since the ES6 spec bug got closed I think we will just have to do this. I doubt
any end users will suffer from these exceptions.

[caitp (gmail), 2015-02-16 16:42:19]

New patchsets have been uploaded after l-g-t-m from arv@chromium.org

[caitp (gmail), 2015-02-16 16:42:53]

https://codereview.chromium.org/924403002/diff/1/src/scopes.h
File src/scopes.h (right):

https://codereview.chromium.org/924403002/diff/1/src/scopes.h#newcode475
src/scopes.h:475: return var != NULL && params_.Contains(var);
On 2015/02/16 09:12:02, marja wrote:
> Why is it not enough to check if params_.Contains(var)?
(Scope::DeclareParameter
> puts the variable in params_ and declares it in variables_ too, so these seem
to
> go together anyway.)

So just `return params_.Contains(variables_.Lookup(name));` ? Done

https://codereview.chromium.org/924403002/diff/1/test/cctest/test-parsing.cc
File test/cctest/test-parsing.cc (right):

https://codereview.chromium.org/924403002/diff/1/test/cctest/test-parsing.cc#...
test/cctest/test-parsing.cc:3978: 
On 2015/02/16 09:12:02, marja wrote:
> Hmm, do we have tests for duplicate params in test-parsing.cc I couldn't find
> any, could you add a test that asserts that duplicate parameters is an error
in
> the strict mode?

Done.

[marja, 2015-02-16 16:47:35]

lgtm

https://codereview.chromium.org/924403002/diff/1/src/scopes.h
File src/scopes.h (right):

https://codereview.chromium.org/924403002/diff/1/src/scopes.h#newcode475
src/scopes.h:475: return var != NULL && params_.Contains(var);
On 2015/02/16 16:42:53, caitp wrote:
> On 2015/02/16 09:12:02, marja wrote:
> > Why is it not enough to check if params_.Contains(var)?
> (Scope::DeclareParameter
> > puts the variable in params_ and declares it in variables_ too, so these
seem
> to
> > go together anyway.)
> 
> So just `return params_.Contains(variables_.Lookup(name));` ? Done

Assuming it does the right thing :) (As far as I understand this CL, it should
be, right?)

[caitp (gmail), 2015-02-16 16:49:19]

New patchsets have been uploaded after l-g-t-m from marja@chromium.org

[caitp (gmail), 2015-02-16 17:00:38]

On 2015/02/16 16:47:35, marja wrote:
> lgtm
> 
> https://codereview.chromium.org/924403002/diff/1/src/scopes.h
> File src/scopes.h (right):
> 
> https://codereview.chromium.org/924403002/diff/1/src/scopes.h#newcode475
> src/scopes.h:475: return var != NULL && params_.Contains(var);
> On 2015/02/16 16:42:53, caitp wrote:
> > On 2015/02/16 09:12:02, marja wrote:
> > > Why is it not enough to check if params_.Contains(var)?
> > (Scope::DeclareParameter
> > > puts the variable in params_ and declares it in variables_ too, so these
> seem
> > to
> > > go together anyway.)
> > 
> > So just `return params_.Contains(variables_.Lookup(name));` ? Done
> 
> Assuming it does the right thing :) (As far as I understand this CL, it should
> be, right?)

well previously, if Lookup() returned null, it wouldn't search through params_
for a matching null value. Now it does, but (probably) never finds a matching
null.

If there is ever a presence of nulls in the parameter list, it could give false
positives(!), but I don't think that's ever the case.

On that note, it does always perform the O(n) check after the lookup, but these
lists are usually short enough that it probably doesn't matter much.

[marja, 2015-02-16 17:10:51]

On 2015/02/16 17:00:38, caitp wrote:
> On 2015/02/16 16:47:35, marja wrote:
> > lgtm
> > 
> > https://codereview.chromium.org/924403002/diff/1/src/scopes.h
> > File src/scopes.h (right):
> > 
> > https://codereview.chromium.org/924403002/diff/1/src/scopes.h#newcode475
> > src/scopes.h:475: return var != NULL && params_.Contains(var);
> > On 2015/02/16 16:42:53, caitp wrote:
> > > On 2015/02/16 09:12:02, marja wrote:
> > > > Why is it not enough to check if params_.Contains(var)?
> > > (Scope::DeclareParameter
> > > > puts the variable in params_ and declares it in variables_ too, so these
> > seem
> > > to
> > > > go together anyway.)
> > > 
> > > So just `return params_.Contains(variables_.Lookup(name));` ? Done
> > 
> > Assuming it does the right thing :) (As far as I understand this CL, it
should
> > be, right?)
> 
> well previously, if Lookup() returned null, it wouldn't search through params_
> for a matching null value. Now it does, but (probably) never finds a matching
> null.
> 
> If there is ever a presence of nulls in the parameter list, it could give
false
> positives(!), but I don't think that's ever the case.
> 
> On that note, it does always perform the O(n) check after the lookup, but
these
> lists are usually short enough that it probably doesn't matter much.

Yeah, there shouldn't be NULL AstStrings anywhere. And the lists are so short
that it's not trivial to see which lookup is actually faster :) (Probably
doesn't matter for real life.) When I read it the first time was not clear to me
whether the lookup in variables_ can change what that func returns... (which
would be the case if stuff is in params_ but we cannot find it in variables_.)

[caitp (gmail), 2015-02-16 17:55:07]

On 2015/02/16 17:10:51, marja wrote:
> On 2015/02/16 17:00:38, caitp wrote:
> > On 2015/02/16 16:47:35, marja wrote:
> > > lgtm
> > > 
> > > https://codereview.chromium.org/924403002/diff/1/src/scopes.h
> > > File src/scopes.h (right):
> > > 
> > > https://codereview.chromium.org/924403002/diff/1/src/scopes.h#newcode475
> > > src/scopes.h:475: return var != NULL && params_.Contains(var);
> > > On 2015/02/16 16:42:53, caitp wrote:
> > > > On 2015/02/16 09:12:02, marja wrote:
> > > > > Why is it not enough to check if params_.Contains(var)?
> > > > (Scope::DeclareParameter
> > > > > puts the variable in params_ and declares it in variables_ too, so
these
> > > seem
> > > > to
> > > > > go together anyway.)
> > > > 
> > > > So just `return params_.Contains(variables_.Lookup(name));` ? Done
> > > 
> > > Assuming it does the right thing :) (As far as I understand this CL, it
> should
> > > be, right?)
> > 
> > well previously, if Lookup() returned null, it wouldn't search through
params_
> > for a matching null value. Now it does, but (probably) never finds a
matching
> > null.
> > 
> > If there is ever a presence of nulls in the parameter list, it could give
> false
> > positives(!), but I don't think that's ever the case.
> > 
> > On that note, it does always perform the O(n) check after the lookup, but
> these
> > lists are usually short enough that it probably doesn't matter much.
> 
> Yeah, there shouldn't be NULL AstStrings anywhere. And the lists are so short
> that it's not trivial to see which lookup is actually faster :) (Probably
> doesn't matter for real life.) When I read it the first time was not clear to
me
> whether the lookup in variables_ can change what that func returns... (which
> would be the case if stuff is in params_ but we cannot find it in variables_.)

If it turns out to be slightly slower, there should be a slight pickup for the
common case by just not calling Contains() if Lookup() returns null.

I'll CQ I guess, feel free to uncheck if there are any other comments

[caitp (gmail), 2015-02-16 17:55:19]

The CQ bit was checked by caitpotter88@gmail.com

[commit-bot: I haz the power, 2015-02-16 17:55:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/924403002/40001

[commit-bot: I haz the power, 2015-02-16 18:18:10]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-02-16 18:18:28]

Patchset 3 (id:??) landed as
https://crrev.com/663a9eeae401f8d915b3f39d6c184519d46a1fd7
Cr-Commit-Position: refs/heads/master@{#26673}