QQmlPropertyBinding: handle reset

Bindings are allowed to toggle between a defined state, and undefined
which calls the property's reset function. Calls to the reset function
must not remove the binding, even when they write to the property.

To support this, we put the binding in a special undefined state, in
which it is still active (and installed on the property), but does not
actually provide its evaluated value to the property. Then, when the
binding later becomes defined again, the binding leaves its undefined
state and works normally again.

Notes:
- Calling the reset function during binding evaluation could have all
  kinds of unwelcome side-effects. We therefore have to suspend binding
  evaluation before the reset call (and restore that state afterwards).
- QObjectCompatProperty expects that we write the current value into the
  propertyDataPtr. If we do not do this, it will overwrite the current
  value with the default constructed value of its property. Arguably, we
  should change the API so that we communicate that nothing has changed;
  but for now, we have to live with that limitation and read the
  current value and write it back again.
- We currently do not handle the case correctly where a non-resettable
  property implemented via QObjectCompatProperty gets assigned undefined
  in a binding. Such a binding is likely unintentional (as the undefined
  assignment only creates a warning), and thus less of a priority.
  Nevertheless, a test marked with QEXPECT_FAIL is added for it.

Fixes: QTBUG-91001
Change-Id: I7ecaa6c8dc1a1f1b33e67b1af65f552c4ca6ffb1
Reviewed-by: Ulf Hermann <ulf.hermann@qt.io>

10 files changed, 275 insertions, 6 deletions

diff --git a/src/qml/qml/qqmlpropertybinding.cpp b/src/qml/qml/qqmlpropertybinding.cpp
index ae6e196375..ecb1efec2c 100644
--- a/src/qml/qml/qqmlpropertybinding.cpp
+++ b/src/qml/qml/qqmlpropertybinding.cpp
@@ -41,9 +41,12 @@
 #include <private/qv4functionobject_p.h>
 #include <private/qv4jscall_p.h>
 #include <qqmlinfo.h>
+#include <QtCore/qloggingcategory.h>
 
 QT_BEGIN_NAMESPACE
 
+Q_LOGGING_CATEGORY(lcQQPropertyBinding, "qt.qml.propertybinding");
+
 namespace {
 constexpr std::size_t jsExpressionOffsetLength() {
     struct composite { QQmlPropertyBinding b; QQmlPropertyBindingJS js; };
@@ -166,7 +169,8 @@ 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, hasBoundFunction};
+    const auto state = hasBoundFunction ? TargetData::HasBoundFunction : TargetData::WithoutBoundFunction;
+    new (&declarativeExtraData) TargetData {target, targetIndex, state};
     errorCallBack = bindingErrorCallback;
 }
 
@@ -182,16 +186,26 @@ bool QQmlPropertyBinding::evaluate(QMetaType metaType, void *dataPtr)
     QQmlEnginePrivate *ep = QQmlEnginePrivate::get(engine);
     ep->referenceScarceResources();
 
-    bool isUndefined = false;
+    bool evaluatedToUndefined = false;
 
     QV4::Scope scope(engine->handle());
     QV4::ScopedValue result(scope, hasBoundFunction()
-                            ? static_cast<QQmlPropertyBindingJSForBoundFunction *>(jsExpression())->evaluate(&isUndefined)
-                            : jsExpression()->evaluate(&isUndefined));
+                            ? static_cast<QQmlPropertyBindingJSForBoundFunction *>(jsExpression())->evaluate(&evaluatedToUndefined)
+                            : jsExpression()->evaluate(&evaluatedToUndefined));
 
     ep->dereferenceScarceResources();
 
-    if (jsExpression()->hasError()) {
+    bool hadError = jsExpression()->hasError();
+    if (!hadError) {
+        if (evaluatedToUndefined) {
+            handleUndefinedAssignment(ep, dataPtr);
+            // if property has been changed due to reset, reset is responsible for
+            // notifying observers
+            return false;
+        } else if (isUndefined()) {
+            setIsUndefined(false);
+        }
+    } else {
         QPropertyBindingError error(QPropertyBindingError::UnknownError, jsExpression()->delayedError()->error().description());
         QPropertyBindingPrivate::currentlyEvaluatingBinding()->setError(std::move(error));
         bindingErrorCallback(this);
@@ -265,6 +279,98 @@ bool QQmlPropertyBinding::evaluate(QMetaType metaType, void *dataPtr)
     return hasChanged;
 }
 
+static QtPrivate::QPropertyBindingData *bindingDataFromPropertyData(QUntypedPropertyData *dataPtr, QMetaType type)
+{
+    // XXX Qt 7: We need a clean way to access the binding data
+    /* This function makes the (dangerous) assumption that if we could not get the binding data
+       from the binding storage, we must have been handed a QProperty.
+       This does hold for anything a user could write, as there the only ways of providing a bindable property
+       are to use the Q_X_BINDABLE macros, or to directly expose a QProperty.
+       As long as we can ensure that any "fancier" property we implement is not resettable, we should be fine.
+       We procede to calculate the address of the binding data pointer from the address of the data pointer
+    */
+    Q_ASSERT(dataPtr);
+    std::byte *qpropertyPointer = reinterpret_cast<std::byte *>(dataPtr);
+    qpropertyPointer += type.sizeOf();
+    constexpr auto alignment = alignof(QtPrivate::QPropertyBindingData *);
+    auto aligned = (quintptr(qpropertyPointer) + alignment - 1) & ~(alignment - 1); // ensure pointer alignment
+    return reinterpret_cast<QtPrivate::QPropertyBindingData *>(aligned);
+}
+
+void QQmlPropertyBinding::handleUndefinedAssignment(QQmlEnginePrivate *ep, void *dataPtr)
+{
+    QQmlPropertyData *propertyData = nullptr;
+    QQmlPropertyData valueTypeData;
+    QQmlData *data = QQmlData::get(target(), false);
+    Q_ASSERT(data);
+    if (Q_UNLIKELY(!data->propertyCache)) {
+        data->propertyCache = ep->cache(target()->metaObject());
+        data->propertyCache->addref();
+    }
+    propertyData = data->propertyCache->property(targetIndex().coreIndex());
+    Q_ASSERT(propertyData);
+    Q_ASSERT(!targetIndex().hasValueTypeIndex());
+    QQmlProperty prop = QQmlPropertyPrivate::restore(target(), *propertyData, &valueTypeData, nullptr);
+    // helper function for writing back value into dataPtr
+    // this is necessary  for QObjectCompatProperty, which doesn't give us the "real" dataPtr
+    // if we don't write the correct value, we would otherwise set the default constructed value
+    auto writeBackCurrentValue = [&](QVariant &&currentValue) {
+        currentValue.convert(valueMetaType());
+        auto metaType = valueMetaType();
+        metaType.destruct(dataPtr);
+        metaType.construct(dataPtr, currentValue.constData());
+    };
+    if (isUndefined()) {
+        // if we are already detached, there is no reason to call reset again (?)
+        return;
+    }
+    if (prop.isResettable()) {
+        // Normally a reset would remove any existing binding; but now we need to keep the binding alive
+        // to handle the case where this binding becomes defined again
+        // We therefore detach the binding, call reset, and reattach again
+        const auto storage = qGetBindingStorage(target());
+        auto bindingData = storage->bindingData(propertyDataPtr);
+        if (!bindingData)
+            bindingData = bindingDataFromPropertyData(propertyDataPtr, propertyData->propType());
+        QPropertyBindingDataPointer bindingDataPointer{bindingData};
+        auto firstObserver = takeObservers();
+        if (firstObserver)
+            bindingDataPointer.setObservers(firstObserver.ptr);
+        else
+            bindingData->d_ptr = 0;
+        setIsUndefined(true);
+        //suspend binding evaluation state for reset and subsequent read
+        auto state = QtPrivate::suspendCurrentBindingStatus();
+        prop.reset();
+        QVariant currentValue = prop.read();
+        QtPrivate::restoreBindingStatus(state);
+        currentValue.convert(valueMetaType());
+        writeBackCurrentValue(std::move(currentValue));
+        // reattach the binding (without causing a new notification)
+        if (Q_UNLIKELY(bindingData->d_ptr & QtPrivate::QPropertyBindingData::BindingBit)) {
+            qCWarning(lcQQPropertyBinding) << "Resetting " << prop.name() << "due to the binding becoming undefined  caused a new binding to be installed\n"
+                       << "The old binding binding will be abandonned";
+            deref();
+            return;
+        }
+        // reset might have changed observers (?), so refresh firstObserver
+        firstObserver = bindingDataPointer.firstObserver();
+        bindingData->d_ptr = reinterpret_cast<quintptr>(this) | QtPrivate::QPropertyBindingData::BindingBit;
+        if (firstObserver)
+            bindingDataPointer.setFirstObserver(firstObserver.ptr);
+    } else {
+        QQmlError qmlError;
+        auto location = jsExpression()->sourceLocation();
+        qmlError.setColumn(location.column);
+        qmlError.setLine(location.line);
+        qmlError.setUrl(QUrl {location.sourceFile});
+        const QString description = QStringLiteral(R"(QML %1: Unable to assign [undefined] to "%2")").arg(QQmlMetaType::prettyTypeName(target()) , prop.name());
+        qmlError.setDescription(description);
+        qmlError.setObject(target());
+        ep->warning(qmlError);
+    }
+}
+
 QString QQmlPropertyBinding::createBindingLoopErrorDescription(QJSEnginePrivate *ep)
 {
     QQmlPropertyData *propertyData = nullptr;
@@ -297,6 +403,16 @@ bool QQmlPropertyBinding::hasBoundFunction()
     return std::launder(reinterpret_cast<TargetData *>(&declarativeExtraData))->hasBoundFunction;
 }
 
+bool QQmlPropertyBinding::isUndefined() const
+{
+    return std::launder(reinterpret_cast<TargetData const *>(&declarativeExtraData))->isUndefined;
+}
+
+void QQmlPropertyBinding::setIsUndefined(bool isUndefined)
+{
+    std::launder(reinterpret_cast<TargetData *>(&declarativeExtraData))->isUndefined = isUndefined;
+}
+
 void QQmlPropertyBinding::bindingErrorCallback(QPropertyBindingPrivate *that)
 {
     auto This = static_cast<QQmlPropertyBinding *>(that);
diff --git a/src/qml/qml/qqmlpropertybinding_p.h b/src/qml/qml/qqmlpropertybinding_p.h
index 7c7fac866a..8417a4410a 100644
--- a/src/qml/qml/qqmlpropertybinding_p.h
+++ b/src/qml/qml/qqmlpropertybinding_p.h
@@ -112,6 +112,17 @@ public:
                                           QV4::ExecutionContext *scope, QObject *target,
                                           QQmlPropertyIndex targetIndex);
 
+    static bool isUndefined(const QUntypedPropertyBinding &binding)
+    {
+        return isUndefined(QPropertyBindingPrivate::get(binding));
+    }
+
+    static bool isUndefined(const QPropertyBindingPrivate *binding)
+    {
+        if (!(binding && binding->hasCustomVTable()))
+            return false;
+        return static_cast<const QQmlPropertyBinding *>(binding)->isUndefined();
+    }
 
     static bool doEvaluate(QMetaType metaType, QUntypedPropertyData *dataPtr, void *f) {
         auto address = static_cast<std::byte*>(f);
@@ -125,17 +136,29 @@ private:
 
     bool evaluate(QMetaType metaType, void *dataPtr);
 
+    Q_NEVER_INLINE void handleUndefinedAssignment(QQmlEnginePrivate *ep, void *dataPtr);
+
     QString createBindingLoopErrorDescription(QJSEnginePrivate *ep);
 
     struct TargetData {
+        enum BoundFunction : bool {
+            WithoutBoundFunction = false,
+            HasBoundFunction = true,
+        };
+        TargetData(QObject *target, QQmlPropertyIndex index, BoundFunction state)
+            : target(target), targetIndex(index), hasBoundFunction(state)
+        {}
         QObject *target;
         QQmlPropertyIndex targetIndex;
         bool hasBoundFunction;
+        bool isUndefined = false;
     };
 
     QObject *target();
     QQmlPropertyIndex targetIndex();
     bool hasBoundFunction();
+    bool isUndefined() const;
+    void setIsUndefined(bool isUndefined);
 
     static void bindingErrorCallback(QPropertyBindingPrivate *);
 };
diff --git a/tests/auto/qml/qqmlecmascript/data/qpropertyBindingUndefined.qml b/tests/auto/qml/qqmlecmascript/data/qpropertyBindingUndefined.qml
new file mode 100644
index 0000000000..757a544b58
--- /dev/null
+++ b/tests/auto/qml/qqmlecmascript/data/qpropertyBindingUndefined.qml
@@ -0,0 +1,6 @@
+import Qt.test 1.0
+ClassWithQProperty {
+    property int anotherValue: 1
+    property bool toggle: false
+    value: toggle ? undefined : anotherValue
+}
diff --git a/tests/auto/qml/qqmlecmascript/data/qpropertyBindingUndefined2.qml b/tests/auto/qml/qqmlecmascript/data/qpropertyBindingUndefined2.qml
new file mode 100644
index 0000000000..5e08a20987
--- /dev/null
+++ b/tests/auto/qml/qqmlecmascript/data/qpropertyBindingUndefined2.qml
@@ -0,0 +1,6 @@
+import Qt.test 1.0
+ClassWithQObjectProperty {
+    property int anotherValue: 1
+    property bool toggle: false
+    value: toggle ? undefined : anotherValue
+}
diff --git a/tests/auto/qml/qqmlecmascript/data/qpropertyBindingUndefinedWithoutReset1.qml b/tests/auto/qml/qqmlecmascript/data/qpropertyBindingUndefinedWithoutReset1.qml
new file mode 100644
index 0000000000..c0b5994887
--- /dev/null
+++ b/tests/auto/qml/qqmlecmascript/data/qpropertyBindingUndefinedWithoutReset1.qml
@@ -0,0 +1,6 @@
+import Qt.test 1.0
+ClassWithQProperty {
+    property int anotherValue: 1
+    property bool toggle: false
+    value2: toggle ? undefined : anotherValue
+}
diff --git a/tests/auto/qml/qqmlecmascript/data/qpropertyBindingUndefinedWithoutReset2.qml b/tests/auto/qml/qqmlecmascript/data/qpropertyBindingUndefinedWithoutReset2.qml
new file mode 100644
index 0000000000..9a74a620d8
--- /dev/null
+++ b/tests/auto/qml/qqmlecmascript/data/qpropertyBindingUndefinedWithoutReset2.qml
@@ -0,0 +1,6 @@
+import Qt.test 1.0
+ClassWithQObjectProperty {
+    property int anotherValue: 1
+    property bool toggle: false
+    value2: toggle ? undefined : anotherValue
+}
diff --git a/tests/auto/qml/qqmlecmascript/data/qpropertyBindingUndefinedWithoutReset3.qml b/tests/auto/qml/qqmlecmascript/data/qpropertyBindingUndefinedWithoutReset3.qml
new file mode 100644
index 0000000000..61a4008fba
--- /dev/null
+++ b/tests/auto/qml/qqmlecmascript/data/qpropertyBindingUndefinedWithoutReset3.qml
@@ -0,0 +1,6 @@
+import Qt.test 1.0
+ClassWithQCompatProperty {
+    property int anotherValue: 1
+    property bool toggle: false
+    value2: toggle ? undefined : anotherValue
+}
diff --git a/tests/auto/qml/qqmlecmascript/testtypes.cpp b/tests/auto/qml/qqmlecmascript/testtypes.cpp
index 688ed2c946..fb3f4ae041 100644
--- a/tests/auto/qml/qqmlecmascript/testtypes.cpp
+++ b/tests/auto/qml/qqmlecmascript/testtypes.cpp
@@ -555,6 +555,7 @@ void registerTypes()
     qmlRegisterType<ClashingNames>("Qt.test", 1, 0, "ClashingNames");
 
     qmlRegisterType<ClassWithQProperty>("Qt.test", 1, 0, "ClassWithQProperty");
+    qmlRegisterType<ClassWithQObjectProperty>("Qt.test", 1, 0, "ClassWithQObjectProperty");
     qmlRegisterType<ClassWithQProperty2>("Qt.test", 1, 0, "ClassWithQProperty2");
 }
 
diff --git a/tests/auto/qml/qqmlecmascript/testtypes.h b/tests/auto/qml/qqmlecmascript/testtypes.h
index 3f626b002b..705d72bf75 100644
--- a/tests/auto/qml/qqmlecmascript/testtypes.h
+++ b/tests/auto/qml/qqmlecmascript/testtypes.h
@@ -1731,15 +1731,34 @@ public:
 struct ClassWithQProperty : public QObject
 {
     Q_OBJECT
-    Q_PROPERTY(float value MEMBER value BINDABLE bindableValue)
+    Q_PROPERTY(float value MEMBER value BINDABLE bindableValue RESET resetValue)
     Q_PROPERTY(float value2 MEMBER value2 BINDABLE bindableValue2)
 public:
+    void resetValue() { value = 2; }
     QProperty<float> value;
     QProperty<float> value2;
     QBindable<float> bindableValue() { return QBindable<float>(&value); }
     QBindable<float> bindableValue2() { return QBindable<float>(&value2); }
 };
 
+struct ClassWithQObjectProperty : public QObject
+{
+    Q_OBJECT
+    Q_PROPERTY(int value READ value WRITE setValue BINDABLE bindableValue RESET resetValue)
+    Q_PROPERTY(int value2 READ value2 WRITE setValue2 BINDABLE bindableValue2)
+public:
+    Q_OBJECT_BINDABLE_PROPERTY(ClassWithQObjectProperty, int, m_value);
+    QBindable<int> bindableValue() {return &m_value;}
+    void resetValue() { m_value = 2; }
+    int value() { return m_value; }
+    void setValue(int i) { m_value = i; }
+
+    Q_OBJECT_BINDABLE_PROPERTY(ClassWithQObjectProperty, int, m_value2);
+    QBindable<int> bindableValue2() {return &m_value2;}
+    int value2() { return m_value2; }
+    void setValue2(int i) { m_value2 = i; }
+};
+
 class VariantConvertObject : public QObject
 {
     Q_OBJECT
diff --git a/tests/auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp b/tests/auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp
index ce0eb5f5a1..43fb3fc1eb 100644
--- a/tests/auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp
+++ b/tests/auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp
@@ -382,6 +382,10 @@ private slots:
     void saveAccumulatorBeforeToInt32();
     void intMinDividedByMinusOne();
     void undefinedPropertiesInObjectWrapper();
+    void qpropertyBindingHandlesUndefinedCorrectly_data();
+    void qpropertyBindingHandlesUndefinedCorrectly();
+    void qpropertyBindingHandlesUndefinedWithoutResetCorrectly_data();
+    void qpropertyBindingHandlesUndefinedWithoutResetCorrectly();
     void hugeRegexpQuantifiers();
     void singletonTypeWrapperLookup();
     void getThisObject();
@@ -9193,6 +9197,82 @@ void tst_qqmlecmascript::undefinedPropertiesInObjectWrapper()
     QVERIFY(!object.isNull());
 }
 
+void tst_qqmlecmascript::qpropertyBindingHandlesUndefinedCorrectly_data()
+{
+    QTest::addColumn<QString>("fileName");
+    QTest::addRow("QProperty") << "qpropertyBindingUndefined.qml";
+    QTest::addRow("QObjectProperty") << "qpropertyBindingUndefined2.qml";
+}
+
+void tst_qqmlecmascript::qpropertyBindingHandlesUndefinedCorrectly()
+{
+    QFETCH(QString, fileName);
+    QQmlEngine engine;
+    QQmlComponent component(&engine, testFileUrl(fileName));
+    QScopedPointer<QObject> root(component.create());
+    QVERIFY2(root, qPrintable(component.errorString()));
+    // sanity check
+    QCOMPARE(root->property("value").toInt(), 1);
+    // If the binding evaluates to undefined,
+    root->setProperty("toggle", true);
+    // then the property gets reset.
+    QCOMPARE(root->property("value").toInt(), 2);
+    // If the binding reevaluates, the value should not change
+    root->setProperty("anotherValue", 3);
+    QCOMPARE(root->property("value").toInt(), 2);
+    // When the binding becomes defined again
+    root->setProperty("toggle", false);
+    // we obtain the correct new value.
+    QCOMPARE(root->property("value").toInt(), 3);
+}
+
+struct ClassWithQCompatProperty : public QObject
+{
+    Q_OBJECT
+    Q_PROPERTY(int value READ value WRITE setValue BINDABLE bindableValue RESET resetValue)
+    Q_PROPERTY(int value2 READ value2 WRITE setValue2 BINDABLE bindableValue2)
+public:
+    QBindable<int> bindableValue() {return &m_value;}
+    void resetValue() { m_value.setValue(2); }
+    int value() { return m_value; }
+    void setValue(int i) { m_value.setValue(i); }
+    Q_OBJECT_COMPAT_PROPERTY(ClassWithQCompatProperty, int, m_value, &ClassWithQCompatProperty::setValue);
+
+    QBindable<int> bindableValue2() {return &m_value2;}
+    int value2() { return m_value2; }
+    void setValue2(int i) { m_value2.setValue(i); }
+    Q_OBJECT_COMPAT_PROPERTY(ClassWithQCompatProperty, int, m_value2, &ClassWithQCompatProperty::setValue2);
+};
+
+void tst_qqmlecmascript::qpropertyBindingHandlesUndefinedWithoutResetCorrectly_data()
+{
+        qmlRegisterType<ClassWithQCompatProperty>("Qt.test", 1, 0, "ClassWithQCompatProperty");
+    QTest::addColumn<QString>("fileName");
+    QTest::addRow("QProperty") << "qpropertyBindingUndefinedWithoutReset1.qml";
+    QTest::addRow("QObjectProperty") << "qpropertyBindingUndefinedWithoutReset2.qml";
+    QTest::addRow("QCompatProperty") << "qpropertyBindingUndefinedWithoutReset3.qml";
+}
+
+void tst_qqmlecmascript::qpropertyBindingHandlesUndefinedWithoutResetCorrectly()
+{
+    QQmlEngine engine;
+    QFETCH(QString, fileName);
+    QQmlComponent component(&engine, testFileUrl(fileName));
+    QScopedPointer<QObject> root(component.create());
+    QVERIFY2(root, qPrintable(component.errorString()));
+    // sanity check
+    QCOMPARE(root->property("value2").toInt(), 1);
+    // If the binding evaluates to undefined,
+    root->setProperty("toggle", true);
+    // then the value still stays the same.
+    QEXPECT_FAIL("QCompatProperty", "Not implemented for QObjectCompatProperty", Continue);
+    QCOMPARE(root->property("value2").toInt(), 1);
+    // and the binding is still active
+    root->setProperty("anotherValue", 2);
+    root->setProperty("toggle", false);
+    QCOMPARE(root->property("value2").toInt(), 2);
+}
+
 void tst_qqmlecmascript::hugeRegexpQuantifiers()
 {
     QJSEngine engine;