Fix use of Component.onCompleted scripts in QQmlPropertyMap sub-types

After commit 9333ea8649838d7e0400b0e94c8cbd4fa5d216b0 when looking up
properties such as "console" in Component.onComplete handlers of
QQmlProperty sub-types, we end up implicitly creating "console"
properties, which is not correct.

At the same time, there are use-cases (and tests such as
tst_QQmlPropertyMap::QTBUG_35233) where a binding expression accesses a
property of a property map sub-type and it doesn't exist. Then later
that property is added and we must re-evaluate that binding. That works
because the implicit property creation when reading it also results in a
property capture.

Therefore we introduce the HFH (hack from hell): Detect the use of a
QQmlPropertyMap sub-type and go back to the old lookup behavior. The
path forward should be a deprecation of QQmlPropertyMap and perhaps the
ability to define abstract interfaces in QML.

Change-Id: Ib87bc1ebae44d7b332be47f1dcb614a19334b559
Reviewed-by: Lars Knoll <lars.knoll@qt.io>

3 files changed, 62 insertions, 6 deletions

diff --git a/src/qml/jsruntime/qv4qmlcontext.cpp b/src/qml/jsruntime/qv4qmlcontext.cpp
index dc69016559..c8ef87b979 100644
--- a/src/qml/jsruntime/qv4qmlcontext.cpp
+++ b/src/qml/jsruntime/qv4qmlcontext.cpp
@@ -125,6 +125,37 @@ ReturnedValue QQmlContextWrapper::virtualGet(const Managed *m, PropertyKey id, c
     QObject *scopeObject = resource->getScopeObject();
 
     ScopedString name(scope, id.asStringOrSymbol());
+
+    const auto performGobalLookUp = [&result, v4, &name, hasProperty]() {
+        bool hasProp = false;
+        result = v4->globalObject->get(name, &hasProp);
+        if (hasProp) {
+            if (hasProperty)
+                *hasProperty = hasProp;
+            return true;
+        }
+        return false;
+    };
+
+    // If the scope object is a QAbstractDynamicMetaObject, then QMetaObject::indexOfProperty
+    // will call createProperty() on the QADMO and implicitly create the property. While that
+    // is questionable behavior, there are two use-cases that we support in the light of this:
+    //
+    //    (1) The implicit creation of properties is necessary because it will also result in
+    //        a recorded capture, which will allow a re-evaluation of bindings when the value
+    //        is populated later. See QTBUG-35233 and the test-case in tst_qqmlpropertymap.
+    //
+    //    (1) Looking up "console" in order to place a console.log() call for example must
+    //        find the console instead of creating a new property. Therefore we prioritize the
+    //        lookup in the global object here.
+    //
+    // Note: The scope object is only a QADMO for example when somebody registers a QQmlPropertyMap
+    // sub-class as QML type and then instantiates it in .qml.
+    if (scopeObject && QQmlPropertyCache::isDynamicMetaObject(scopeObject->metaObject())) {
+        if (performGobalLookUp())
+            return result->asReturnedValue();
+    }
+
     if (context->imports && name->startsWithUpper()) {
         // Search for attached properties, enums and imported scripts
         QQmlTypeNameCache::Result r = context->imports->query(name, QQmlImport::AllowRecursion);
@@ -217,12 +248,8 @@ ReturnedValue QQmlContextWrapper::virtualGet(const Managed *m, PropertyKey id, c
 
     // Do a lookup in the global object here to avoid expressionContext->unresolvedNames becoming
     // true if we access properties of the global object.
-    result = v4->globalObject->get(name, &hasProp);
-    if (hasProp) {
-        if (hasProperty)
-            *hasProperty = hasProp;
+    if (performGobalLookUp())
         return result->asReturnedValue();
-    }
 
     expressionContext->unresolvedNames = true;
 
diff --git a/tests/auto/qml/qqmlpropertymap/data/PropertyMapSubType.qml b/tests/auto/qml/qqmlpropertymap/data/PropertyMapSubType.qml
new file mode 100644
index 0000000000..90ff179408
--- /dev/null
+++ b/tests/auto/qml/qqmlpropertymap/data/PropertyMapSubType.qml
@@ -0,0 +1,9 @@
+import QtQml 2.0
+import Test 1.0
+
+SimplePropertyMap {
+    Component.onCompleted: {
+        console.log("expected output")
+        newProperty = 42
+    }
+}
diff --git a/tests/auto/qml/qqmlpropertymap/tst_qqmlpropertymap.cpp b/tests/auto/qml/qqmlpropertymap/tst_qqmlpropertymap.cpp
index 86229b95f3..237b65679e 100644
--- a/tests/auto/qml/qqmlpropertymap/tst_qqmlpropertymap.cpp
+++ b/tests/auto/qml/qqmlpropertymap/tst_qqmlpropertymap.cpp
@@ -35,7 +35,7 @@
 #include <QSignalSpy>
 #include <QDebug>
 
-class tst_QQmlPropertyMap : public QObject
+class tst_QQmlPropertyMap : public QQmlDataTest
 {
     Q_OBJECT
 public:
@@ -61,6 +61,7 @@ private slots:
     void disallowExtending();
     void QTBUG_35906();
     void QTBUG_48136();
+    void lookupsInSubTypes();
 };
 
 class LazyPropertyMap : public QQmlPropertyMap, public QQmlParserStatus
@@ -89,9 +90,18 @@ private:
     int value = 0;
 };
 
+class SimplePropertyMap: public QQmlPropertyMap
+{
+    Q_OBJECT
+public:
+    SimplePropertyMap() : QQmlPropertyMap(this, nullptr) {}
+};
+
 void tst_QQmlPropertyMap::initTestCase()
 {
+    QQmlDataTest::initTestCase();
     qmlRegisterType<LazyPropertyMap>("QTBUG_35233", 1, 0, "LazyPropertyMap");
+    qmlRegisterType<SimplePropertyMap>("Test", 1, 0, "SimplePropertyMap");
 }
 
 void tst_QQmlPropertyMap::insert()
@@ -496,6 +506,16 @@ void tst_QQmlPropertyMap::QTBUG_48136()
     QCOMPARE(valueChangedSpy.count(), 1);
 }
 
+void tst_QQmlPropertyMap::lookupsInSubTypes()
+{
+    QQmlEngine engine;
+    QQmlComponent component(&engine, testFileUrl("PropertyMapSubType.qml"));
+    QTest::ignoreMessage(QtDebugMsg, "expected output");
+    QScopedPointer<QObject> object(component.create());
+    QVERIFY(!object.isNull());
+    QCOMPARE(object->property("newProperty").toInt(), 42);
+}
+
 QTEST_MAIN(tst_QQmlPropertyMap)
 
 #include "tst_qqmlpropertymap.moc"