change most cases of variable redeclaration from TypeError to SyntaxError

change most cases of variable redeclaration from TypeError to SyntaxError.

Code like `let a; eval("var a;");` should throw a SyntaxError, not a TypeError
(this caused a test262 failure.). However, the code `eval("function NaN() {}");`
should actually throw a TypeError. This patch changes most cases of
redeclaration errors from TypeError to SyntaxError. See the test
mjsunit/regress/redeclaration-error-types for a thorough analysis with spec
references.

The relevant sections of the spec are ES#sec-globaldeclarationinstantiation and
ES#sec-evaldeclarationinstantiation

BUG=v8:4955
LOG=y

Committed: https://crrev.com/2b787561763d0f7e8dab698652715a742cf78291
Cr-Commit-Position: refs/heads/master@{#36940}

[jwolfe, 2016-06-07 17:42:02]

Note, there is actually a bug in my code here. I intend to redo this patch.

https://codereview.chromium.org/2048703002/diff/1/src/runtime/runtime-scopes.cc
File src/runtime/runtime-scopes.cc (right):

https://codereview.chromium.org/2048703002/diff/1/src/runtime/runtime-scopes....
src/runtime/runtime-scopes.cc:55: RedeclarationError::kSyntaxError);
15.1.11:6.a: "If envRec.HasLexicalDeclaration(name) is true, throw a SyntaxError
exception."

$ d8 -e 'let a;' -e 'var a;'

https://codereview.chromium.org/2048703002/diff/1/src/runtime/runtime-scopes....
src/runtime/runtime-scopes.cc:68: RedeclarationError::kSyntaxError);
I was unable to get this code to run. It does not seem to be used in the test
suite or in test262.

https://codereview.chromium.org/2048703002/diff/1/src/runtime/runtime-scopes....
src/runtime/runtime-scopes.cc:86: return ThrowRedeclarationError(isolate, name,
redeclaration_error_type);
15.1.11:5.d: "If hasRestrictedGlobal is true, throw a SyntaxError exception."

$ d8 -e 'function NaN() {}'


18.2.1.3:8.a.iv.1.b: "If fnDefinable is false, throw a TypeError exception."

$ d8 -e 'eval("function NaN() {}");'

https://codereview.chromium.org/2048703002/diff/1/src/runtime/runtime-scopes....
src/runtime/runtime-scopes.cc:157: is_var, is_const, is_function,
RedeclarationError::kSyntaxError);
15.1.11:5.d: "If hasRestrictedGlobal is true, throw a SyntaxError exception."

$ d8 -e 'function NaN() {}'

https://codereview.chromium.org/2048703002/diff/1/src/runtime/runtime-scopes....
src/runtime/runtime-scopes.cc:253: RedeclarationError::kSyntaxError);
18.2.1.3:5.a.i.1: "If varEnvRec.HasLexicalDeclaration(name) is true, throw a
SyntaxError exception."

$ d8 -e 'let a; eval("var a;");'


18.2.1.3:5.d.ii.2.a.i: "Throw a SyntaxError exception."

$ d8 -e '(function() { let a; eval("var a;"); })();'

https://codereview.chromium.org/2048703002/diff/1/src/runtime/runtime-scopes....
src/runtime/runtime-scopes.cc:275: RedeclarationError::kTypeError);
18.2.1.3:8.a.iv.1.b: "If fnDefinable is false, throw a TypeError exception."

$ d8 -e 'eval("function NaN() {}");'

https://codereview.chromium.org/2048703002/diff/1/src/runtime/runtime-scopes....
src/runtime/runtime-scopes.cc:281: is_function,
RedeclarationError::kSyntaxError);
I believe, this call can never cause the redeclaration_error_type parameter to
be used. This code runs for functions declared in functions inside an eval, such
as this:

$ d8 -e 'eval("function f() { function b() {var thisDeclarationHere;} } f();");'

Note that the following code actually goes through the call on line 273 to
trigger the redeclaration error:

$ d8 -e 'eval("function f() { function b() {(0, eval)(`function NaN(){}`);} b();
} f();");'

https://codereview.chromium.org/2048703002/diff/1/src/runtime/runtime-scopes....
src/runtime/runtime-scopes.cc:287: is_function,
RedeclarationError::kSyntaxError);
18.2.1.3:8.a.iv.1.b: "If fnDefinable is false, throw a TypeError exception."

$ d8 -e 'let a; try {eval("function a() {}");} catch (e) {} eval("function NaN()
{}");'

(this was the shortest testcase I could find to get this code to run.)

https://codereview.chromium.org/2048703002/diff/1/src/runtime/runtime-scopes....
src/runtime/runtime-scopes.cc:294: RedeclarationError::kSyntaxError);
I was unable to get this code to run. It does not seem to be used in the test
suite or in test262.

https://codereview.chromium.org/2048703002/diff/1/src/runtime/runtime-scopes....
src/runtime/runtime-scopes.cc:634: RedeclarationError::kSyntaxError);
15.1.11:5.b: "If envRec.HasLexicalDeclaration(name) is true, throw a SyntaxError
exception."

$ d8 -e 'let a;' -e 'let a;'

https://codereview.chromium.org/2048703002/diff/1/src/runtime/runtime-scopes....
src/runtime/runtime-scopes.cc:645: RedeclarationError::kSyntaxError);
15.1.11:5.a: "If envRec.HasVarDeclaration(name) is true, throw a SyntaxError
exception."

$ d8 -e 'var a;' -e 'let a;'

[jwolfe, 2016-06-08 01:28:38]

Description was changed from

==========
variable redeclaration should usually be SyntaxError, sometimes TypeError

More information to follow.

BUG=v8:4955
LOG=y
==========

to

==========
variable redeclaration should usually be SyntaxError, sometimes TypeError

More information to follow.

BUG=v8:4955
LOG=y
==========

[jwolfe, 2016-06-08 01:28:38]

jwolfe@igalia.com changed reviewers:
+ adamk@chromium.org

[adamk, 2016-06-09 07:53:40]

Is this ready for review?

[jwolfe, 2016-06-09 08:15:38]

> Is this ready for review?

Yes. Sorry, I left that unclear in a previous comment. This is ready for review
now.

[adamk, 2016-06-09 15:08:42]

Please flesh out the CL description.

https://codereview.chromium.org/2048703002/diff/60001/src/runtime/runtime-sco...
File src/runtime/runtime-scopes.cc (right):

https://codereview.chromium.org/2048703002/diff/60001/src/runtime/runtime-sco...
src/runtime/runtime-scopes.cc:19: struct RedeclarationError {
You can use 'enum class' to get the same thing with slightly less boilerplate.
I'd call this "RedeclarationType", to avoid having to type "Error" twice.

https://codereview.chromium.org/2048703002/diff/60001/src/runtime/runtime-sco...
src/runtime/runtime-scopes.cc:69: if (is_const) {
Did you say this code is unreachable?

https://codereview.chromium.org/2048703002/diff/60001/src/runtime/runtime-sco...
src/runtime/runtime-scopes.cc:261: return ThrowRedeclarationError(isolate, name,
Why no comment here?

https://codereview.chromium.org/2048703002/diff/60001/src/runtime/runtime-sco...
src/runtime/runtime-scopes.cc:303: if (is_const || (attributes & READ_ONLY) !=
0) {
Was this also unreachable?

https://codereview.chromium.org/2048703002/diff/60001/src/runtime/runtime-sco...
src/runtime/runtime-scopes.cc:645: // If envRec.HasLexicalDeclaration(name) is
true, throw a SyntaxError
I also see this line in EvalDeclarationInstantiation, see step 5.a.i.1:
https://tc39.github.io/ecma262/#sec-evaldeclarationinstantiation

Given the test you changed, it's not clear to me which of these bits of the spec
applies.

[adamk, 2016-06-09 15:37:31]

Regarding the dead is_const branches, I've sent out a CL to remove them:
https://codereview.chromium.org/2051073004/

[jwolfe, 2016-06-09 16:57:48]

Description was changed from

==========
variable redeclaration should usually be SyntaxError, sometimes TypeError

More information to follow.

BUG=v8:4955
LOG=y
==========

to

==========
variable redeclaration should usually be a SyntaxError, but sometimes a
TypeError.

I've added spec references in code comments for which type each case is supposed
to be. Below are some test cases to get coverage on all the code in this patch
(except for the dead code).


These are SyntaxErrors:

d8 -e 'let a; eval("var a;");'
d8 -e 'let a; eval("function a() {}");'
d8 -e '(function() { let a; eval("var a;"); })();'
d8 -e '(function() { let a; eval("function a() {}"); })();'
d8 -e 'let a;' -e 'let a;'
d8 -e 'let a;' -e 'var a;'
d8 -e 'var a;' -e 'let a;'
d8 -e 'let NaN;'
d8 -e 'function NaN() {}'

These are TypeErrors:

d8 -e 'eval("function NaN() {}");'
d8 -e 'let a; try {eval("function a() {}");} catch (e) {} eval("function NaN()
{}");'
d8 -e 'eval("function f() { function b() {(0, eval)(`function NaN(){}`);} b(); }
f();");'


BUG=v8:4955
LOG=y
==========

[jwolfe, 2016-06-09 17:05:23]

https://codereview.chromium.org/2048703002/diff/60001/src/runtime/runtime-sco...
File src/runtime/runtime-scopes.cc (right):

https://codereview.chromium.org/2048703002/diff/60001/src/runtime/runtime-sco...
src/runtime/runtime-scopes.cc:261: return ThrowRedeclarationError(isolate, name,
On 2016/06/09 15:08:42, adamk wrote:
> Why no comment here?

Oops. Missed a spot. I've got a comment prepared and everything; just forgot to
include it.

https://codereview.chromium.org/2048703002/diff/60001/src/runtime/runtime-sco...
src/runtime/runtime-scopes.cc:645: // If envRec.HasLexicalDeclaration(name) is
true, throw a SyntaxError
I don't believe this code runs for eval. This code runs for:

d8 -e 'let a;' -e 'let a;'

The test cases I changed and the part of the spec you're referencing are the
line 261 throw above that I forgot to comment:

d8 -e 'let a; eval("var a;");'

[adamk, 2016-06-10 06:52:10]

Please add a new test file and move the test cases there instead of putting them
in the CL description. You can use d8's Realm.eval API to simulate the case of
multiple global scripts.

As an aside,
https://www.chromium.org/developers/contributing-code#TOC-Writing-change-list...
is a nice concise explanation of how Chromium/Blink/V8 change descriptions are
generally structured.

[adamk, 2016-06-10 06:52:45]

Oh, and if you merge to trunk there shouldn't be any dead code left (if there
is, we should try to understand why and possibly delete it).

[jwolfe, 2016-06-10 20:36:06]

i still need to add tests.

https://codereview.chromium.org/2048703002/diff/120001/src/runtime/runtime-sc...
File src/runtime/runtime-scopes.cc (right):

https://codereview.chromium.org/2048703002/diff/120001/src/runtime/runtime-sc...
src/runtime/runtime-scopes.cc:286: RedeclarationType::kSyntaxError);
this branch seems unreachable.

[jwolfe, 2016-06-12 20:37:56]

Description was changed from

==========
variable redeclaration should usually be a SyntaxError, but sometimes a
TypeError.

I've added spec references in code comments for which type each case is supposed
to be. Below are some test cases to get coverage on all the code in this patch
(except for the dead code).


These are SyntaxErrors:

d8 -e 'let a; eval("var a;");'
d8 -e 'let a; eval("function a() {}");'
d8 -e '(function() { let a; eval("var a;"); })();'
d8 -e '(function() { let a; eval("function a() {}"); })();'
d8 -e 'let a;' -e 'let a;'
d8 -e 'let a;' -e 'var a;'
d8 -e 'var a;' -e 'let a;'
d8 -e 'let NaN;'
d8 -e 'function NaN() {}'

These are TypeErrors:

d8 -e 'eval("function NaN() {}");'
d8 -e 'let a; try {eval("function a() {}");} catch (e) {} eval("function NaN()
{}");'
d8 -e 'eval("function f() { function b() {(0, eval)(`function NaN(){}`);} b(); }
f();");'


BUG=v8:4955
LOG=y
==========

to

==========
change most cases of variable redeclaration from TypeError to SyntaxError.

Code like `let a; eval("var a;");` should throw a SyntaxError, not a TypeError
(this caused a test262 failure.). However, the code `eval("function NaN() {}");`
should actually throw a TypeError. This patch changes most cases of
redeclaration errors from TypeError to SyntaxError. See the test
mjsunit/regress/redeclaration-error-types for a thorough analysis with spec
references.

The relevant sections of the spec are ES#sec-globaldeclarationinstantiation and
ES#sec-evaldeclarationinstantiation

BUG=v8:4955
LOG=y
==========

[jwolfe, 2016-06-12 20:38:57]

There is still some dead code in this patch. See my comment on Patch Set 7.

[adamk, 2016-06-13 12:44:27]

Thanks for the test, I like the generality, makes it easy to read.

One more comment on the CL description: please wrap at 80 chars to make git log
happier (and apologies that Rietveld doesn't know how to do that for you).

lgtm besides that!

https://codereview.chromium.org/2048703002/diff/120001/src/runtime/runtime-sc...
File src/runtime/runtime-scopes.cc (right):

https://codereview.chromium.org/2048703002/diff/120001/src/runtime/runtime-sc...
src/runtime/runtime-scopes.cc:286: RedeclarationType::kSyntaxError);
On 2016/06/10 20:36:05, jwolfe wrote:
> this branch seems unreachable.

I'm working on a CL to remove this code, I agree that it's dead.

[jwolfe, 2016-06-13 17:43:03]

Description was changed from

==========
change most cases of variable redeclaration from TypeError to SyntaxError.

Code like `let a; eval("var a;");` should throw a SyntaxError, not a TypeError
(this caused a test262 failure.). However, the code `eval("function NaN() {}");`
should actually throw a TypeError. This patch changes most cases of
redeclaration errors from TypeError to SyntaxError. See the test
mjsunit/regress/redeclaration-error-types for a thorough analysis with spec
references.

The relevant sections of the spec are ES#sec-globaldeclarationinstantiation and
ES#sec-evaldeclarationinstantiation

BUG=v8:4955
LOG=y
==========

to

==========
change most cases of variable redeclaration from TypeError to SyntaxError.

Code like `let a; eval("var a;");` should throw a SyntaxError, not a TypeError
(this caused a test262 failure.). However, the code `eval("function NaN() {}");`
should actually throw a TypeError. This patch changes most cases of
redeclaration errors from TypeError to SyntaxError. See the test
mjsunit/regress/redeclaration-error-types for a thorough analysis with spec
references.

The relevant sections of the spec are ES#sec-globaldeclarationinstantiation and
ES#sec-evaldeclarationinstantiation

BUG=v8:4955
LOG=y
==========

[jwolfe, 2016-06-13 17:45:07]

The CQ bit was checked by jwolfe@igalia.com

[commit-bot: I haz the power, 2016-06-13 17:45:14]

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

[commit-bot: I haz the power, 2016-06-13 18:11:04]

Description was changed from

==========
change most cases of variable redeclaration from TypeError to SyntaxError.

Code like `let a; eval("var a;");` should throw a SyntaxError, not a TypeError
(this caused a test262 failure.). However, the code `eval("function NaN() {}");`
should actually throw a TypeError. This patch changes most cases of
redeclaration errors from TypeError to SyntaxError. See the test
mjsunit/regress/redeclaration-error-types for a thorough analysis with spec
references.

The relevant sections of the spec are ES#sec-globaldeclarationinstantiation and
ES#sec-evaldeclarationinstantiation

BUG=v8:4955
LOG=y
==========

to

==========
change most cases of variable redeclaration from TypeError to SyntaxError.

Code like `let a; eval("var a;");` should throw a SyntaxError, not a TypeError
(this caused a test262 failure.). However, the code `eval("function NaN() {}");`
should actually throw a TypeError. This patch changes most cases of
redeclaration errors from TypeError to SyntaxError. See the test
mjsunit/regress/redeclaration-error-types for a thorough analysis with spec
references.

The relevant sections of the spec are ES#sec-globaldeclarationinstantiation and
ES#sec-evaldeclarationinstantiation

BUG=v8:4955
LOG=y
==========

[commit-bot: I haz the power, 2016-06-13 18:11:05]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-06-13 18:14:51]

Description was changed from

==========
change most cases of variable redeclaration from TypeError to SyntaxError.

Code like `let a; eval("var a;");` should throw a SyntaxError, not a TypeError
(this caused a test262 failure.). However, the code `eval("function NaN() {}");`
should actually throw a TypeError. This patch changes most cases of
redeclaration errors from TypeError to SyntaxError. See the test
mjsunit/regress/redeclaration-error-types for a thorough analysis with spec
references.

The relevant sections of the spec are ES#sec-globaldeclarationinstantiation and
ES#sec-evaldeclarationinstantiation

BUG=v8:4955
LOG=y
==========

to

==========
change most cases of variable redeclaration from TypeError to SyntaxError.

Code like `let a; eval("var a;");` should throw a SyntaxError, not a TypeError
(this caused a test262 failure.). However, the code `eval("function NaN() {}");`
should actually throw a TypeError. This patch changes most cases of
redeclaration errors from TypeError to SyntaxError. See the test
mjsunit/regress/redeclaration-error-types for a thorough analysis with spec
references.

The relevant sections of the spec are ES#sec-globaldeclarationinstantiation and
ES#sec-evaldeclarationinstantiation

BUG=v8:4955
LOG=y

Committed: https://crrev.com/2b787561763d0f7e8dab698652715a742cf78291
Cr-Commit-Position: refs/heads/master@{#36940}
==========

[commit-bot: I haz the power, 2016-06-13 18:14:52]

Patchset 8 (id:??) landed as
https://crrev.com/2b787561763d0f7e8dab698652715a742cf78291
Cr-Commit-Position: refs/heads/master@{#36940}

[Dan Ehrenberg, 2016-06-13 18:21:40]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/2064793002/ by littledan@chromium.org.

The reason for reverting is: This is going to break the LayoutTest
inspector-protocol/console/console-let-const-with-api.html as seen in
https://build.chromium.org/p/tryserver.v8/builders/v8_linux_blink_rel/builds/...
. Please run this test manually, using instructions at
https://www.chromium.org/developers/testing/webkit-layout-tests , and fix on the
Chrome side if needed before resubmitting this patch..

[jwolfe, 2016-06-21 18:00:57]

Description was changed from

==========
change most cases of variable redeclaration from TypeError to SyntaxError.

Code like `let a; eval("var a;");` should throw a SyntaxError, not a TypeError
(this caused a test262 failure.). However, the code `eval("function NaN() {}");`
should actually throw a TypeError. This patch changes most cases of
redeclaration errors from TypeError to SyntaxError. See the test
mjsunit/regress/redeclaration-error-types for a thorough analysis with spec
references.

The relevant sections of the spec are ES#sec-globaldeclarationinstantiation and
ES#sec-evaldeclarationinstantiation

BUG=v8:4955
LOG=y

Committed: https://crrev.com/2b787561763d0f7e8dab698652715a742cf78291
Cr-Commit-Position: refs/heads/master@{#36940}
==========

to

==========
change most cases of variable redeclaration from TypeError to SyntaxError.

Code like `let a; eval("var a;");` should throw a SyntaxError, not a TypeError
(this caused a test262 failure.). However, the code `eval("function NaN() {}");`
should actually throw a TypeError. This patch changes most cases of
redeclaration errors from TypeError to SyntaxError. See the test
mjsunit/regress/redeclaration-error-types for a thorough analysis with spec
references.

The relevant sections of the spec are ES#sec-globaldeclarationinstantiation and
ES#sec-evaldeclarationinstantiation

BUG=v8:4955
LOG=y

Committed: https://crrev.com/2b787561763d0f7e8dab698652715a742cf78291
Cr-Commit-Position: refs/heads/master@{#36940}
==========

[jwolfe, 2016-06-21 18:01:56]

reopening to append a rebase patch, then retry once
https://codereview.chromium.org/2084963002 lands in chromium.

[jwolfe, 2016-06-21 18:28:35]

never mind. i'll make a re-land CL.