Cleanup defineOwnProperty

Make it a vtable method as required by the ES7 spec.
Change all calls sites to call through the virtual
function.

Adjust ArgumentsObject and give it it's own
defineOwnProperty implementation instead of hacking
it into the base implementation.

Move the array object specific handling into a
reimplementation.

Change-Id: I48c960c4c69f99b178628c94b4808be2bab0dccc
Reviewed-by: Simon Hausmann <simon.hausmann@qt.io>

8 files changed, 152 insertions, 148 deletions

diff --git a/src/qml/jsruntime/qv4argumentsobject.cpp b/src/qml/jsruntime/qv4argumentsobject.cpp
index 907d2ebfc8..ba9bf9652d 100644
--- a/src/qml/jsruntime/qv4argumentsobject.cpp
+++ b/src/qml/jsruntime/qv4argumentsobject.cpp
@@ -120,45 +120,49 @@ void ArgumentsObject::fullyCreate()
     d()->fullyCreated = true;
 }
 
-bool ArgumentsObject::defineOwnProperty(ExecutionEngine *engine, uint index, const Property *desc, PropertyAttributes attrs)
+bool ArgumentsObject::defineOwnProperty(Managed *m, Identifier id, const Property *desc, PropertyAttributes attrs)
 {
-    fullyCreate();
+    if (!id.isArrayIndex())
+        return Object::defineOwnProperty(m, id, desc, attrs);
 
-    Scope scope(engine);
+    ArgumentsObject *a = static_cast<ArgumentsObject *>(m);
+    a->fullyCreate();
+
+    uint index = id.asArrayIndex();
+    Scope scope(m);
     ScopedProperty map(scope);
     PropertyAttributes mapAttrs;
-    uint numAccessors = qMin(d()->nFormals, context()->argc());
+    uint numAccessors = qMin(a->d()->nFormals, a->context()->argc());
     bool isMapped = false;
-    if (arrayData() && index < numAccessors &&
-        arrayData()->attributes(index).isAccessor() &&
-        arrayData()->get(index) == scope.engine->argumentsAccessors[index].getter()->asReturnedValue())
+    if (a->arrayData() && index < numAccessors &&
+        a->arrayData()->attributes(index).isAccessor() &&
+        a->arrayData()->get(index) == scope.engine->argumentsAccessors[index].getter()->asReturnedValue())
         isMapped = true;
 
     if (isMapped) {
-        Q_ASSERT(arrayData());
-        mapAttrs = arrayData()->attributes(index);
-        arrayData()->getProperty(index, map, &mapAttrs);
-        setArrayAttributes(index, Attr_Data);
-        PropertyIndex arrayIndex{ arrayData(), arrayData()->values.values + arrayData()->mappedIndex(index) };
-        arrayIndex.set(scope.engine, d()->mappedArguments->values[index]);
+        Q_ASSERT(a->arrayData());
+        mapAttrs = a->arrayData()->attributes(index);
+        a->arrayData()->getProperty(index, map, &mapAttrs);
+        a->setArrayAttributes(index, Attr_Data);
+        PropertyIndex arrayIndex{ a->arrayData(), a->arrayData()->values.values + a->arrayData()->mappedIndex(index) };
+        arrayIndex.set(scope.engine, a->d()->mappedArguments->values[index]);
     }
 
-    bool result = Object::defineOwnProperty2(scope.engine, index, desc, attrs);
-    if (!result) {
+    bool result = Object::defineOwnProperty(m, id, desc, attrs);
+    if (!result)
         return false;
-    }
 
     if (isMapped && attrs.isData()) {
-        Q_ASSERT(arrayData());
+        Q_ASSERT(a->arrayData());
         ScopedFunctionObject setter(scope, map->setter());
         JSCallData jsCallData(scope, 1);
-        *jsCallData->thisObject = this->asReturnedValue();
+        *jsCallData->thisObject = a->asReturnedValue();
         jsCallData->args[0] = desc->value;
         setter->call(jsCallData);
 
         if (attrs.isWritable()) {
-            setArrayAttributes(index, mapAttrs);
-            arrayData()->setProperty(engine, index, map);
+            a->setArrayAttributes(index, mapAttrs);
+            a->arrayData()->setProperty(m->engine(), index, map);
         }
     }
 
diff --git a/src/qml/jsruntime/qv4argumentsobject_p.h b/src/qml/jsruntime/qv4argumentsobject_p.h
index 79175b8767..46110e465f 100644
--- a/src/qml/jsruntime/qv4argumentsobject_p.h
+++ b/src/qml/jsruntime/qv4argumentsobject_p.h
@@ -146,7 +146,7 @@ struct ArgumentsObject: Object {
         return m->vtable() == staticVTable();
     }
 
-    bool defineOwnProperty(ExecutionEngine *engine, uint index, const Property *desc, PropertyAttributes attrs);
+    static bool defineOwnProperty(Managed *m, Identifier id, const Property *desc, PropertyAttributes attrs);
     static ReturnedValue getIndexed(const Managed *m, uint index, bool *hasProperty);
     static bool putIndexed(Managed *m, uint index, const Value &value);
     static bool deleteProperty(Managed *m, Identifier id);
diff --git a/src/qml/jsruntime/qv4context.cpp b/src/qml/jsruntime/qv4context.cpp
index ebe0ce53ea..0ae79c8b36 100644
--- a/src/qml/jsruntime/qv4context.cpp
+++ b/src/qml/jsruntime/qv4context.cpp
@@ -178,12 +178,13 @@ void ExecutionContext::createMutableBinding(String *name, bool deletable)
         ctx = ctx->d()->outer;
     }
 
-    if (activation->getOwnProperty(name->toPropertyKey()) != Attr_Invalid)
+    Identifier id = name->toPropertyKey();
+    if (activation->getOwnProperty(id) != Attr_Invalid)
         return;
     ScopedProperty desc(scope);
     PropertyAttributes attrs(Attr_Data);
     attrs.setConfigurable(deletable);
-    if (!activation->__defineOwnProperty__(scope.engine, name, desc, attrs))
+    if (!activation->defineOwnProperty(id, desc, attrs))
         scope.engine->throwTypeError();
 }
 
diff --git a/src/qml/jsruntime/qv4object.cpp b/src/qml/jsruntime/qv4object.cpp
index e09022b60f..1597c35319 100644
--- a/src/qml/jsruntime/qv4object.cpp
+++ b/src/qml/jsruntime/qv4object.cpp
@@ -661,110 +661,7 @@ bool Object::internalDeleteProperty(Identifier id)
     return true;
 }
 
-// Section 8.12.9
-bool Object::__defineOwnProperty__(ExecutionEngine *engine, StringOrSymbol *name, const Property *p, PropertyAttributes attrs)
-{
-    uint idx = name->asArrayIndex();
-    if (idx != UINT_MAX)
-        return __defineOwnProperty__(engine, idx, p, attrs);
-
-    Scope scope(engine);
-    name->makeIdentifier();
-
-    uint memberIndex;
-
-    if (isArrayObject() && name->identifier() == engine->id_length()->identifier()) {
-        Q_ASSERT(Heap::ArrayObject::LengthPropertyIndex == internalClass()->find(engine->id_length()->identifier()));
-        ScopedProperty lp(scope);
-        PropertyAttributes cattrs;
-        getProperty(Heap::ArrayObject::LengthPropertyIndex, lp, &cattrs);
-        if (attrs.isEmpty() || p->isSubset(attrs, lp, cattrs))
-            return true;
-        if (!cattrs.isWritable() || attrs.type() == PropertyAttributes::Accessor || attrs.isConfigurable() || attrs.isEnumerable())
-            return false;
-        bool succeeded = true;
-        if (attrs.type() == PropertyAttributes::Data) {
-            bool ok;
-            uint l = p->value.asArrayLength(&ok);
-            if (!ok) {
-                ScopedValue v(scope, p->value);
-                engine->throwRangeError(v);
-                return false;
-            }
-            succeeded = setArrayLength(l);
-        }
-        if (attrs.hasWritable() && !attrs.isWritable()) {
-            cattrs.setWritable(false);
-            Heap::InternalClass::changeMember(this, engine->id_length()->identifier(), cattrs);
-        }
-        if (!succeeded)
-            return false;
-        return true;
-    }
-
-    // Clause 1
-    memberIndex = internalClass()->find(name->identifier());
-
-    if (memberIndex == UINT_MAX) {
-        // clause 3
-        if (!isExtensible())
-            return false;
-        // clause 4
-        ScopedProperty pd(scope);
-        pd->copy(p, attrs);
-        pd->fullyPopulated(&attrs);
-        insertMember(name, pd, attrs);
-        return true;
-    }
-
-    return __defineOwnProperty__(engine, memberIndex, name, p, attrs);
-}
-
-bool Object::__defineOwnProperty__(ExecutionEngine *engine, uint index, const Property *p, PropertyAttributes attrs)
-{
-    // 15.4.5.1, 4b
-    if (isArrayObject() && index >= getLength() && !internalClass()->propertyData[Heap::ArrayObject::LengthPropertyIndex].isWritable())
-        return false;
-
-    if (ArgumentsObject::isNonStrictArgumentsObject(this))
-        return static_cast<ArgumentsObject *>(this)->defineOwnProperty(engine, index, p, attrs);
-
-    return defineOwnProperty2(engine, index, p, attrs);
-}
-
-bool Object::defineOwnProperty2(ExecutionEngine *engine, uint index, const Property *p, PropertyAttributes attrs)
-{
-    bool hasProperty = 0;
-
-    // Clause 1
-    if (arrayData()) {
-        hasProperty = arrayData()->mappedIndex(index) != UINT_MAX;
-        if (!hasProperty && isStringObject())
-            hasProperty = (index < static_cast<StringObject *>(this)->length());
-    }
-
-    if (!hasProperty) {
-        // clause 3
-        if (!isExtensible())
-            return false;
-        // clause 4
-        Scope scope(engine);
-        ScopedProperty pp(scope);
-        pp->copy(p, attrs);
-        pp->fullyPopulated(&attrs);
-        if (attrs == Attr_Data) {
-            ScopedValue v(scope, pp->value);
-            arraySet(index, v);
-        } else {
-            arraySet(index, pp, attrs);
-        }
-        return true;
-    }
-
-    return __defineOwnProperty__(engine, index, nullptr, p, attrs);
-}
-
-bool Object::__defineOwnProperty__(ExecutionEngine *engine, uint index, StringOrSymbol *member, const Property *p, PropertyAttributes attrs)
+bool Object::internalDefineOwnProperty(ExecutionEngine *engine, uint index, StringOrSymbol *member, const Property *p, PropertyAttributes attrs)
 {
     // clause 5
     if (attrs.isEmpty())
@@ -852,15 +749,6 @@ bool Object::__defineOwnProperty__(ExecutionEngine *engine, uint index, StringOr
     return true;
 }
 
-
-bool Object::__defineOwnProperty__(ExecutionEngine *engine, const QString &name, const Property *p, PropertyAttributes attrs)
-{
-    Scope scope(engine);
-    ScopedString s(scope, engine->newString(name));
-    return __defineOwnProperty__(engine, s, p, attrs);
-}
-
-
 void Object::copyArrayData(Object *other)
 {
     Q_ASSERT(isArrayObject());
@@ -989,6 +877,58 @@ PropertyAttributes Object::getOwnProperty(Managed *m, Identifier id, Property *p
     return Attr_Invalid;
 }
 
+bool Object::defineOwnProperty(Managed *m, Identifier id, const Property *p, PropertyAttributes attrs)
+{
+    Object *o = static_cast<Object *>(m);
+    Scope scope(o);
+
+    if (id.isArrayIndex()) {
+        uint index = id.asArrayIndex();
+
+        bool hasProperty = false;
+
+        if (o->arrayData()) {
+            hasProperty = o->arrayData()->mappedIndex(index) != UINT_MAX;
+            if (!hasProperty && o->isStringObject())
+                hasProperty = (index < static_cast<StringObject *>(o)->length());
+        }
+
+        if (!hasProperty) {
+            if (!o->isExtensible())
+                return false;
+
+            ScopedProperty pp(scope);
+            pp->copy(p, attrs);
+            pp->fullyPopulated(&attrs);
+            if (attrs == Attr_Data) {
+                ScopedValue v(scope, pp->value);
+                o->arraySet(index, v);
+            } else {
+                o->arraySet(index, pp, attrs);
+            }
+            return true;
+        }
+
+        return o->internalDefineOwnProperty(scope.engine, index, nullptr, p, attrs);
+    }
+
+    uint memberIndex = o->internalClass()->find(id);
+    Scoped<StringOrSymbol> name(scope, id.asHeapObject());
+
+    if (memberIndex == UINT_MAX) {
+        if (!o->isExtensible())
+            return false;
+
+        ScopedProperty pd(scope);
+        pd->copy(p, attrs);
+        pd->fullyPopulated(&attrs);
+        o->insertMember(name, pd, attrs);
+        return true;
+    }
+
+    return o->internalDefineOwnProperty(scope.engine, memberIndex, name, p, attrs);
+}
+
 bool Object::isExtensible(const Managed *m)
 {
     return m->d()->internalClass->extensible;
@@ -1103,3 +1043,57 @@ QStringList ArrayObject::toQStringList() const
     }
     return result;
 }
+
+bool ArrayObject::defineOwnProperty(Managed *m, Identifier id, const Property *p, PropertyAttributes attrs)
+{
+    Q_ASSERT(m->isArrayObject());
+    ArrayObject *a = static_cast<ArrayObject *>(m);
+
+    if (id.isArrayIndex()) {
+        uint index = id.asArrayIndex();
+        uint len = a->getLength();
+        if (index >= len && !a->internalClass()->propertyData[Heap::ArrayObject::LengthPropertyIndex].isWritable())
+            return false;
+
+        bool succeeded = Object::defineOwnProperty(m, id, p, attrs);
+        if (!succeeded)
+            return false;
+
+        if (index >= len)
+            a->setArrayLengthUnchecked(index + 1);
+
+        return true;
+    }
+
+    ExecutionEngine *engine = m->engine();
+    if (id == engine->id_length()->identifier()) {
+        Scope scope(engine);
+        Q_ASSERT(Heap::ArrayObject::LengthPropertyIndex == a->internalClass()->find(engine->id_length()->identifier()));
+        ScopedProperty lp(scope);
+        PropertyAttributes cattrs;
+        a->getProperty(Heap::ArrayObject::LengthPropertyIndex, lp, &cattrs);
+        if (attrs.isEmpty() || p->isSubset(attrs, lp, cattrs))
+            return true;
+        if (!cattrs.isWritable() || attrs.type() == PropertyAttributes::Accessor || attrs.isConfigurable() || attrs.isEnumerable())
+            return false;
+        bool succeeded = true;
+        if (attrs.type() == PropertyAttributes::Data) {
+            bool ok;
+            uint l = p->value.asArrayLength(&ok);
+            if (!ok) {
+                ScopedValue v(scope, p->value);
+                engine->throwRangeError(v);
+                return false;
+            }
+            succeeded = a->setArrayLength(l);
+        }
+        if (attrs.hasWritable() && !attrs.isWritable()) {
+            cattrs.setWritable(false);
+            Heap::InternalClass::changeMember(a, engine->id_length()->identifier(), cattrs);
+        }
+        if (!succeeded)
+            return false;
+        return true;
+    }
+    return Object::defineOwnProperty(m, id, p, attrs);
+}
diff --git a/src/qml/jsruntime/qv4object_p.h b/src/qml/jsruntime/qv4object_p.h
index 07866b4e16..e57708d8e4 100644
--- a/src/qml/jsruntime/qv4object_p.h
+++ b/src/qml/jsruntime/qv4object_p.h
@@ -176,6 +176,7 @@ struct ObjectVTable
     bool (*deleteProperty)(Managed *m, Identifier id);
     bool (*hasProperty)(const Managed *m, Identifier id);
     PropertyAttributes (*getOwnProperty)(Managed *m, Identifier id, Property *p);
+    bool (*defineOwnProperty)(Managed *m, Identifier id, const Property *p, PropertyAttributes attrs);
     bool (*isExtensible)(const Managed *);
     bool (*preventExtensions)(Managed *);
     Heap::Object *(*getPrototypeOf)(const Managed *);
@@ -198,6 +199,7 @@ const QV4::ObjectVTable classname::static_vtbl =    \
     deleteProperty,                             \
     hasProperty,                                \
     getOwnProperty,                             \
+    defineOwnProperty,                          \
     isExtensible,                               \
     preventExtensions,                          \
     getPrototypeOf,                             \
@@ -258,11 +260,9 @@ struct Q_QML_EXPORT Object: Managed {
         return vtable()->hasProperty(this, id);
     }
 
-    bool __defineOwnProperty__(ExecutionEngine *engine, uint index, StringOrSymbol *member, const Property *p, PropertyAttributes attrs);
-    bool __defineOwnProperty__(ExecutionEngine *engine, StringOrSymbol *name, const Property *p, PropertyAttributes attrs);
-    bool __defineOwnProperty__(ExecutionEngine *engine, uint index, const Property *p, PropertyAttributes attrs);
-    bool __defineOwnProperty__(ExecutionEngine *engine, const QString &name, const Property *p, PropertyAttributes attrs);
-    bool defineOwnProperty2(ExecutionEngine *engine, uint index, const Property *p, PropertyAttributes attrs);
+    bool defineOwnProperty(Identifier id, const Property *p, PropertyAttributes attrs) {
+        return vtable()->defineOwnProperty(this, id, p, attrs);
+    }
 
     //
     // helpers
@@ -443,6 +443,7 @@ protected:
     static bool deleteProperty(Managed *m, Identifier id);
     static bool hasProperty(const Managed *m, Identifier id);
     static PropertyAttributes getOwnProperty(Managed *m, Identifier id, Property *p);
+    static bool defineOwnProperty(Managed *m, Identifier id, const Property *p, PropertyAttributes attrs);
     static bool isExtensible(const Managed *m);
     static bool preventExtensions(Managed *);
     static Heap::Object *getPrototypeOf(const Managed *);
@@ -452,6 +453,7 @@ protected:
     static ReturnedValue instanceOf(const Object *typeObject, const Value &var);
 
 private:
+    bool internalDefineOwnProperty(ExecutionEngine *engine, uint index, StringOrSymbol *member, const Property *p, PropertyAttributes attrs);
     ReturnedValue internalGet(StringOrSymbol *name, bool *hasProperty) const;
     ReturnedValue internalGetIndexed(uint index, bool *hasProperty) const;
     bool internalPut(StringOrSymbol *name, const Value &value);
@@ -532,6 +534,9 @@ struct ArrayObject: Object {
     static qint64 getLength(const Managed *m);
 
     QStringList toQStringList() const;
+protected:
+    static bool defineOwnProperty(Managed *m, Identifier id, const Property *p, PropertyAttributes attrs);
+
 };
 
 inline void Object::setArrayLengthUnchecked(uint l)
diff --git a/src/qml/jsruntime/qv4objectproto.cpp b/src/qml/jsruntime/qv4objectproto.cpp
index 9d52ab1980..ef3e4268c0 100644
--- a/src/qml/jsruntime/qv4objectproto.cpp
+++ b/src/qml/jsruntime/qv4objectproto.cpp
@@ -295,7 +295,7 @@ ReturnedValue ObjectPrototype::method_defineProperty(const FunctionObject *b, co
     if (scope.engine->hasException)
         return QV4::Encode::undefined();
 
-    if (!O->__defineOwnProperty__(scope.engine, name, pd, attrs))
+    if (!O->defineOwnProperty(name->toPropertyKey(), pd, attrs))
         THROW_TYPE_ERROR();
 
     return O.asReturnedValue();
@@ -332,9 +332,9 @@ ReturnedValue ObjectPrototype::method_defineProperties(const FunctionObject *b,
         return QV4::Encode::undefined();
         bool ok;
         if (name)
-            ok = O->__defineOwnProperty__(scope.engine, name, n, nattrs);
+            ok = O->defineOwnProperty(name->toPropertyKey(), n, nattrs);
         else
-            ok = O->__defineOwnProperty__(scope.engine, index, n, nattrs);
+            ok = O->defineOwnProperty(Identifier::fromArrayIndex(index), n, nattrs);
         if (!ok)
             THROW_TYPE_ERROR();
     }
@@ -644,7 +644,7 @@ ReturnedValue ObjectPrototype::method_defineGetter(const FunctionObject *b, cons
     ScopedProperty pd(scope);
     pd->value = f;
     pd->set = Primitive::emptyValue();
-    bool ok = o->__defineOwnProperty__(scope.engine, prop, pd, Attr_Accessor);
+    bool ok = o->defineOwnProperty(prop->toPropertyKey(), pd, Attr_Accessor);
     if (!ok)
         THROW_TYPE_ERROR();
     RETURN_UNDEFINED();
@@ -674,7 +674,7 @@ ReturnedValue ObjectPrototype::method_defineSetter(const FunctionObject *b, cons
     ScopedProperty pd(scope);
     pd->value = Primitive::emptyValue();
     pd->set = f;
-    bool ok = o->__defineOwnProperty__(scope.engine, prop, pd, Attr_Accessor);
+    bool ok = o->defineOwnProperty(prop->toPropertyKey(), pd, Attr_Accessor);
     if (!ok)
         THROW_TYPE_ERROR();
     RETURN_UNDEFINED();
diff --git a/src/qml/jsruntime/qv4reflect.cpp b/src/qml/jsruntime/qv4reflect.cpp
index 9b4a5a2375..2594f53b14 100644
--- a/src/qml/jsruntime/qv4reflect.cpp
+++ b/src/qml/jsruntime/qv4reflect.cpp
@@ -131,7 +131,7 @@ ReturnedValue Reflect::method_defineProperty(const FunctionObject *f, const Valu
     if (scope.engine->hasException)
         return QV4::Encode::undefined();
 
-    bool result = O->__defineOwnProperty__(scope.engine, name, pd, attrs);
+    bool result = O->defineOwnProperty(name->toPropertyKey(), pd, attrs);
 
     return Encode(result);
 }
diff --git a/src/qml/jsruntime/qv4runtime.cpp b/src/qml/jsruntime/qv4runtime.cpp
index 04aff9cad0..64efaa9a86 100644
--- a/src/qml/jsruntime/qv4runtime.cpp
+++ b/src/qml/jsruntime/qv4runtime.cpp
@@ -1473,7 +1473,7 @@ ReturnedValue Runtime::method_objectLiteral(ExecutionEngine *engine, int classId
             pd->value = Primitive::emptyValue();
             pd->set = args[2];
         }
-        bool ok = o->__defineOwnProperty__(scope.engine, name, pd, (arg == ObjectLiteralArgument::Value ? Attr_Data : Attr_Accessor));
+        bool ok = o->defineOwnProperty(name->toPropertyKey(), pd, (arg == ObjectLiteralArgument::Value ? Attr_Data : Attr_Accessor));
         if (!ok)
             return engine->throwTypeError();