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>

4 files changed, 103 insertions, 57 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;