[new compiler] Fix error reporting for group properties

Report errors when setting group properties multiple times, or when
assigning values to them. Some of this can be done right after parsing,
some of it requires knowledge about the type of group property (QObject or
value type)

Change-Id: I1aa33e64a5f64dfa4f625469f5b6a84cd8dfb18d
Reviewed-by: Lars Knoll <lars.knoll@digia.com>

5 files changed, 129 insertions, 54 deletions

diff --git a/src/qml/compiler/qqmlcodegenerator.cpp b/src/qml/compiler/qqmlcodegenerator.cpp
index 658413d91d..40e678a6aa 100644
--- a/src/qml/compiler/qqmlcodegenerator.cpp
+++ b/src/qml/compiler/qqmlcodegenerator.cpp
@@ -165,10 +165,10 @@ QString QmlObject::appendBinding(Binding *b, bool isListBinding)
         && b->type != QV4::CompiledData::Binding::Type_GroupProperty
         && b->type != QV4::CompiledData::Binding::Type_AttachedProperty
         && !(b->flags & QV4::CompiledData::Binding::IsOnAssignment)) {
-        if (bindingNames.contains(b->propertyNameIndex))
+        Binding *existing = findBinding(b->propertyNameIndex);
+        if (existing && existing->isValueBinding() == b->isValueBinding() && !(existing->flags & QV4::CompiledData::Binding::IsOnAssignment))
             return tr("Property value set multiple times");
     }
-    bindingNames.insert(b->propertyNameIndex);
     if (isListBinding) {
         bindings->append(b);
     } else if (bindingToDefaultProperty) {
@@ -180,6 +180,14 @@ QString QmlObject::appendBinding(Binding *b, bool isListBinding)
     return QString(); // no error
 }
 
+Binding *QmlObject::findBinding(quint32 nameIndex) const
+{
+    for (Binding *b = bindings->first; b; b = b->next)
+        if (b->propertyNameIndex == nameIndex)
+            return b;
+    return 0;
+}
+
 QStringList Signal::parameterStringList(const QStringList &stringPool) const
 {
     QStringList result;
@@ -313,7 +321,8 @@ bool QQmlCodeGenerator::visit(QQmlJS::AST::UiObjectDefinition *node)
         int idx = 0;
         if (!defineQMLObject(&idx, node))
             return false;
-        appendBinding(node->qualifiedTypeNameId->identifierToken, emptyStringIndex, idx);
+        const QQmlJS::AST::SourceLocation nameLocation = node->qualifiedTypeNameId->identifierToken;
+        appendBinding(nameLocation, nameLocation, emptyStringIndex, idx);
     } else {
         int idx = 0;
         if (!defineQMLObject(&idx, /*qualfied type name id*/0, node->qualifiedTypeNameId->firstSourceLocation(), node->initializer, /*declarations should go here*/_object))
@@ -340,6 +349,7 @@ bool QQmlCodeGenerator::visit(QQmlJS::AST::UiScriptBinding *node)
 
 bool QQmlCodeGenerator::visit(QQmlJS::AST::UiArrayBinding *node)
 {
+    const QQmlJS::AST::SourceLocation qualifiedNameLocation = node->qualifiedId->identifierToken;
     QmlObject *object = 0;
     QQmlJS::AST::UiQualifiedId *name = node->qualifiedId;
     if (!resolveQualifiedId(&name, &object))
@@ -349,7 +359,7 @@ bool QQmlCodeGenerator::visit(QQmlJS::AST::UiArrayBinding *node)
 
     const int propertyNameIndex = registerString(name->name.toString());
 
-    if (bindingsTarget()->hasBinding(propertyNameIndex)) {
+    if (bindingsTarget()->findBinding(propertyNameIndex) != 0) {
         recordError(name->identifierToken, tr("Property value set multiple times"));
         return false;
     }
@@ -361,7 +371,7 @@ bool QQmlCodeGenerator::visit(QQmlJS::AST::UiArrayBinding *node)
         int idx = 0;
         if (!defineQMLObject(&idx, def))
             return false;
-        appendBinding(name->identifierToken, propertyNameIndex, idx, /*isListItem*/ true);
+        appendBinding(qualifiedNameLocation, name->identifierToken, propertyNameIndex, idx, /*isListItem*/ true);
 
         member = member->next;
     }
@@ -816,7 +826,7 @@ bool QQmlCodeGenerator::visit(QQmlJS::AST::UiPublicMember *node)
              property->aliasPropertyValueIndex = registerString(propertyValue);
         } else if (node->statement) {
             qSwap(_propertyDeclaration, property);
-            appendBinding(node->identifierToken, _propertyDeclaration->nameIndex, node->statement);
+            appendBinding(node->identifierToken, node->identifierToken, _propertyDeclaration->nameIndex, node->statement);
             qSwap(_propertyDeclaration, property);
         }
 
@@ -963,25 +973,27 @@ void QQmlCodeGenerator::setBindingValue(QV4::CompiledData::Binding *binding, QQm
 
 void QQmlCodeGenerator::appendBinding(QQmlJS::AST::UiQualifiedId *name, QQmlJS::AST::Statement *value)
 {
+    const QQmlJS::AST::SourceLocation qualifiedNameLocation = name->identifierToken;
     QmlObject *object = 0;
     if (!resolveQualifiedId(&name, &object))
         return;
     qSwap(_object, object);
-    appendBinding(name->identifierToken, registerString(name->name.toString()), value);
+    appendBinding(qualifiedNameLocation, name->identifierToken, registerString(name->name.toString()), value);
     qSwap(_object, object);
 }
 
 void QQmlCodeGenerator::appendBinding(QQmlJS::AST::UiQualifiedId *name, int objectIndex, bool isOnAssignment)
 {
+    const QQmlJS::AST::SourceLocation qualifiedNameLocation = name->identifierToken;
     QmlObject *object = 0;
     if (!resolveQualifiedId(&name, &object))
         return;
     qSwap(_object, object);
-    appendBinding(name->identifierToken, registerString(name->name.toString()), objectIndex, /*isListItem*/false, isOnAssignment);
+    appendBinding(qualifiedNameLocation, name->identifierToken, registerString(name->name.toString()), objectIndex, /*isListItem*/false, isOnAssignment);
     qSwap(_object, object);
 }
 
-void QQmlCodeGenerator::appendBinding(const QQmlJS::AST::SourceLocation &nameLocation, quint32 propertyNameIndex, QQmlJS::AST::Statement *value)
+void QQmlCodeGenerator::appendBinding(const QQmlJS::AST::SourceLocation &qualifiedNameLocation, const QQmlJS::AST::SourceLocation &nameLocation, quint32 propertyNameIndex, QQmlJS::AST::Statement *value)
 {
     if (stringAt(propertyNameIndex) == QStringLiteral("id")) {
         setId(nameLocation, value);
@@ -996,11 +1008,11 @@ void QQmlCodeGenerator::appendBinding(const QQmlJS::AST::SourceLocation &nameLoc
     setBindingValue(binding, value);
     QString error = bindingsTarget()->appendBinding(binding, /*isListBinding*/false);
     if (!error.isEmpty()) {
-        recordError(nameLocation, error);
+        recordError(qualifiedNameLocation, error);
     }
 }
 
-void QQmlCodeGenerator::appendBinding(const QQmlJS::AST::SourceLocation &nameLocation, quint32 propertyNameIndex, int objectIndex, bool isListItem, bool isOnAssignment)
+void QQmlCodeGenerator::appendBinding(const QQmlJS::AST::SourceLocation &qualifiedNameLocation, const QQmlJS::AST::SourceLocation &nameLocation, quint32 propertyNameIndex, int objectIndex, bool isListItem, bool isOnAssignment)
 {
     if (stringAt(propertyNameIndex) == QStringLiteral("id")) {
         recordError(nameLocation, tr("Invalid component id specification"));
@@ -1034,7 +1046,7 @@ void QQmlCodeGenerator::appendBinding(const QQmlJS::AST::SourceLocation &nameLoc
     binding->value.objectIndex = objectIndex;
     QString error = bindingsTarget()->appendBinding(binding, isListItem);
     if (!error.isEmpty()) {
-        recordError(nameLocation, error);
+        recordError(qualifiedNameLocation, error);
     }
 }
 
@@ -1119,29 +1131,47 @@ bool QQmlCodeGenerator::resolveQualifiedId(QQmlJS::AST::UiQualifiedId **nameToRe
 
     *object = _object;
     while (qualifiedIdElement->next) {
-        Binding *binding = New<Binding>();
-        binding->propertyNameIndex = registerString(currentName);
-        binding->location.line = qualifiedIdElement->identifierToken.startLine;
-        binding->location.column = qualifiedIdElement->identifierToken.startColumn;
-        binding->valueLocation.line = binding->valueLocation.column = 0;
-        binding->flags = 0;
-
-        if (qualifiedIdElement->name.unicode()->isUpper())
-            binding->type = QV4::CompiledData::Binding::Type_AttachedProperty;
-        else
-            binding->type = QV4::CompiledData::Binding::Type_GroupProperty;
+        const quint32 propertyNameIndex = registerString(currentName);
+        const bool isAttachedProperty = qualifiedIdElement->name.unicode()->isUpper();
+
+        Binding *binding = (*object)->findBinding(propertyNameIndex);
+        if (binding) {
+            if (isAttachedProperty) {
+                if (!binding->isAttachedProperty())
+                    binding = 0;
+            } else if (!binding->isGroupProperty()) {
+                binding = 0;
+            }
+        }
+        if (!binding) {
+            binding = New<Binding>();
+            binding->propertyNameIndex = propertyNameIndex;
+            binding->location.line = qualifiedIdElement->identifierToken.startLine;
+            binding->location.column = qualifiedIdElement->identifierToken.startColumn;
+            binding->valueLocation.line = qualifiedIdElement->next->identifierToken.startLine;
+            binding->valueLocation.column = qualifiedIdElement->next->identifierToken.startColumn;
+            binding->flags = 0;
+
+            if (isAttachedProperty)
+                binding->type = QV4::CompiledData::Binding::Type_AttachedProperty;
+            else
+                binding->type = QV4::CompiledData::Binding::Type_GroupProperty;
 
-        int objIndex = 0;
-        if (!defineQMLObject(&objIndex, 0, QQmlJS::AST::SourceLocation(), 0, 0))
-            return false;
-        binding->value.objectIndex = objIndex;
+            int objIndex = 0;
+            if (!defineQMLObject(&objIndex, 0, QQmlJS::AST::SourceLocation(), 0, 0))
+                return false;
+            binding->value.objectIndex = objIndex;
 
-        QString error = (*object)->appendBinding(binding, /*isListBinding*/false);
-        if (!error.isEmpty()) {
-            recordError(qualifiedIdElement->identifierToken, error);
-            return false;
+            QString error = (*object)->appendBinding(binding, /*isListBinding*/false);
+            if (!error.isEmpty()) {
+                recordError(qualifiedIdElement->identifierToken, error);
+                return false;
+            }
+            *object = _objects.at(objIndex);
+        } else {
+            Q_ASSERT(binding->isAttachedProperty() || binding->isGroupProperty());
+            *object = _objects.at(binding->value.objectIndex);
         }
-        *object = _objects[objIndex];
 
         qualifiedIdElement = qualifiedIdElement->next;
         if (qualifiedIdElement)
diff --git a/src/qml/compiler/qqmlcodegenerator_p.h b/src/qml/compiler/qqmlcodegenerator_p.h
index d8357f298e..cbf5cd668a 100644
--- a/src/qml/compiler/qqmlcodegenerator_p.h
+++ b/src/qml/compiler/qqmlcodegenerator_p.h
@@ -226,7 +226,7 @@ public:
     void appendFunction(QtQml::Function *f);
 
     QString appendBinding(Binding *b, bool isListBinding);
-    bool hasBinding(int nameIndex) const { return bindingNames.contains(nameIndex); }
+    Binding *findBinding(quint32 nameIndex) const;
 
 private:
     PoolList<QmlProperty> *properties;
@@ -235,7 +235,6 @@ private:
     PoolList<Function> *functions;
 
     QSet<int> propertyNames;
-    QSet<int> bindingNames;
     QSet<int> signalNames;
 };
 
@@ -323,8 +322,8 @@ public:
 
     void appendBinding(QQmlJS::AST::UiQualifiedId *name, QQmlJS::AST::Statement *value);
     void appendBinding(QQmlJS::AST::UiQualifiedId *name, int objectIndex, bool isOnAssignment = false);
-    void appendBinding(const QQmlJS::AST::SourceLocation &nameLocation, quint32 propertyNameIndex, QQmlJS::AST::Statement *value);
-    void appendBinding(const QQmlJS::AST::SourceLocation &nameLocation, quint32 propertyNameIndex, int objectIndex, bool isListItem = false, bool isOnAssignment = false);
+    void appendBinding(const QQmlJS::AST::SourceLocation &qualifiedNameLocation, const QQmlJS::AST::SourceLocation &nameLocation, quint32 propertyNameIndex, QQmlJS::AST::Statement *value);
+    void appendBinding(const QQmlJS::AST::SourceLocation &qualifiedNameLocation, const QQmlJS::AST::SourceLocation &nameLocation, quint32 propertyNameIndex, int objectIndex, bool isListItem = false, bool isOnAssignment = false);
 
     QmlObject *bindingsTarget() const;
 
diff --git a/src/qml/compiler/qqmltypecompiler.cpp b/src/qml/compiler/qqmltypecompiler.cpp
index 7da2a63e94..71fc4c5d6a 100644
--- a/src/qml/compiler/qqmltypecompiler.cpp
+++ b/src/qml/compiler/qqmltypecompiler.cpp
@@ -1425,7 +1425,9 @@ QQmlBinding::Identifier QQmlPropertyValidator::bindingIdentifier(const QV4::Comp
     return id;
 }
 
-bool QQmlPropertyValidator::validateObject(int objectIndex, const QV4::CompiledData::Binding *instantiatingBinding)
+typedef QVarLengthArray<const QV4::CompiledData::Binding *, 8> GroupPropertyVector;
+
+bool QQmlPropertyValidator::validateObject(int objectIndex, const QV4::CompiledData::Binding *instantiatingBinding, bool populatingValueTypeGroupProperty)
 {
     const QV4::CompiledData::Object *obj = qmlUnit->objectAt(objectIndex);
 
@@ -1446,6 +1448,29 @@ bool QQmlPropertyValidator::validateObject(int objectIndex, const QV4::CompiledD
         customParser = objectType->type->customParser();
     QList<const QV4::CompiledData::Binding*> customBindings;
 
+    struct GroupPropertyFinder {
+        bool operator()(const QV4::CompiledData::Binding *binding, quint32 name) const {
+            return binding->propertyNameIndex < name;
+        }
+    };
+
+    // Collect group properties first for sanity checking
+    // vector values are sorted by property name string index.
+    GroupPropertyVector groupProperties;
+    const QV4::CompiledData::Binding *binding = obj->bindingTable();
+    for (quint32 i = 0; i < obj->nBindings; ++i, ++binding) {
+        if (!binding->isGroupProperty())
+                continue;
+
+        if (populatingValueTypeGroupProperty) {
+            recordError(binding->location, tr("Property assignment expected"));
+            return false;
+        }
+
+        GroupPropertyVector::const_iterator pos = std::lower_bound(groupProperties.constBegin(), groupProperties.constEnd(), binding->propertyNameIndex, GroupPropertyFinder());
+        groupProperties.insert(pos, binding);
+    }
+
     PropertyResolver propertyResolver(propertyCache);
 
     QString defaultPropertyName;
@@ -1459,7 +1484,7 @@ bool QQmlPropertyValidator::validateObject(int objectIndex, const QV4::CompiledD
         defaultProperty = propertyCache->defaultProperty();
     }
 
-    const QV4::CompiledData::Binding *binding = obj->bindingTable();
+    binding = obj->bindingTable();
     for (quint32 i = 0; i < obj->nBindings; ++i, ++binding) {
 
         if (customParser) {
@@ -1479,18 +1504,6 @@ bool QQmlPropertyValidator::validateObject(int objectIndex, const QV4::CompiledD
             }
         }
 
-        if (binding->type >= QV4::CompiledData::Binding::Type_Object) {
-            if (!validateObject(binding->value.objectIndex, binding))
-                return false;
-            if (binding->type == QV4::CompiledData::Binding::Type_AttachedProperty) {
-                if (instantiatingBinding && (instantiatingBinding->isAttachedProperty() || instantiatingBinding->isGroupProperty())) {
-                    recordError(binding->location, tr("Attached properties cannot be used here"));
-                    return false;
-                }
-                continue;
-            }
-        }
-
         // Signal handlers were resolved and checked earlier in the signal handler conversion pass.
         if (binding->flags & QV4::CompiledData::Binding::IsSignalHandlerExpression
             || binding->flags & QV4::CompiledData::Binding::IsSignalHandlerObject)
@@ -1498,7 +1511,7 @@ bool QQmlPropertyValidator::validateObject(int objectIndex, const QV4::CompiledD
 
         QString name = stringAt(binding->propertyNameIndex);
 
-        if (name.constData()->isUpper()) {
+        if (name.constData()->isUpper() && !binding->isAttachedProperty()) {
             QQmlType *type = 0;
             QQmlImportNamespace *typeNamespace = 0;
             compiler->imports()->resolveType(stringAt(binding->propertyNameIndex), &type, 0, 0, &typeNamespace);
@@ -1537,13 +1550,32 @@ bool QQmlPropertyValidator::validateObject(int objectIndex, const QV4::CompiledD
            bindingToDefaultProperty = true;
         }
 
+        if (binding->type >= QV4::CompiledData::Binding::Type_Object) {
+            if (!validateObject(binding->value.objectIndex, binding, pd && QQmlValueTypeFactory::valueType(pd->propType)))
+                return false;
+            if (binding->type == QV4::CompiledData::Binding::Type_AttachedProperty) {
+                if (instantiatingBinding && (instantiatingBinding->isAttachedProperty() || instantiatingBinding->isGroupProperty())) {
+                    recordError(binding->location, tr("Attached properties cannot be used here"));
+                    return false;
+                }
+                continue;
+            }
+        }
+
         if (pd) {
+            GroupPropertyVector::const_iterator assignedGroupProperty = std::lower_bound(groupProperties.constBegin(), groupProperties.constEnd(), binding->propertyNameIndex, GroupPropertyFinder());
+            const bool assigningToGroupProperty = assignedGroupProperty != groupProperties.constEnd() && !(binding->propertyNameIndex < (*assignedGroupProperty)->propertyNameIndex);
+
             if (!pd->isWritable()
                 && !pd->isQList()
                 && binding->type != QV4::CompiledData::Binding::Type_GroupProperty
                 && !(binding->flags & QV4::CompiledData::Binding::InitializerForReadOnlyDeclaration)
                 ) {
-                recordError(binding->valueLocation, tr("Invalid property assignment: \"%1\" is a read-only property").arg(name));
+
+                if (assigningToGroupProperty && binding->type < QV4::CompiledData::Binding::Type_Object)
+                    recordError(binding->valueLocation, tr("Cannot assign a value directly to a grouped property"));
+                else
+                    recordError(binding->valueLocation, tr("Invalid property assignment: \"%1\" is a read-only property").arg(name));
                 return false;
             }
 
@@ -1557,6 +1589,20 @@ bool QQmlPropertyValidator::validateObject(int objectIndex, const QV4::CompiledD
                 return false;
             }
 
+            if (!bindingToDefaultProperty
+                && !binding->isGroupProperty()
+                && assigningToGroupProperty) {
+                QV4::CompiledData::Location loc = binding->valueLocation;
+                if (loc < (*assignedGroupProperty)->valueLocation)
+                    loc = (*assignedGroupProperty)->valueLocation;
+
+                if (pd && QQmlValueTypeFactory::isValueType(pd->propType))
+                    recordError(loc, tr("Property has already been assigned a value"));
+                else
+                    recordError(loc, tr("Cannot assign a value directly to a grouped property"));
+                return false;
+            }
+
             if (binding->type < QV4::CompiledData::Binding::Type_Script) {
                 if (!validateLiteralBinding(propertyCache, pd, binding))
                     return false;
diff --git a/src/qml/compiler/qqmltypecompiler_p.h b/src/qml/compiler/qqmltypecompiler_p.h
index 0a20e0dd54..3ef3bbf55f 100644
--- a/src/qml/compiler/qqmltypecompiler_p.h
+++ b/src/qml/compiler/qqmltypecompiler_p.h
@@ -226,7 +226,7 @@ public:
     virtual QQmlBinding::Identifier bindingIdentifier(const QV4::CompiledData::Binding *binding, QQmlCustomParser *parser);
 
 private:
-    bool validateObject(int objectIndex, const QV4::CompiledData::Binding *instantiatingBinding);
+    bool validateObject(int objectIndex, const QV4::CompiledData::Binding *instantiatingBinding, bool populatingValueTypeGroupProperty = false);
     bool validateLiteralBinding(QQmlPropertyCache *propertyCache, QQmlPropertyData *property, const QV4::CompiledData::Binding *binding);
     bool validateObjectBinding(QQmlPropertyData *property, const QString &propertyName, const QV4::CompiledData::Binding *binding);
 
diff --git a/src/qml/parser/qqmljsastfwd_p.h b/src/qml/parser/qqmljsastfwd_p.h
index f8cba4981c..4304991260 100644
--- a/src/qml/parser/qqmljsastfwd_p.h
+++ b/src/qml/parser/qqmljsastfwd_p.h
@@ -64,7 +64,7 @@ namespace QQmlJS { namespace AST {
 class SourceLocation
 {
 public:
-    SourceLocation(quint32 offset = 0, quint32 length = 0, quint32 line = 0, quint32 column = 0)
+    explicit SourceLocation(quint32 offset = 0, quint32 length = 0, quint32 line = 0, quint32 column = 0)
         : offset(offset), length(length),
           startLine(line), startColumn(column)
     { }