Rework QQmlJSScope::causesImplicitComponentWrapping()

It does not seem correct to be able to use this function for both sides
of binding expressions: the property type and the assigned QML type
(`<property> : <qml type> {}`). Instead, it should really be a function
that considers both sides together (and actually checks the sides
differently!)

What we really should do for the assigned QML type is figure whether
its *first* non-composite base is a QQmlComponent - not just that it
is a QQmlComponent itself. Leave the QQmlAbstractDelegateComponent check
in place since this part is correct

The property itself, on the other hand, seems to cause an implicit
component wrapping if its type is QQmlComponent-derived (which means we
have to check the full C++ hierarchy)

As a drive by, also update QQmlJSScope::canAssign() since it used the
old function internally and was ignoring the case when a to-be-assigned
type lacks a C++ base type hierarchy (e.g. because it does not exist
ahead of time)

Pick-to: 6.3
Change-Id: I23524fa54d45d9140e1cafd9f81ef1f68d95f3a7
Reviewed-by: Ulf Hermann <ulf.hermann@qt.io>

src/qmlcompiler/qqmljsimportvisitor.cpp

@@ -569,10 +569,8 @@ void QQmlJSImportVisitor::processDefaultProperties()
             // Assigning any element to a QQmlComponent property implicitly wraps it into a Component
             // Check whether the property can be assigned the scope
             if (propType->canAssign(scope)) {
-                if (propType->causesImplicitComponentWrapping()) {
-                    // mark the scope as implicitly wrapped, unless it is a Component
-                    scope->setIsWrappedInImplicitComponent(!scope->causesImplicitComponentWrapping());
-                }
+                scope->setIsWrappedInImplicitComponent(
+                        QQmlJSScope::causesImplicitComponentWrapping(defaultProp, scope));
                 continue;
             }
 
@@ -679,9 +677,8 @@ void QQmlJSImportVisitor::processPropertyBindingObjects()
             continue;
         }
 
-        if (property.type()->causesImplicitComponentWrapping())
-            objectBinding.childScope->setIsWrappedInImplicitComponent(
-                    !objectBinding.childScope->causesImplicitComponentWrapping());
+        objectBinding.childScope->setIsWrappedInImplicitComponent(
+                QQmlJSScope::causesImplicitComponentWrapping(property, childScope));
 
         // unique because it's per-scope and per-property
         const auto uniqueBindingId = qMakePair(objectBinding.scope, objectBinding.name);

src/qmlcompiler/qqmljsscope.cpp

@@ -247,26 +247,44 @@ QHash<QString, QQmlJSMetaEnum> QQmlJSScope::enumerations() const
 }
 
 /*!
-    Returns if assigning to a property of this type would cause
-    implicit component wrapping for non-Component types.
-
-    \note This method can also be used to check whether a type needs
-    to be implicitly wrapped: A type for which this function returns true
-    doesn't need to be actually wrapped.
+    Returns if assigning \a assignedType to \a property would require an
+    implicit component wrapping.
  */
-bool QQmlJSScope::causesImplicitComponentWrapping() const {
-    if (internalName() == u"QQmlComponent")
-        return true;
-    else if (isComposite()) // composite types are never treated as Component
-        return false;
-    // A class which is derived from component is not treated as a Component
-    // However isUsableComponent considers also QQmlAbstractDelegateComponent
-    // See isUsableComponent in qqmltypecompiler.cpp
+bool QQmlJSScope::causesImplicitComponentWrapping(const QQmlJSMetaProperty &property,
+                                                  const QQmlJSScope::ConstPtr &assignedType)
+{
+    // See QQmlComponentAndAliasResolver::findAndRegisterImplicitComponents()
+    // for the logic in qqmltypecompiler
 
-    for (auto cppBase = nonCompositeBaseType(baseType()); cppBase; cppBase = cppBase->baseType())
-        if (cppBase->internalName() == u"QQmlAbstractDelegateComponent")
-            return true;
-    return false;
+    // Note: unlike findAndRegisterImplicitComponents() we do not check whether
+    // the property type is *derived* from QQmlComponent at some point because
+    // this is actually meaningless (and in the case of QQmlComponent::create()
+    // gets rejected in QQmlPropertyValidator): if the type is not a
+    // QQmlComponent, we have a type mismatch because of assigning a Component
+    // object to a non-Component property
+    const bool propertyVerdict = property.type()->internalName() == u"QQmlComponent";
+
+    const bool assignedTypeVerdict = [&assignedType]() {
+        // Note: nonCompositeBaseType covers the case when assignedType itself
+        // is non-composite
+        auto cppBase = nonCompositeBaseType(assignedType);
+        Q_ASSERT(cppBase); // any QML type has (or must have) a C++ base type
+
+        // See isUsableComponent() in qqmltypecompiler.cpp: along with checking
+        // whether a type has a QQmlComponent static meta object (which we
+        // substitute here with checking the first non-composite base for being
+        // a QQmlComponent), it also excludes QQmlAbstractDelegateComponent
+        // subclasses from implicit wrapping
+        if (cppBase->internalName() == u"QQmlComponent")
+            return false;
+        for (; cppBase; cppBase = cppBase->baseType()) {
+            if (cppBase->internalName() == u"QQmlAbstractDelegateComponent")
+                return false;
+        }
+        return true;
+    }();
+
+    return propertyVerdict && assignedTypeVerdict;
 }
 
 /*!
@@ -879,7 +897,24 @@ bool QQmlJSScope::canAssign(const QQmlJSScope::ConstPtr &derived) const
     if (!derived)
         return false;
 
-    bool isBaseComponent = causesImplicitComponentWrapping();
+    // expect this and derived types to have non-composite bases
+    Q_ASSERT(!isComposite() || nonCompositeBaseType(baseType()));
+    Q_ASSERT(nonCompositeBaseType(derived));
+
+    // the logic with isBaseComponent (as well as the way we set this flag)
+    // feels wrong - QTBUG-101940
+    const bool isBaseComponent = [this]() {
+        if (internalName() == u"QQmlComponent")
+            return true;
+        else if (isComposite())
+            return false;
+        for (auto cppBase = nonCompositeBaseType(baseType()); cppBase;
+             cppBase = cppBase->baseType()) {
+            if (cppBase->internalName() == u"QQmlAbstractDelegateComponent")
+                return true;
+        }
+        return false;
+    }();
 
     QDuplicateTracker<QQmlJSScope::ConstPtr> seen;
     for (auto scope = derived; !scope.isNull() && !seen.hasSeen(scope);

src/qmlcompiler/qqmljsscope_p.h

@@ -249,7 +249,8 @@ public:
         return m_internalName + suffix;
     }
 
-    bool causesImplicitComponentWrapping() const;
+    static bool causesImplicitComponentWrapping(const QQmlJSMetaProperty &property,
+                                                const QQmlJSScope::ConstPtr &assignedType);
     bool isComponentRootElement() const;
 
     void setInterfaceNames(const QStringList& interfaces) { m_interfaceNames = interfaces; }
@@ -358,6 +359,7 @@ public:
     bool hasCustomParser() const { return m_flags & CustomParser; }
     bool isArrayScope() const { return m_flags & Array; }
     bool isInlineComponent() const { return m_flags & InlineComponent; }
+    bool isWrappedInImplicitComponent() const { return m_flags & WrappedInImplicitComponent; }
     void setIsSingleton(bool v) { m_flags.setFlag(Singleton, v); }
     void setIsCreatable(bool v) { m_flags.setFlag(Creatable, v); }
     void setIsComposite(bool v) { m_flags.setFlag(Composite, v); }

tests/auto/qml/qmllint/data/assignNonExistingTypeToVarProp.qml

@@ -0,0 +1,6 @@
+import QtQuick
+
+ListView {
+    // model is a QVariant, so we can theoretically assign anything to it
+    model: NonExistingType {}
+}

tests/auto/qml/qmllint/tst_qmllint.cpp

@@ -990,6 +990,10 @@ expression: \${expr} \${expr} \\\${expr} \\\${expr}`)",
             << Result { { Message { QStringLiteral(
                        "Property \"callLater\" is a QJSValue property. It may or may not be "
                        "a method. Use a regular Q_INVOKABLE instead.") } } };
+    QTest::newRow("assignNonExistingTypeToVarProp")
+            << QStringLiteral("assignNonExistingTypeToVarProp.qml")
+            << Result { { Message { QStringLiteral(
+                       "NonExistingType was not found. Did you add all import paths?") } } };
 }
 
 void TestQmllint::dirtyQmlCode()

tests/auto/qml/qqmljsscope/CMakeLists.txt

@@ -9,7 +9,12 @@ qt_internal_add_test(tst_qqmljsscope
         data/orderedBindings.qml
 )
 
-add_dependencies(tst_qqmljsscope Qt::Quick) # we need QtQuick QML module
+# we need the following QML modules implicitly (since we import them):
+add_dependencies(tst_qqmljsscope Qt::Quick)
+if(QT_FEATURE_qml_delegate_model AND TARGET Qt::LabsQmlModels)
+    add_dependencies(tst_qqmljsscope Qt::LabsQmlModels)
+    target_compile_definitions(tst_qqmljsscope PRIVATE LABS_QML_MODELS_PRESENT)
+endif()
 
 qt_internal_extend_target(tst_qqmljsscope CONDITION ANDROID OR IOS
     DEFINES

tests/auto/qml/qqmljsscope/data/ComponentType.qml

@@ -0,0 +1,6 @@
+import QtQml
+Component {
+    QtObject { // having a type inside Component is required
+        objectName: "enclosed"
+    }
+}

tests/auto/qml/qqmljsscope/data/componentWrappedObjects.qml

@@ -0,0 +1,10 @@
+import QtQuick
+import Qt.labs.qmlmodels
+Item {
+    // Note: use properties instead of ids to uniquely identify types
+    property Component nonWrapped1: Component { property int nonWrapped1; Rectangle {} }
+    property Component nonWrapped2: ComponentType { property int nonWrapped2 }
+    property Component nonWrapped3: DelegateChooser { property int nonWrapped3 }
+
+    property Component wrapped: Text { property int wrapped }
+}

tests/auto/qml/qqmljsscope/data/unknownCppBaseAssigningToVar.qml

@@ -0,0 +1,7 @@
+import QtQuick
+
+ListView {
+    // model is a QVariant, so we can theoretically assign anything to it, even
+    // the non existing type!
+    model: NonExistingType {}
+}

tests/auto/qml/qqmljsscope/tst_qqmljsscope.cpp

@@ -95,6 +95,10 @@ private Q_SLOTS:
     void signalCreationDifferences();
     void allTypesAvailable();
     void shadowing();
+#ifdef LABS_QML_MODELS_PRESENT
+    void componentWrappedObjects();
+#endif
+    void unknownCppBase();
 
 public:
     tst_qqmljsscope()
@@ -210,5 +214,34 @@ void tst_qqmljsscope::shadowing()
     QCOMPARE(methods[u"method_shadowed"_qs].parameterNames().size(), 0);
 }
 
+#ifdef LABS_QML_MODELS_PRESENT
+void tst_qqmljsscope::componentWrappedObjects()
+{
+    QQmlJSScope::ConstPtr root = run(u"componentWrappedObjects.qml"_qs);
+    QVERIFY(root);
+
+    auto children = root->childScopes();
+    QCOMPARE(children.size(), 4);
+
+    const auto isGoodType = [](const QQmlJSScope::ConstPtr &type, const QString &propertyName,
+                               bool isWrapped) {
+        return type->hasOwnProperty(propertyName)
+                && type->isWrappedInImplicitComponent() == isWrapped;
+    };
+
+    QVERIFY(isGoodType(children[0], u"nonWrapped1"_qs, false));
+    QVERIFY(isGoodType(children[1], u"nonWrapped2"_qs, false));
+    QVERIFY(isGoodType(children[2], u"nonWrapped3"_qs, false));
+    QVERIFY(isGoodType(children[3], u"wrapped"_qs, true));
+}
+#endif
+
+void tst_qqmljsscope::unknownCppBase()
+{
+    QQmlJSScope::ConstPtr root = run(u"unknownCppBaseAssigningToVar.qml"_qs);
+    QVERIFY(root);
+    // we should not crash here, then it is a success
+}
+
 QTEST_MAIN(tst_qqmljsscope)
 #include "tst_qqmljsscope.moc"