qmllint: Unify injected and "normal" JavaScript identifiers

The specifics of how to warn about the injected identifiers are moved
out of ScopeTree as that is not related to the structure of the scopes.

Change-Id: I26418c3fa492da8339abf045a4034a8464b7bbb8
Reviewed-by: Fabian Kosmale <fabian.kosmale@qt.io>

6 files changed, 142 insertions, 87 deletions

diff --git a/tools/qmllint/checkidentifiers.cpp b/tools/qmllint/checkidentifiers.cpp
index 9334c13e43..6df5a3fd1a 100644
--- a/tools/qmllint/checkidentifiers.cpp
+++ b/tools/qmllint/checkidentifiers.cpp
@@ -285,8 +285,10 @@ bool CheckIdentifiers::checkMemberAccess(const QVector<ScopeTree::FieldMember> &
     return true;
 }
 
-bool CheckIdentifiers::operator()(const QHash<QString, ScopeTree::ConstPtr> &qmlIDs,
-                                  const ScopeTree::ConstPtr &root, const QString &rootId) const
+bool CheckIdentifiers::operator()(
+        const QHash<QString, ScopeTree::ConstPtr> &qmlIDs,
+        const QHash<QQmlJS::SourceLocation, SignalHandler> &signalHandlers,
+        const ScopeTree::ConstPtr &root, const QString &rootId) const
 {
     bool noUnqualifiedIdentifier = true;
 
@@ -311,7 +313,8 @@ bool CheckIdentifiers::operator()(const QHash<QString, ScopeTree::ConstPtr> &qml
                 continue;
 
             const auto memberAccessBase = memberAccessChain.takeFirst();
-            if (currentScope->isIdInCurrentJSScopes(memberAccessBase.m_name))
+            const auto jsId = currentScope->findJSIdentifier(memberAccessBase.m_name);
+            if (jsId.has_value() && jsId->kind != JavaScriptIdentifier::Injected)
                 continue;
 
             auto it = qmlIDs.find(memberAccessBase.m_name);
@@ -404,49 +407,30 @@ bool CheckIdentifiers::operator()(const QHash<QString, ScopeTree::ConstPtr> &qml
                 m_colorOut->write(rootId + QLatin1Char('.'), Hint);
                 m_colorOut->write(issueLocationWithContext.issueText().toString(), Normal);
                 m_colorOut->write(issueLocationWithContext.afterText() + QLatin1Char('\n'), Normal);
-            } else if (currentScope->isIdInjectedFromSignal(memberAccessBase.m_name)) {
-                auto methodUsages = ScopeTree::findCurrentQMLScope(currentScope)
-                        ->injectedSignalIdentifiers().values(memberAccessBase.m_name);
-                auto location = memberAccessBase.m_location;
-                // 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](const MethodUsage &methodUsage) {
-                    return location.startLine < methodUsage.loc.startLine
-                            || (location.startLine == methodUsage.loc.startLine
-                                && location.startColumn < methodUsage.loc.startColumn);
-                });
-                auto methodUsage = *(--oneBehindIt);
+            } else if (jsId.has_value() && jsId->kind == JavaScriptIdentifier::Injected) {
+                const JavaScriptIdentifier id = jsId.value();
                 m_colorOut->write(QLatin1String("Note: "), Info);
                 m_colorOut->write(
                             memberAccessBase.m_name + QString::fromLatin1(
                                 " is accessible in this scope because "
                                 "you are handling a signal at %1:%2:%3\n")
                             .arg(m_fileName)
-                            .arg(methodUsage.loc.startLine).arg(methodUsage.loc.startColumn),
+                            .arg(id.location.startLine).arg(id.location.startColumn),
                             Normal);
                 m_colorOut->write(QLatin1String("Consider using a function instead\n"), Normal);
-                IssueLocationWithContext context {m_code, methodUsage.loc};
+                IssueLocationWithContext context {m_code, id.location};
                 m_colorOut->write(context.beforeText() + QLatin1Char(' '));
-                m_colorOut->write(QLatin1String(methodUsage.hasMultilineHandlerBody
-                                             ? "function("
-                                             : "("),
-                               Hint);
-                const auto parameters = methodUsage.method.parameterNames();
+
+                const auto handler = signalHandlers[id.location];
+
+                m_colorOut->write(QLatin1String(handler.isMultiline ? "function(" : "("), Hint);
+                const auto parameters = handler.signal.parameterNames();
                 for (int numParams = parameters.size(); numParams > 0; --numParams) {
                     m_colorOut->write(parameters.at(parameters.size() - numParams), Hint);
                     if (numParams > 1)
                         m_colorOut->write(QLatin1String(", "), Hint);
                 }
-                m_colorOut->write(QLatin1String(methodUsage.hasMultilineHandlerBody ? ")" : ") => "),
-                               Hint);
+                m_colorOut->write(QLatin1String(handler.isMultiline ? ")" : ") => "), Hint);
                 m_colorOut->write(QLatin1String(" {..."), Normal);
             }
             m_colorOut->write(QLatin1String("\n\n\n"), Normal);
diff --git a/tools/qmllint/checkidentifiers.h b/tools/qmllint/checkidentifiers.h
index bdd83f1e57..373767420b 100644
--- a/tools/qmllint/checkidentifiers.h
+++ b/tools/qmllint/checkidentifiers.h
@@ -30,10 +30,15 @@
 #define CHECKIDENTIFIERS_H
 
 #include "scopetree.h"
-#include "findwarnings.h"
+#include "qmljsimporter.h"
 
 class ColorOutput;
 
+struct SignalHandler {
+    MetaMethod signal;
+    bool isMultiline;
+};
+
 class CheckIdentifiers
 {
 public:
@@ -43,6 +48,7 @@ public:
     {}
 
     bool operator ()(const QHash<QString, ScopeTree::ConstPtr> &qmlIDs,
+                     const QHash<QQmlJS::SourceLocation, SignalHandler> &signalHandlers,
                      const ScopeTree::ConstPtr &root, const QString &rootId) const;
 
 private:
diff --git a/tools/qmllint/findwarnings.cpp b/tools/qmllint/findwarnings.cpp
index 9fe3bf99bd..309c7aa6ed 100644
--- a/tools/qmllint/findwarnings.cpp
+++ b/tools/qmllint/findwarnings.cpp
@@ -103,6 +103,16 @@ void FindWarningVisitor::importExportedNames(ScopeTree::ConstPtr scope)
     }
 }
 
+void FindWarningVisitor::flushPendingSignalParameters()
+{
+    const SignalHandler handler = m_signalHandlers[m_pendingSingalHandler];
+    for (const QString &parameter : handler.signal.parameterNames()) {
+        m_currentScope->insertJSIdentifier(
+                    parameter, { JavaScriptIdentifier::Injected, m_pendingSingalHandler});
+    }
+    m_pendingSingalHandler = QQmlJS::SourceLocation();
+}
+
 void FindWarningVisitor::throwRecursionDepthError()
 {
     m_colorOut.write(QStringLiteral("Error"), Error);
@@ -185,9 +195,28 @@ void FindWarningVisitor::endVisit(QQmlJS::AST::ForEachStatement *)
     leaveEnvironment();
 }
 
+bool FindWarningVisitor::visit(QQmlJS::AST::ExpressionStatement *)
+{
+    if (m_pendingSingalHandler.isValid()) {
+        enterEnvironment(ScopeType::JSFunctionScope, "signalhandler");
+        flushPendingSignalParameters();
+    }
+    return true;
+}
+
+void FindWarningVisitor::endVisit(QQmlJS::AST::ExpressionStatement *)
+{
+    if (m_currentScope->scopeType() == ScopeType::JSFunctionScope
+            && m_currentScope->baseTypeName() == "signalhandler") {
+        leaveEnvironment();
+    }
+}
+
 bool FindWarningVisitor::visit(QQmlJS::AST::Block *)
 {
     enterEnvironment(ScopeType::JSLexicalScope, "block");
+    if (m_pendingSingalHandler.isValid())
+        flushPendingSignalParameters();
     return true;
 }
 
@@ -210,8 +239,11 @@ void FindWarningVisitor::endVisit(QQmlJS::AST::CaseBlock *)
 bool FindWarningVisitor::visit(QQmlJS::AST::Catch *catchStatement)
 {
     enterEnvironment(ScopeType::JSLexicalScope, "catch");
-    m_currentScope->insertJSIdentifier(catchStatement->patternElement->bindingIdentifier.toString(),
-                                       ScopeType::JSLexicalScope);
+    m_currentScope->insertJSIdentifier(
+                catchStatement->patternElement->bindingIdentifier.toString(), {
+                    JavaScriptIdentifier::LexicalScoped,
+                    catchStatement->patternElement->firstSourceLocation()
+                });
     return true;
 }
 
@@ -300,13 +332,13 @@ bool FindWarningVisitor::visit(QQmlJS::AST::UiScriptBinding *uisb)
         for (auto method = methodsRange.first; method != methodsRange.second; ++method) {
             if (method->methodType() != MetaMethod::Signal)
                 continue;
-            for (auto const &param : method->parameterNames()) {
-                const auto firstSourceLocation = statement->firstSourceLocation();
-                bool hasMultilineStatementBody
-                        = statement->lastSourceLocation().startLine > firstSourceLocation.startLine;
-                m_currentScope->insertSignalIdentifier(param, *method, firstSourceLocation,
-                                                       hasMultilineStatementBody);
-            }
+
+            const auto firstSourceLocation = statement->firstSourceLocation();
+            bool hasMultilineStatementBody
+                    = statement->lastSourceLocation().startLine > firstSourceLocation.startLine;
+            m_pendingSingalHandler = firstSourceLocation;
+            m_signalHandlers.insert(firstSourceLocation, {*method, hasMultilineStatementBody});
+            break; // If there are multiple candidates for the signal, it's a mess anyway.
         }
         return true;
     }
@@ -383,14 +415,18 @@ FindWarningVisitor::FindWarningVisitor(
         // XMLHttpRequest
         QLatin1String("XMLHttpRequest")
     };
+
+    JavaScriptIdentifier globalJavaScript = {
+        JavaScriptIdentifier::LexicalScoped,
+        QQmlJS::SourceLocation()
+    };
     for (const char **globalName = QV4::Compiler::Codegen::s_globalNames;
          *globalName != nullptr;
          ++globalName) {
-        m_currentScope->insertJSIdentifier(QString::fromLatin1(*globalName),
-                                           ScopeType::JSLexicalScope);
+        m_currentScope->insertJSIdentifier(QString::fromLatin1(*globalName), globalJavaScript);
     }
     for (const auto& jsGlobVar: jsGlobVars)
-        m_currentScope->insertJSIdentifier(jsGlobVar, ScopeType::JSLexicalScope);
+        m_currentScope->insertJSIdentifier(jsGlobVar, globalJavaScript);
 }
 
 bool FindWarningVisitor::check()
@@ -411,7 +447,7 @@ bool FindWarningVisitor::check()
         return true;
 
     CheckIdentifiers check(&m_colorOut, m_code, m_rootScopeImports, m_filePath);
-    return check(m_qmlid2scope, m_rootScope, m_rootId);
+    return check(m_qmlid2scope, m_signalHandlers, m_rootScope, m_rootId);
 }
 
 bool FindWarningVisitor::visit(QQmlJS::AST::VariableDeclarationList *vdl)
@@ -419,9 +455,12 @@ bool FindWarningVisitor::visit(QQmlJS::AST::VariableDeclarationList *vdl)
     while (vdl) {
         m_currentScope->insertJSIdentifier(
                     vdl->declaration->bindingIdentifier.toString(),
-                    (vdl->declaration->scope == QQmlJS::AST::VariableScope::Var)
-                        ? ScopeType::JSFunctionScope
-                        : ScopeType::JSLexicalScope);
+                    {
+                        (vdl->declaration->scope == QQmlJS::AST::VariableScope::Var)
+                            ? JavaScriptIdentifier::FunctionScoped
+                            : JavaScriptIdentifier::LexicalScoped,
+                        vdl->declaration->firstSourceLocation()
+                    });
         vdl = vdl->next;
     }
     return true;
@@ -432,10 +471,13 @@ void FindWarningVisitor::visitFunctionExpressionHelper(QQmlJS::AST::FunctionExpr
     using namespace QQmlJS::AST;
     auto name = fexpr->name.toString();
     if (!name.isEmpty()) {
-        if (m_currentScope->scopeType() == ScopeType::QMLScope)
+        if (m_currentScope->scopeType() == ScopeType::QMLScope) {
             m_currentScope->addMethod(MetaMethod(name, QLatin1String("void")));
-        else
-            m_currentScope->insertJSIdentifier(name, ScopeType::JSLexicalScope);
+        } else {
+            m_currentScope->insertJSIdentifier(
+                        name,
+                        { JavaScriptIdentifier::LexicalScoped, fexpr->firstSourceLocation() });
+        }
         enterEnvironment(ScopeType::JSFunctionScope, name);
     } else {
         enterEnvironment(ScopeType::JSFunctionScope, QLatin1String("<anon>"));
@@ -466,8 +508,11 @@ void FindWarningVisitor::endVisit(QQmlJS::AST::FunctionDeclaration *)
 
 bool FindWarningVisitor::visit(QQmlJS::AST::FormalParameterList *fpl)
 {
-    for (auto const &boundName : fpl->boundNames())
-        m_currentScope->insertJSIdentifier(boundName.id, ScopeType::JSLexicalScope);
+    for (auto const &boundName : fpl->boundNames()) {
+        m_currentScope->insertJSIdentifier(
+                    boundName.id,
+                    {JavaScriptIdentifier::Parameter, fpl->firstSourceLocation() });
+    }
     return true;
 }
 
@@ -625,9 +670,12 @@ bool FindWarningVisitor::visit(QQmlJS::AST::PatternElement *element)
         element->boundNames(&names);
         for (const auto &name : names) {
             m_currentScope->insertJSIdentifier(
-                        name.id, (element->scope == QQmlJS::AST::VariableScope::Var)
-                            ? ScopeType::JSFunctionScope
-                            : ScopeType::JSLexicalScope);
+                        name.id, {
+                            (element->scope == QQmlJS::AST::VariableScope::Var)
+                                ? JavaScriptIdentifier::FunctionScoped
+                                : JavaScriptIdentifier::LexicalScoped,
+                            element->firstSourceLocation()
+                        });
         }
     }
 
diff --git a/tools/qmllint/findwarnings.h b/tools/qmllint/findwarnings.h
index 382c4cab11..e160e8751c 100644
--- a/tools/qmllint/findwarnings.h
+++ b/tools/qmllint/findwarnings.h
@@ -43,6 +43,7 @@
 #include "scopetree.h"
 #include "qcoloroutput.h"
 #include "qmljsimporter.h"
+#include "checkidentifiers.h"
 
 #include <QtQml/private/qqmldirparser_p.h>
 #include <QtQml/private/qqmljsastvisitor_p.h>
@@ -63,6 +64,9 @@ public:
 private:
     QmlJSImporter::ImportedTypes m_rootScopeImports;
 
+    QHash<QQmlJS::SourceLocation, SignalHandler> m_signalHandlers;
+    QQmlJS::SourceLocation m_pendingSingalHandler;
+
     ScopeTree::Ptr m_rootScope;
     ScopeTree::Ptr m_currentScope;
     QQmlJS::AST::ExpressionNode *m_fieldMemberBase = nullptr;
@@ -97,6 +101,7 @@ private:
 
     void parseHeaders(QQmlJS::AST::UiHeaderItemList *headers);
     ScopeTree::Ptr parseProgram(QQmlJS::AST::Program *program, const QString &name);
+    void flushPendingSignalParameters();
 
     void throwRecursionDepthError() override;
 
@@ -116,6 +121,9 @@ private:
     bool visit(QQmlJS::AST::ForEachStatement *ast) override;
     void endVisit(QQmlJS::AST::ForEachStatement *ast) override;
 
+    bool visit(QQmlJS::AST::ExpressionStatement *ast) override;
+    void endVisit(QQmlJS::AST::ExpressionStatement *ast) override;
+
     bool visit(QQmlJS::AST::Block *ast) override;
     void endVisit(QQmlJS::AST::Block *ast) override;
 
diff --git a/tools/shared/scopetree.cpp b/tools/shared/scopetree.cpp
index e7ce631b1d..933f3f88d7 100644
--- a/tools/shared/scopetree.cpp
+++ b/tools/shared/scopetree.cpp
@@ -49,28 +49,21 @@ ScopeTree::Ptr ScopeTree::create(ScopeType type, const ScopeTree::Ptr &parentSco
     return childScope;
 }
 
-void ScopeTree::insertJSIdentifier(const QString &id, ScopeType scope)
+void ScopeTree::insertJSIdentifier(const QString &name, const JavaScriptIdentifier &identifier)
 {
     Q_ASSERT(m_scopeType != ScopeType::QMLScope);
-    Q_ASSERT(scope != ScopeType::QMLScope);
-    if (scope != ScopeType::JSFunctionScope || m_scopeType == ScopeType::JSFunctionScope) {
-        m_jsIdentifiers.insert(id);
+    if (identifier.kind == JavaScriptIdentifier::LexicalScoped
+            || identifier.kind == JavaScriptIdentifier::Injected
+            || m_scopeType == ScopeType::JSFunctionScope) {
+        m_jsIdentifiers.insert(name, identifier);
     } else {
         auto targetScope = parentScope();
         while (targetScope->m_scopeType != ScopeType::JSFunctionScope)
             targetScope = targetScope->parentScope();
-        targetScope->m_jsIdentifiers.insert(id);
+        targetScope->m_jsIdentifiers.insert(name, identifier);
     }
 }
 
-void ScopeTree::insertSignalIdentifier(const QString &id, const MetaMethod &method,
-                                       const QQmlJS::SourceLocation &loc,
-                                       bool hasMultilineHandlerBody)
-{
-    Q_ASSERT(m_scopeType == ScopeType::QMLScope);
-    m_injectedSignalIdentifiers.insert(id, {method, loc, hasMultilineHandlerBody});
-}
-
 void ScopeTree::insertPropertyIdentifier(const MetaProperty &property)
 {
     addProperty(property);
@@ -127,9 +120,22 @@ bool ScopeTree::isIdInCurrentJSScopes(const QString &id) const
 
 bool ScopeTree::isIdInjectedFromSignal(const QString &id) const
 {
-    if (m_scopeType == ScopeType::QMLScope)
-        return m_injectedSignalIdentifiers.contains(id);
-    return findCurrentQMLScope(parentScope())->m_injectedSignalIdentifiers.contains(id);
+    const auto found = findJSIdentifier(id);
+    return found.has_value() && found->kind == JavaScriptIdentifier::Injected;
+}
+
+std::optional<JavaScriptIdentifier> ScopeTree::findJSIdentifier(const QString &id) const
+{
+    for (const auto *scope = this; scope; scope = scope->parentScope().data()) {
+        if (scope->m_scopeType == ScopeType::JSFunctionScope
+                || scope->m_scopeType == ScopeType::JSLexicalScope) {
+            auto it = scope->m_jsIdentifiers.find(id);
+            if (it != scope->m_jsIdentifiers.end())
+                return *it;
+        }
+    }
+
+    return std::optional<JavaScriptIdentifier>{};
 }
 
 void ScopeTree::resolveTypes(const QHash<QString, ScopeTree::ConstPtr> &contextualTypes)
diff --git a/tools/shared/scopetree.h b/tools/shared/scopetree.h
index 37eee26d16..b751815172 100644
--- a/tools/shared/scopetree.h
+++ b/tools/shared/scopetree.h
@@ -48,6 +48,8 @@
 #include <QtCore/qstring.h>
 #include <QtCore/qversionnumber.h>
 
+#include <optional>
+
 enum class ScopeType
 {
     JSFunctionScope,
@@ -55,11 +57,17 @@ enum class ScopeType
     QMLScope
 };
 
-struct MethodUsage
+struct JavaScriptIdentifier
 {
-    MetaMethod method;
-    QQmlJS::SourceLocation loc;
-    bool hasMultilineHandlerBody;
+    enum Kind {
+        Parameter,
+        FunctionScoped,
+        LexicalScoped,
+        Injected
+    };
+
+    Kind kind = FunctionScoped;
+    QQmlJS::SourceLocation location;
 };
 
 class ScopeTree
@@ -115,9 +123,8 @@ public:
 
     ScopeTree::Ptr parentScope() const { return m_parentScope.toStrongRef(); }
 
-    void insertJSIdentifier(const QString &id, ScopeType scope);
-    void insertSignalIdentifier(const QString &id, const MetaMethod &method,
-                                const QQmlJS::SourceLocation &loc, bool hasMultilineHandlerBody);
+    void insertJSIdentifier(const QString &name, const JavaScriptIdentifier &identifier);
+
     // inserts property as qml identifier as well as the corresponding
     void insertPropertyIdentifier(const MetaProperty &prop);
     void addUnmatchedSignalHandler(const QString &handler,
@@ -196,23 +203,19 @@ public:
     bool isIdInCurrentJSScopes(const QString &id) const;
     bool isIdInjectedFromSignal(const QString &id) const;
 
+    std::optional<JavaScriptIdentifier> findJSIdentifier(const QString &id) const;
+
     QVector<ScopeTree::Ptr> childScopes() const
     {
         return m_childScopes;
     }
 
-    QMultiHash<QString, MethodUsage> injectedSignalIdentifiers() const
-    {
-        return m_injectedSignalIdentifiers;
-    }
-
     void resolveTypes(const QHash<QString, ConstPtr> &contextualTypes);
 
 private:
     ScopeTree(ScopeType type, const ScopeTree::Ptr &parentScope = ScopeTree::Ptr());
 
-    QSet<QString> m_jsIdentifiers;
-    QMultiHash<QString, MethodUsage> m_injectedSignalIdentifiers;
+    QHash<QString, JavaScriptIdentifier> m_jsIdentifiers;
 
     QMultiHash<QString, MetaMethod> m_methods;
     QHash<QString, MetaProperty> m_properties;