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 <ulf.hermann@qt.io>
(cherry picked from commit 1829d7c64c1d8d5b42241135bfead030521cb398)

9 files changed, 117 insertions, 4 deletions

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<FixSuggestion> 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()