QV4QObjectWrapper: handle bindable properties

A function created by Qt.binding should lead to the binding being set
(and replacing any previously existing binding). This was not the case
for bindable properties so far. For those, we would have kept any
existing C++ or QQmlPropertyBinding binding, and instead created a brand
new QML binding which would have been set in addition.
This patch also introduces a new class,
QQmlPropertyBindingJSForBoundFunction, which is used to handle the case
where the binding function is not a simple function, but has its
parameters bound instead.

Change-Id: Ia1417162b9822efb3f17ca4a6ecc02f959392927
Reviewed-by: Ulf Hermann <ulf.hermann@qt.io>

7 files changed, 175 insertions, 16 deletions

diff --git a/src/qml/jsruntime/qv4qobjectwrapper.cpp b/src/qml/jsruntime/qv4qobjectwrapper.cpp
index d3ffec9049..4a7a80d3bf 100644
--- a/src/qml/jsruntime/qv4qobjectwrapper.cpp
+++ b/src/qml/jsruntime/qv4qobjectwrapper.cpp
@@ -72,6 +72,7 @@
 #include <private/qv4mm_p.h>
 #include <private/qqmlscriptstring_p.h>
 #include <private/qv4compileddata_p.h>
+#include <private/qqmlpropertybinding_p.h>
 
 #include <QtQml/qjsvalue.h>
 #include <QtCore/qjsonarray.h>
@@ -488,19 +489,42 @@ void QObjectWrapper::setProperty(ExecutionEngine *engine, QObject *object, QQmlP
                 return;
             }
         } else {
-            // binding assignment.
-            QQmlRefPointer<QQmlContextData> callingQmlContext = scope.engine->callingQmlContext();
 
+            QQmlRefPointer<QQmlContextData> callingQmlContext = scope.engine->callingQmlContext();
             QV4::Scoped<QQmlBindingFunction> bindingFunction(scope, (const Value &)f);
-
             QV4::ScopedFunctionObject f(scope, bindingFunction->bindingFunction());
-            QV4::ScopedContext ctx(scope, bindingFunction->scope());
-            QQmlBinding *newBinding = QQmlBinding::create(property, f->function(), object, callingQmlContext, ctx);
-            newBinding->setSourceLocation(bindingFunction->currentLocation());
-            if (f->isBoundFunction())
+            QV4::ScopedContext ctx(scope, f->scope());
+
+            // binding assignment.
+            if (property->isBindable()) {
+                const QQmlPropertyIndex idx(property->coreIndex(), /*not a value type*/-1);
+                auto [targetObject, targetIndex] = QQmlPropertyPrivate::findAliasTarget(object, idx);
+                QUntypedPropertyBinding binding;
+                if (f->isBoundFunction()) {
+                    auto boundFunction = static_cast<QV4::BoundFunction *>(f.getPointer());
+                    binding = QQmlPropertyBinding::createFromBoundFunction(property, boundFunction, object, callingQmlContext,
+                                                                           ctx, targetObject, targetIndex);
+                } else {
+                    binding = QQmlPropertyBinding::create(property, f->function(), object, callingQmlContext,
+                                                           ctx, targetObject, targetIndex);
+                }
+                QUntypedBindable bindable;
+                void *argv = {&bindable};
+                // indirect metacall in case interceptors are installed
+                targetObject->metaObject()->metacall(targetObject, QMetaObject::BindableProperty, targetIndex.coreIndex(), &argv);
+                bool ok = bindable.setBinding(binding);
+                if (!ok) {
+                    auto error = QStringLiteral("Failed to set binding on %1::%2.").arg(object->metaObject()->className(), property->name(object));
+                    scope.engine->throwError(error);
+                }
+            } else {
+                QQmlBinding *newBinding = QQmlBinding::create(property, f->function(), object, callingQmlContext, ctx);
+                newBinding->setSourceLocation(bindingFunction->currentLocation());
+                if (f->isBoundFunction())
                 newBinding->setBoundFunction(static_cast<QV4::BoundFunction *>(f.getPointer()));
-            newBinding->setTarget(object, *property, nullptr);
-            QQmlPropertyPrivate::setBinding(newBinding);
+                newBinding->setTarget(object, *property, nullptr);
+                QQmlPropertyPrivate::setBinding(newBinding);
+            }
             return;
         }
     }
diff --git a/src/qml/qml/qqmlpropertybinding.cpp b/src/qml/qml/qqmlpropertybinding.cpp
index 23c829e887..6757618b86 100644
--- a/src/qml/qml/qqmlpropertybinding.cpp
+++ b/src/qml/qml/qqmlpropertybinding.cpp
@@ -38,6 +38,8 @@
 ****************************************************************************/
 
 #include "qqmlpropertybinding_p.h"
+#include <private/qv4functionobject_p.h>
+#include <private/qv4jscall_p.h>
 #include <qqmlinfo.h>
 
 QT_BEGIN_NAMESPACE
@@ -83,7 +85,7 @@ QUntypedPropertyBinding QQmlPropertyBinding::create(const QQmlPropertyData *pd,
     }
 
     auto buffer = new std::byte[sizeof(QQmlPropertyBinding)+sizeof(QQmlPropertyBindingJS)+jsExpressionOffsetLength()]; // QQmlPropertyBinding uses delete[]
-    auto binding = new(buffer) QQmlPropertyBinding(QMetaType(pd->propType()), target, targetIndex);
+    auto binding = new(buffer) QQmlPropertyBinding(QMetaType(pd->propType()), target, targetIndex, false);
     auto js = new(buffer + sizeof(QQmlPropertyBinding) + jsExpressionOffsetLength()) QQmlPropertyBindingJS();
     Q_ASSERT(binding->jsExpression() == js);
     Q_ASSERT(js->asBinding() == binding);
@@ -95,6 +97,22 @@ QUntypedPropertyBinding QQmlPropertyBinding::create(const QQmlPropertyData *pd,
     return QUntypedPropertyBinding(static_cast<QPropertyBindingPrivate *>(QPropertyBindingPrivatePtr(binding).data()));
 }
 
+QUntypedPropertyBinding QQmlPropertyBinding::createFromBoundFunction(const QQmlPropertyData *pd, QV4::BoundFunction *function, QObject *obj, const QQmlRefPointer<QQmlContextData> &ctxt, QV4::ExecutionContext *scope, QObject *target, QQmlPropertyIndex targetIndex)
+{
+    auto buffer = new std::byte[sizeof(QQmlPropertyBinding)+sizeof(QQmlPropertyBindingJSForBoundFunction)+jsExpressionOffsetLength()]; // QQmlPropertyBinding uses delete[]
+    auto binding = new(buffer) QQmlPropertyBinding(QMetaType(pd->propType()), target, targetIndex, true);
+    auto js = new(buffer + sizeof(QQmlPropertyBinding) + jsExpressionOffsetLength()) QQmlPropertyBindingJSForBoundFunction();
+    Q_ASSERT(binding->jsExpression() == js);
+    Q_ASSERT(js->asBinding() == binding);
+    Q_UNUSED(js);
+    binding->jsExpression()->setNotifyOnValueChanged(true);
+    binding->jsExpression()->setContext(ctxt);
+    binding->jsExpression()->setScopeObject(obj);
+    binding->jsExpression()->setupFunction(scope, function->function());
+    js->m_boundFunction.set(function->engine(), *function);
+    return QUntypedPropertyBinding(static_cast<QPropertyBindingPrivate *>(QPropertyBindingPrivatePtr(binding).data()));
+}
+
 void QQmlPropertyBindingJS::expressionChanged()
 {
     const auto currentTag = m_error.tag();
@@ -124,7 +142,7 @@ const QQmlPropertyBinding *QQmlPropertyBindingJS::asBinding() const
     return std::launder(reinterpret_cast<QQmlPropertyBinding const *>(reinterpret_cast<std::byte const*>(this) - sizeof(QQmlPropertyBinding) - jsExpressionOffsetLength()));
 }
 
-QQmlPropertyBinding::QQmlPropertyBinding(QMetaType mt, QObject *target, QQmlPropertyIndex targetIndex)
+QQmlPropertyBinding::QQmlPropertyBinding(QMetaType mt, QObject *target, QQmlPropertyIndex targetIndex, bool hasBoundFunction)
     : QPropertyBindingPrivate(mt,
                               &QtPrivate::bindingFunctionVTable<QQmlPropertyBinding>,
                               QPropertyBindingSourceLocation(), true)
@@ -132,7 +150,7 @@ QQmlPropertyBinding::QQmlPropertyBinding(QMetaType mt, QObject *target, QQmlProp
     static_assert (std::is_trivially_destructible_v<TargetData>);
     static_assert (sizeof(TargetData) + sizeof(DeclarativeErrorCallback) <= sizeof(QPropertyBindingSourceLocation));
     static_assert (alignof(TargetData) <= alignof(QPropertyBindingSourceLocation));
-    new (&declarativeExtraData) TargetData {target, targetIndex};
+    new (&declarativeExtraData) TargetData {target, targetIndex, hasBoundFunction};
     errorCallBack = bindingErrorCallback;
 }
 
@@ -151,7 +169,9 @@ bool QQmlPropertyBinding::evaluate(QMetaType metaType, void *dataPtr)
     bool isUndefined = false;
 
     QV4::Scope scope(engine->handle());
-    QV4::ScopedValue result(scope, jsExpression()->evaluate(&isUndefined));
+    QV4::ScopedValue result(scope, hasBoundFunction()
+                            ? static_cast<QQmlPropertyBindingJSForBoundFunction *>(jsExpression())->evaluate(&isUndefined)
+                            : jsExpression()->evaluate(&isUndefined));
 
     ep->dereferenceScarceResources();
 
@@ -256,6 +276,11 @@ QQmlPropertyIndex QQmlPropertyBinding::targetIndex()
     return std::launder(reinterpret_cast<TargetData *>(&declarativeExtraData))->targetIndex;
 }
 
+bool QQmlPropertyBinding::hasBoundFunction()
+{
+    return std::launder(reinterpret_cast<TargetData *>(&declarativeExtraData))->hasBoundFunction;
+}
+
 void QQmlPropertyBinding::bindingErrorCallback(QPropertyBindingPrivate *that)
 {
     auto This = static_cast<QQmlPropertyBinding *>(that);
@@ -299,4 +324,25 @@ QUntypedPropertyBinding QQmlTranslationPropertyBinding::create(const QQmlPropert
     return QUntypedPropertyBinding(QMetaType(pd->propType()), translationBinding, QPropertyBindingSourceLocation());
 }
 
+QV4::ReturnedValue QQmlPropertyBindingJSForBoundFunction::evaluate(bool *isUndefined)
+{
+    QV4::ExecutionEngine *v4 = engine()->handle();
+    int argc = 0;
+    const QV4::Value *argv = nullptr;
+    const QV4::Value *thisObject = nullptr;
+    QV4::BoundFunction *b = nullptr;
+    if ((b = static_cast<QV4::BoundFunction *>(m_boundFunction.valueRef()))) {
+        QV4::Heap::MemberData *args = b->boundArgs();
+        if (args) {
+            argc = args->values.size;
+            argv = args->values.data();
+        }
+        thisObject = &b->d()->boundThis;
+    }
+    QV4::Scope scope(v4);
+    QV4::JSCallData jsCall(scope, argc, argv, thisObject);
+
+    return QQmlJavaScriptExpression::evaluate(jsCall.callData(), isUndefined);
+}
+
 QT_END_NAMESPACE
diff --git a/src/qml/qml/qqmlpropertybinding_p.h b/src/qml/qml/qqmlpropertybinding_p.h
index ae7cc28ae2..429dc09036 100644
--- a/src/qml/qml/qqmlpropertybinding_p.h
+++ b/src/qml/qml/qqmlpropertybinding_p.h
@@ -61,6 +61,10 @@
 
 QT_BEGIN_NAMESPACE
 
+namespace QV4 {
+    class BoundFunction;
+}
+
 class QQmlPropertyBinding;
 
 class Q_QML_PRIVATE_EXPORT QQmlPropertyBindingJS : public QQmlJavaScriptExpression
@@ -74,6 +78,13 @@ class Q_QML_PRIVATE_EXPORT QQmlPropertyBindingJS : public QQmlJavaScriptExpressi
     QQmlPropertyBinding const *asBinding() const;;
 };
 
+class Q_QML_PRIVATE_EXPORT QQmlPropertyBindingJSForBoundFunction : public QQmlPropertyBindingJS
+{
+public:
+    QV4::ReturnedValue evaluate(bool *isUndefined);
+    QV4::PersistentValue m_boundFunction;
+};
+
 class Q_QML_PRIVATE_EXPORT QQmlPropertyBinding : public QPropertyBindingPrivate
 
 {
@@ -91,6 +102,11 @@ public:
                                           QV4::ExecutionContext *scope, QObject *target,
                                           QQmlPropertyIndex targetIndex);
 
+    static QUntypedPropertyBinding createFromBoundFunction(const QQmlPropertyData *pd, QV4::BoundFunction *function,
+                                          QObject *obj, const QQmlRefPointer<QQmlContextData> &ctxt,
+                                          QV4::ExecutionContext *scope, QObject *target,
+                                          QQmlPropertyIndex targetIndex);
+
 
     static bool doEvaluate(QMetaType metaType, QUntypedPropertyData *dataPtr, void *f) {
         auto address = static_cast<std::byte*>(f);
@@ -100,7 +116,7 @@ public:
     }
 
 private:
-    QQmlPropertyBinding(QMetaType metaType, QObject *target, QQmlPropertyIndex targetIndex);
+    QQmlPropertyBinding(QMetaType metaType, QObject *target, QQmlPropertyIndex targetIndex, bool hasBoundFunction);
 
     bool evaluate(QMetaType metaType, void *dataPtr);
 
@@ -109,10 +125,12 @@ private:
     struct TargetData {
         QObject *target;
         QQmlPropertyIndex targetIndex;
+        bool hasBoundFunction;
     };
 
     QObject *target();
     QQmlPropertyIndex targetIndex();
+    bool hasBoundFunction();
 
     static void bindingErrorCallback(QPropertyBindingPrivate *);
 };
diff --git a/tests/auto/qml/qmlcompiler_manual/tst_qmlcompiler_manual.cpp b/tests/auto/qml/qmlcompiler_manual/tst_qmlcompiler_manual.cpp
index 466a1960b0..fdac105f1c 100644
--- a/tests/auto/qml/qmlcompiler_manual/tst_qmlcompiler_manual.cpp
+++ b/tests/auto/qml/qmlcompiler_manual/tst_qmlcompiler_manual.cpp
@@ -472,8 +472,7 @@ void tst_qmlcompiler_manual::changingBindings()
     // test resetting value through C++
     created.setP2(0);
     created.setP1(-10);
-    QEXPECT_FAIL("", "Direct set doesn't clear Qt.binding() (and C++ binding hides Qt.binding())",
-                 Continue);
+
     QCOMPARE(created.property("p2").toInt(), 0);
     QCOMPARE(created.initialBindingCallCount, 2);
 
diff --git a/tests/auto/qml/qqmlecmascript/data/qpropertyAndQtBinding.qml b/tests/auto/qml/qqmlecmascript/data/qpropertyAndQtBinding.qml
new file mode 100644
index 0000000000..ff83945955
--- /dev/null
+++ b/tests/auto/qml/qqmlecmascript/data/qpropertyAndQtBinding.qml
@@ -0,0 +1,29 @@
+import QtQuick 2.6
+import test
+
+Tester {
+    id: tester
+    property var num: 100
+    property int i: 42
+
+
+    Component.onCompleted: {
+        function s(x) {
+            return x
+        }
+        function c(x) {
+            return x + num
+        }
+
+        try {
+            tester.readOnlyBindable = Qt.binding(() => tester.i)
+        } catch (e) {
+            console.warn(e)
+        }
+        tester.nonBound = Qt.binding(() => tester.i)
+        let bound = s.bind(undefined, 100)
+        tester.simple = Qt.binding(bound)
+        bound = c.bind(undefined, 100)
+        tester.complex = Qt.binding(bound)
+    }
+}
diff --git a/tests/auto/qml/qqmlecmascript/testtypes.h b/tests/auto/qml/qqmlecmascript/testtypes.h
index 7074ffaa10..0124b6c604 100644
--- a/tests/auto/qml/qqmlecmascript/testtypes.h
+++ b/tests/auto/qml/qqmlecmascript/testtypes.h
@@ -1781,6 +1781,30 @@ public:
     // QNotifiedProperty<float, &ClassWithQProperty2::callback> value;
 };
 
+struct QPropertyQtBindingTester : public QObject
+{
+    Q_OBJECT
+public:
+    Q_PROPERTY(int nonBound READ nonBound WRITE setNonBound BINDABLE bindableNonBound)
+    Q_PROPERTY(int simple MEMBER simple BINDABLE bindableSimple)
+    Q_PROPERTY(int complex MEMBER complex BINDABLE bindableComplex)
+    Q_PROPERTY(int readOnlyBindable MEMBER readOnlyBindable BINDABLE bindableReadOnlyBindable)
+
+    int nonBound() { return m_nonBound; }
+
+    void setNonBound(int i) {m_nonBound = i;}
+
+    QBindable<int> bindableNonBound() { return &m_nonBound; }
+    QBindable<int> bindableSimple() { return &simple; }
+    QBindable<int> bindableComplex() {return &complex; }
+    QBindable<int> bindableReadOnlyBindable() const {return &readOnlyBindable; }
+
+    QProperty<int> readOnlyBindable;
+    QProperty<int> m_nonBound;
+    QProperty<int> simple;
+    QProperty<int> complex;
+};
+
 void registerTypes();
 
 #endif // TESTTYPES_H
diff --git a/tests/auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp b/tests/auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp
index 775026de2e..8554955a39 100644
--- a/tests/auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp
+++ b/tests/auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp
@@ -303,6 +303,7 @@ private slots:
     void tryStatement();
     void replaceBinding();
     void bindingBoundFunctions();
+    void qpropertyAndQtBinding();
     void deleteRootObjectInCreation();
     void onDestruction();
     void onDestructionViaGC();
@@ -7657,6 +7658,24 @@ void tst_qqmlecmascript::bindingBoundFunctions()
     delete obj;
 }
 
+void tst_qqmlecmascript::qpropertyAndQtBinding()
+{
+    QQmlEngine engine;
+    qmlRegisterType<QPropertyQtBindingTester>("test", 1, 0, "Tester");
+    QQmlComponent c(&engine, testFileUrl("qpropertyAndQtBinding.qml"));
+    QTest::ignoreMessage(QtMsgType::QtWarningMsg, QRegularExpression("Error: Failed to set binding on QPropertyQtBindingTester.*::readOnlyBindable."));
+    QScopedPointer<QPropertyQtBindingTester> root(qobject_cast<QPropertyQtBindingTester *>(c.create()));
+    QVERIFY(root);
+    QCOMPARE(root->nonBound(), 42);
+    bool ok = root->setProperty("i", 24);
+    QVERIFY(ok);
+    QCOMPARE(root->simple.value(), 100);
+    QCOMPARE(root->complex.value(), 200);
+    ok = root->setProperty("num", 50);
+    QVERIFY(ok);
+    QCOMPARE(root->complex.value(), 150);
+}
+
 void tst_qqmlecmascript::deleteRootObjectInCreation()
 {
     QQmlEngine engine;