change most cases of variable redeclaration from TypeError to SyntaxError

src/runtime/runtime-scopes.cc

Index: src/runtime/runtime-scopes.cc
diff --git a/src/runtime/runtime-scopes.cc b/src/runtime/runtime-scopes.cc
index e36fd842149b28e22ee2fe3477dcba5c3e0d666d..32856c875a21c215a7b7a6b1af5db0f3ff0fd110 100644
--- a/src/runtime/runtime-scopes.cc
+++ b/src/runtime/runtime-scopes.cc
@@ -16,10 +16,21 @@
 namespace v8 {
 namespace internal {
 
-static Object* ThrowRedeclarationError(Isolate* isolate, Handle<String> name) {
+struct RedeclarationError {
+  enum Type { kSyntaxError = 0, kTypeError = 1 };
+};
+
+static Object* ThrowRedeclarationError(
+    Isolate* isolate, Handle<String> name,
+    RedeclarationError::Type redeclaration_error_type) {
   HandleScope scope(isolate);
-  THROW_NEW_ERROR_RETURN_FAILURE(
-      isolate, NewTypeError(MessageTemplate::kVarRedeclaration, name));
+  if (redeclaration_error_type == RedeclarationError::kSyntaxError) {
+    THROW_NEW_ERROR_RETURN_FAILURE(
+        isolate, NewSyntaxError(MessageTemplate::kVarRedeclaration, name));
+  } else {
+    THROW_NEW_ERROR_RETURN_FAILURE(
+        isolate, NewTypeError(MessageTemplate::kVarRedeclaration, name));
+  }
 }
 
 
@@ -31,16 +42,17 @@ RUNTIME_FUNCTION(Runtime_ThrowConstAssignError) {
 
 
 // May throw a RedeclarationError.
-static Object* DeclareGlobals(Isolate* isolate, Handle<JSGlobalObject> global,
-                              Handle<String> name, Handle<Object> value,
-                              PropertyAttributes attr, bool is_var,
-                              bool is_const, bool is_function) {
+static Object* DeclareGlobals(
+    Isolate* isolate, Handle<JSGlobalObject> global, Handle<String> name,
+    Handle<Object> value, PropertyAttributes attr, bool is_var, bool is_const,
+    bool is_function, RedeclarationError::Type redeclaration_error_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);
+    return ThrowRedeclarationError(isolate, name,
+                                   RedeclarationError::kSyntaxError);

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

15.1.11:6.a: "If envRec.HasLexicalDeclaration(name) is true, throw a SyntaxError
exception."

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

   }
 
   // Do the lookup own properties only, see ES5 erratum.
@@ -51,7 +63,10 @@ static Object* DeclareGlobals(Isolate* isolate, Handle<JSGlobalObject> global,
   if (it.IsFound()) {
     PropertyAttributes old_attributes = maybe.FromJust();
     // The name was declared before; check for conflicting re-declarations.
-    if (is_const) return ThrowRedeclarationError(isolate, name);
+    if (is_const) {
+      return ThrowRedeclarationError(isolate, name,
+                                     RedeclarationError::kSyntaxError);

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

I was unable to get this code to run. It does not seem to be used in the test
suite or in test262.

+    }
 
     // Skip var re-declarations.
     if (is_var) return isolate->heap()->undefined_value();
@@ -68,7 +83,7 @@ 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);
+        return ThrowRedeclarationError(isolate, name, redeclaration_error_type);

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

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() {}");'

       }
       // If the existing property is not configurable, keep its attributes. Do
       attr = old_attributes;
@@ -137,9 +152,9 @@ RUNTIME_FUNCTION(Runtime_DeclareGlobals) {
     if (is_function && is_native) attr |= READ_ONLY;
     if (!is_const && !is_eval) attr |= DONT_DELETE;
 
-    Object* result = DeclareGlobals(isolate, global, name, value,
-                                    static_cast<PropertyAttributes>(attr),
-                                    is_var, is_const, is_function);
+    Object* result = DeclareGlobals(
+        isolate, global, name, value, static_cast<PropertyAttributes>(attr),
+        is_var, is_const, is_function, RedeclarationError::kSyntaxError);

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

15.1.11:5.d: "If hasRestrictedGlobal is true, throw a SyntaxError exception."

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

     if (isolate->has_pending_exception()) return result;
   });
 
@@ -234,7 +249,8 @@ Object* DeclareLookupSlot(Isolate* isolate, Handle<String> name,
     context_arg->Lookup(name, LEXICAL_TEST, &index, &attributes,
                         &binding_flags);
     if (attributes != ABSENT && binding_flags == BINDING_CHECK_INITIALIZED) {
-      return ThrowRedeclarationError(isolate, name);
+      return ThrowRedeclarationError(isolate, name,
+                                     RedeclarationError::kSyntaxError);

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

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;"); })();'

     }
     attr = static_cast<PropertyAttributes>(attr & ~EVAL_DECLARED);
   }
@@ -255,25 +271,27 @@ Object* DeclareLookupSlot(Isolate* isolate, Handle<String> name,
   // but by DeclareGlobals instead.
   if (attributes != ABSENT && holder->IsJSGlobalObject()) {
     return DeclareGlobals(isolate, Handle<JSGlobalObject>::cast(holder), name,
-                          value, attr, is_var, is_const, is_function);
+                          value, attr, is_var, is_const, is_function,
+                          RedeclarationError::kTypeError);

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

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

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

   }
   if (context_arg->extension()->IsJSGlobalObject()) {
     Handle<JSGlobalObject> global(
         JSGlobalObject::cast(context_arg->extension()), isolate);
     return DeclareGlobals(isolate, global, name, value, attr, is_var, is_const,
-                          is_function);
+                          is_function, RedeclarationError::kSyntaxError);

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

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();");'

   } else if (context->IsScriptContext()) {
     DCHECK(context->global_object()->IsJSGlobalObject());
     Handle<JSGlobalObject> global(
         JSGlobalObject::cast(context->global_object()), isolate);
     return DeclareGlobals(isolate, global, name, value, attr, is_var, is_const,
-                          is_function);
+                          is_function, RedeclarationError::kSyntaxError);

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

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.)

   }
 
   if (attributes != ABSENT) {
     // The name was declared before; check for conflicting re-declarations.
     if (is_const || (attributes & READ_ONLY) != 0) {
-      return ThrowRedeclarationError(isolate, name);
+      return ThrowRedeclarationError(isolate, name,
+                                     RedeclarationError::kSyntaxError);

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

I was unable to get this code to run. It does not seem to be used in the test
suite or in test262.

     }
 
     // Skip var re-declarations.
@@ -612,7 +630,8 @@ 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);
+        return ThrowRedeclarationError(isolate, name,
+                                       RedeclarationError::kSyntaxError);

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

15.1.11:5.b: "If envRec.HasLexicalDeclaration(name) is true, throw a SyntaxError
exception."

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

       }
     }
 
@@ -622,7 +641,8 @@ 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);
+        return ThrowRedeclarationError(isolate, name,
+                                       RedeclarationError::kSyntaxError);

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

15.1.11:5.a: "If envRec.HasVarDeclaration(name) is true, throw a SyntaxError
exception."

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

       }
 
       JSGlobalObject::InvalidatePropertyCell(global_object, name);