Fix creation of object literals

Our method to create object literals wasn't compliant with the
ES7 spec, as we would in some cases re-order the properties.

This violated the spec which required properties to be created
in order, so that for-of would also iterate over them in creation
order.

As a nice side effect, this simplifies the code and gets a couple
of test cases using computed property names to pass.

Task-number: QTBUG-62512
Change-Id: I6dfe004357c5d46a0890027f4fd9e2d1e1a2a17a
Reviewed-by: Simon Hausmann <simon.hausmann@qt.io>

12 files changed, 117 insertions, 179 deletions

diff --git a/src/qml/compiler/qv4codegen.cpp b/src/qml/compiler/qv4codegen.cpp
index 8d0d0695f5..9d15e2ef6b 100644
--- a/src/qml/compiler/qv4codegen.cpp
+++ b/src/qml/compiler/qv4codegen.cpp
@@ -1944,138 +1944,94 @@ bool Codegen::visit(ObjectPattern *ast)
 
     RegisterScope scope(this);
 
-    for (PatternPropertyList *it = ast->properties; it; it = it->next) {
+    QStringList members;
+
+    int argc = 0;
+    int args = 0;
+    auto push = [this, &args, &argc](const Reference &arg) {
+        int temp = bytecodeGenerator->newRegister();
+        if (argc == 0)
+            args = temp;
+        (void) arg.storeOnStack(temp);
+        ++argc;
+    };
+
+    PatternPropertyList *it = ast->properties;
+    for (; it; it = it->next) {
         PatternProperty *p = it->property;
         AST::ComputedPropertyName *cname = AST::cast<AST::ComputedPropertyName *>(p->name);
-        if (cname) {
-            Reference name = expression(cname->expression);
-            if (hasError)
-                return false;
-            name = name.storeOnStack();
-            computedProperties.append({name, ObjectPropertyValue()});
-        }
+        if (cname || p->type == PatternProperty::Getter || p->type == PatternProperty::Setter)
+            break;
         QString name = p->name->asString();
-        ObjectPropertyValue &v = cname ? computedProperties.last().second : valueMap[name];
-        if (p->type == PatternProperty::Literal) {
+        uint arrayIndex = QV4::String::toArrayIndex(name);
+        if (arrayIndex != UINT_MAX)
+            break;
+        if (members.contains(name))
+            break;
+        members.append(name);
+
+        {
+            RegisterScope innerScope(this);
             Reference value = expression(p->initializer);
             if (hasError)
                 return false;
-
-            v.rvalue = value.storeOnStack();
-            v.getter = v.setter = -1;
-        } else if (p->type == PatternProperty::Getter || p->type == PatternProperty::Setter) {
-            FunctionExpression *f = AST::cast<FunctionExpression *>(p->initializer);
-            Q_ASSERT(f);
-            const int function = defineFunction(name, f, f->formals, f->body);
-            v.rvalue = Reference();
-            if (p->type == PatternProperty::Getter)
-                v.getter = function;
-            else
-                v.setter = function;
-        } else {
-            Q_UNREACHABLE();
+            value.loadInAccumulator();
         }
+        push(Reference::fromAccumulator(this));
     }
 
-    QVector<QString> nonArrayKey, arrayKeyWithValue, arrayKeyWithGetterSetter;
-    bool needSparseArray = false; // set to true if any array index is bigger than 16
-
-    for (QMap<QString, ObjectPropertyValue>::iterator it = valueMap.begin(), eit = valueMap.end();
-            it != eit; ++it) {
-        QString name = it.key();
-        uint keyAsIndex = QV4::String::toArrayIndex(name);
-        if (keyAsIndex != std::numeric_limits<uint>::max()) {
-            it->keyAsIndex = keyAsIndex;
-            if (keyAsIndex > 16)
-                needSparseArray = true;
-            if (it->hasSetter() || it->hasGetter())
-                arrayKeyWithGetterSetter.append(name);
-            else
-                arrayKeyWithValue.append(name);
-        } else {
-            nonArrayKey.append(name);
-        }
-    }
+    int classId = jsUnitGenerator->registerJSClass(members);
 
-    int args = -1;
-    auto push = [this, &args](const Reference &arg) {
-        int temp = bytecodeGenerator->newRegister();
-        if (args == -1)
-            args = temp;
-        (void) arg.storeOnStack(temp);
-    };
+    // handle complex property setters
+    for (; it; it = it->next) {
+        PatternProperty *p = it->property;
+        AST::ComputedPropertyName *cname = AST::cast<AST::ComputedPropertyName *>(p->name);
+        ObjectLiteralArgument argType = ObjectLiteralArgument::Value;
+        if (p->type == PatternProperty::Getter)
+            argType = ObjectLiteralArgument::Getter;
+        else if (p->type == PatternProperty::Setter)
+            argType = ObjectLiteralArgument::Setter;
 
-    QVector<QV4::Compiler::JSUnitGenerator::MemberInfo> members;
+        Reference::fromConst(this, Encode(int(argType))).loadInAccumulator();
+        push(Reference::fromAccumulator(this));
 
-    Reference acc = Reference::fromAccumulator(this);
-    // generate the key/value pairs
-    for (const QString &key : qAsConst(nonArrayKey)) {
-        const ObjectPropertyValue &prop = valueMap[key];
-
-        if (prop.hasGetter() || prop.hasSetter()) {
-            Q_ASSERT(!prop.rvalue.isValid());
-            loadClosure(prop.getter);
-            push(acc);
-            loadClosure(prop.setter);
-            push(acc);
-            members.append({ key, true });
+        if (cname) {
+            RegisterScope innerScope(this);
+            Reference name = expression(cname->expression);
+            if (hasError)
+                return false;
+            name.loadInAccumulator();
         } else {
-            Q_ASSERT(prop.rvalue.isValid());
-            push(prop.rvalue);
-            members.append({ key, false });
+            QString name = p->name->asString();
+#if 0
+            uint arrayIndex = QV4::String::toArrayIndex(name);
+            if (arrayIndex != UINT_MAX) {
+                Reference::fromConst(this, Encode(arrayIndex)).loadInAccumulator();
+            } else
+#endif
+            {
+                Instruction::LoadRuntimeString instr;
+                instr.stringId = registerString(name);
+                bytecodeGenerator->addInstruction(instr);
+            }
         }
+        push(Reference::fromAccumulator(this));
+        {
+            RegisterScope innerScope(this);
+            Reference value = expression(p->initializer);
+            if (hasError)
+                return false;
+            value.loadInAccumulator();
+        }
+        push(Reference::fromAccumulator(this));
     }
 
-    // generate array entries with values
-    for (const QString &key : qAsConst(arrayKeyWithValue)) {
-        const ObjectPropertyValue &prop = valueMap[key];
-        Q_ASSERT(!prop.hasGetter() && !prop.hasSetter());
-        push(Reference::fromConst(this, Encode(prop.keyAsIndex)));
-        push(prop.rvalue);
-    }
-
-    // generate array entries with both a value and a setter
-    for (const QString &key : qAsConst(arrayKeyWithGetterSetter)) {
-        const ObjectPropertyValue &prop = valueMap[key];
-        Q_ASSERT(!prop.rvalue.isValid());
-        push(Reference::fromConst(this, Encode(prop.keyAsIndex)));
-        loadClosure(prop.getter);
-        push(acc);
-        loadClosure(prop.setter);
-        push(acc);
-    }
-
-    int classId = jsUnitGenerator->registerJSClass(members);
-
-    uint arrayGetterSetterCountAndFlags = arrayKeyWithGetterSetter.size();
-    arrayGetterSetterCountAndFlags |= needSparseArray << 30;
-
-    if (args == -1)
-        args = 0;
-
     Instruction::DefineObjectLiteral call;
     call.internalClassId = classId;
-    call.arrayValueCount = arrayKeyWithValue.size();
-    call.arrayGetterSetterCountAndFlags = arrayGetterSetterCountAndFlags;
+    call.argc = argc;
     call.args = Moth::StackSlot::createRegister(args);
     bytecodeGenerator->addInstruction(call);
     Reference result = Reference::fromAccumulator(this);
-
-    if (!computedProperties.isEmpty()) {
-        result = result.storeOnStack();
-        for (const auto &c : qAsConst(computedProperties)) {
-            // ### if RHS is an anonymous FunctionExpression, we need to set it's name to the computed name
-            Reference element = Reference::fromSubscript(result, c.first);
-            if (c.second.getter >= 0 || c.second.setter >= 0) {
-                throwSyntaxError(ast->firstSourceLocation(), QLatin1String("getter/setter with computed property names unimplemented."));
-                return false;// ###
-            } else {
-                c.second.rvalue.loadInAccumulator();
-                element.storeConsumeAccumulator();
-            }
-        }
-    }
-
     _expr.setResult(result);
     return false;
 }
diff --git a/src/qml/compiler/qv4compiler.cpp b/src/qml/compiler/qv4compiler.cpp
index 9b60ad14e6..d5a02a3ac8 100644
--- a/src/qml/compiler/qv4compiler.cpp
+++ b/src/qml/compiler/qv4compiler.cpp
@@ -188,7 +188,7 @@ QV4::ReturnedValue QV4::Compiler::JSUnitGenerator::constant(int idx)
     return constants.at(idx);
 }
 
-int QV4::Compiler::JSUnitGenerator::registerJSClass(const QVector<MemberInfo> &members)
+int QV4::Compiler::JSUnitGenerator::registerJSClass(const QStringList &members)
 {
     // ### re-use existing class definitions.
 
@@ -202,31 +202,15 @@ int QV4::Compiler::JSUnitGenerator::registerJSClass(const QVector<MemberInfo> &m
     jsClass->nMembers = members.size();
     CompiledData::JSClassMember *member = reinterpret_cast<CompiledData::JSClassMember*>(jsClass + 1);
 
-    for (const MemberInfo &memberInfo : members) {
-        member->nameOffset = registerString(memberInfo.name);
-        member->isAccessor = memberInfo.isAccessor;
+    for (const auto &name : members) {
+        member->nameOffset = registerString(name);
+        member->isAccessor = false;
         ++member;
     }
 
     return jsClassOffsets.size() - 1;
 }
 
-int QV4::Compiler::JSUnitGenerator::registerJSClass(int count, CompiledData::JSClassMember *members)
-{
-    const int size = CompiledData::JSClass::calculateSize(count);
-    jsClassOffsets.append(jsClassData.size());
-    const int oldSize = jsClassData.size();
-    jsClassData.resize(jsClassData.size() + size);
-    memset(jsClassData.data() + oldSize, 0, size);
-
-    CompiledData::JSClass *jsClass = reinterpret_cast<CompiledData::JSClass*>(jsClassData.data() + oldSize);
-    jsClass->nMembers = count;
-    CompiledData::JSClassMember *jsClassMembers = reinterpret_cast<CompiledData::JSClassMember*>(jsClass + 1);
-    memcpy(jsClassMembers, members, sizeof(CompiledData::JSClassMember)*count);
-
-    return jsClassOffsets.size() - 1;
-}
-
 QV4::CompiledData::Unit *QV4::Compiler::JSUnitGenerator::generateUnit(GeneratorOption option)
 {
     registerString(module->fileName);
diff --git a/src/qml/compiler/qv4compiler_p.h b/src/qml/compiler/qv4compiler_p.h
index bd330d3353..c40d49213a 100644
--- a/src/qml/compiler/qv4compiler_p.h
+++ b/src/qml/compiler/qv4compiler_p.h
@@ -116,8 +116,7 @@ struct Q_QML_PRIVATE_EXPORT JSUnitGenerator {
     int registerConstant(ReturnedValue v);
     ReturnedValue constant(int idx);
 
-    int registerJSClass(const QVector<MemberInfo> &members);
-    int registerJSClass(int count, CompiledData::JSClassMember *members);
+    int registerJSClass(const QStringList &members);
 
     enum GeneratorOption {
         GenerateWithStringTable,
diff --git a/src/qml/compiler/qv4instr_moth.cpp b/src/qml/compiler/qv4instr_moth.cpp
index 95bc459df0..c50735b61e 100644
--- a/src/qml/compiler/qv4instr_moth.cpp
+++ b/src/qml/compiler/qv4instr_moth.cpp
@@ -470,10 +470,9 @@ void dumpBytecode(const char *code, int len, int nLocals, int nFormals, int /*st
         MOTH_END_INSTR(DefineArray)
 
         MOTH_BEGIN_INSTR(DefineObjectLiteral)
-            d << dumpRegister(args, nFormals)
-              << ", " << internalClassId
-              << ", " << arrayValueCount
-              << ", " << arrayGetterSetterCountAndFlags;
+            d << internalClassId
+              << ", " << argc
+              << ", " << dumpRegister(args, nFormals);
         MOTH_END_INSTR(DefineObjectLiteral)
 
         MOTH_BEGIN_INSTR(CreateMappedArgumentsObject)
diff --git a/src/qml/compiler/qv4instr_moth_p.h b/src/qml/compiler/qv4instr_moth_p.h
index a783f9352c..2223400787 100644
--- a/src/qml/compiler/qv4instr_moth_p.h
+++ b/src/qml/compiler/qv4instr_moth_p.h
@@ -134,8 +134,7 @@ QT_BEGIN_NAMESPACE
 #define INSTR_TypeofValue(op) INSTRUCTION(op, TypeofValue, 0)
 #define INSTR_DeclareVar(op) INSTRUCTION(op, DeclareVar, 2, varName, isDeletable)
 #define INSTR_DefineArray(op) INSTRUCTION(op, DefineArray, 2, argc, args)
-// arrayGetterSetterCountAndFlags contains 30 bits for count, 1 bit for needsSparseArray boolean
-#define INSTR_DefineObjectLiteral(op) INSTRUCTION(op, DefineObjectLiteral, 4, internalClassId, arrayValueCount, arrayGetterSetterCountAndFlags, args)
+#define INSTR_DefineObjectLiteral(op) INSTRUCTION(op, DefineObjectLiteral, 3, internalClassId, argc, args)
 #define INSTR_CreateMappedArgumentsObject(op) INSTRUCTION(op, CreateMappedArgumentsObject, 0)
 #define INSTR_CreateUnmappedArgumentsObject(op) INSTRUCTION(op, CreateUnmappedArgumentsObject, 0)
 #define INSTR_CreateRestParameter(op) INSTRUCTION(op, CreateRestParameter, 1, argIndex)
diff --git a/src/qml/jit/qv4baselinejit.cpp b/src/qml/jit/qv4baselinejit.cpp
index cd07ac4d58..d8f0976774 100644
--- a/src/qml/jit/qv4baselinejit.cpp
+++ b/src/qml/jit/qv4baselinejit.cpp
@@ -804,14 +804,12 @@ void BaselineJIT::generate_DefineArray(int argc, int args)
     JIT_GENERATE_RUNTIME_CALL(Runtime::method_arrayLiteral, Assembler::ResultInAccumulator);
 }
 
-void BaselineJIT::generate_DefineObjectLiteral(int internalClassId, int arrayValueCount,
-                                             int arrayGetterSetterCountAndFlags, int args)
+void BaselineJIT::generate_DefineObjectLiteral(int internalClassId, int argc, int args)
 {
-    as->prepareCallWithArgCount(5);
-    as->passInt32AsArg(arrayGetterSetterCountAndFlags, 4);
-    as->passInt32AsArg(arrayValueCount, 3);
-    as->passInt32AsArg(internalClassId, 2);
-    as->passRegAsArg(args, 1);
+    as->prepareCallWithArgCount(4);
+    as->passRegAsArg(args, 3);
+    as->passInt32AsArg(argc, 2);
+    as->passInt32AsArg(internalClassId, 1);
     as->passEngineAsArg(0);
     JIT_GENERATE_RUNTIME_CALL(Runtime::method_objectLiteral, Assembler::ResultInAccumulator);
 }
diff --git a/src/qml/jit/qv4baselinejit_p.h b/src/qml/jit/qv4baselinejit_p.h
index 12a3f27e1b..c83b985bd6 100644
--- a/src/qml/jit/qv4baselinejit_p.h
+++ b/src/qml/jit/qv4baselinejit_p.h
@@ -151,9 +151,7 @@ public:
     void generate_TypeofValue() override;
     void generate_DeclareVar(int varName, int isDeletable) override;
     void generate_DefineArray(int argc, int args) override;
-    void generate_DefineObjectLiteral(int internalClassId, int arrayValueCount,
-                                      int arrayGetterSetterCountAndFlags,
-                                      int args) override;
+    void generate_DefineObjectLiteral(int internalClassId, int argc, int args) override;
     void generate_CreateMappedArgumentsObject() override;
     void generate_CreateUnmappedArgumentsObject() override;
     void generate_CreateRestParameter(int argIndex) override;
diff --git a/src/qml/jsruntime/qv4global_p.h b/src/qml/jsruntime/qv4global_p.h
index 87eec81afb..607f8b4d28 100644
--- a/src/qml/jsruntime/qv4global_p.h
+++ b/src/qml/jsruntime/qv4global_p.h
@@ -372,6 +372,12 @@ struct Q_QML_EXPORT StackFrame {
 };
 typedef QVector<StackFrame> StackTrace;
 
+enum class ObjectLiteralArgument {
+    Value,
+    Getter,
+    Setter
+};
+
 }
 
 Q_DECLARE_TYPEINFO(QV4::PropertyAttributes, Q_PRIMITIVE_TYPE);
diff --git a/src/qml/jsruntime/qv4runtime.cpp b/src/qml/jsruntime/qv4runtime.cpp
index b712b40897..5eec51c1e4 100644
--- a/src/qml/jsruntime/qv4runtime.cpp
+++ b/src/qml/jsruntime/qv4runtime.cpp
@@ -1382,45 +1382,45 @@ ReturnedValue Runtime::method_arrayLiteral(ExecutionEngine *engine, Value *value
     return engine->newArrayObject(values, length)->asReturnedValue();
 }
 
-ReturnedValue Runtime::method_objectLiteral(ExecutionEngine *engine, const QV4::Value *args, int classId, int arrayValueCount, int arrayGetterSetterCountAndFlags)
+ReturnedValue Runtime::method_objectLiteral(ExecutionEngine *engine, int classId, int argc, const QV4::Value *args)
 {
     Scope scope(engine);
     Scoped<InternalClass> klass(scope, engine->currentStackFrame->v4Function->compilationUnit->runtimeClasses[classId]);
     ScopedObject o(scope, engine->newObject(klass->d()));
 
-    {
-        bool needSparseArray = arrayGetterSetterCountAndFlags >> 30;
-        if (needSparseArray)
-            o->initSparseArray();
-    }
+    Q_ASSERT(uint(argc) >= klass->d()->size);
 
     for (uint i = 0; i < klass->d()->size; ++i)
         o->setProperty(i, *args++);
 
-    if (arrayValueCount > 0) {
-        ScopedValue entry(scope);
-        for (int i = 0; i < arrayValueCount; ++i) {
-            uint idx = args->toUInt32();
-            ++args;
-            entry = *args++;
-            o->arraySet(idx, entry);
-        }
-    }
+    Q_ASSERT((argc - klass->d()->size) % 3 == 0);
+    int additionalArgs = (argc - int(klass->d()->size))/3;
 
-    uint arrayGetterSetterCount = arrayGetterSetterCountAndFlags & ((1 << 30) - 1);
-    if (arrayGetterSetterCount > 0) {
-        ScopedProperty pd(scope);
-        for (uint i = 0; i < arrayGetterSetterCount; ++i) {
-            uint idx = args->toUInt32();
-            ++args;
-            pd->value = *args;
-            ++args;
-            pd->set = *args;
-            ++args;
-            o->arraySet(idx, pd, Attr_Accessor);
+    if (!additionalArgs)
+        return o->asReturnedValue();
+
+    Scoped<StringOrSymbol> name(scope);
+    ScopedProperty pd(scope);
+    for (int i = 0; i < additionalArgs; ++i) {
+        Q_ASSERT(args->isInteger());
+        ObjectLiteralArgument arg = ObjectLiteralArgument(args->integerValue());
+        name = args[1].toPropertyKey(engine);
+        if (engine->hasException)
+            return Encode::undefined();
+        Q_ASSERT(arg == ObjectLiteralArgument::Value || args[2].isFunctionObject());
+        if (arg == ObjectLiteralArgument::Value || arg == ObjectLiteralArgument::Getter) {
+            pd->value = args[2];
+            pd->set = Primitive::emptyValue();
+        } else {
+            pd->value = Primitive::emptyValue();
+            pd->set = args[2];
         }
-    }
+        bool ok = o->__defineOwnProperty__(scope.engine, name, pd, (arg == ObjectLiteralArgument::Value ? Attr_Data : Attr_Accessor));
+        if (!ok)
+            return engine->throwTypeError();
 
+        args += 3;
+    }
     return o.asReturnedValue();
 }
 
diff --git a/src/qml/jsruntime/qv4runtimeapi_p.h b/src/qml/jsruntime/qv4runtimeapi_p.h
index 694cd3ea78..66d3bb2a81 100644
--- a/src/qml/jsruntime/qv4runtimeapi_p.h
+++ b/src/qml/jsruntime/qv4runtimeapi_p.h
@@ -142,7 +142,7 @@ struct ExceptionCheck<void (*)(QV4::NoThrowEngine *, A, B, C)> {
     \
     /* literals */ \
     F(ReturnedValue, arrayLiteral, (ExecutionEngine *engine, Value *values, uint length)) \
-    F(ReturnedValue, objectLiteral, (ExecutionEngine *engine, const Value *args, int classId, int arrayValueCount, int arrayGetterSetterCountAndFlags)) \
+    F(ReturnedValue, objectLiteral, (ExecutionEngine *engine, int classId, int argc, const Value *args)) \
     \
     /* for-in, for-of and array destructuring */ \
     F(ReturnedValue, getIterator, (ExecutionEngine *engine, const Value &in, int iterator)) \
diff --git a/src/qml/jsruntime/qv4value_p.h b/src/qml/jsruntime/qv4value_p.h
index 59a8adca8f..16bbf241ff 100644
--- a/src/qml/jsruntime/qv4value_p.h
+++ b/src/qml/jsruntime/qv4value_p.h
@@ -919,7 +919,6 @@ struct ValueArray {
 // have wrong offsets between host and target.
 Q_STATIC_ASSERT(offsetof(ValueArray<0>, values) == 8);
 
-
 }
 
 QT_END_NAMESPACE
diff --git a/src/qml/jsruntime/qv4vme_moth.cpp b/src/qml/jsruntime/qv4vme_moth.cpp
index 052d6f773b..d5e3ef28bd 100644
--- a/src/qml/jsruntime/qv4vme_moth.cpp
+++ b/src/qml/jsruntime/qv4vme_moth.cpp
@@ -1034,7 +1034,7 @@ QV4::ReturnedValue VME::interpret(CppStackFrame &frame, const char *code)
 
     MOTH_BEGIN_INSTR(DefineObjectLiteral)
         QV4::Value *arguments = stack + args;
-        acc = Runtime::method_objectLiteral(engine, arguments, internalClassId, arrayValueCount, arrayGetterSetterCountAndFlags);
+        acc = Runtime::method_objectLiteral(engine, internalClassId, argc, arguments);
     MOTH_END_INSTR(DefineObjectLiteral)
 
     MOTH_BEGIN_INSTR(CreateMappedArgumentsObject)