Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(788)

Issue 2086063002: Reland: change most cases of variable redeclaration from TypeError to SyntaxError. (Closed)

Created:
4 years, 6 months ago by jwolfe
Modified:
4 years, 6 months ago
Reviewers:
adamk
CC:
v8-reviews_googlegroups.com
Base URL:
https://chromium.googlesource.com/v8/v8.git@master
Target Ref:
refs/pending/heads/master
Project:
v8
Visibility:
Public.

Description

Reland: change most cases of variable redeclaration from TypeError to SyntaxError. Reland of https://codereview.chromium.org/2048703002/ 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 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel R=adamk Committed: https://crrev.com/d8147eb98cacfc65a802b94b3c0dc9cf88f546ab Cr-Commit-Position: refs/heads/master@{#37156}

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+204 lines, -26 lines) Patch
M src/runtime/runtime-scopes.cc View 8 chunks +53 lines, -15 lines 0 comments Download
M test/mjsunit/es6/block-eval-var-over-let.js View 4 chunks +6 lines, -6 lines 0 comments Download
A test/mjsunit/regress/redeclaration-error-types.js View 1 chunk +145 lines, -0 lines 0 comments Download
M test/test262/test262.status View 1 chunk +0 lines, -5 lines 0 comments Download

Messages

Total messages: 8 (2 generated)
jwolfe
4 years, 6 months ago (2016-06-21 18:31:04 UTC) #1
jwolfe
still waiting for https://codereview.chromium.org/2084963002 to land...
4 years, 6 months ago (2016-06-21 18:31:55 UTC) #2
adamk
lgtm when Chromium's ready for the patch
4 years, 6 months ago (2016-06-21 18:40:33 UTC) #3
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2086063002/1
4 years, 6 months ago (2016-06-21 19:37:16 UTC) #5
commit-bot: I haz the power
Committed patchset #1 (id:1)
4 years, 6 months ago (2016-06-21 20:18:02 UTC) #6
commit-bot: I haz the power
4 years, 6 months ago (2016-06-21 20:20:14 UTC) #8
Message was sent while issue was closed.
Patchset 1 (id:??) landed as
https://crrev.com/d8147eb98cacfc65a802b94b3c0dc9cf88f546ab
Cr-Commit-Position: refs/heads/master@{#37156}

Powered by Google App Engine
This is Rietveld 408576698