QML ListModel: Emit a warning when adding an object with undefined or null member

The current code in ListModel simply did a reset of an existing
property, in case a role existed and was set to null/undefined. If the
role did not exist, the code would simply skip over the member and do
nothing.
However, this does not make any sense for newly inserted items, and
most likely indicates a misunderstanding of how ListModel works.

Creating an undefined/null role does not really make sense, as those
could only ever store a undefined/null value.

Change-Id: I4c1361647a82146565eaffe064598c94c748b4f5
Task-number: QTBUG-63569
Reviewed-by: Mitch Curtis <mitch.curtis@qt.io>
Reviewed-by: Ulf Hermann <ulf.hermann@qt.io>

3 files changed, 47 insertions, 8 deletions

diff --git a/src/qmlmodels/qqmllistmodel.cpp b/src/qmlmodels/qqmllistmodel.cpp
index d68815cbc1..e0a66e7170 100644
--- a/src/qmlmodels/qqmllistmodel.cpp
+++ b/src/qmlmodels/qqmllistmodel.cpp
@@ -634,7 +634,7 @@ void ListModel::set(int elementIndex, QV4::Object *object, QVector<int> *roles)
         mo->updateValues(*roles);
 }
 
-void ListModel::set(int elementIndex, QV4::Object *object)
+void ListModel::set(int elementIndex, QV4::Object *object, ListModel::SetElement reason)
 {
     if (!object)
         return;
@@ -684,7 +684,7 @@ void ListModel::set(int elementIndex, QV4::Object *object)
         } else if (QV4::DateObject *date = propertyValue->as<QV4::DateObject>()) {
             const ListLayout::Role &r = m_layout->getRoleOrCreate(propertyName, ListLayout::Role::DateTime);
             if (r.type == ListLayout::Role::DateTime) {
-                QDateTime dt = date->toQDateTime();;
+                QDateTime dt = date->toQDateTime();
                 e->setDateTimePropertyFast(r, dt);
             }
         } else if (QV4::Object *o = propertyValue->as<QV4::Object>()) {
@@ -699,9 +699,16 @@ void ListModel::set(int elementIndex, QV4::Object *object)
                     e->setVariantMapFast(role, o);
             }
         } else if (propertyValue->isNullOrUndefined()) {
-            const ListLayout::Role *r = m_layout->getExistingRole(propertyName);
-            if (r)
-                e->clearProperty(*r);
+            if (reason == SetElement::WasJustInserted) {
+                QQmlError err;
+                auto memberName = propertyName->toString(m_modelCache->engine())->toQString();
+                err.setDescription(QString::fromLatin1("%1 is %2. Adding an object with a %2 member does not create a role for it.").arg(memberName, propertyValue->isNull() ? QLatin1String("null") : QLatin1String("undefined")));
+                qmlWarning(nullptr, err);
+            } else {
+                const ListLayout::Role *r = m_layout->getExistingRole(propertyName);
+                if (r)
+                    e->clearProperty(*r);
+            }
         }
     }
 }
@@ -725,13 +732,13 @@ QVector<std::function<void()>> ListModel::remove(int index, int count)
 void ListModel::insert(int elementIndex, QV4::Object *object)
 {
     insertElement(elementIndex);
-    set(elementIndex, object);
+    set(elementIndex, object, SetElement::WasJustInserted);
 }
 
 int ListModel::append(QV4::Object *object)
 {
     int elementIndex = appendElement();
-    set(elementIndex, object);
+    set(elementIndex, object, SetElement::WasJustInserted);
     return elementIndex;
 }
 
diff --git a/src/qmlmodels/qqmllistmodel_p_p.h b/src/qmlmodels/qqmllistmodel_p_p.h
index a0d0e9ad89..2ad5158050 100644
--- a/src/qmlmodels/qqmllistmodel_p_p.h
+++ b/src/qmlmodels/qqmllistmodel_p_p.h
@@ -381,8 +381,10 @@ public:
         return elements.count();
     }
 
+    enum class SetElement {WasJustInserted, IsCurrentlyUpdated};
+
     void set(int elementIndex, QV4::Object *object, QVector<int> *roles);
-    void set(int elementIndex, QV4::Object *object);
+    void set(int elementIndex, QV4::Object *object, SetElement reason = SetElement::IsCurrentlyUpdated);
 
     int append(QV4::Object *object);
     void insert(int elementIndex, QV4::Object *object);
diff --git a/tests/auto/qml/qqmllistmodel/tst_qqmllistmodel.cpp b/tests/auto/qml/qqmllistmodel/tst_qqmllistmodel.cpp
index b47062ee94..75a932b6f4 100644
--- a/tests/auto/qml/qqmllistmodel/tst_qqmllistmodel.cpp
+++ b/tests/auto/qml/qqmllistmodel/tst_qqmllistmodel.cpp
@@ -129,6 +129,7 @@ private slots:
     void crash_append_empty_array();
     void dynamic_roles_crash_QTBUG_38907();
     void nestedListModelIteration();
+    void undefinedAppendShouldCauseError();
 };
 
 bool tst_qqmllistmodel::compareVariantList(const QVariantList &testList, QVariant object)
@@ -1694,6 +1695,35 @@ void tst_qqmllistmodel::nestedListModelIteration()
     QScopedPointer<QObject>(component.create());
 }
 
+// QTBUG-63569
+void tst_qqmllistmodel::undefinedAppendShouldCauseError()
+{
+    QQmlEngine engine;
+    QQmlComponent component(&engine);
+    component.setData(
+            R"(import QtQuick 2.5
+            Item {
+                width: 640
+                height: 480
+                ListModel {
+                    id : model
+                }
+                Component.onCompleted: {
+                        var tempData = {
+                            faulty: undefined
+                        }
+                        model.insert(0, tempData)
+                        tempData.faulty = null
+                        model.insert(0, tempData)
+                }
+            })",
+            QUrl());
+    QTest::ignoreMessage(QtMsgType::QtWarningMsg, "<Unknown File>: faulty is undefined. Adding an object with a undefined member does not create a role for it.");
+    QTest::ignoreMessage(QtMsgType::QtWarningMsg, "<Unknown File>: faulty is null. Adding an object with a null member does not create a role for it.");
+    QScopedPointer<QObject>(component.create());
+}
+
+
 QTEST_MAIN(tst_qqmllistmodel)
 
 #include "tst_qqmllistmodel.moc"