From 650e6739c403b4c5dbf5ebdd8883a0366b6260fa Mon Sep 17 00:00:00 2001 From: Maximilian Goldstein Date: Wed, 24 Nov 2021 12:02:55 +0100 Subject: [PATCH] qmllint: Fix attached object reuse false positives Previously the warning would be triggered even when no property is accessed or only an enum access occurred. Since neither of these actually cause the attached object to be created, do not warn in these cases. Change-Id: I53d2886268e2364f2d9bc088e0ba02dc1cfe4ee3 Reviewed-by: Ulf Hermann --- src/qmlcompiler/qqmljstypepropagator.cpp | 90 ++++++++++--------- .../qml/qmllint/data/attachedPropEnum.qml | 9 ++ tests/auto/qml/qmllint/tst_qmllint.cpp | 10 ++- 3 files changed, 62 insertions(+), 47 deletions(-) create mode 100644 tests/auto/qml/qmllint/data/attachedPropEnum.qml diff --git a/src/qmlcompiler/qqmljstypepropagator.cpp b/src/qmlcompiler/qqmljstypepropagator.cpp index 4da6b6f436..9e49cb3cb5 100644 --- a/src/qmlcompiler/qqmljstypepropagator.cpp +++ b/src/qmlcompiler/qqmljstypepropagator.cpp @@ -428,50 +428,6 @@ void QQmlJSTypePropagator::generate_LoadQmlContextPropertyLookup(int index) + u'.' + name : name); - if (m_typeInfo != nullptr - && m_state.accumulatorOut.variant() == QQmlJSRegisterContent::ScopeAttached) { - QQmlJSScope::ConstPtr attachedType = m_state.accumulatorOut.scopeType(); - - for (QQmlJSScope::ConstPtr scope = m_function->qmlScope->parentScope(); !scope.isNull(); - scope = scope->parentScope()) { - if (m_typeInfo->usedAttachedTypes.values(scope).contains(attachedType)) { - auto it = std::find(m_function->addressableScopes.constBegin(), - m_function->addressableScopes.constEnd(), scope); - - m_logger->logWarning( - u"Using attached type %1 already initialized in a parent scope."_qs - .arg(name), - Log_AttachedPropertyReuse, getCurrentSourceLocation()); - - FixSuggestion suggestion { Log_AttachedPropertyReuse, {} }; - - QQmlJS::SourceLocation fixLocation = getCurrentSourceLocation(); - fixLocation.length = 0; - - QString id = it == m_function->addressableScopes.constEnd() ? u""_qs : it.key(); - - suggestion.fixes << FixSuggestion::Fix { - u"Reference it by id instead:"_qs, - fixLocation, id + u"."_qs - }; - - fixLocation = scope->sourceLocation(); - fixLocation.length = 0; - - if (it == m_function->addressableScopes.constEnd()) { - suggestion.fixes - << FixSuggestion::Fix { u"You first have to give the element an id"_qs, - QQmlJS::SourceLocation {}, - {} }; - } - - m_logger->suggestFix(suggestion); - } - } - - m_typeInfo->usedAttachedTypes.insert(m_function->qmlScope, attachedType); - } - if (!m_state.accumulatorOut.isValid() && m_typeResolver->isPrefix(name)) { const QQmlJSRegisterContent inType = m_state.accumulatorIn.isValid() ? m_state.accumulatorIn @@ -562,6 +518,52 @@ void QQmlJSTypePropagator::propagatePropertyLookup(const QString &propertyName) + u'.' + propertyName : propertyName); + if (m_typeInfo != nullptr + && m_state.accumulatorIn.variant() == QQmlJSRegisterContent::ScopeAttached) { + QQmlJSScope::ConstPtr attachedType = m_state.accumulatorIn.scopeType(); + + for (QQmlJSScope::ConstPtr scope = m_function->qmlScope->parentScope(); !scope.isNull(); + scope = scope->parentScope()) { + if (m_typeInfo->usedAttachedTypes.values(scope).contains(attachedType)) { + + // Ignore enum accesses, as these will not cause the attached object to be created + if (m_state.accumulatorOut.isValid() && m_state.accumulatorOut.isEnumeration()) + continue; + + auto it = std::find(m_function->addressableScopes.constBegin(), + m_function->addressableScopes.constEnd(), scope); + + m_logger->logWarning( + u"Using attached type %1 already initialized in a parent scope."_qs.arg( + m_state.accumulatorIn.scopeType()->internalName()), + Log_AttachedPropertyReuse, getCurrentSourceLocation()); + + FixSuggestion suggestion { Log_AttachedPropertyReuse, {} }; + + QQmlJS::SourceLocation fixLocation = getCurrentSourceLocation(); + fixLocation.length = 0; + + QString id = it == m_function->addressableScopes.constEnd() ? u""_qs : it.key(); + + suggestion.fixes << FixSuggestion::Fix { u"Reference it by id instead:"_qs, + fixLocation, id + u"."_qs }; + + fixLocation = scope->sourceLocation(); + fixLocation.length = 0; + + if (it == m_function->addressableScopes.constEnd()) { + suggestion.fixes + << FixSuggestion::Fix { u"You first have to give the element an id"_qs, + QQmlJS::SourceLocation {}, + {} }; + } + + m_logger->suggestFix(suggestion); + } + } + m_typeInfo->usedAttachedTypes.insert(m_function->qmlScope, attachedType); + } + if (!m_state.accumulatorOut.isValid()) { if (m_typeResolver->isPrefix(propertyName)) { Q_ASSERT(m_state.accumulatorIn.isValid()); diff --git a/tests/auto/qml/qmllint/data/attachedPropEnum.qml b/tests/auto/qml/qmllint/data/attachedPropEnum.qml new file mode 100644 index 0000000000..3149de04a0 --- /dev/null +++ b/tests/auto/qml/qmllint/data/attachedPropEnum.qml @@ -0,0 +1,9 @@ +import QtQuick + +Text { + id: root + text: KeyNavigation.priority == KeyNavigation.BeforeItem ? "before" : "after" + Text { + text: root.KeyNavigation.priority == KeyNavigation.BeforeItem ? "before" : "after" + } +} diff --git a/tests/auto/qml/qmllint/tst_qmllint.cpp b/tests/auto/qml/qmllint/tst_qmllint.cpp index 3604efe0c5..511d25acc3 100644 --- a/tests/auto/qml/qmllint/tst_qmllint.cpp +++ b/tests/auto/qml/qmllint/tst_qmllint.cpp @@ -1104,9 +1104,13 @@ void TestQmllint::attachedPropertyReuse() QStringList() << "--multiple-attached-objects" << "warning", false) - .contains(QStringLiteral( - "Using attached type KeyNavigation already initialized in a parent " - "scope"))); + .contains(QStringLiteral("Using attached type QQuickKeyNavigationAttached " + "already initialized in a parent " + "scope"))); + QVERIFY(runQmllint("attachedPropEnum.qml", true, + QStringList() << "--multiple-attached-objects" + << "warning") + .isEmpty()); } void TestQmllint::shadowable()