From 9b48322f729aac3569973b76594699a52731d62f Mon Sep 17 00:00:00 2001 From: Maximilian Goldstein Date: Mon, 21 Feb 2022 16:08:50 +0100 Subject: qmllint: Properly warn about calling properties Previously whenever a property was called the warnings qmllint emitted made it seem like the property was just not found instead of not being callable. Now we will warn if you try to call a property and warn about properties shadowing methods, slots or signals. We also discourage calling variant properties now that may or may not be a function. This change also fixes an assert being hit in isMissingPropertyType due to our previous, lackluster checks. Fixes: QTBUG-101074 Change-Id: I0790b4a4584f3430ee1d8ddf549611225a36cc5b Reviewed-by: Ulf Hermann (cherry picked from commit 1829d7c64c1d8d5b42241135bfead030521cb398) --- src/qmlcompiler/qqmljstypepropagator.cpp | 55 ++++++++++++++++++++-- src/qmlcompiler/qqmljstypepropagator_p.h | 3 +- tests/auto/qml/qmllint/data/callJSValueProp.qml | 6 +++ tests/auto/qml/qmllint/data/callVarProp.qml | 7 +++ tests/auto/qml/qmllint/data/shadowedMethod.qml | 8 ++++ tests/auto/qml/qmllint/data/shadowedSignal.qml | 6 +++ .../auto/qml/qmllint/data/shadowedSignalWithId.qml | 6 +++ tests/auto/qml/qmllint/data/shadowedSlot.qml | 6 +++ tests/auto/qml/qmllint/tst_qmllint.cpp | 24 ++++++++++ 9 files changed, 117 insertions(+), 4 deletions(-) create mode 100644 tests/auto/qml/qmllint/data/callJSValueProp.qml create mode 100644 tests/auto/qml/qmllint/data/callVarProp.qml create mode 100644 tests/auto/qml/qmllint/data/shadowedMethod.qml create mode 100644 tests/auto/qml/qmllint/data/shadowedSignal.qml create mode 100644 tests/auto/qml/qmllint/data/shadowedSignalWithId.qml create mode 100644 tests/auto/qml/qmllint/data/shadowedSlot.qml diff --git a/src/qmlcompiler/qqmljstypepropagator.cpp b/src/qmlcompiler/qqmljstypepropagator.cpp index 203890d89b..6222b8acb1 100644 --- a/src/qmlcompiler/qqmljstypepropagator.cpp +++ b/src/qmlcompiler/qqmljstypepropagator.cpp @@ -257,7 +257,7 @@ QQmlJS::SourceLocation QQmlJSTypePropagator::getCurrentBindingSourceLocation() c return combine(entries.constFirst().location, entries.constLast().location); } -void QQmlJSTypePropagator::handleUnqualifiedAccess(const QString &name) const +void QQmlJSTypePropagator::handleUnqualifiedAccess(const QString &name, bool isMethod) const { auto location = getCurrentSourceLocation(); @@ -268,8 +268,12 @@ void QQmlJSTypePropagator::handleUnqualifiedAccess(const QString &name) const return; } - if (isMissingPropertyType(m_function->qmlScope, name)) + if (isMethod) { + if (isCallingProperty(m_function->qmlScope, name)) + return; + } else if (isMissingPropertyType(m_function->qmlScope, name)) { return; + } std::optional suggestion; @@ -445,6 +449,46 @@ bool QQmlJSTypePropagator::isMissingPropertyType(QQmlJSScope::ConstPtr scope, return true; } +bool QQmlJSTypePropagator::isCallingProperty(QQmlJSScope::ConstPtr scope, const QString &name) const +{ + auto property = scope->property(name); + if (!property.isValid()) + return false; + + QString propertyType = u"Property"_qs; + + auto methods = scope->methods(name); + + QString errorType; + if (!methods.isEmpty()) { + errorType = u"shadowed by a property."_qs; + switch (methods.first().methodType()) { + case QQmlJSMetaMethod::Signal: + propertyType = u"Signal"_qs; + break; + case QQmlJSMetaMethod::Slot: + propertyType = u"Slot"_qs; + break; + case QQmlJSMetaMethod::Method: + propertyType = u"Method"_qs; + break; + } + } else if (property.type() == m_typeResolver->varType()) { + errorType = + u"a variant property. It may or may not be a method. Use a regular function instead."_qs; + } else if (property.type() == m_typeResolver->jsValueType()) { + errorType = + u"a QJSValue property. It may or may not be a method. Use a regular Q_INVOKABLE instead."_qs; + } else { + errorType = u"not a method"_qs; + } + + m_logger->logWarning(u"%1 \"%2\" is %3"_qs.arg(propertyType, name, errorType), Log_Type, + getCurrentSourceLocation(), true, true, {}); + + return true; +} + void QQmlJSTypePropagator::generate_LoadQmlContextPropertyLookup(int index) { const int nameIndex = m_jsUnitGenerator->lookupNameIndex(index); @@ -475,7 +519,7 @@ void QQmlJSTypePropagator::generate_LoadQmlContextPropertyLookup(int index) setError(u"Cannot access value for name "_qs + name); if (!isRestricted) - handleUnqualifiedAccess(name); + handleUnqualifiedAccess(name, false); } else if (m_typeResolver->genericType(m_state.accumulatorOut.storedType()).isNull()) { // It should really be valid. // We get the generic type from aotContext->loadQmlContextPropertyIdLookup(). @@ -777,6 +821,9 @@ void QQmlJSTypePropagator::generate_CallProperty(int nameIndex, int base, int ar setError(u"Type %1 does not have a property %2 for calling"_qs .arg(callBase.descriptiveName(), propertyName)); + if (callBase.isType() && isCallingProperty(callBase.type(), propertyName)) + return; + if (checkRestricted(propertyName)) return; @@ -909,6 +956,8 @@ void QQmlJSTypePropagator::propagateScopeLookupCall(const QString &functionName, m_state.accumulatorOut = m_typeResolver->globalType(m_typeResolver->jsValueType()); setError(u"Cannot find function '%1'"_qs.arg(functionName)); + + handleUnqualifiedAccess(functionName, true); } void QQmlJSTypePropagator::generate_CallGlobalLookup(int index, int argc, int argv) diff --git a/src/qmlcompiler/qqmljstypepropagator_p.h b/src/qmlcompiler/qqmljstypepropagator_p.h index bab88a003a..4b48fe064d 100644 --- a/src/qmlcompiler/qqmljstypepropagator_p.h +++ b/src/qmlcompiler/qqmljstypepropagator_p.h @@ -208,9 +208,10 @@ private: bool needsMorePasses = false; }; - void handleUnqualifiedAccess(const QString &name) const; + void handleUnqualifiedAccess(const QString &name, bool isMethod) const; void checkDeprecated(QQmlJSScope::ConstPtr scope, const QString &name, bool isMethod) const; bool checkRestricted(const QString &propertyName) const; + bool isCallingProperty(QQmlJSScope::ConstPtr scope, const QString &name) const; bool isMissingPropertyType(QQmlJSScope::ConstPtr scope, const QString &type) const; QQmlJS::SourceLocation getCurrentSourceLocation() const; QQmlJS::SourceLocation getCurrentBindingSourceLocation() const; diff --git a/tests/auto/qml/qmllint/data/callJSValueProp.qml b/tests/auto/qml/qmllint/data/callJSValueProp.qml new file mode 100644 index 0000000000..357c810383 --- /dev/null +++ b/tests/auto/qml/qmllint/data/callJSValueProp.qml @@ -0,0 +1,6 @@ +import QtQml + +QtObject { + function foo() {} + Component.onCompleted: Qt.callLater(foo); +} diff --git a/tests/auto/qml/qmllint/data/callVarProp.qml b/tests/auto/qml/qmllint/data/callVarProp.qml new file mode 100644 index 0000000000..ad69e2a679 --- /dev/null +++ b/tests/auto/qml/qmllint/data/callVarProp.qml @@ -0,0 +1,7 @@ +import QtQml + +QtObject { + property var foo: () => {} + + Component.onCompleted: foo() +} diff --git a/tests/auto/qml/qmllint/data/shadowedMethod.qml b/tests/auto/qml/qmllint/data/shadowedMethod.qml new file mode 100644 index 0000000000..72f18aaec7 --- /dev/null +++ b/tests/auto/qml/qmllint/data/shadowedMethod.qml @@ -0,0 +1,8 @@ +import QtQuick + +QtObject { + function foo() {} + property bool foo: false + + Component.onCompleted: foo() +} diff --git a/tests/auto/qml/qmllint/data/shadowedSignal.qml b/tests/auto/qml/qmllint/data/shadowedSignal.qml new file mode 100644 index 0000000000..e4bb003495 --- /dev/null +++ b/tests/auto/qml/qmllint/data/shadowedSignal.qml @@ -0,0 +1,6 @@ +import QtQuick + +MouseArea { + id: mouseArea + Component.onCompleted: pressed() +} diff --git a/tests/auto/qml/qmllint/data/shadowedSignalWithId.qml b/tests/auto/qml/qmllint/data/shadowedSignalWithId.qml new file mode 100644 index 0000000000..ed7cc9f6c4 --- /dev/null +++ b/tests/auto/qml/qmllint/data/shadowedSignalWithId.qml @@ -0,0 +1,6 @@ +import QtQuick + +MouseArea { + id: mouseArea + Component.onCompleted: mouseArea.pressed() +} diff --git a/tests/auto/qml/qmllint/data/shadowedSlot.qml b/tests/auto/qml/qmllint/data/shadowedSlot.qml new file mode 100644 index 0000000000..cb09645746 --- /dev/null +++ b/tests/auto/qml/qmllint/data/shadowedSlot.qml @@ -0,0 +1,6 @@ +import QtQml + +ObjectModel { + property bool move: false + Component.onCompleted: move() +} diff --git a/tests/auto/qml/qmllint/tst_qmllint.cpp b/tests/auto/qml/qmllint/tst_qmllint.cpp index 20819b30e6..31faaacc07 100644 --- a/tests/auto/qml/qmllint/tst_qmllint.cpp +++ b/tests/auto/qml/qmllint/tst_qmllint.cpp @@ -812,6 +812,30 @@ void TestQmllint::dirtyQmlCode_data() << QStringLiteral("badCppPropertyChangeHandlers4.qml") << QStringLiteral("no matching signal found for handler \"onWannabeSignal\"") << QString() << QString() << false; + QTest::newRow("shadowedSignal") + << QStringLiteral("shadowedSignal.qml") + << QStringLiteral("Signal \"pressed\" is shadowed by a property.") << QString() + << QString() << false; + QTest::newRow("shadowedSignalWithId") + << QStringLiteral("shadowedSignalWithId.qml") + << QStringLiteral("Signal \"pressed\" is shadowed by a property") << QString() + << QString() << false; + QTest::newRow("shadowedSlot") << QStringLiteral("shadowedSlot.qml") + << QStringLiteral("Slot \"move\" is shadowed by a property") + << QString() << QString() << false; + QTest::newRow("shadowedMethod") << QStringLiteral("shadowedMethod.qml") + << QStringLiteral("Method \"foo\" is shadowed by a property.") + << QString() << QString() << false; + QTest::newRow("callVarProp") + << QStringLiteral("callVarProp.qml") + << QStringLiteral("Property \"foo\" is a variant property. It may or may not be a " + "method. Use a regular function instead.") + << QString() << QString() << false; + QTest::newRow("callJSValue") + << QStringLiteral("callJSValueProp.qml") + << QStringLiteral("Property \"callLater\" is a QJSValue property. It may or may not be " + "a method. Use a regular Q_INVOKABLE instead.") + << QString() << QString() << false; } void TestQmllint::dirtyQmlCode() -- cgit v1.2.3