Compiler: Don't make aliases to required properties required themselves

When a property is declared as required, it needs to be bound when creating the component containing that type. Required properties can be aliased just like any other property. Before this patch, aliases targeting required properties would themselves be marked as required. This is incorrect as this forces the aliased required property to be set through the alias. It should be legal to alias a required property and not set required value through the alias. Therefore, stop marking the alias as required and instead check directly that the required property is bound in some scope for each object creation or that it is forwarded through a root-level alias. A required property will now also be satisfied if it is set through an alias. For this to work, a property alias now also stores its target scope and target property name. This change also adds checking for object definition bindings of the following form. These weren't checked before but really should be. ``` QtObject { property QtObject o: QtObject { required property int i // was not checked } } ``` These changes affect the way qmltc enforces setting required properties. Because it relied on aliases to required properties to be required themselves, it will now fail to enforce certain required properties. Created QTBUG-131777 and marked affected tests as QEXPECT_FAIL. Fixes: QTBUG-127098 Pick-to: 6.9 6.8 6.5 Change-Id: Ib36a43fbf3cc9c79eba6db39cbaf8769f85e2b31 Reviewed-by: Ulf Hermann <ulf.hermann@qt.io>
author: Olivier De Cannière <olivier.decanniere@qt.io> 2025-03-04 12:52:37 +0100
committer: Olivier De Cannière <olivier.decanniere@qt.io> 2025-03-04 17:15:13 +0100
commit: daf57e29de918b7b4be7bb0d469db0c51d41bb07 (patch)
tree: 5940e7eddbbfd820be46a82853b75ec629236113 /src/qmlcompiler/qqmljsimportvisitor.cpp
parent: 3f6823aee9d04f9e67a0815a576310b9ec782eb7 (diff)
1 files changed, 166 insertions, 87 deletions
diff --git a/src/qmlcompiler/qqmljsimportvisitor.cpp b/src/qmlcompiler/qqmljsimportvisitor.cpp
index dc29fcb27e..26ba79d59f 100644
--- a/src/qmlcompiler/qqmljsimportvisitor.cpp
+++ b/src/qmlcompiler/qqmljsimportvisitor.cpp
@@ -310,10 +310,15 @@ void QQmlJSImportVisitor::resolveAliases()
                 newProperty.setIsWritable(targetProperty.isWritable());
                 newProperty.setIsPointer(targetProperty.isPointer());
 
-                if (!typeScope.isNull() && !object->isPropertyLocallyRequired(property.propertyName())) {
-                    object->setPropertyLocallyRequired(
-                            newProperty.propertyName(),
-                            typeScope->isPropertyRequired(targetProperty.propertyName()));
+                const bool onlyId = !property.aliasExpression().contains(u'.');
+                if (onlyId) {
+                    newProperty.setAliasTargetScope(type);
+                    newProperty.setAliasTargetName(QStringLiteral("id-only-alias"));
+                } else {
+                    const auto &ownerScope = QQmlJSScope::ownerOfProperty(
+                                                     typeScope, targetProperty.propertyName()).scope;
+                    newProperty.setAliasTargetScope(ownerScope);
+                    newProperty.setAliasTargetName(targetProperty.propertyName());
                 }
 
                 if (const QString internalName = type->internalName(); !internalName.isEmpty())
@@ -321,6 +326,7 @@ void QQmlJSImportVisitor::resolveAliases()
 
                 Q_ASSERT(newProperty.index() >= 0); // this property is already in object
                 object->addOwnProperty(newProperty);
+                m_aliasDefinitions.append({ object, property.propertyName() });
             }
         }
 
@@ -849,9 +855,30 @@ void QQmlJSImportVisitor::processPropertyBindingObjects()
     }
 }
 
+void QQmlJSImportVisitor::populatePropertyAliases()
+{
+    for (const auto &alias : std::as_const(m_aliasDefinitions)) {
+        const auto &[aliasScope, aliasName] = alias;
+        if (aliasScope.isNull())
+            continue;
+
+        auto property = aliasScope->ownProperty(aliasName);
+        if (!property.isValid() || !property.aliasTargetScope())
+            continue;
+
+        Property target(property.aliasTargetScope(), property.aliasTargetName());
+
+        do {
+            m_propertyAliases[target].append(alias);
+            property = target.scope->property(target.name);
+            target = Property(property.aliasTargetScope(), property.aliasTargetName());
+        } while (property.isAlias());
+    }
+}
+
 void QQmlJSImportVisitor::checkRequiredProperties()
 {
-    for (const auto &required : m_requiredProperties) {
+    for (const auto &required : std::as_const(m_requiredProperties)) {
         if (!required.scope->hasProperty(required.name)) {
             m_logger->log(
                     QStringLiteral("Property \"%1\" was marked as required but does not exist.")
@@ -860,7 +887,108 @@ void QQmlJSImportVisitor::checkRequiredProperties()
         }
     }
 
-    for (const auto &defScope : m_objectDefinitionScopes) {
+    const auto isInComponent = [this](const QQmlJSScope::ConstPtr &requiredScope) {
+        const auto compType = m_rootScopeImports.type(u"Component"_s).scope;
+        for (auto s = requiredScope; s; s = s->parentScope()) {
+            if (s->isWrappedInImplicitComponent() || s->baseType() == compType)
+                return true;
+        }
+        return false;
+    };
+
+    const auto requiredHasBinding = [](const QList<QQmlJSScope::ConstPtr> &scopesToSearch,
+                                       const QString &propName) {
+        for (const auto &scope : scopesToSearch) {
+            const auto &[begin, end] = scope->ownPropertyBindings(propName);
+            for (auto it = begin; it != end; ++it) {
+                if (!scope->property(propName).isAlias())
+                    return true;
+            }
+        }
+
+        return false;
+    };
+
+    const auto requiredUsedInRootAlias = [&](const QQmlJSScope::ConstPtr &defScope,
+                                             const QQmlJSScope::ConstPtr &requiredScope,
+                                             const QString &propName) {
+        if (defScope->filePath() == requiredScope->filePath()) {
+            QQmlJSScope::ConstPtr fileRootScope = requiredScope;
+            while (fileRootScope->parentScope() != m_globalScope)
+                fileRootScope = fileRootScope->parentScope();
+
+            const auto &rootProperties = fileRootScope->ownProperties();
+            for (const auto &p : rootProperties) {
+                if (p.isAlias() && p.aliasTargetScope() == requiredScope
+                    && p.aliasTargetName() == propName) {
+                    return true;
+                }
+            }
+        }
+
+        return false;
+    };
+
+    const auto requiredSetThroughAlias = [&](const QList<QQmlJSScope::ConstPtr> &scopesToSearch,
+                                             const QQmlJSScope::ConstPtr &requiredScope,
+                                             const QString &propName) {
+        const auto &propertyDefScope = QQmlJSScope::ownerOfProperty(requiredScope, propName);
+        const auto &propertyAliases = m_propertyAliases[{ propertyDefScope.scope, propName }];
+        for (const auto &alias : propertyAliases) {
+            for (const auto &s : scopesToSearch) {
+                if (s->hasOwnPropertyBindings(alias.name))
+                    return true;
+            }
+        }
+        return false;
+    };
+
+    const auto warn = [this](const QList<QQmlJSScope::ConstPtr> scopesToSearch,
+                             QQmlJSScope::ConstPtr prevRequiredScope,
+                             const QString &propName,
+                             QQmlJSScope::ConstPtr defScope,
+                             QQmlJSScope::ConstPtr requiredScope,
+                             QQmlJSScope::ConstPtr descendant) {
+        const QQmlJSScope::ConstPtr propertyScope = scopesToSearch.size() > 1
+                ? scopesToSearch.at(scopesToSearch.size() - 2)
+                : QQmlJSScope::ConstPtr();
+
+        const QString propertyScopeName = !propertyScope.isNull()
+                ? getScopeName(propertyScope, QQmlSA::ScopeType::QMLScope)
+                : u"here"_s;
+
+        std::optional<QQmlJSFixSuggestion> suggestion;
+
+        QString message = QStringLiteral("Component is missing required property %1 from %2")
+                                  .arg(propName)
+                                  .arg(propertyScopeName);
+        if (requiredScope != descendant) {
+            const QString requiredScopeName = prevRequiredScope
+                    ? getScopeName(prevRequiredScope, QQmlSA::ScopeType::QMLScope)
+                    : u"here"_s;
+
+            if (!prevRequiredScope.isNull()) {
+                auto sourceScope = prevRequiredScope->baseType();
+                suggestion = QQmlJSFixSuggestion{
+                    "%1:%2:%3: Property marked as required in %4."_L1
+                            .arg(sourceScope->filePath())
+                            .arg(sourceScope->sourceLocation().startLine)
+                            .arg(sourceScope->sourceLocation().startColumn)
+                            .arg(requiredScopeName),
+                    sourceScope->sourceLocation()
+                };
+                suggestion->setFilename(sourceScope->filePath());
+            } else {
+                message += " (marked as required by %1)"_L1.arg(requiredScopeName);
+            }
+        }
+
+        m_logger->log(message, qmlRequired, defScope->sourceLocation(), true, true, suggestion);
+    };
+
+    populatePropertyAliases();
+
+    for (const auto &[_, defScope] : m_scopesByIrLocation.asKeyValueRange()) {
         if (defScope->isFileRootComponent() || defScope->isInlineComponent()
                 || defScope->componentRootStatus() != QQmlJSScope::IsComponentRoot::No) {
             continue;
@@ -868,89 +996,40 @@ void QQmlJSImportVisitor::checkRequiredProperties()
 
         QVector<QQmlJSScope::ConstPtr> scopesToSearch;
         for (QQmlJSScope::ConstPtr scope = defScope; scope; scope = scope->baseType()) {
-            scopesToSearch << scope;
-            const auto ownProperties = scope->ownProperties();
-            for (auto propertyIt = ownProperties.constBegin();
-                 propertyIt != ownProperties.constEnd(); ++propertyIt) {
-                const QString propName = propertyIt.key();
-
-                QQmlJSScope::ConstPtr prevRequiredScope;
-                for (QQmlJSScope::ConstPtr requiredScope : scopesToSearch) {
-                    if (requiredScope->isPropertyLocallyRequired(propName)) {
-                        bool found =
-                                std::find_if(scopesToSearch.constBegin(), scopesToSearch.constEnd(),
-                                             [&](QQmlJSScope::ConstPtr scope) {
-                                                 return scope->hasPropertyBindings(propName);
-                                             })
-                                != scopesToSearch.constEnd();
-
-                        if (!found) {
-                            const QString scopeId = m_scopesById.id(defScope, scope);
-                            bool propertyUsedInRootAlias = false;
-                            if (!scopeId.isEmpty()) {
-                                for (const QQmlJSMetaProperty &property :
-                                     m_exportedRootScope->ownProperties()) {
-                                    if (!property.isAlias())
-                                        continue;
-
-                                    QStringList aliasExpression =
-                                            property.aliasExpression().split(u'.');
-
-                                    if (aliasExpression.size() != 2)
-                                        continue;
-                                    if (aliasExpression[0] == scopeId
-                                        && aliasExpression[1] == propName) {
-                                        propertyUsedInRootAlias = true;
-                                        break;
-                                    }
-                                }
-                            }
-
-                            if (propertyUsedInRootAlias)
-                                continue;
-
-                            const QQmlJSScope::ConstPtr propertyScope = scopesToSearch.size() > 1
-                                    ? scopesToSearch.at(scopesToSearch.size() - 2)
-                                    : QQmlJSScope::ConstPtr();
-
-                            const QString propertyScopeName = !propertyScope.isNull()
-                                    ? getScopeName(propertyScope, QQmlSA::ScopeType::QMLScope)
-                                    : u"here"_s;
-
-                            const QString requiredScopeName = prevRequiredScope
-                                    ? getScopeName(prevRequiredScope, QQmlSA::ScopeType::QMLScope)
-                                    : u"here"_s;
-
-                            std::optional<QQmlJSFixSuggestion> suggestion;
-
-                            QString message =
-                                    QStringLiteral(
-                                            "Component is missing required property %1 from %2")
-                                            .arg(propName)
-                                            .arg(propertyScopeName);
-                            if (requiredScope != scope) {
-                                if (!prevRequiredScope.isNull()) {
-                                    auto sourceScope = prevRequiredScope->baseType();
-                                    suggestion = QQmlJSFixSuggestion{
-                                        "%1:%2:%3: Property marked as required in %4."_L1
-                                                .arg(sourceScope->filePath())
-                                                .arg(sourceScope->sourceLocation().startLine)
-                                                .arg(sourceScope->sourceLocation().startColumn)
-                                                .arg(requiredScopeName),
-                                        sourceScope->sourceLocation()
-                                    };
-                                    suggestion->setFilename(sourceScope->filePath());
-                                } else {
-                                    message += QStringLiteral(" (marked as required by %1)")
-                                                       .arg(requiredScopeName);
-                                }
-                            }
-
-                            m_logger->log(message, qmlRequired, defScope->sourceLocation(), true,
-                                          true, suggestion);
+            const auto descendants = QList<QQmlJSScope::ConstPtr>() << scope << scope->descendantScopes();
+            for (QQmlJSScope::ConstPtr descendant : std::as_const(descendants)) {
+                if (descendant->scopeType() != QQmlSA::ScopeType::QMLScope)
+                    continue;
+                scopesToSearch << descendant;
+                const auto ownProperties = descendant->ownProperties();
+                for (auto propertyIt = ownProperties.constBegin();
+                     propertyIt != ownProperties.constEnd(); ++propertyIt) {
+                    const QString propName = propertyIt.key();
+
+                    QQmlJSScope::ConstPtr prevRequiredScope;
+                    for (QQmlJSScope::ConstPtr requiredScope : std::as_const(scopesToSearch)) {
+                        if (isInComponent(requiredScope))
+                            continue;
+
+                        if (!requiredScope->isPropertyLocallyRequired(propName)) {
+                            prevRequiredScope = requiredScope;
+                            continue;
                         }
+
+                        if (requiredHasBinding(scopesToSearch, propName))
+                            continue;
+
+                        if (requiredUsedInRootAlias(defScope, requiredScope, propName))
+                            continue;
+
+                        if (requiredSetThroughAlias(scopesToSearch, requiredScope, propName))
+                            continue;
+
+                        warn(scopesToSearch, prevRequiredScope, propName, defScope,
+                             requiredScope, descendant);
+
+                        prevRequiredScope = requiredScope;
                     }
-                    prevRequiredScope = requiredScope;
                 }
             }
         }
author	Olivier De Cannière <olivier.decanniere@qt.io>	2025-03-04 12:52:37 +0100
committer	Olivier De Cannière <olivier.decanniere@qt.io>	2025-03-04 17:15:13 +0100
commit	daf57e29de918b7b4be7bb0d469db0c51d41bb07 (patch)
tree	5940e7eddbbfd820be46a82853b75ec629236113 /src/qmlcompiler/qqmljsimportvisitor.cpp
parent	3f6823aee9d04f9e67a0815a576310b9ec782eb7 (diff)