QML: Warn about usage of injected signal parameters

You should declare functions with formal parameters if you want to use
parameters passed by the signal. We need to generate two different
warnings because there are two code paths by which such parameters are
injected. If we compile with qmlcachegen, it simply inserts a lookup
instruction in to the byte code. This lookup then triggers our special
hack expressly made for signal parameters. If we don't compile using
qmlcachegen, a function declaration with formal parameters is
synthesized. We mark those formal parameters as injected and warn if
we see one of them used.

[ChangeLog][QML][Important Behavior Changes] The automatic injection of
signal parameters into signal handlers is deprecated. This is because we
cannot determine the names of the signal parameters at compile time.
Furthermore, also for human readers it is difficult to discern between
arguments, context properties, properties of the current object, and
properties of the root object of the component. Requiring the signal
parameters to be explicitly named resolves some of this confusion. You
can turn the deprecation warning off using the "qt.qml.compiler" and
"qt.qml.context" logging categories.

Task-number: QTBUG-89943
Pick-to: 6.1
Change-Id: If0a5082adb735a73efd793868b3a55bc7d694cbe
Reviewed-by: Mitch Curtis <mitch.curtis@qt.io>
Reviewed-by: Fabian Kosmale <fabian.kosmale@qt.io>
Reviewed-by: Andrei Golubev <andrei.golubev@qt.io>

4 files changed, 43 insertions, 15 deletions

diff --git a/src/qml/compiler/qv4codegen.cpp b/src/qml/compiler/qv4codegen.cpp
index 38ac04ccbe..ab40f0ceba 100644
--- a/src/qml/compiler/qv4codegen.cpp
+++ b/src/qml/compiler/qv4codegen.cpp
@@ -2393,6 +2393,15 @@ Codegen::Reference Codegen::referenceForName(const QString &name, bool isLhs, co
                     << resolved.declarationLocation.startColumn << ".";
         }
 
+        if (resolved.isInjected && accessLocation.isValid()) {
+            qCWarning(lcQmlCompiler).nospace().noquote()
+                    << url().toString() << ":" << accessLocation.startLine
+                    << ":" << accessLocation.startColumn << " Parameter \"" << name
+                    << "\" is not declared."
+                    << " Injection of parameters into signal handlers is deprecated."
+                    << " Use JavaScript functions with formal parameters instead.";
+        }
+
         Reference r;
         switch (resolved.type) {
         case Context::ResolvedName::Local:
diff --git a/src/qml/compiler/qv4compilercontext.cpp b/src/qml/compiler/qv4compilercontext.cpp
index 94c7f0631e..bce46f59be 100644
--- a/src/qml/compiler/qv4compilercontext.cpp
+++ b/src/qml/compiler/qv4compilercontext.cpp
@@ -86,8 +86,10 @@ bool Context::Member::requiresTDZCheck(const SourceLocation &accessLocation, boo
     return accessLocation.begin() < declarationLocation.end();
 }
 
-bool Context::addLocalVar(const QString &name, Context::MemberType type, VariableScope scope, FunctionExpression *function,
-                          const QQmlJS::SourceLocation &declarationLocation)
+bool Context::addLocalVar(
+        const QString &name, Context::MemberType type, VariableScope scope,
+        FunctionExpression *function, const QQmlJS::SourceLocation &declarationLocation,
+        bool isInjected)
 {
     // ### can this happen?
     if (name.isEmpty())
@@ -119,6 +121,7 @@ bool Context::addLocalVar(const QString &name, Context::MemberType type, Variabl
     m.function = function;
     m.scope = scope;
     m.declarationLocation = declarationLocation;
+    m.isInjected = isInjected;
     members.insert(name, m);
     return true;
 }
@@ -147,9 +150,10 @@ Context::ResolvedName Context::resolveName(const QString &name, const QQmlJS::So
             if (c->isStrict && (name == QLatin1String("arguments") || name == QLatin1String("eval")))
                 result.isArgOrEval = true;
             result.declarationLocation = m.declarationLocation;
+            result.isInjected = m.isInjected;
             return result;
         }
-        const int argIdx = c->findArgument(name);
+        const int argIdx = c->findArgument(name, &result.isInjected);
         if (argIdx != -1) {
             if (c->argumentsCanEscape) {
                 result.index = argIdx + c->locals.size();
diff --git a/src/qml/compiler/qv4compilercontext_p.h b/src/qml/compiler/qv4compilercontext_p.h
index da9d637c5f..f67bee6dea 100644
--- a/src/qml/compiler/qv4compilercontext_p.h
+++ b/src/qml/compiler/qv4compilercontext_p.h
@@ -182,6 +182,7 @@ struct Context {
         int index = -1;
         QQmlJS::AST::VariableScope scope = QQmlJS::AST::VariableScope::Var;
         mutable bool canEscape = false;
+        bool isInjected = false;
         QQmlJS::AST::FunctionExpression *function = nullptr;
         QQmlJS::SourceLocation declarationLocation;
 
@@ -289,12 +290,20 @@ struct Context {
             isStrict = true;
     }
 
-    int findArgument(const QString &name) const
+    bool hasArgument(const QString &name) const
+    {
+        return arguments.contains(name);
+    }
+
+    int findArgument(const QString &name, bool *isInjected) const
     {
         // search backwards to handle duplicate argument names correctly
         for (int i = arguments.size() - 1; i >= 0; --i) {
-            if (arguments.at(i).id == name)
+            const auto &arg = arguments.at(i);
+            if (arg.id == name) {
+                *isInjected = arg.isInjected();
                 return i;
+            }
         }
         return -1;
     }
@@ -330,8 +339,11 @@ struct Context {
         usedVariables.insert(name);
     }
 
-    bool addLocalVar(const QString &name, MemberType contextType, QQmlJS::AST::VariableScope scope, QQmlJS::AST::FunctionExpression *function = nullptr,
-                     const QQmlJS::SourceLocation &declarationLocation = QQmlJS::SourceLocation());
+    bool addLocalVar(
+            const QString &name, MemberType contextType, QQmlJS::AST::VariableScope scope,
+            QQmlJS::AST::FunctionExpression *function = nullptr,
+            const QQmlJS::SourceLocation &declarationLocation = QQmlJS::SourceLocation(),
+            bool isInjected = false);
 
     struct ResolvedName {
         enum Type {
@@ -346,6 +358,7 @@ struct Context {
         bool isArgOrEval = false;
         bool isConst = false;
         bool requiresTDZCheck = false;
+        bool isInjected = false;
         int scope = -1;
         int index = -1;
         QQmlJS::SourceLocation declarationLocation;
diff --git a/src/qml/compiler/qv4compilerscanfunctions.cpp b/src/qml/compiler/qv4compilerscanfunctions.cpp
index c4f46ecf05..ebc9a86a16 100644
--- a/src/qml/compiler/qv4compilerscanfunctions.cpp
+++ b/src/qml/compiler/qv4compilerscanfunctions.cpp
@@ -703,22 +703,24 @@ bool ScanFunctions::enterFunction(Node *ast, const QString &name, FormalParamete
 
     const BoundNames boundNames = formals ? formals->boundNames() : BoundNames();
     for (int i = 0; i < boundNames.size(); ++i) {
-        const QString &arg = boundNames.at(i).id;
+        const auto &arg = boundNames.at(i);
         if (_context->isStrict || !isSimpleParameterList) {
-            bool duplicate = (boundNames.indexOf(arg, i + 1) != -1);
+            bool duplicate = (boundNames.indexOf(arg.id, i + 1) != -1);
             if (duplicate) {
-                _cg->throwSyntaxError(formals->firstSourceLocation(), QStringLiteral("Duplicate parameter name '%1' is not allowed.").arg(arg));
+                _cg->throwSyntaxError(formals->firstSourceLocation(), QStringLiteral("Duplicate parameter name '%1' is not allowed.").arg(arg.id));
                 return false;
             }
         }
         if (_context->isStrict) {
-            if (arg == QLatin1String("eval") || arg == QLatin1String("arguments")) {
-                _cg->throwSyntaxError(formals->firstSourceLocation(), QStringLiteral("'%1' cannot be used as parameter name in strict mode").arg(arg));
+            if (arg.id == QLatin1String("eval") || arg.id == QLatin1String("arguments")) {
+                _cg->throwSyntaxError(formals->firstSourceLocation(), QStringLiteral("'%1' cannot be used as parameter name in strict mode").arg(arg.id));
                 return false;
             }
         }
-        if (!_context->arguments.contains(arg))
-            _context->addLocalVar(arg, Context::VariableDefinition, VariableScope::Var);
+        if (!_context->arguments.contains(arg.id)) {
+            _context->addLocalVar(arg.id, Context::VariableDefinition, VariableScope::Var, nullptr,
+                                  QQmlJS::SourceLocation(), arg.isInjected());
+        }
     }
 
     return true;
@@ -787,7 +789,7 @@ void ScanFunctions::calcEscapingVariables()
                     }
                     break;
                 }
-                if (c->findArgument(var) != -1) {
+                if (c->hasArgument(var)) {
                     c->argumentsCanEscape = true;
                     c->requiresExecutionContext = true;
                     break;