change most cases of variable redeclaration from TypeError to SyntaxError

src/runtime/runtime-scopes.cc

 // Copyright 2014 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.
 
 #include "src/runtime/runtime-utils.h"
 
 #include "src/accessors.h"
 #include "src/arguments.h"
 #include "src/ast/scopeinfo.h"
 #include "src/ast/scopes.h"
 #include "src/deoptimizer.h"
 #include "src/frames-inl.h"
 #include "src/isolate-inl.h"
 #include "src/messages.h"
 
 namespace v8 {
 namespace internal {
 
-static Object* ThrowRedeclarationError(Isolate* isolate, Handle<String> name) {
+struct RedeclarationError {

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

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.

+  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));
+  }
 }
 
 
 RUNTIME_FUNCTION(Runtime_ThrowConstAssignError) {
   HandleScope scope(isolate);
   THROW_NEW_ERROR_RETURN_FAILURE(isolate,
                                  NewTypeError(MessageTemplate::kConstAssign));
 }
 
 
 // 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);
+    // ES#sec-globaldeclarationinstantiation 6.a:
+    // If envRec.HasLexicalDeclaration(name) is true, throw a SyntaxError
+    // exception.
+    return ThrowRedeclarationError(isolate, name,
+                                   RedeclarationError::kSyntaxError);
   }
 
   // Do the lookup own properties only, see ES5 erratum.
   LookupIterator it(global, name, global, LookupIterator::OWN_SKIP_INTERCEPTOR);
   Maybe<PropertyAttributes> maybe = JSReceiver::GetPropertyAttributes(&it);
   if (!maybe.IsJust()) return isolate->heap()->exception();
 
   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) {

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

Did you say this code is unreachable?

+      return ThrowRedeclarationError(isolate, name,
+                                     RedeclarationError::kSyntaxError);
+    }
 
     // Skip var re-declarations.
     if (is_var) return isolate->heap()->undefined_value();
 
     DCHECK(is_function);
     if ((old_attributes & DONT_DELETE) != 0) {
       // Only allow reconfiguring globals to functions in user code (no
       // natives, which are marked as read-only).
       DCHECK((attr & READ_ONLY) == 0);
 
       // Check whether we can reconfigure the existing property into a
       // function.
       PropertyDetails old_details = it.property_details();
       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_error_type);
       }
       // If the existing property is not configurable, keep its attributes. Do
       attr = old_attributes;
     }
 
     // If the current state is ACCESSOR, this could mean it's an AccessorInfo
     // type property. We are not allowed to call into such setters during global
     // function declaration since this would break e.g., onload. Meaning
     // 'function onload() {}' would invalidly register that function as the
     // onload callback. To avoid this situation, we first delete the property
@@ skipping 48 matching lines @@
 
     // Compute the property attributes. According to ECMA-262,
     // the property must be non-configurable except in eval.
     bool is_native = DeclareGlobalsNativeFlag::decode(flags);
     bool is_eval = DeclareGlobalsEvalFlag::decode(flags);
     int attr = NONE;
     if (is_const) attr |= READ_ONLY;
     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);
+    // 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_const, is_function, RedeclarationError::kSyntaxError);
     if (isolate->has_pending_exception()) return result;
   });
 
   return isolate->heap()->undefined_value();
 }
 
 
 RUNTIME_FUNCTION(Runtime_InitializeVarGlobal) {
   HandleScope scope(isolate);
   // args[0] == name
@@ skipping 74 matching lines @@
 
   int index;
   PropertyAttributes attributes;
   BindingFlags binding_flags;
 
   if ((attr & EVAL_DECLARED) != 0) {
     // 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);
+      return ThrowRedeclarationError(isolate, name,

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

Why no comment here?

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

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

+                                     RedeclarationError::kSyntaxError);
     }
     attr = static_cast<PropertyAttributes>(attr & ~EVAL_DECLARED);
   }
 
   Handle<Object> holder = context->Lookup(name, DONT_FOLLOW_CHAINS, &index,
                                           &attributes, &binding_flags);
   if (holder.is_null()) {
     // In case of JSProxy, an exception might have been thrown.
     if (isolate->has_pending_exception()) return isolate->heap()->exception();
   }
 
   Handle<JSObject> object;
   Handle<Object> value =
       is_function ? initial_value
                   : Handle<Object>::cast(isolate->factory()->undefined_value());
 
   // TODO(verwaest): This case should probably not be covered by this function,
   // but by DeclareGlobals instead.
   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, attr, is_var, is_const, is_function);
+                          value, attr, is_var, is_const, is_function,
+                          RedeclarationError::kTypeError);
   }
   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::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, attr, is_var, is_const,
-                          is_function);
+                          is_function, RedeclarationError::kTypeError);
   }
 
   if (attributes != ABSENT) {
     // The name was declared before; check for conflicting re-declarations.
     if (is_const || (attributes & READ_ONLY) != 0) {

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

Was this also unreachable?

-      return ThrowRedeclarationError(isolate, name);
+      return ThrowRedeclarationError(isolate, name,
+                                     RedeclarationError::kSyntaxError);
     }
 
     // Skip var re-declarations.
     if (is_var) return isolate->heap()->undefined_value();
 
     DCHECK(is_function);
     if (index != Context::kNotFound) {
       DCHECK(holder.is_identical_to(context));
       context->set(index, *initial_value);
       return isolate->heap()->undefined_value();
@@ skipping 318 matching lines @@
 static Object* FindNameClash(Handle<ScopeInfo> scope_info,
                              Handle<JSGlobalObject> global_object,
                              Handle<ScriptContextTable> script_context) {
   Isolate* isolate = scope_info->GetIsolate();
   for (int var = 0; var < scope_info->ContextLocalCount(); var++) {
     Handle<String> name(scope_info->ContextLocalName(var));
     VariableMode mode = scope_info->ContextLocalMode(var);
     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

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

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.

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

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

+        // exception.
+        return ThrowRedeclarationError(isolate, name,
+                                       RedeclarationError::kSyntaxError);
       }
     }
 
     if (IsLexicalVariableMode(mode)) {
       LookupIterator it(global_object, name, global_object,
                         LookupIterator::OWN_SKIP_INTERCEPTOR);
       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.
+        return ThrowRedeclarationError(isolate, name,
+                                       RedeclarationError::kSyntaxError);
       }
 
       JSGlobalObject::InvalidatePropertyCell(global_object, name);
     }
   }
   return isolate->heap()->undefined_value();
 }
 
 
 RUNTIME_FUNCTION(Runtime_NewScriptContext) {
@@ skipping 385 matching lines @@
 RUNTIME_FUNCTION(Runtime_StoreLookupSlot_Strict) {
   HandleScope scope(isolate);
   DCHECK_EQ(2, args.length());
   CONVERT_ARG_HANDLE_CHECKED(String, name, 0);
   CONVERT_ARG_HANDLE_CHECKED(Object, value, 1);
   RETURN_RESULT_OR_FAILURE(isolate, StoreLookupSlot(name, value, STRICT));
 }
 
 }  // namespace internal
 }  // namespace v8