qmllint: Improve signal handler recommendations

- Fix the case where multiple unqualified accesses would be mapped to
  the same signal (wrong in all but one cases), as the event parameter
  has the same name and we were using a QHash. Fixed by using QMultiHash
  and searching for the matching signal handler (by location)
- Recommend arrow functions for single line event handlers

Change-Id: I3cbb85fe0e49b454908ca03b4c86318ef02e364c
Reviewed-by: Simon Hausmann <simon.hausmann@qt.io>

3 files changed, 25 insertions, 9 deletions

diff --git a/tools/qmllint/findunqualified.cpp b/tools/qmllint/findunqualified.cpp
index 3abdfcfa53..e6a72df44c 100644
--- a/tools/qmllint/findunqualified.cpp
+++ b/tools/qmllint/findunqualified.cpp
@@ -503,7 +503,9 @@ bool FindUnqualifiedIDVisitor::visit(QQmlJS::AST::UiScriptBinding *uisb)
         } else {
             auto method = m_currentScope->methods()[signal];
             for (auto const &param : method.parameterNames()) {
-                m_currentScope->insertSignalIdentifier(param, method, uisb->statement->firstSourceLocation());
+                auto firstSourceLocation = uisb->statement->firstSourceLocation();
+                bool hasMultilineStatementBody = uisb->statement->lastSourceLocation().startLine > firstSourceLocation.startLine;
+                m_currentScope->insertSignalIdentifier(param, method, firstSourceLocation, hasMultilineStatementBody);
             }
         }
         return true;
diff --git a/tools/qmllint/scopetree.cpp b/tools/qmllint/scopetree.cpp
index b064117dd4..f58fde061e 100644
--- a/tools/qmllint/scopetree.cpp
+++ b/tools/qmllint/scopetree.cpp
@@ -30,6 +30,8 @@
 
 #include "qcoloroutput_p.h"
 
+#include <algorithm>
+
 #include <QQueue>
 
 ScopeTree::ScopeTree(ScopeType type, QString name, ScopeTree *parentScope)
@@ -66,10 +68,10 @@ void ScopeTree::insertQMLIdentifier(QString id)
     m_currentScopeQMLIdentifiers.insert(id);
 }
 
-void ScopeTree::insertSignalIdentifier(QString id, LanguageUtils::FakeMetaMethod method, QQmlJS::AST::SourceLocation loc)
+void ScopeTree::insertSignalIdentifier(QString id, LanguageUtils::FakeMetaMethod method, QQmlJS::AST::SourceLocation loc, bool hasMultilineHandlerBody)
 {
     Q_ASSERT(m_scopeType == ScopeType::QMLScope);
-    m_injectedSignalIdentifiers.insert(id, {method, loc});
+    m_injectedSignalIdentifiers.insert(id, {method, loc, hasMultilineHandlerBody});
 }
 
 void ScopeTree::insertPropertyIdentifier(QString id)
@@ -164,13 +166,24 @@ bool ScopeTree::recheckIdentifiers(const QString& code, const QHash<QString, Lan
                 colorOut.write(issueLocationWithContext.afterText + QLatin1Char('\n'), Normal);
             } else if (currentScope->isIdInjectedFromSignal(idLocationPair.first)) {
                 auto qmlScope = currentScope->getCurrentQMLScope();
-                MethodUsage methodUsage = qmlScope->m_injectedSignalIdentifiers[idLocationPair.first];
+                auto methodUsages = qmlScope->m_injectedSignalIdentifiers.values(idLocationPair.first);
+                auto location = idLocationPair.second;
+                // sort the list of signal handlers by their occurrence in the source code
+                // then, we select the first one whose location is after the unqualified id
+                // and go one step backwards to get the one which we actually need
+                std::sort(methodUsages.begin(), methodUsages.end(), [](const MethodUsage m1, const MethodUsage m2) {
+                    return m1.loc.startLine < m2.loc.startLine || (m1.loc.startLine == m2.loc.startLine && m1.loc.startColumn < m2.loc.startColumn);
+                });
+                auto oneBehindIt = std::find_if(methodUsages.begin(), methodUsages.end(), [&location](MethodUsage methodUsage) {
+                    return location.startLine < methodUsage.loc.startLine || (location.startLine == methodUsage.loc.startLine && location.startColumn < methodUsage.loc.startColumn);
+                });
+                auto methodUsage = *(--oneBehindIt);
                 colorOut.write("Note:", Info);
                 colorOut.write(idLocationPair.first + QString::asprintf(" is accessible in this scope because you are handling a signal at %d:%d\n", methodUsage.loc.startLine, methodUsage.loc.startColumn), Normal);
                 colorOut.write("Consider using a function instead\n", Normal);
                 IssueLocationWithContext context {code, methodUsage.loc};
                 colorOut.write(context.beforeText + QLatin1Char(' '));
-                colorOut.write("function(", Hint);
+                colorOut.write(methodUsage.hasMultilineHandlerBody ? "function(" : "(", Hint);
                 const auto parameters = methodUsage.method.parameterNames();
                 for (int numParams = parameters.size(); numParams > 0; --numParams) {
                     colorOut.write(parameters.at(parameters.size() - numParams), Hint);
@@ -178,10 +191,10 @@ bool ScopeTree::recheckIdentifiers(const QString& code, const QHash<QString, Lan
                         colorOut.write(", ", Hint);
                     }
                 }
-                colorOut.write(")", Hint);
+                colorOut.write(methodUsage.hasMultilineHandlerBody ? ")" : ") => ", Hint);
                 colorOut.write(" {...", Normal);
             }
-            colorOut.write("\n", Normal);
+            colorOut.write("\n\n\n", Normal);
         }
         for (auto const& childScope: currentScope->m_childScopes) {
             workQueue.enqueue(childScope);
diff --git a/tools/qmllint/scopetree.h b/tools/qmllint/scopetree.h
index cb495584c1..a65ed30d61 100644
--- a/tools/qmllint/scopetree.h
+++ b/tools/qmllint/scopetree.h
@@ -56,6 +56,7 @@ struct MethodUsage
 {
     LanguageUtils::FakeMetaMethod method;
     QQmlJS::AST::SourceLocation loc;
+    bool hasMultilineHandlerBody;
 };
 
 class ColorOutput;
@@ -70,7 +71,7 @@ public:
 
     void insertJSIdentifier(QString id, QQmlJS::AST::VariableScope scope);
     void insertQMLIdentifier(QString id);
-    void insertSignalIdentifier(QString id, LanguageUtils::FakeMetaMethod method, QQmlJS::AST::SourceLocation loc);
+    void insertSignalIdentifier(QString id, LanguageUtils::FakeMetaMethod method, QQmlJS::AST::SourceLocation loc, bool hasMultilineHandlerBody);
     void insertPropertyIdentifier(QString id); // inserts property as qml identifier as well as the corresponding
 
     bool isIdInCurrentScope(QString const &id) const;
@@ -88,7 +89,7 @@ public:
 private:
     QSet<QString> m_currentScopeJSIdentifiers;
     QSet<QString> m_currentScopeQMLIdentifiers;
-    QHash<QString, MethodUsage> m_injectedSignalIdentifiers; // need iteration in insertion order for hints, rarely more than 4 parameters needed
+    QMultiHash<QString, MethodUsage> m_injectedSignalIdentifiers;
     QMap<QString, LanguageUtils::FakeMetaMethod> m_methods;
     QVector<QPair<QString, QQmlJS::AST::SourceLocation>> m_accessedIdentifiers;
     QVector<ScopeTree*> m_childScopes;