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>

5 files changed, 76 insertions, 139 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)