From f74a08d8ee4e1b5428f672557c9cb4e1476347e8 Mon Sep 17 00:00:00 2001 From: "ricow@chromium.org" Date: Wed, 3 Feb 2010 13:10:03 +0000 Subject: [PATCH] Added Object.defineProperty + needed internal functionality: DefineOwnProperty (changed to allow for redefinition of existing property) SameValue Extra info on propertydescriptor GetProperty HasProperty Currently the DefineOrRedefineAccessorProperty deletes the existing property on the object if it is a dataproperty (FIELD or NORMAL) and adds a new one. This can potentially be optimized. Review URL: http://codereview.chromium.org/555149 git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@3786 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/messages.js | 2 + src/runtime.cc | 68 ++++- src/runtime.h | 2 + src/runtime.js | 39 ++- src/v8natives.js | 144 +++++++++-- test/cctest/test-api.cc | 97 +++++++ test/es5conform/es5conform.status | 75 +----- test/mjsunit/object-define-property.js | 454 +++++++++++++++++++++++++++++++++ 8 files changed, 783 insertions(+), 98 deletions(-) create mode 100644 test/mjsunit/object-define-property.js diff --git a/src/messages.js b/src/messages.js index df008c9..ca82afe 100644 --- a/src/messages.js +++ b/src/messages.js @@ -162,6 +162,8 @@ function FormatMessage(message) { value_and_accessor: "Invalid property. A property cannot both have accessors and be writable or have a value: %0", proto_object_or_null: "Object prototype may only be an Object or null", property_desc_object: "Property description must be an object: %0", + redefine_disallowed: "Cannot redefine property: %0", + define_disallowed: "Cannot define property, object is not extensible: %0", // RangeError invalid_array_length: "Invalid array length", stack_overflow: "Maximum call stack size exceeded", diff --git a/src/runtime.cc b/src/runtime.cc index 515343b..3da4212 100644 --- a/src/runtime.cc +++ b/src/runtime.cc @@ -596,8 +596,9 @@ static Object* Runtime_GetOwnProperty(Arguments args) { if (result.type() == CALLBACKS) { Object* structure = result.GetCallbackObject(); - if (structure->IsProxy()) { - // Property that is internally implemented as a callback. + if (structure->IsProxy() || structure->IsAccessorInfo()) { + // Property that is internally implemented as a callback or + // an API defined callback. Object* value = obj->GetPropertyWithCallback( obj, structure, name, result.holder()); elms->set(0, Heap::false_value()); @@ -609,7 +610,6 @@ static Object* Runtime_GetOwnProperty(Arguments args) { elms->set(1, FixedArray::cast(structure)->get(0)); elms->set(2, FixedArray::cast(structure)->get(1)); } else { - // TODO(ricow): Handle API callbacks. return Heap::undefined_value(); } } else { @@ -619,7 +619,7 @@ static Object* Runtime_GetOwnProperty(Arguments args) { } elms->set(3, Heap::ToBoolean(!result.IsDontEnum())); - elms->set(4, Heap::ToBoolean(!result.IsReadOnly())); + elms->set(4, Heap::ToBoolean(!result.IsDontDelete())); return *desc; } @@ -2888,6 +2888,66 @@ static Object* Runtime_KeyedGetProperty(Arguments args) { } +static Object* Runtime_DefineOrRedefineAccessorProperty(Arguments args) { + ASSERT(args.length() == 5); + HandleScope scope; + Handle obj = args.at(0); + CONVERT_CHECKED(String, name, args[1]); + CONVERT_CHECKED(Smi, flag_setter, args[2]); + CONVERT_CHECKED(JSFunction, fun, args[3]); + CONVERT_CHECKED(Smi, flag_attr, args[4]); + int unchecked = flag_attr->value(); + RUNTIME_ASSERT((unchecked & ~(READ_ONLY | DONT_ENUM | DONT_DELETE)) == 0); + + LookupResult result; + obj->LocalLookupRealNamedProperty(name, &result); + + PropertyAttributes attr = static_cast(unchecked); + // If an existing property is either FIELD, NORMAL or CONSTANT_FUNCTION + // delete it to avoid running into trouble in DefineAccessor, which + // handles this incorrectly if the property is readonly (does nothing) + if (result.type() == FIELD || result.type() == NORMAL + || result.type() == CONSTANT_FUNCTION) + obj->DeleteProperty(name, JSObject::NORMAL_DELETION); + + return obj->DefineAccessor(name, flag_setter->value() == 0, fun, attr); +} + +static Object* Runtime_DefineOrRedefineDataProperty(Arguments args) { + ASSERT(args.length() == 4); + HandleScope scope; + Handle obj = args.at(0); + Handle name = args.at(1); + Handle obj_value = args.at(2); + Handle js_object = Handle::cast(obj); + Handle key_string = Handle::cast(name); + + CONVERT_CHECKED(Smi, flag, args[3]); + int unchecked = flag->value(); + RUNTIME_ASSERT((unchecked & ~(READ_ONLY | DONT_ENUM | DONT_DELETE)) == 0); + + LookupResult result; + js_object->LocalLookupRealNamedProperty(*key_string, &result); + + PropertyAttributes attr = static_cast(unchecked); + + // Take special care when attributes are different and there is already + // a property. For simplicity we normalize the property which enables us + // to not worry about changing the instance_descriptor and creating a new + // map. The current version of SetObjectProperty does not handle attributes + // correctly in the case where a property is a field and is reset with + // new attributes. + if (result.IsProperty() && attr != result.GetAttributes()) { + PropertyDetails details = PropertyDetails(attr, NORMAL); + // New attributes - normalize to avoid writing to instance descriptor + js_object->NormalizeProperties(KEEP_INOBJECT_PROPERTIES, 0); + return js_object->SetNormalizedProperty(*key_string, *obj_value, details); + } + + return Runtime::SetObjectProperty(js_object, name, obj_value, attr); +} + + Object* Runtime::SetObjectProperty(Handle object, Handle key, Handle value, diff --git a/src/runtime.h b/src/runtime.h index b2b8609..96796d0 100644 --- a/src/runtime.h +++ b/src/runtime.h @@ -215,6 +215,8 @@ namespace internal { F(ResolvePossiblyDirectEval, 3, 2) \ \ F(SetProperty, -1 /* 3 or 4 */, 1) \ + F(DefineOrRedefineDataProperty, 4, 1) \ + F(DefineOrRedefineAccessorProperty, 5, 1) \ F(IgnoreAttributesAndSetProperty, -1 /* 3 or 4 */, 1) \ \ /* Arrays */ \ diff --git a/src/runtime.js b/src/runtime.js index c4c855e..10ef98e 100644 --- a/src/runtime.js +++ b/src/runtime.js @@ -506,6 +506,16 @@ function ToPrimitive(x, hint) { } +// ECMA-262, section 9.2, page 30 +function ToBoolean(x) { + if (IS_BOOLEAN(x)) return x; + if (IS_STRING(x)) return x.length != 0; + if (x == null) return false; + if (IS_NUMBER(x)) return !((x == 0) || NUMBER_IS_NAN(x)); + return true; +} + + // ECMA-262, section 9.3, page 31. function ToNumber(x) { if (IS_NUMBER(x)) return x; @@ -526,16 +536,6 @@ function ToString(x) { } -// ... where did this come from? -function ToBoolean(x) { - if (IS_BOOLEAN(x)) return x; - if (IS_STRING(x)) return x.length != 0; - if (x == null) return false; - if (IS_NUMBER(x)) return !((x == 0) || NUMBER_IS_NAN(x)); - return true; -} - - // ECMA-262, section 9.9, page 36. function ToObject(x) { if (IS_STRING(x)) return new $String(x); @@ -569,6 +569,25 @@ function ToInt32(x) { } +// ES5, section 9.12 +function SameValue(x, y) { + if (typeof x != typeof y) return false; + if (IS_NULL_OR_UNDEFINED(x)) return true; + if (IS_NUMBER(x)) { + if (NUMBER_IS_NAN(x) && NUMBER_IS_NAN(y)) return true; + // x is +0 and y is -0 or vice versa + if (x === 0 && y === 0 && !%_IsSmi(x) && !%_IsSmi(y) && + ((1 / x < 0 && 1 / y > 0) || (1 / x > 0 && 1 / y < 0))) { + return false; + } + return x == y; + } + if (IS_STRING(x)) return %StringEquals(x, y); + if (IS_BOOLEAN(x))return %NumberEquals(%ToNumber(x),%ToNumber(y)); + + return %_ObjectEquals(x, y); +} + /* --------------------------------- - - - U t i l i t i e s - - - diff --git a/src/v8natives.js b/src/v8natives.js index 7475065..08bdf8b 100644 --- a/src/v8natives.js +++ b/src/v8natives.js @@ -307,7 +307,7 @@ function IsInconsistentDescriptor(desc) { // ES5 8.10.4 function FromPropertyDescriptor(desc) { - if(IS_UNDEFINED(desc)) return desc; + if (IS_UNDEFINED(desc)) return desc; var obj = new $Object(); if (IsDataDescriptor(desc)) { obj.value = desc.getValue(); @@ -333,7 +333,6 @@ function ToPropertyDescriptor(obj) { desc.setEnumerable(ToBoolean(obj.enumerable)); } - if ("configurable" in obj) { desc.setConfigurable(ToBoolean(obj.configurable)); } @@ -377,7 +376,9 @@ function PropertyDescriptor() { this.writable_ = false; this.hasWritable_ = false; this.enumerable_ = false; + this.hasEnumerable_ = false; this.configurable_ = false; + this.hasConfigurable_ = false; this.get_ = void 0; this.hasGetter_ = false; this.set_ = void 0; @@ -396,8 +397,14 @@ PropertyDescriptor.prototype.getValue = function() { } +PropertyDescriptor.prototype.hasValue = function() { + return this.hasValue_; +} + + PropertyDescriptor.prototype.setEnumerable = function(enumerable) { this.enumerable_ = enumerable; + this.hasEnumerable_ = true; } @@ -406,6 +413,11 @@ PropertyDescriptor.prototype.isEnumerable = function () { } +PropertyDescriptor.prototype.hasEnumerable = function() { + return this.hasEnumerable_; +} + + PropertyDescriptor.prototype.setWritable = function(writable) { this.writable_ = writable; this.hasWritable_ = true; @@ -419,6 +431,12 @@ PropertyDescriptor.prototype.isWritable = function() { PropertyDescriptor.prototype.setConfigurable = function(configurable) { this.configurable_ = configurable; + this.hasConfigurable_ = true; +} + + +PropertyDescriptor.prototype.hasConfigurable = function() { + return this.hasConfigurable_; } @@ -438,6 +456,11 @@ PropertyDescriptor.prototype.getGet = function() { } +PropertyDescriptor.prototype.hasGetter = function() { + return this.hasGetter_; +} + + PropertyDescriptor.prototype.setSet = function(set) { this.set_ = set; this.hasSetter_ = true; @@ -449,6 +472,12 @@ PropertyDescriptor.prototype.getSet = function() { } +PropertyDescriptor.prototype.hasSetter = function() { + return this.hasSetter_; +} + + + // ES5 section 8.12.1. function GetOwnProperty(obj, p) { var desc = new PropertyDescriptor(); @@ -458,8 +487,7 @@ function GetOwnProperty(obj, p) { // obj is an accessor [true, Get, Set, Enumerable, Configurable] var props = %GetOwnProperty(ToObject(obj), ToString(p)); - if (IS_UNDEFINED(props)) - return void 0; + if (IS_UNDEFINED(props)) return void 0; // This is an accessor if (props[0]) { @@ -476,16 +504,89 @@ function GetOwnProperty(obj, p) { } -// ES5 8.12.9. This version cannot cope with the property p already -// being present on obj. +// ES5 section 8.12.2. +function GetProperty(obj, p) { + var prop = GetOwnProperty(obj); + if (!IS_UNDEFINED(prop)) return prop; + var proto = obj.__proto__; + if (IS_NULL(proto)) return void 0; + return GetProperty(proto, p); +} + + +// ES5 section 8.12.6 +function HasProperty(obj, p) { + var desc = GetProperty(obj, p); + return IS_UNDEFINED(desc) ? false : true; +} + + +// ES5 8.12.9. function DefineOwnProperty(obj, p, desc, should_throw) { - var flag = desc.isEnumerable() ? 0 : DONT_ENUM; - if (IsDataDescriptor(desc)) { - flag |= desc.isWritable() ? 0 : (DONT_DELETE | READ_ONLY); - %SetProperty(obj, p, desc.getValue(), flag); + var current = GetOwnProperty(obj, p); + var extensible = %IsExtensible(ToObject(obj)); + + // Error handling according to spec. + // Step 3 + if (IS_UNDEFINED(current) && !extensible) + throw MakeTypeError("define_disallowed", ["defineProperty"]); + + if (!IS_UNDEFINED(current) && !current.isConfigurable()) { + // Step 7 + if (desc.isConfigurable() || desc.isEnumerable() != current.isEnumerable()) + throw MakeTypeError("redefine_disallowed", ["defineProperty"]); + // Step 9 + if (IsDataDescriptor(current) != IsDataDescriptor(desc)) + throw MakeTypeError("redefine_disallowed", ["defineProperty"]); + // Step 10 + if (IsDataDescriptor(current) && IsDataDescriptor(desc)) { + if (!current.isWritable() && desc.isWritable()) + throw MakeTypeError("redefine_disallowed", ["defineProperty"]); + if (!current.isWritable() && desc.hasValue() && + !SameValue(desc.getValue(), current.getValue())) { + throw MakeTypeError("redefine_disallowed", ["defineProperty"]); + } + } + // Step 11 + if (IsAccessorDescriptor(desc) && IsAccessorDescriptor(current)) { + if (desc.hasSetter() && !SameValue(desc.getSet(), current.getSet())){ + throw MakeTypeError("redefine_disallowed", ["defineProperty"]); + } + if (desc.hasGetter() && !SameValue(desc.getGet(),current.getGet())) + throw MakeTypeError("redefine_disallowed", ["defineProperty"]); + } + } + + // Send flags - enumerable and configurable are common - writable is + // only send to the data descriptor. + // Take special care if enumerable and configurable is not defined on + // desc (we need to preserve the existing values from current). + var flag = NONE; + if (desc.hasEnumerable()) { + flag |= desc.isEnumerable() ? 0 : DONT_ENUM; + } else if (!IS_UNDEFINED(current)) { + flag |= current.isEnumerable() ? 0 : DONT_ENUM; } else { - if (IS_FUNCTION(desc.getGet())) %DefineAccessor(obj, p, GETTER, desc.getGet(), flag); - if (IS_FUNCTION(desc.getSet())) %DefineAccessor(obj, p, SETTER, desc.getSet(), flag); + flag |= DONT_ENUM; + } + + if (desc.hasConfigurable()) { + flag |= desc.isConfigurable() ? 0 : DONT_DELETE; + } else if (!IS_UNDEFINED(current)) { + flag |= current.isConfigurable() ? 0 : DONT_DELETE; + } else + flag |= DONT_DELETE; + + if (IsDataDescriptor(desc) || IsGenericDescriptor(desc)) { + flag |= desc.isWritable() ? 0 : READ_ONLY; + %DefineOrRedefineDataProperty(obj, p, desc.getValue(), flag); + } else { + if (desc.hasGetter() && IS_FUNCTION(desc.getGet())) { + %DefineOrRedefineAccessorProperty(obj, p, GETTER, desc.getGet(), flag); + } + if (desc.hasSetter() && IS_FUNCTION(desc.getSet())) { + %DefineOrRedefineAccessorProperty(obj, p, SETTER, desc.getSet(), flag); + } } return true; } @@ -558,10 +659,21 @@ function ObjectCreate(proto, properties) { } -// ES5 section 15.2.3.7. This version cannot cope with the properies already -// being present on obj. Therefore it is not exposed as -// Object.defineProperties yet. +// ES5 section 15.2.3.6. +function ObjectDefineProperty(obj, p, attributes) { + if ((!IS_OBJECT(obj) || IS_NULL_OR_UNDEFINED(obj)) && !IS_FUNCTION(obj)) + throw MakeTypeError("obj_ctor_property_non_object", ["defineProperty"]); + var name = ToString(p); + var desc = ToPropertyDescriptor(attributes); + DefineOwnProperty(obj, name, desc, true); + return obj; +} + + +// ES5 section 15.2.3.7. function ObjectDefineProperties(obj, properties) { + if ((!IS_OBJECT(obj) || IS_NULL_OR_UNDEFINED(obj)) && !IS_FUNCTION(obj)) + throw MakeTypeError("obj_ctor_property_non_object", ["defineProperties"]); var props = ToObject(properties); var key_values = []; for (var key in props) { @@ -611,6 +723,8 @@ function SetupObject() { InstallFunctions($Object, DONT_ENUM, $Array( "keys", ObjectKeys, "create", ObjectCreate, + "defineProperty", ObjectDefineProperty, + "defineProperties", ObjectDefineProperties, "getPrototypeOf", ObjectGetPrototypeOf, "getOwnPropertyDescriptor", ObjectGetOwnPropertyDescriptor, "getOwnPropertyNames", ObjectGetOwnPropertyNames diff --git a/test/cctest/test-api.cc b/test/cctest/test-api.cc index f71b325..4aa0280 100644 --- a/test/cctest/test-api.cc +++ b/test/cctest/test-api.cc @@ -2297,6 +2297,103 @@ THREADED_TEST(SimplePropertyRead) { } } +THREADED_TEST(DefinePropertyOnAPIAccessor) { + v8::HandleScope scope; + Local templ = ObjectTemplate::New(); + templ->SetAccessor(v8_str("x"), GetXValue, NULL, v8_str("donut")); + LocalContext context; + context->Global()->Set(v8_str("obj"), templ->NewInstance()); + + // Uses getOwnPropertyDescriptor to check the configurable status + Local