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>

3 files changed, 77 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>