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

Review-Url: https://codereview.chromium.org/2086063002
Cr-Commit-Position: refs/heads/master@{#37156}

src/runtime/runtime-scopes.cc

@@ -16,10 +16,18 @@
 namespace v8 {
 namespace internal {
 
-static Object* ThrowRedeclarationError(Isolate* isolate, Handle<String> name) {
+enum class RedeclarationType { kSyntaxError = 0, kTypeError = 1 };
+
+static Object* ThrowRedeclarationError(Isolate* isolate, Handle<String> name,
+                                       RedeclarationType redeclaration_type) {
   HandleScope scope(isolate);
-  THROW_NEW_ERROR_RETURN_FAILURE(
-      isolate, NewTypeError(MessageTemplate::kVarRedeclaration, name));
+  if (redeclaration_type == RedeclarationType::kSyntaxError) {
+    THROW_NEW_ERROR_RETURN_FAILURE(
+        isolate, NewSyntaxError(MessageTemplate::kVarRedeclaration, name));
+  } else {
+    THROW_NEW_ERROR_RETURN_FAILURE(
+        isolate, NewTypeError(MessageTemplate::kVarRedeclaration, name));
+  }
 }
 
 
@@ -34,13 +42,18 @@ RUNTIME_FUNCTION(Runtime_ThrowConstAssignError) {
 static Object* DeclareGlobals(Isolate* isolate, Handle<JSGlobalObject> global,
                               Handle<String> name, Handle<Object> value,
                               PropertyAttributes attr, bool is_var,
-                              bool is_function) {
+                              bool is_function,
+                              RedeclarationType redeclaration_type) {
   Handle<ScriptContextTable> script_contexts(
       global->native_context()->script_context_table());
   ScriptContextTable::LookupResult lookup;
   if (ScriptContextTable::Lookup(script_contexts, name, &lookup) &&
       IsLexicalVariableMode(lookup.mode)) {
-    return ThrowRedeclarationError(isolate, name);
+    // ES#sec-globaldeclarationinstantiation 6.a:
+    // If envRec.HasLexicalDeclaration(name) is true, throw a SyntaxError
+    // exception.
+    return ThrowRedeclarationError(isolate, name,
+                                   RedeclarationType::kSyntaxError);
   }
 
   // Do the lookup own properties only, see ES5 erratum.
@@ -67,7 +80,11 @@ static Object* DeclareGlobals(Isolate* isolate, Handle<JSGlobalObject> global,
       if (old_details.IsReadOnly() || old_details.IsDontEnum() ||
           (it.state() == LookupIterator::ACCESSOR &&
            it.GetAccessors()->IsAccessorPair())) {
-        return ThrowRedeclarationError(isolate, name);
+        // ES#sec-globaldeclarationinstantiation 5.d:
+        // If hasRestrictedGlobal is true, throw a SyntaxError exception.
+        // ES#sec-evaldeclarationinstantiation 8.a.iv.1.b:
+        // If fnDefinable is false, throw a TypeError exception.
+        return ThrowRedeclarationError(isolate, name, redeclaration_type);
       }
       // If the existing property is not configurable, keep its attributes. Do
       attr = old_attributes;
@@ -130,9 +147,11 @@ RUNTIME_FUNCTION(Runtime_DeclareGlobals) {
     if (is_function && is_native) attr |= READ_ONLY;
     if (!is_eval) attr |= DONT_DELETE;
 
-    Object* result = DeclareGlobals(isolate, global, name, value,
-                                    static_cast<PropertyAttributes>(attr),
-                                    is_var, is_function);
+    // ES#sec-globaldeclarationinstantiation 5.d:
+    // If hasRestrictedGlobal is true, throw a SyntaxError exception.
+    Object* result = DeclareGlobals(
+        isolate, global, name, value, static_cast<PropertyAttributes>(attr),
+        is_var, is_function, RedeclarationType::kSyntaxError);
     if (isolate->has_pending_exception()) return result;
   });
 
@@ -214,7 +233,13 @@ Object* DeclareEvalHelper(Isolate* isolate, Handle<String> name,
   // Check for a conflict with a lexically scoped variable
   context_arg->Lookup(name, LEXICAL_TEST, &index, &attributes, &binding_flags);
   if (attributes != ABSENT && binding_flags == BINDING_CHECK_INITIALIZED) {
-    return ThrowRedeclarationError(isolate, name);
+    // ES#sec-evaldeclarationinstantiation 5.a.i.1:
+    // If varEnvRec.HasLexicalDeclaration(name) is true, throw a SyntaxError
+    // exception.
+    // ES#sec-evaldeclarationinstantiation 5.d.ii.2.a.i:
+    // Throw a SyntaxError exception.
+    return ThrowRedeclarationError(isolate, name,
+                                   RedeclarationType::kSyntaxError);
   }
 
   Handle<Object> holder = context->Lookup(name, DONT_FOLLOW_CHAINS, &index,
@@ -224,20 +249,23 @@ Object* DeclareEvalHelper(Isolate* isolate, Handle<String> name,
   Handle<JSObject> object;
 
   if (attributes != ABSENT && holder->IsJSGlobalObject()) {
+    // ES#sec-evaldeclarationinstantiation 8.a.iv.1.b:
+    // If fnDefinable is false, throw a TypeError exception.
     return DeclareGlobals(isolate, Handle<JSGlobalObject>::cast(holder), name,
-                          value, NONE, is_var, is_function);
+                          value, NONE, is_var, is_function,
+                          RedeclarationType::kTypeError);
   }
   if (context_arg->extension()->IsJSGlobalObject()) {
     Handle<JSGlobalObject> global(
         JSGlobalObject::cast(context_arg->extension()), isolate);
     return DeclareGlobals(isolate, global, name, value, NONE, is_var,
-                          is_function);
+                          is_function, RedeclarationType::kTypeError);
   } else if (context->IsScriptContext()) {
     DCHECK(context->global_object()->IsJSGlobalObject());
     Handle<JSGlobalObject> global(
         JSGlobalObject::cast(context->global_object()), isolate);
     return DeclareGlobals(isolate, global, name, value, NONE, is_var,
-                          is_function);
+                          is_function, RedeclarationType::kTypeError);
   }
 
   if (attributes != ABSENT) {
@@ -581,7 +609,11 @@ static Object* FindNameClash(Handle<ScopeInfo> scope_info,
     ScriptContextTable::LookupResult lookup;
     if (ScriptContextTable::Lookup(script_context, name, &lookup)) {
       if (IsLexicalVariableMode(mode) || IsLexicalVariableMode(lookup.mode)) {
-        return ThrowRedeclarationError(isolate, name);
+        // ES#sec-globaldeclarationinstantiation 5.b:
+        // If envRec.HasLexicalDeclaration(name) is true, throw a SyntaxError
+        // exception.
+        return ThrowRedeclarationError(isolate, name,
+                                       RedeclarationType::kSyntaxError);
       }
     }
 
@@ -591,7 +623,13 @@ static Object* FindNameClash(Handle<ScopeInfo> scope_info,
       Maybe<PropertyAttributes> maybe = JSReceiver::GetPropertyAttributes(&it);
       if (!maybe.IsJust()) return isolate->heap()->exception();
       if ((maybe.FromJust() & DONT_DELETE) != 0) {
-        return ThrowRedeclarationError(isolate, name);
+        // ES#sec-globaldeclarationinstantiation 5.a:
+        // If envRec.HasVarDeclaration(name) is true, throw a SyntaxError
+        // exception.
+        // ES#sec-globaldeclarationinstantiation 5.d:
+        // If hasRestrictedGlobal is true, throw a SyntaxError exception.
+        return ThrowRedeclarationError(isolate, name,
+                                       RedeclarationType::kSyntaxError);
       }
 
       JSGlobalObject::InvalidatePropertyCell(global_object, name);

test/mjsunit/es6/block-eval-var-over-let.js

@@ -8,13 +8,13 @@
 assertThrows(function() {
   let x = 1;
   eval('var x');
-}, TypeError);
+}, SyntaxError);
 
 // If the eval is in its own block scope, throws
 assertThrows(function() {
   let y = 1;
   { eval('var y'); }
-}, TypeError);
+}, SyntaxError);
 
 // If the let is in its own block scope, with the eval, throws
 assertThrows(function() {
@@ -22,7 +22,7 @@ assertThrows(function() {
     let x = 1;
     eval('var x');
   }
-}, TypeError);
+}, SyntaxError);
 
 // Legal if the let is no longer visible
 assertDoesNotThrow(function() {
@@ -37,13 +37,13 @@ assertDoesNotThrow(function() {
 assertThrows(function() {
   const x = 1;
   eval('var x');
-}, TypeError);
+}, SyntaxError);
 
 // If the eval is in its own block scope, throws
 assertThrows(function() {
   const y = 1;
   { eval('var y'); }
-}, TypeError);
+}, SyntaxError);
 
 // If the const is in its own block scope, with the eval, throws
 assertThrows(function() {
@@ -51,7 +51,7 @@ assertThrows(function() {
     const x = 1;
     eval('var x');
   }
-}, TypeError);
+}, SyntaxError);
 
 // Legal if the const is no longer visible
 assertDoesNotThrow(function() {

test/mjsunit/regress/redeclaration-error-types.js

+// Copyright 2016 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+
+function doTest(scripts, expectedError) {
+  var realm = Realm.create();
+
+  for (var i = 0; i < scripts.length - 1; i++) {
+    Realm.eval(realm, scripts[i]);
+  }
+  assertThrows(function() {
+    Realm.eval(realm, scripts[scripts.length - 1]);
+  }, Realm.eval(realm, expectedError));
+
+  Realm.dispose(realm);
+}
+
+var tests = [
+  {
+    // ES#sec-globaldeclarationinstantiation 5.a:
+    // If envRec.HasVarDeclaration(name) is true, throw a SyntaxError
+    // exception.
+    scripts: [
+      "var a;",
+      "let a;",
+    ],
+    expectedError: "SyntaxError",
+  },
+  {
+    // ES#sec-globaldeclarationinstantiation 6.a:
+    // If envRec.HasLexicalDeclaration(name) is true, throw a SyntaxError
+    // exception.
+    scripts: [
+      "let a;",
+      "var a;",
+    ],
+    expectedError: "SyntaxError",
+  },
+  {
+    // ES#sec-globaldeclarationinstantiation 5.b:
+    // If envRec.HasLexicalDeclaration(name) is true, throw a SyntaxError
+    // exception.
+    scripts: [
+      "let a;",
+      "let a;",
+    ],
+    expectedError: "SyntaxError",
+  },
+  {
+    // ES#sec-evaldeclarationinstantiation 5.a.i.1:
+    // If varEnvRec.HasLexicalDeclaration(name) is true, throw a SyntaxError
+    // exception.
+    scripts: [
+      'let a; eval("var a;");',
+    ],
+    expectedError: "SyntaxError",
+  },
+  {
+    // ES#sec-evaldeclarationinstantiation 5.a.i.1:
+    // If varEnvRec.HasLexicalDeclaration(name) is true, throw a SyntaxError
+    // exception.
+    scripts: [
+      'let a; eval("function a() {}");',
+    ],
+    expectedError: "SyntaxError",
+  },
+  {
+    // ES#sec-evaldeclarationinstantiation 5.d.ii.2.a.i:
+    // Throw a SyntaxError exception.
+    scripts: [
+      '(function() { let a; eval("var a;"); })();',
+    ],
+    expectedError: "SyntaxError",
+  },
+  {
+    // ES#sec-evaldeclarationinstantiation 5.d.ii.2.a.i:
+    // Throw a SyntaxError exception.
+    scripts: [
+      '(function() { let a; eval("function a() {}"); })();',
+    ],
+    expectedError: "SyntaxError",
+  },
+  {
+    // ES#sec-globaldeclarationinstantiation 5.d:
+    // If hasRestrictedGlobal is true, throw a SyntaxError exception.
+    scripts: [
+      'let NaN;',
+    ],
+    expectedError: "SyntaxError",
+  },
+  {
+    // ES#sec-globaldeclarationinstantiation 5.d:
+    // If hasRestrictedGlobal is true, throw a SyntaxError exception.
+    scripts: [
+      'function NaN() {}',
+    ],
+    expectedError: "SyntaxError",
+  },
+
+  {
+    // ES#sec-evaldeclarationinstantiation 8.a.iv.1.b:
+    // If fnDefinable is false, throw a TypeError exception.
+    scripts: [
+      'eval("function NaN() {}");',
+    ],
+    expectedError: "TypeError",
+  },
+  {
+    // ES#sec-evaldeclarationinstantiation 8.a.iv.1.b:
+    // If fnDefinable is false, throw a TypeError exception.
+    scripts: [
+      `
+        let a;
+        try {
+          eval("function a() {}");
+        } catch (e) {}
+        eval("function NaN() {}");
+      `,
+    ],
+    expectedError: "TypeError",
+  },
+  {
+    // ES#sec-evaldeclarationinstantiation 8.a.iv.1.b:
+    // If fnDefinable is false, throw a TypeError exception.
+    scripts: [
+      `
+        eval("
+          function f() {
+            function b() {
+              (0, eval)('function NaN() {}');
+            }
+            b();
+          }
+          f();
+        ");
+      `.replace(/"/g, '`'),
+    ],
+    expectedError: "TypeError",
+  },
+];
+
+tests.forEach(function(test) {
+  doTest(test.scripts, test.expectedError);
+});

test/test262/test262.status

@@ -263,11 +263,6 @@
   'language/eval-code/indirect/non-definable-function-with-function': [FAIL],
   'language/eval-code/indirect/non-definable-function-with-variable': [FAIL],
 
-  # https://bugs.chromium.org/p/v8/issues/detail?id=4955
-  'language/eval-code/direct/var-env-global-lex-non-strict': [FAIL],
-  'language/eval-code/direct/var-env-lower-lex-non-strict': [FAIL],
-  'language/eval-code/indirect/var-env-global-lex-non-strict': [FAIL],
-
   # https://bugs.chromium.org/p/v8/issues/detail?id=4124
   'built-ins/Simd/*': [SKIP],