[compiler] Always pass closure argument to with, catch and block context creation.

Up until now we sometimes pass Smi 0 around as closure and expect the
runtime to translate that appropriately. But we need to be careful in
some places to not confuse the Smi 0 with a real closure. However, we
could instead just pass the correct closure extracted from the native
context.

This addresses three long-standing TODOs in the JSTypedLowering pass.

Drive-by-fix: Further unify error message reporting for ToObject (we had
a special message in case of ToObject error in with context creation).

R=yangguo@chromium.org

Review URL: https://codereview.chromium.org/1475383002

Cr-Commit-Position: refs/heads/master@{#32336}

src/compiler/ast-graph-builder.cc

@@ -461,8 +461,7 @@ Node* AstGraphBuilder::GetFunctionClosureForContext() {
       closure_scope->is_module_scope()) {
     // Contexts nested in the native context have a canonical empty function as
     // their closure, not the anonymous closure containing the global code.
-    // Pass a SMI sentinel and let the runtime look up the empty function.
-    return jsgraph()->SmiConstant(0);
+    return BuildLoadNativeContextField(Context::CLOSURE_INDEX);
   } else {
     DCHECK(closure_scope->is_function_scope());
     return GetFunctionClosure();

src/compiler/js-typed-lowering.cc

@@ -1838,13 +1838,8 @@ Reduction JSTypedLowering::ReduceJSCreateFunctionContext(Node* node) {
   int slot_count = OpParameter<int>(node->op());
   Node* const closure = NodeProperties::GetValueInput(node, 0);
 
-  // The closure can be NumberConstant(0) if the closure is global code
-  // (rather than a function). We exclude that case here.
-  // TODO(jarin) Find a better way to check that the closure is a function.
-
   // Use inline allocation for function contexts up to a size limit.
-  if (slot_count < kFunctionContextAllocationLimit &&
-      closure->opcode() != IrOpcode::kNumberConstant) {
+  if (slot_count < kFunctionContextAllocationLimit) {
     // JSCreateFunctionContext[slot_count < limit]](fun)
     Node* const effect = NodeProperties::GetEffectInput(node);
     Node* const control = NodeProperties::GetControlInput(node);
@@ -1892,15 +1887,10 @@ Reduction JSTypedLowering::ReduceJSCreateWithContext(Node* node) {
   DCHECK_EQ(IrOpcode::kJSCreateWithContext, node->opcode());
   Node* const input = NodeProperties::GetValueInput(node, 0);
   Node* const closure = NodeProperties::GetValueInput(node, 1);
-  Type* input_type = NodeProperties::GetType(input);
-
-  // The closure can be NumberConstant(0) if the closure is global code
-  // (rather than a function). We exclude that case here.
-  // TODO(jarin) Find a better way to check that the closure is a function.
+  Type* const input_type = NodeProperties::GetType(input);
 
   // Use inline allocation for with contexts for regular objects.
-  if (input_type->Is(Type::Receiver()) &&
-      closure->opcode() != IrOpcode::kNumberConstant) {
+  if (input_type->Is(Type::Receiver())) {
     // JSCreateWithContext(o:receiver, fun)
     Node* const effect = NodeProperties::GetEffectInput(node);
     Node* const control = NodeProperties::GetControlInput(node);
@@ -1931,13 +1921,8 @@ Reduction JSTypedLowering::ReduceJSCreateBlockContext(Node* node) {
   int context_length = scope_info->ContextLength();
   Node* const closure = NodeProperties::GetValueInput(node, 0);
 
-  // The closure can be NumberConstant(0) if the closure is global code
-  // (rather than a function). We exclude that case here.
-  // TODO(jarin) Find a better way to check that the closure is a function.
-
   // Use inline allocation for block contexts up to a size limit.
-  if (context_length < kBlockContextAllocationLimit &&
-      closure->opcode() != IrOpcode::kNumberConstant) {
+  if (context_length < kBlockContextAllocationLimit) {
     // JSCreateBlockContext[scope[length < limit]](fun)
     Node* const effect = NodeProperties::GetEffectInput(node);
     Node* const control = NodeProperties::GetControlInput(node);

src/full-codegen/arm/full-codegen-arm.cc

@@ -4795,9 +4795,10 @@ void FullCodeGenerator::PushFunctionArgumentForContextAllocation() {
       closure_scope->is_module_scope()) {
     // Contexts nested in the native context have a canonical empty function
     // as their closure, not the anonymous closure containing the global
-    // code.  Pass a smi sentinel and let the runtime look up the empty
-    // function.
-    __ mov(ip, Operand(Smi::FromInt(0)));
+    // code.
+    __ ldr(ip, GlobalObjectOperand());
+    __ ldr(ip, FieldMemOperand(ip, JSGlobalObject::kNativeContextOffset));
+    __ ldr(ip, ContextOperand(ip, Context::CLOSURE_INDEX));
   } else if (closure_scope->is_eval_scope()) {
     // Contexts created by a call to eval have the same closure as the
     // context calling eval, not the anonymous closure containing the eval

src/full-codegen/arm64/full-codegen-arm64.cc

@@ -4831,21 +4831,21 @@ void FullCodeGenerator::PushFunctionArgumentForContextAllocation() {
       closure_scope->is_module_scope()) {
     // Contexts nested in the native context have a canonical empty function
     // as their closure, not the anonymous closure containing the global
-    // code.  Pass a smi sentinel and let the runtime look up the empty
-    // function.
+    // code.
     DCHECK(kSmiTag == 0);
-    __ Push(xzr);
+    __ Ldr(x10, GlobalObjectMemOperand());
+    __ Ldr(x10, FieldMemOperand(x10, JSGlobalObject::kNativeContextOffset));
+    __ Ldr(x10, ContextMemOperand(x10, Context::CLOSURE_INDEX));
   } else if (closure_scope->is_eval_scope()) {
     // Contexts created by a call to eval have the same closure as the
     // context calling eval, not the anonymous closure containing the eval
     // code.  Fetch it from the context.
     __ Ldr(x10, ContextMemOperand(cp, Context::CLOSURE_INDEX));
-    __ Push(x10);
   } else {
     DCHECK(closure_scope->is_function_scope());
     __ Ldr(x10, MemOperand(fp, JavaScriptFrameConstants::kFunctionOffset));
-    __ Push(x10);
   }
+  __ Push(x10);
 }
 
 

src/full-codegen/ia32/full-codegen-ia32.cc

@@ -4718,9 +4718,10 @@ void FullCodeGenerator::PushFunctionArgumentForContextAllocation() {
       closure_scope->is_module_scope()) {
     // Contexts nested in the native context have a canonical empty function
     // as their closure, not the anonymous closure containing the global
-    // code.  Pass a smi sentinel and let the runtime look up the empty
-    // function.
-    __ push(Immediate(Smi::FromInt(0)));
+    // code.
+    __ mov(eax, GlobalObjectOperand());
+    __ mov(eax, FieldOperand(eax, JSGlobalObject::kNativeContextOffset));
+    __ push(ContextOperand(eax, Context::CLOSURE_INDEX));
   } else if (closure_scope->is_eval_scope()) {
     // Contexts nested inside eval code have the same closure as the context
     // calling eval, not the anonymous closure containing the eval code.

src/full-codegen/mips/full-codegen-mips.cc

@@ -4818,9 +4818,10 @@ void FullCodeGenerator::PushFunctionArgumentForContextAllocation() {
       closure_scope->is_module_scope()) {
     // Contexts nested in the native context have a canonical empty function
     // as their closure, not the anonymous closure containing the global
-    // code.  Pass a smi sentinel and let the runtime look up the empty
-    // function.
-    __ li(at, Operand(Smi::FromInt(0)));
+    // code.
+    __ lw(at, GlobalObjectOperand());
+    __ lw(at, FieldMemOperand(at, JSGlobalObject::kNativeContextOffset));
+    __ lw(at, ContextOperand(at, Context::CLOSURE_INDEX));
   } else if (closure_scope->is_eval_scope()) {
     // Contexts created by a call to eval have the same closure as the
     // context calling eval, not the anonymous closure containing the eval

src/full-codegen/mips64/full-codegen-mips64.cc

@@ -4824,9 +4824,10 @@ void FullCodeGenerator::PushFunctionArgumentForContextAllocation() {
       closure_scope->is_module_scope()) {
     // Contexts nested in the native context have a canonical empty function
     // as their closure, not the anonymous closure containing the global
-    // code.  Pass a smi sentinel and let the runtime look up the empty
-    // function.
-    __ li(at, Operand(Smi::FromInt(0)));
+    // code.
+    __ ld(at, GlobalObjectOperand());
+    __ ld(at, FieldMemOperand(at, JSGlobalObject::kNativeContextOffset));
+    __ ld(at, ContextOperand(at, Context::CLOSURE_INDEX));
   } else if (closure_scope->is_eval_scope()) {
     // Contexts created by a call to eval have the same closure as the
     // context calling eval, not the anonymous closure containing the eval

src/full-codegen/x64/full-codegen-x64.cc

@@ -4731,9 +4731,10 @@ void FullCodeGenerator::PushFunctionArgumentForContextAllocation() {
       closure_scope->is_module_scope()) {
     // Contexts nested in the native context have a canonical empty function
     // as their closure, not the anonymous closure containing the global
-    // code.  Pass a smi sentinel and let the runtime look up the empty
-    // function.
-    __ Push(Smi::FromInt(0));
+    // code.
+    __ movp(rax, GlobalObjectOperand());
+    __ movp(rax, FieldOperand(rax, JSGlobalObject::kNativeContextOffset));
+    __ Push(ContextOperand(rax, Context::CLOSURE_INDEX));
   } else if (closure_scope->is_eval_scope()) {
     // Contexts created by a call to eval have the same closure as the
     // context calling eval, not the anonymous closure containing the eval

src/messages.h

@@ -252,7 +252,6 @@ class CallSite {
     "Invalid property descriptor. Cannot both specify accessors and a value "  \
     "or writable attribute, %")                                                \
   T(VarRedeclaration, "Identifier '%' has already been declared")              \
-  T(WithExpression, "% has no properties")                                     \
   T(WrongArgs, "%: Arguments list has wrong type")                             \
   /* ReferenceError */                                                         \
   T(NonMethod, "'super' is referenced from non-method")                        \

src/runtime/runtime-scopes.cc

@@ -705,31 +705,14 @@ RUNTIME_FUNCTION(Runtime_NewFunctionContext) {
 
 RUNTIME_FUNCTION(Runtime_PushWithContext) {
   HandleScope scope(isolate);
-  DCHECK(args.length() == 2);
+  DCHECK_EQ(2, args.length());
+  CONVERT_ARG_HANDLE_CHECKED(Object, value, 0);
+  CONVERT_ARG_HANDLE_CHECKED(JSFunction, function, 1);
   Handle<JSReceiver> extension_object;
-  if (args[0]->IsJSReceiver()) {
-    extension_object = args.at<JSReceiver>(0);
-  } else {
-    // Try to convert the object to a proper JavaScript object.
-    MaybeHandle<JSReceiver> maybe_object =
-        Object::ToObject(isolate, args.at<Object>(0));
-    if (!maybe_object.ToHandle(&extension_object)) {
-      Handle<Object> handle = args.at<Object>(0);
-      THROW_NEW_ERROR_RETURN_FAILURE(
-          isolate, NewTypeError(MessageTemplate::kWithExpression, handle));
-    }
-  }
-
-  Handle<JSFunction> function;
-  if (args[1]->IsSmi()) {
-    // A smi sentinel indicates a context nested inside global code rather
-    // than some function.  There is a canonical empty function that can be
-    // gotten from the native context.
-    function = handle(isolate->native_context()->closure());
-  } else {
-    function = args.at<JSFunction>(1);
+  if (!Object::ToObject(isolate, value).ToHandle(&extension_object)) {
+    THROW_NEW_ERROR_RETURN_FAILURE(
+        isolate, NewTypeError(MessageTemplate::kUndefinedOrNullToObject));
   }
-
   Handle<Context> current(isolate->context());
   Handle<Context> context =
       isolate->factory()->NewWithContext(function, current, extension_object);
@@ -740,18 +723,10 @@ RUNTIME_FUNCTION(Runtime_PushWithContext) {
 
 RUNTIME_FUNCTION(Runtime_PushCatchContext) {
   HandleScope scope(isolate);
-  DCHECK(args.length() == 3);
+  DCHECK_EQ(3, args.length());
   CONVERT_ARG_HANDLE_CHECKED(String, name, 0);
   CONVERT_ARG_HANDLE_CHECKED(Object, thrown_object, 1);
-  Handle<JSFunction> function;
-  if (args[2]->IsSmi()) {
-    // A smi sentinel indicates a context nested inside global code rather
-    // than some function.  There is a canonical empty function that can be
-    // gotten from the native context.
-    function = handle(isolate->native_context()->closure());
-  } else {
-    function = args.at<JSFunction>(2);
-  }
+  CONVERT_ARG_HANDLE_CHECKED(JSFunction, function, 2);
   Handle<Context> current(isolate->context());
   Handle<Context> context = isolate->factory()->NewCatchContext(
       function, current, name, thrown_object);
@@ -762,17 +737,9 @@ RUNTIME_FUNCTION(Runtime_PushCatchContext) {
 
 RUNTIME_FUNCTION(Runtime_PushBlockContext) {
   HandleScope scope(isolate);
-  DCHECK(args.length() == 2);
+  DCHECK_EQ(2, args.length());
   CONVERT_ARG_HANDLE_CHECKED(ScopeInfo, scope_info, 0);
-  Handle<JSFunction> function;
-  if (args[1]->IsSmi()) {
-    // A smi sentinel indicates a context nested inside global code rather
-    // than some function.  There is a canonical empty function that can be
-    // gotten from the native context.
-    function = handle(isolate->native_context()->closure());
-  } else {
-    function = args.at<JSFunction>(1);
-  }
+  CONVERT_ARG_HANDLE_CHECKED(JSFunction, function, 1);
   Handle<Context> current(isolate->context());
   Handle<Context> context =
       isolate->factory()->NewBlockContext(function, current, scope_info);

test/mjsunit/messages.js

@@ -335,11 +335,6 @@ test(function() {
 }, "Invalid property descriptor. Cannot both specify accessors " +
    "and a value or writable attribute, #<Object>", TypeError);
 
-// kWithExpression
-test(function() {
-  with (null) {}
-}, "null has no properties", TypeError);
-
 // kWrongArgs
 test(function() {
   (function() {}).apply({}, 1);