QQmlPropertyBinding: improve error reporting

This change ensures that bindings created in QML between new-style
properties contain information about which property caused the loop. To
do this, we store additional information about the property involved to
retrieve its name and position at a later point. We print the warning in
case we detect a binding loop in evaluate, and also set the error
reporting callback correctly, so that the condition can be reported when
the loop is detected in another part of the binding evaluation.

In addition, we do not only set the QPropertyBinding's error member when
JS evaluation results in an error, but also print the warning with
qmlWarning.

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

9 files changed, 195 insertions, 7 deletions

diff --git a/src/qml/qml/qqmlobjectcreator.cpp b/src/qml/qml/qqmlobjectcreator.cpp
index 0a50234136..47d1b5e785 100644
--- a/src/qml/qml/qqmlobjectcreator.cpp
+++ b/src/qml/qml/qqmlobjectcreator.cpp
@@ -922,7 +922,8 @@ bool QQmlObjectCreator::setPropertyBinding(const QQmlPropertyData *bindingProper
                 qmlBinding = QQmlTranslationPropertyBinding::create(bindingProperty, compilationUnit, binding);
             } else {
                 QV4::Function *runtimeFunction = compilationUnit->runtimeFunctions[binding->value.compiledScriptIndex];
-                qmlBinding = QQmlPropertyBinding::create(bindingProperty, runtimeFunction, _scopeObject, context, currentQmlContext());
+                QQmlPropertyIndex index(bindingProperty->coreIndex(), -1);
+                qmlBinding = QQmlPropertyBinding::create(bindingProperty, runtimeFunction, _scopeObject, context, currentQmlContext(), _bindingTarget, index);
             }
             sharedState.data()->allQPropertyBindings.emplaceBack(_bindingTarget, bindingProperty->coreIndex(), qmlBinding);
         } else {
diff --git a/src/qml/qml/qqmlpropertybinding.cpp b/src/qml/qml/qqmlpropertybinding.cpp
index eaf3524487..313cd7f395 100644
--- a/src/qml/qml/qqmlpropertybinding.cpp
+++ b/src/qml/qml/qqmlpropertybinding.cpp
@@ -44,7 +44,7 @@ QT_BEGIN_NAMESPACE
 
 QUntypedPropertyBinding QQmlPropertyBinding::create(const QQmlPropertyData *pd, QV4::Function *function,
                                                     QObject *obj, const QQmlRefPointer<QQmlContextData> &ctxt,
-                                                    QV4::ExecutionContext *scope)
+                                                    QV4::ExecutionContext *scope, QObject *target, QQmlPropertyIndex targetIndex)
 {
     if (auto aotFunction = function->aotFunction; aotFunction && aotFunction->returnType == pd->propType()) {
         return QUntypedPropertyBinding(aotFunction->returnType,
@@ -69,7 +69,7 @@ QUntypedPropertyBinding QQmlPropertyBinding::create(const QQmlPropertyData *pd,
     }
 
     auto buffer = new std::byte[sizeof(QQmlPropertyBinding)]; // QQmlPropertyBinding uses delete[]
-    auto binding = new (buffer) QQmlPropertyBinding(QMetaType(pd->propType()));
+    auto binding = new(buffer) QQmlPropertyBinding(QMetaType(pd->propType()), target, targetIndex);
     binding->setNotifyOnValueChanged(true);
     binding->setContext(ctxt);
     binding->setScopeObject(obj);
@@ -87,6 +87,7 @@ void QQmlPropertyBinding::expressionChanged()
         err.setLine(location.line);
         err.setColumn(location.column);
         err.setDescription(QString::fromLatin1("Binding loop detected"));
+        err.setObject(target());
         qmlWarning(this->scopeObject(), err);
         return;
     }
@@ -95,11 +96,16 @@ void QQmlPropertyBinding::expressionChanged()
     m_error.setTag(currentTag);
 }
 
-QQmlPropertyBinding::QQmlPropertyBinding(QMetaType mt)
+QQmlPropertyBinding::QQmlPropertyBinding(QMetaType mt, QObject *target, QQmlPropertyIndex targetIndex)
     : QPropertyBindingPrivate(mt,
                               &QtPrivate::bindingFunctionVTable<QQmlPropertyBinding>,
-                              QPropertyBindingSourceLocation())
+                              QPropertyBindingSourceLocation(), true)
 {
+    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};
+    errorCallBack = bindingErrorCallback;
 }
 
 bool QQmlPropertyBinding::evaluate(QMetaType metaType, void *dataPtr)
@@ -124,6 +130,7 @@ bool QQmlPropertyBinding::evaluate(QMetaType metaType, void *dataPtr)
     if (hasError()) {
         QPropertyBindingError error(QPropertyBindingError::UnknownError, delayedError()->error().description());
         QPropertyBindingPrivate::currentlyEvaluatingBinding()->setError(std::move(error));
+        bindingErrorCallback(this);
         return false;
     }
 
@@ -194,6 +201,57 @@ bool QQmlPropertyBinding::evaluate(QMetaType metaType, void *dataPtr)
     return hasChanged;
 }
 
+QString QQmlPropertyBinding::createBindingLoopErrorDescription(QJSEnginePrivate *ep)
+{
+    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);
+    return QStringLiteral(R"(QML %1: Binding loop detected for property "%2")").arg(QQmlMetaType::prettyTypeName(target()) , prop.name());
+}
+
+QObject *QQmlPropertyBinding::target()
+{
+    return std::launder(reinterpret_cast<TargetData *>(&declarativeExtraData))->target;
+}
+
+QQmlPropertyIndex QQmlPropertyBinding::targetIndex()
+{
+    return std::launder(reinterpret_cast<TargetData *>(&declarativeExtraData))->targetIndex;
+}
+
+void QQmlPropertyBinding::bindingErrorCallback(QPropertyBindingPrivate *that)
+{
+    auto This = static_cast<QQmlPropertyBinding *>(that);
+    auto target = This->target();
+    auto engine = qmlEngine(target);
+    if (!engine)
+        return;
+    auto ep = QQmlEnginePrivate::get(engine);
+
+    auto error = This->bindingError();
+    QQmlError qmlError;
+    auto location = This->QQmlJavaScriptExpression::sourceLocation();
+    qmlError.setColumn(location.column);
+    qmlError.setLine(location.line);
+    qmlError.setUrl(QUrl {location.sourceFile});
+    auto description = error.description();
+    if (error.type() == QPropertyBindingError::BindingLoop) {
+        description = This->createBindingLoopErrorDescription(ep);
+    }
+    qmlError.setDescription(description);
+    qmlError.setObject(target);
+    ep->warning(qmlError);
+}
+
 QUntypedPropertyBinding QQmlTranslationPropertyBinding::create(const QQmlPropertyData *pd, const QQmlRefPointer<QV4::ExecutableCompilationUnit> &compilationUnit, const QV4::CompiledData::Binding *binding)
 {
     auto translationBinding = [compilationUnit, binding](const QMetaType &metaType, void *dataPtr) -> bool {
diff --git a/src/qml/qml/qqmlpropertybinding_p.h b/src/qml/qml/qqmlpropertybinding_p.h
index 1ea82b26be..4a9d1f4558 100644
--- a/src/qml/qml/qqmlpropertybinding_p.h
+++ b/src/qml/qml/qqmlpropertybinding_p.h
@@ -74,7 +74,8 @@ public:
 
     static QUntypedPropertyBinding create(const QQmlPropertyData *pd, QV4::Function *function,
                                           QObject *obj, const QQmlRefPointer<QQmlContextData> &ctxt,
-                                          QV4::ExecutionContext *scope);
+                                          QV4::ExecutionContext *scope, QObject *target,
+                                          QQmlPropertyIndex targetIndex);
 
     void expressionChanged() override;
 
@@ -87,11 +88,21 @@ public:
     }
 
 private:
-    QQmlPropertyBinding(QMetaType metaType);
+    QQmlPropertyBinding(QMetaType metaType, QObject *target, QQmlPropertyIndex targetIndex);
 
     bool evaluate(QMetaType metaType, void *dataPtr);
 
+    QString createBindingLoopErrorDescription(QJSEnginePrivate *ep);
 
+    struct TargetData {
+        QObject *target;
+        QQmlPropertyIndex targetIndex;
+    };
+
+    QObject *target();
+    QQmlPropertyIndex targetIndex();
+
+    static void bindingErrorCallback(QPropertyBindingPrivate *);
 };
 
 template <auto I>
diff --git a/tests/auto/qml/qqmlecmascript/data/bindingLoopEagerEager.qml b/tests/auto/qml/qqmlecmascript/data/bindingLoopEagerEager.qml
new file mode 100644
index 0000000000..a320ba56e6
--- /dev/null
+++ b/tests/auto/qml/qqmlecmascript/data/bindingLoopEagerEager.qml
@@ -0,0 +1,6 @@
+import test
+
+BindingLoop {
+    eager1: eager2+1
+    eager2: eager1+1
+}
diff --git a/tests/auto/qml/qqmlecmascript/data/bindingLoopEagerLazy.qml b/tests/auto/qml/qqmlecmascript/data/bindingLoopEagerLazy.qml
new file mode 100644
index 0000000000..aabd41e61c
--- /dev/null
+++ b/tests/auto/qml/qqmlecmascript/data/bindingLoopEagerLazy.qml
@@ -0,0 +1,13 @@
+import test
+import QtQml
+
+QtObject {
+    property BindingLoop a: BindingLoop {
+        value: b.eager1+1
+    }
+
+    property BindingLoop b: BindingLoop {
+        eager1: a.value+1
+    }
+}
+
diff --git a/tests/auto/qml/qqmlecmascript/data/bindingLoopEagerOld.qml b/tests/auto/qml/qqmlecmascript/data/bindingLoopEagerOld.qml
new file mode 100644
index 0000000000..88553ed4a7
--- /dev/null
+++ b/tests/auto/qml/qqmlecmascript/data/bindingLoopEagerOld.qml
@@ -0,0 +1,6 @@
+import test
+
+BindingLoop {
+    eager1: oldprop+1
+    oldprop: eager1+1
+}
diff --git a/tests/auto/qml/qqmlecmascript/data/bindingLoopLazyEager.qml b/tests/auto/qml/qqmlecmascript/data/bindingLoopLazyEager.qml
new file mode 100644
index 0000000000..98fef4cbda
--- /dev/null
+++ b/tests/auto/qml/qqmlecmascript/data/bindingLoopLazyEager.qml
@@ -0,0 +1,6 @@
+import test
+
+BindingLoop {
+    eager1: value+1
+    value: eager1+1
+}
diff --git a/tests/auto/qml/qqmlecmascript/data/bindingLoopLazyLazy.qml b/tests/auto/qml/qqmlecmascript/data/bindingLoopLazyLazy.qml
new file mode 100644
index 0000000000..c054282f93
--- /dev/null
+++ b/tests/auto/qml/qqmlecmascript/data/bindingLoopLazyLazy.qml
@@ -0,0 +1,12 @@
+import test
+import QtQml
+
+BindingLoop {
+    id: loop
+    value: value2 + 1
+    value2: value + 1
+
+    Component.onCompleted: {
+        let x = loop.value2 // if we do not read the value, we don't detect the loop...
+    }
+}
diff --git a/tests/auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp b/tests/auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp
index 02d044aa63..86bd32ccbe 100644
--- a/tests/auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp
+++ b/tests/auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp
@@ -53,6 +53,7 @@
 #include <private/qv4objectiterator_p.h>
 #include <private/qqmlabstractbinding_p.h>
 #include <private/qqmlvaluetypeproxybinding_p.h>
+#include <QtCore/private/qproperty_p.h>
 
 #ifdef Q_CC_MSVC
 #define NO_INLINE __declspec(noinline)
@@ -92,6 +93,8 @@ private slots:
     void signalAssignment();
     void signalArguments();
     void bindingLoop();
+    void cppPropertyBindingLoop_data();
+    void cppPropertyBindingLoop();
     void basicExpressions();
     void basicExpressions_data();
     void arrayExpressions();
@@ -883,6 +886,78 @@ void tst_qqmlecmascript::bindingLoop()
     delete object;
 }
 
+
+struct QPropertyBindingLoop : public QObject
+{
+    Q_OBJECT
+    Q_PROPERTY(float value MEMBER value BINDABLE bindableValue)
+    Q_PROPERTY(float value2 MEMBER value2 BINDABLE bindableValue2)
+    Q_PROPERTY(float eager1 READ eager1 WRITE setEager1 BINDABLE bindableEager1)
+    Q_PROPERTY(float eager2 READ eager2 WRITE setEager2 BINDABLE bindableEager2)
+    Q_PROPERTY(float oldprop READ oldprop WRITE setOldprop NOTIFY oldpropChanged)
+signals:
+    void oldpropChanged();
+public:
+
+    float eager1() {return eager1Data;}
+    void setEager1(float f) {eager1Data = f;}
+    float eager2() {return eager2Data;}
+    void setEager2(float f) {eager2Data = f;}
+    QProperty<float> value;
+    QProperty<float> value2;
+    QBindable<float> bindableValue() { return QBindable<float>(&value); }
+    QBindable<float> bindableValue2() { return QBindable<float>(&value2); }
+    QBindable<float> bindableEager1() {return QBindable<float>(&eager1Data);}
+    QBindable<float> bindableEager2() {return QBindable<float>(&eager2Data);}
+    float m_oldprop;
+
+    float oldprop() const;
+    void setOldprop(float oldprop);
+
+private:
+    Q_OBJECT_COMPAT_PROPERTY(QPropertyBindingLoop, float, eager1Data, &QPropertyBindingLoop::setEager1);
+    Q_OBJECT_COMPAT_PROPERTY(QPropertyBindingLoop, float, eager2Data, &QPropertyBindingLoop::setEager2);
+
+};
+
+
+float QPropertyBindingLoop::oldprop() const
+{
+    return m_oldprop;
+}
+
+void QPropertyBindingLoop::setOldprop(float oldprop)
+{
+    m_oldprop = oldprop;
+    emit oldpropChanged();
+}
+
+void tst_qqmlecmascript::cppPropertyBindingLoop_data()
+{
+    QTest::addColumn<QString>("file");
+    QTest::addColumn<QString>("warningMsg");
+
+    QTest::newRow("eager eager") << "bindingLoopEagerEager.qml" << R"(:4:5: QML BindingLoop: Binding loop detected for property "eager1")";
+    QTest::newRow("lazy lazy") << "bindingLoopLazyLazy.qml" << R"(:7:5: QML BindingLoop: Binding loop detected for property "value2")";
+    QTest::newRow("lazy eager") << "bindingLoopLazyEager.qml" << R"(:4:5: QML BindingLoop: Binding loop detected for property "eager1")";
+    QTest::newRow("eager lazy") << "bindingLoopEagerLazy.qml" << R"(:10:9: QML BindingLoop: Binding loop detected for property "eager1")";
+    QTest::newRow("eager old") << "bindingLoopEagerOld.qml" << R"(:4:5: QML BindingLoop: Binding loop detected for property "eager1")";
+
+    qmlRegisterType<QPropertyBindingLoop>("test", 1, 0, "BindingLoop");
+}
+
+void tst_qqmlecmascript::cppPropertyBindingLoop()
+{
+    QFETCH(QString, file);
+    QFETCH(QString, warningMsg);
+    QQmlEngine engine;
+    QQmlComponent component(&engine, testFileUrl(file));
+    QString warning = component.url().toString() + warningMsg;
+    QTest::ignoreMessage(QtWarningMsg, warning.toLatin1().constData());
+    QScopedPointer<QObject> object { component.create() };
+    QVERIFY2(object, qPrintable(component.errorString()));
+}
+
 void tst_qqmlecmascript::basicExpressions_data()
 {
     QTest::addColumn<QString>("expression");