Do not store instantiation errors in QQmlComponent

QQmlComponent::createWithInitialProperties() allows to pass non-existent
properties to create the object with. In this case, we still return a
valid pointer (pre-existing), however, the state of the component is
affected: there are errors internally that fail repeated calls even with
valid properties specified. Fix this, aligning to the behavior of
QQmlComponent::createObject() which is a QML counterpart of the same
function (to a degree)

Additionally, make required properties warnings also not break the
component state: if a required property is not set, we return nullptr
instead of object. The error state is affected but it gets cleaned
on the next creation call, allowing one to set the missing required
properties

Unify the logic (with slight deviations) between create() (the
non-incubator-accepting version), createWithInitialProperties() and
createObject() as all they all should do roughly equivalent things

[ChangeLog][QQmlComponent][Important Behavior Changes] Setting properties
via createWithInitialProperties() and setInitialProperties() no longer
affects the QQmlComponent's error state unless there are unset required
properties. A warning is issued when a property could not be set.

Fixes: QTBUG-101439
Change-Id: I814c00bc3715d7a54004c3fcd0af6ee6d50b0726
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
Reviewed-by: Ulf Hermann <ulf.hermann@qt.io>

4 files changed, 108 insertions, 84 deletions

diff --git a/src/qml/qml/qqmlcomponent.cpp b/src/qml/qml/qqmlcomponent.cpp
index 8c74272457..ad416b04fd 100644
--- a/src/qml/qml/qqmlcomponent.cpp
+++ b/src/qml/qml/qqmlcomponent.cpp
@@ -303,7 +303,8 @@ void QQmlComponentPrivate::fromTypeData(const QQmlRefPointer<QQmlTypeData> &data
 
     if (!compilationUnit) {
         Q_ASSERT(data->isError());
-        state.errors = data->errors();
+        state.errors.clear();
+        state.appendErrors(data->errors());
     }
 }
 
@@ -359,7 +360,7 @@ bool QQmlComponentPrivate::setInitialProperty(
         segment = scope.engine->newString(properties.last());
         object->put(segment, scope.engine->metaTypeToJS(value.metaType(), value.constData()));
         if (scope.engine->hasException) {
-            state.errors.push_back(scope.engine->catchExceptionAsQmlError());
+            qmlWarning(base, scope.engine->catchExceptionAsQmlError());
             scope.engine->hasException = false;
             return false;
         }
@@ -380,7 +381,7 @@ bool QQmlComponentPrivate::setInitialProperty(
                                                 "%2 does not have a property called %1")
                                          .arg(name, QQmlMetaType::prettyTypeName(base)));
         }
-        state.errors.push_back(error);
+        qmlWarning(base, error);
         return false;
     }
 
@@ -408,8 +409,8 @@ QQmlComponent::~QQmlComponent()
 
         if (isError()) {
             qWarning() << "This may have been caused by one of the following errors:";
-            for (const QQmlError &error : qAsConst(d->state.errors))
-                qWarning().nospace().noquote() << QLatin1String("    ") << error;
+            for (const QQmlComponentPrivate::AnnotatedQmlError &e : qAsConst(d->state.errors))
+                qWarning().nospace().noquote() << QLatin1String("    ") << e.error;
         }
 
         // we might not have the creator anymore if the engine is gone
@@ -710,7 +711,7 @@ void QQmlComponentPrivate::loadUrl(const QUrl &newUrl, QQmlComponent::Compilatio
     if (newUrl.isEmpty()) {
         QQmlError error;
         error.setDescription(QQmlComponent::tr("Invalid empty URL"));
-        state.errors << error;
+        state.errors.emplaceBack(error);
         return;
     }
 
@@ -746,10 +747,11 @@ void QQmlComponentPrivate::loadUrl(const QUrl &newUrl, QQmlComponent::Compilatio
 QList<QQmlError> QQmlComponent::errors() const
 {
     Q_D(const QQmlComponent);
-    if (isError())
-        return d->state.errors;
-    else
-        return QList<QQmlError>();
+    QList<QQmlError> errors;
+    errors.reserve(d->state.errors.size());
+    for (const QQmlComponentPrivate::AnnotatedQmlError &annotated : d->state.errors)
+        errors.emplaceBack(annotated.error);
+    return errors;
 }
 
 /*!
@@ -773,7 +775,8 @@ QString QQmlComponent::errorString() const
     QString ret;
     if(!isError())
         return ret;
-    for (const QQmlError &e : d->state.errors) {
+    for (const QQmlComponentPrivate::AnnotatedQmlError &annotated : d->state.errors) {
+        const QQmlError &e = annotated.error;
         ret += e.url().toString() + QLatin1Char(':') +
                QString::number(e.line()) + QLatin1Char(' ') +
                e.description() + QLatin1Char('\n');
@@ -824,23 +827,7 @@ QQmlComponent::QQmlComponent(QQmlComponentPrivate &dd, QObject *parent)
 QObject *QQmlComponent::create(QQmlContext *context)
 {
     Q_D(QQmlComponent);
-
-    QObject *rv = d->doBeginCreate(this, context);
-    if (rv) {
-        completeCreate();
-    } else if (d->state.completePending) {
-        // overridden completCreate might assume that
-        // the object has actually been created
-        ++creationDepth;
-        QQmlEnginePrivate *ep = QQmlEnginePrivate::get(d->engine);
-        d->complete(ep, &d->state);
-        --creationDepth;
-    }
-    if (rv && !d->requiredProperties().empty()) {
-        delete  rv;
-        return nullptr;
-    }
-    return rv;
+    return d->createWithProperties(nullptr, QVariantMap {}, context);
 }
 
 /*!
@@ -851,9 +838,9 @@ QObject *QQmlComponent::create(QQmlContext *context)
     TODO: also mention errorString() when QTBUG-93239 is fixed
     \endomit
 
-    If any of the \c initialProperties cannot be set, \l isError() will return
-    \c true, and the \l errors() function can be used to
-    get detailed information about the error(s).
+    If any of the \a initialProperties cannot be set, a warning is issued. If
+    there are unset required properties, the object creation fails and returns
+    \c nullptr, in which case \l isError() will return \c true.
 
     \sa QQmlComponent::create
     \since 5.14
@@ -861,16 +848,47 @@ QObject *QQmlComponent::create(QQmlContext *context)
 QObject *QQmlComponent::createWithInitialProperties(const QVariantMap& initialProperties, QQmlContext *context)
 {
     Q_D(QQmlComponent);
+    return d->createWithProperties(nullptr, initialProperties, context);
+}
 
-    QObject *rv = d->doBeginCreate(this, context);
-    if (rv) {
-        setInitialProperties(rv, initialProperties);
-        completeCreate();
-    }
-    if (!d->requiredProperties().empty()) {
-        d->requiredProperties().clear();
+static void QQmlComponent_setQmlParent(QObject *me, QObject *parent); // forward declaration
+
+/*! \internal
+ */
+QObject *QQmlComponentPrivate::createWithProperties(QObject *parent, const QVariantMap &properties,
+                                                    QQmlContext *context, CreateBehavior behavior)
+{
+    Q_Q(QQmlComponent);
+
+    QObject *rv = doBeginCreate(q, context);
+    if (!rv) {
+        if (state.completePending) {
+            // overridden completCreate might assume that
+            // the object has actually been created
+            ++creationDepth;
+            QQmlEnginePrivate *ep = QQmlEnginePrivate::get(engine);
+            complete(ep, &state);
+            --creationDepth;
+        }
         return nullptr;
     }
+
+    QQmlComponent_setQmlParent(rv, parent); // internally checks if parent is nullptr
+
+    q->setInitialProperties(rv, properties);
+    q->completeCreate();
+
+    if (!requiredProperties().empty()) {
+        if (behavior == CreateWarnAboutRequiredProperties) {
+            const RequiredProperties &unsetRequiredProperties = requiredProperties();
+            for (const auto &unsetRequiredProperty : unsetRequiredProperties) {
+                const QQmlError error = unsetRequiredPropertyToQQmlError(unsetRequiredProperty);
+                qmlWarning(rv, error);
+            }
+        }
+        delete rv;
+        rv = nullptr;
+    }
     return rv;
 }
 
@@ -915,7 +933,7 @@ QObject *QQmlComponentPrivate::beginCreate(QQmlRefPointer<QQmlContextData> conte
     auto cleanup = qScopeGuard([this] {
         if (!state.errors.isEmpty() && lcQmlComponentGeneral().isDebugEnabled()) {
             for (const auto &e : qAsConst(state.errors)) {
-                qCDebug(lcQmlComponentGeneral) << "QQmlComponent: " << e.toString();
+                qCDebug(lcQmlComponentGeneral) << "QQmlComponent: " << e.error.toString();
             }
         }
     });
@@ -939,6 +957,16 @@ QObject *QQmlComponentPrivate::beginCreate(QQmlRefPointer<QQmlContextData> conte
         return nullptr;
     }
 
+    // filter out temporary errors as they do not really affect component's
+    // state (they are not part of the document compilation)
+    state.errors.erase(std::remove_if(state.errors.begin(), state.errors.end(),
+                                      [](const QQmlComponentPrivate::AnnotatedQmlError &e) {
+                                          return e.isTransient;
+                                      }),
+                       state.errors.end());
+    if (state.creator)
+        requiredProperties().clear();
+
     if (!q->isReady()) {
         qWarning("QQmlComponent: Component is not ready");
         return nullptr;
@@ -962,7 +990,7 @@ QObject *QQmlComponentPrivate::beginCreate(QQmlRefPointer<QQmlContextData> conte
     state.creator.reset(new QQmlObjectCreator(std::move(context), compilationUnit, creationContext));
     rv = state.creator->create(start);
     if (!rv)
-        state.errors = state.creator->errors;
+        state.appendErrors(state.creator->errors);
     enginePriv->dereferenceScarceResources();
 
     if (rv) {
@@ -997,7 +1025,7 @@ void QQmlComponentPrivate::beginDeferred(QQmlEnginePrivate *enginePriv,
                                  QQmlRefPointer<QQmlContextData>()));
 
         if (!state.creator->populateDeferredProperties(object, deferredData))
-            state.errors << state.creator->errors;
+            state.appendErrors(state.creator->errors);
         deferredData->bindings.clear();
 
         deferredState->push_back(std::move(state));
@@ -1103,7 +1131,7 @@ void QQmlComponentPrivate::completeCreate()
     const RequiredProperties& unsetRequiredProperties = requiredProperties();
     for (const auto& unsetRequiredProperty: unsetRequiredProperties) {
         QQmlError error = unsetRequiredPropertyToQQmlError(unsetRequiredProperty);
-        state.errors.push_back(error);
+        state.errors.push_back(QQmlComponentPrivate::AnnotatedQmlError { error, true });
     }
     if (state.completePending) {
         ++creationDepth;
@@ -1545,46 +1573,14 @@ QObject *QQmlComponent::createObject(QObject *parent, const QVariantMap &propert
 {
     Q_D(QQmlComponent);
     Q_ASSERT(d->engine);
-
-    QQmlContext *ctxt = creationContext();
-    if (!ctxt)
-        ctxt = d->engine->rootContext();
-
-    QObject *rv = beginCreate(ctxt);
-    if (!rv)
-        return nullptr;
-
-    Q_ASSERT(d->state.errors.isEmpty()); // otherwise beginCreate() would return nullptr
-
-    QQmlComponent_setQmlParent(rv, parent);
-
-    if (!properties.isEmpty()) {
-        setInitialProperties(rv, properties);
-        if (!d->state.errors.isEmpty()) {
-            qmlWarning(rv, d->state.errors);
-            d->state.errors.clear();
-        }
-    }
-
-    if (!d->requiredProperties().empty()) {
-        QList<QQmlError> errors;
-        for (const auto &requiredProperty: qAsConst(d->requiredProperties())) {
-            errors.push_back(QQmlComponentPrivate::unsetRequiredPropertyToQQmlError(
-                    requiredProperty));
-        }
-        if (!errors.isEmpty())
-            qmlWarning(rv, errors);
-        delete rv;
-        return nullptr;
+    QObject *rv = d->createWithProperties(parent, properties, creationContext(),
+                                          QQmlComponentPrivate::CreateWarnAboutRequiredProperties);
+    if (rv) {
+        QQmlData *qmlData = QQmlData::get(rv);
+        Q_ASSERT(qmlData);
+        qmlData->explicitIndestructibleSet = false;
+        qmlData->indestructible = false;
     }
-
-    d->completeCreate();
-
-    QQmlData *qmlData = QQmlData::get(rv);
-    Q_ASSERT(qmlData);
-    qmlData->explicitIndestructibleSet = false;
-    qmlData->indestructible = false;
-
     return rv;
 }
 
diff --git a/src/qml/qml/qqmlcomponent_p.h b/src/qml/qml/qqmlcomponent_p.h
index 3edd330ee8..d4a1e10f12 100644
--- a/src/qml/qml/qqmlcomponent_p.h
+++ b/src/qml/qml/qqmlcomponent_p.h
@@ -75,10 +75,31 @@ public:
     bool hadTopLevelRequiredProperties() const;
     QQmlRefPointer<QV4::ExecutableCompilationUnit> compilationUnit;
 
+    struct AnnotatedQmlError
+    {
+        AnnotatedQmlError() = default;
+        AnnotatedQmlError(const QQmlError &error) // convenience ctor
+            : error(error)
+        {
+        }
+        AnnotatedQmlError(const QQmlError &error, bool transient)
+            : error(error), isTransient(transient)
+        {
+        }
+        QQmlError error;
+        bool isTransient = false; // tells if the error is temporary (e.g. unset required property)
+    };
+
     struct ConstructionState {
         std::unique_ptr<QQmlObjectCreator> creator;
-        QList<QQmlError> errors;
+        QList<AnnotatedQmlError> errors;
         bool completePending = false;
+
+        void appendErrors(const QList<QQmlError> &qmlErrors)
+        {
+            for (const QQmlError &e : qmlErrors)
+                errors.emplaceBack(e);
+        }
     };
     ConstructionState state;
 
@@ -103,6 +124,13 @@ public:
     QObject *doBeginCreate(QQmlComponent *q, QQmlContext *context);
     bool setInitialProperty(QObject *component, const QString &name, const QVariant& value);
 
+    enum CreateBehavior {
+        CreateDefault,
+        CreateWarnAboutRequiredProperties,
+    };
+    QObject *createWithProperties(QObject *parent, const QVariantMap &properties,
+                                  QQmlContext *context, CreateBehavior behavior = CreateDefault);
+
     bool isBound() const {
         return compilationUnit->unitData()->flags & QV4::CompiledData::Unit::ComponentsBound;
     }
diff --git a/src/qml/types/qqmlbind.cpp b/src/qml/types/qqmlbind.cpp
index fb2664da73..522dc48c92 100644
--- a/src/qml/types/qqmlbind.cpp
+++ b/src/qml/types/qqmlbind.cpp
@@ -885,7 +885,7 @@ void QQmlBindPrivate::buildBindEntries(QQmlBind *q, QQmlComponentPrivate::Deferr
                 if (constructionState.creator.get()) {
                     ++ep->inProgressCreations;
                     constructionState.creator->finalizePopulateDeferred();
-                    constructionState.errors << constructionState.creator->errors;
+                    constructionState.appendErrors(constructionState.creator->errors);
                     deferredState->push_back(std::move(constructionState));
                 }
             } else {
diff --git a/src/quicktemplates2/qquickdeferredexecute.cpp b/src/quicktemplates2/qquickdeferredexecute.cpp
index f15082f9a1..2e593bc6be 100644
--- a/src/quicktemplates2/qquickdeferredexecute.cpp
+++ b/src/quicktemplates2/qquickdeferredexecute.cpp
@@ -55,7 +55,7 @@ static bool beginDeferred(QQmlEnginePrivate *enginePriv, const QQmlProperty &pro
         for (const QV4::CompiledData::Binding *binding : reversedBindings)
             state.creator->populateDeferredBinding(property, deferData->deferredIdx, binding);
         state.creator->finalizePopulateDeferred();
-        state.errors << state.creator->errors;
+        state.appendErrors(state.creator->errors);
 
         deferredState->push_back(std::move(state));