Detect conflicting variable bindings in harmony mode.

Detect conflicting variable bindings in harmony mode.

BUG=
TEST=mjsunit/harmony/block-conflicts.js

Committed: http://code.google.com/p/v8/source/detail?r=9102

[Steven, 2011-08-31 21:33:00]

PTAL

http://codereview.chromium.org/7756014/diff/1/src/parser.cc
File src/parser.cc (right):

http://codereview.chromium.org/7756014/diff/1/src/parser.cc#newcode4108
src/parser.cc:4108: ReportMessage("redeclaration", args);
This ReportMessage should rather be a ReportMessageAt. The proxy does store a
position but what is the correct way to turn it into a Scanner::Location?

[Lasse Reichstein, 2011-09-01 07:34:04]

LGTM

http://codereview.chromium.org/7756014/diff/1/src/ia32/full-codegen-ia32.cc
File src/ia32/full-codegen-ia32.cc (right):

http://codereview.chromium.org/7756014/diff/1/src/ia32/full-codegen-ia32.cc#n...
src/ia32/full-codegen-ia32.cc:2162: // is a strict mode eval call.
How come this invariant isn't shown in the strict_mode_flag to begin with? I.e.,
should strict_mode_flag be able to return anything but kStrictMode in Harmony
mode?

http://codereview.chromium.org/7756014/diff/1/src/parser.cc
File src/parser.cc (right):

http://codereview.chromium.org/7756014/diff/1/src/parser.cc#newcode1355
src/parser.cc:1355: // of var declared variables.
This checks only for a redeclaration in the same scope, right?

http://codereview.chromium.org/7756014/diff/1/src/parser.h
File src/parser.h (right):

http://codereview.chromium.org/7756014/diff/1/src/parser.h#newcode650
src/parser.h:650: // scope over a let binding of the same name.
Be more explicit (define what you mean by "over", or, prefereably, don't say
anything about hoisting - just say what the invalid syntax structure is).
Am I understanding it correctly if the invalid case is:
A var declaration inside a nested scope/block of a scope with a
let(/const/function)-declaration of the same name. I.e., it doesn't check for
redeclaration in the same scope (that's handled inline)?

http://codereview.chromium.org/7756014/diff/1/src/scopes.cc
File src/scopes.cc (right):

http://codereview.chromium.org/7756014/diff/1/src/scopes.cc#newcode479
src/scopes.cc:479: Variable* other_var = scope->variables_.Lookup(name);
This should be able to skip the first scope, where it only finds the decl
variable itself?

http://codereview.chromium.org/7756014/diff/1/src/scopes.cc#newcode480
src/scopes.cc:480: if (other_var != NULL && other_var->mode() == Variable::LET)
{
Is it only LET declarations that can conflict, or any non-VAR?

[Steven, 2011-09-01 15:01:33]

http://codereview.chromium.org/7756014/diff/1/src/ia32/full-codegen-ia32.cc
File src/ia32/full-codegen-ia32.cc (right):

http://codereview.chromium.org/7756014/diff/1/src/ia32/full-codegen-ia32.cc#n...
src/ia32/full-codegen-ia32.cc:2162: // is a strict mode eval call.
In fact, harmony mode should be an extension of the strict mode, so that we
could set that in strict_mode_flag() and assume strict mode semantics
everywhere. However, then we would have to do the same for the parser for
consistency. Andreas and I agreed that we would want to make the impact of new
features on existing functionality as small as possible if it does not
complicate things. Leaking
var declarations from 'eval' is nasty so we would want to disallow that.
On 2011/09/01 07:34:04, Lasse Reichstein wrote:
> How come this invariant isn't shown in the strict_mode_flag to begin with?
I.e.,
> should strict_mode_flag be able to return anything but kStrictMode in Harmony
> mode?

http://codereview.chromium.org/7756014/diff/1/src/parser.cc
File src/parser.cc (right):

http://codereview.chromium.org/7756014/diff/1/src/parser.cc#newcode1355
src/parser.cc:1355: // of var declared variables.
Yes. Clarified it in the comment.
On 2011/09/01 07:34:04, Lasse Reichstein wrote:
> This checks only for a redeclaration in the same scope, right?

http://codereview.chromium.org/7756014/diff/1/src/parser.h
File src/parser.h (right):

http://codereview.chromium.org/7756014/diff/1/src/parser.h#newcode650
src/parser.h:650: // scope over a let binding of the same name.
On 2011/09/01 07:34:04, Lasse Reichstein wrote:
> Be more explicit (define what you mean by "over", or, prefereably, don't say
> anything about hoisting - just say what the invalid syntax structure is).
> Am I understanding it correctly if the invalid case is:
> A var declaration inside a nested scope/block of a scope with a
> let(/const/function)-declaration of the same name. I.e., it doesn't check for
> redeclaration in the same scope (that's handled inline)?

Done.

http://codereview.chromium.org/7756014/diff/1/src/scopes.cc
File src/scopes.cc (right):

http://codereview.chromium.org/7756014/diff/1/src/scopes.cc#newcode479
src/scopes.cc:479: Variable* other_var = scope->variables_.Lookup(name);
When by first scope you mean the innermost scope where the declaration is
'physically' located, then no, that one should not be skipped. However, the code
as of now skips the outermost scope (function/eval/global). I will add it for
consistency, even though the parser already checks for that case.
On 2011/09/01 07:34:04, Lasse Reichstein wrote:
> This should be able to skip the first scope, where it only finds the decl
> variable itself?

http://codereview.chromium.org/7756014/diff/1/src/scopes.cc#newcode480
src/scopes.cc:480: if (other_var != NULL && other_var->mode() == Variable::LET)
{
Yes any non-VAR. Will fix it.
On 2011/09/01 07:34:04, Lasse Reichstein wrote:
> Is it only LET declarations that can conflict, or any non-VAR?