summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorUlf Hermann <ulf.hermann@qt.io>2021-02-11 15:09:53 +0100
committerUlf Hermann <ulf.hermann@qt.io>2021-02-12 12:29:18 +0100
commitdf70d4f76f9c1c7b3de9ae91877df803c18b1264 (patch)
treead6c1149b514841937bf87b48adb8f7363d70f13
parentf3281ca869420df83d618c255aa7d62e63a102d5 (diff)
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>
-rw-r--r--src/qml/compiler/qv4codegen.cpp9
-rw-r--r--src/qml/compiler/qv4compilercontext.cpp10
-rw-r--r--src/qml/compiler/qv4compilercontext_p.h21
-rw-r--r--src/qml/compiler/qv4compilerscanfunctions.cpp18
-rw-r--r--src/qml/jsruntime/qv4function_p.h1
-rw-r--r--src/qml/jsruntime/qv4qmlcontext.cpp24
-rw-r--r--src/qml/parser/qqmljsast.cpp11
-rw-r--r--src/qml/parser/qqmljsast_p.h13
-rw-r--r--src/qml/qml/qqmltypecompiler.cpp1
-rw-r--r--tests/auto/qml/qqmllanguage/tst_qqmllanguage.cpp19
10 files changed, 104 insertions, 23 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;
diff --git a/src/qml/jsruntime/qv4function_p.h b/src/qml/jsruntime/qv4function_p.h
index ce9151c225..c022f84205 100644
--- a/src/qml/jsruntime/qv4function_p.h
+++ b/src/qml/jsruntime/qv4function_p.h
@@ -114,6 +114,7 @@ public:
uint nFormals;
int interpreterCallCount = 0;
bool isEval = false;
+ bool detectedInjectedParameters = false;
static Function *create(ExecutionEngine *engine, ExecutableCompilationUnit *unit,
const CompiledData::Function *function,
diff --git a/src/qml/jsruntime/qv4qmlcontext.cpp b/src/qml/jsruntime/qv4qmlcontext.cpp
index fbfc66e8d8..ec3aa00341 100644
--- a/src/qml/jsruntime/qv4qmlcontext.cpp
+++ b/src/qml/jsruntime/qv4qmlcontext.cpp
@@ -57,8 +57,12 @@
#include <private/qv4lookup_p.h>
#include <private/qv4identifiertable_p.h>
+#include <QtCore/qloggingcategory.h>
+
QT_BEGIN_NAMESPACE
+Q_LOGGING_CATEGORY(lcQmlContext, "qt.qml.context");
+
using namespace QV4;
DEFINE_OBJECT_VTABLE(QQmlContextWrapper);
@@ -457,8 +461,9 @@ bool QQmlContextWrapper::virtualPut(Managed *m, PropertyKey id, const Value &val
ReturnedValue QQmlContextWrapper::resolveQmlContextPropertyLookupGetter(Lookup *l, ExecutionEngine *engine, Value *base)
{
Scope scope(engine);
- PropertyKey name =engine->identifierTable->asPropertyKey(engine->currentStackFrame->v4Function->compilationUnit->
- runtimeStrings[l->nameIndex]);
+ auto *func = engine->currentStackFrame->v4Function;
+ PropertyKey name =engine->identifierTable->asPropertyKey(
+ func->compilationUnit->runtimeStrings[l->nameIndex]);
// Special hack for bounded signal expressions, where the parameters of signals are injected
// into the handler expression through the locals of the call context. So for onClicked: { ... }
@@ -467,8 +472,21 @@ ReturnedValue QQmlContextWrapper::resolveQmlContextPropertyLookupGetter(Lookup *
for (Heap::ExecutionContext *ctx = engine->currentContext()->d(); ctx; ctx = ctx->outer) {
if (ctx->type == Heap::ExecutionContext::Type_CallContext) {
const uint index = ctx->internalClass->indexOfValueOrGetter(name);
- if (index < std::numeric_limits<uint>::max())
+ if (index < std::numeric_limits<uint>::max()) {
+ if (!func->detectedInjectedParameters) {
+ const auto location = func->sourceLocation();
+ qCWarning(lcQmlContext).nospace().noquote()
+ << location.sourceFile << ":" << location.line << ":" << location.column
+ << " Parameter \"" << name.toQString() << "\" is not declared."
+ << " Injection of parameters into signal handlers is deprecated."
+ << " Use JavaScript functions with formal parameters instead.";
+
+ // Don't warn over and over for the same function
+ func->detectedInjectedParameters = true;
+ }
+
return static_cast<Heap::CallContext *>(ctx)->locals[index].asReturnedValue();
+ }
}
// Skip only block and call contexts.
diff --git a/src/qml/parser/qqmljsast.cpp b/src/qml/parser/qqmljsast.cpp
index 7309cf8162..4b5d866662 100644
--- a/src/qml/parser/qqmljsast.cpp
+++ b/src/qml/parser/qqmljsast.cpp
@@ -1009,7 +1009,13 @@ BoundNames FormalParameterList::formals() const
// change the name of the earlier argument to enforce the lookup semantics from the spec
formals[duplicateIndex].id += QLatin1String("#") + QString::number(i);
}
- formals += {name, it->element->typeAnnotation};
+ formals += {
+ name,
+ it->element->typeAnnotation,
+ it->element->isInjectedSignalParameter
+ ? BoundName::Injected
+ : BoundName::Declared
+ };
}
++i;
}
@@ -1412,7 +1418,8 @@ void PatternElement::boundNames(BoundNames *names)
else if (PatternPropertyList *p = propertyList())
p->boundNames(names);
} else {
- names->append({bindingIdentifier.toString(), typeAnnotation});
+ names->append({bindingIdentifier.toString(), typeAnnotation,
+ isInjectedSignalParameter ? BoundName::Injected : BoundName::Declared});
}
}
diff --git a/src/qml/parser/qqmljsast_p.h b/src/qml/parser/qqmljsast_p.h
index f846a5a3dc..cb6d5fa3ee 100644
--- a/src/qml/parser/qqmljsast_p.h
+++ b/src/qml/parser/qqmljsast_p.h
@@ -887,13 +887,19 @@ public:
struct QML_PARSER_EXPORT BoundName
{
+ enum Type {
+ Declared,
+ Injected,
+ };
+
QString id;
- TypeAnnotation *typeAnnotation = nullptr;
- BoundName(const QString &id, TypeAnnotation *typeAnnotation)
- : id(id), typeAnnotation(typeAnnotation)
+ QTaggedPointer<TypeAnnotation, Type> typeAnnotation;
+ BoundName(const QString &id, TypeAnnotation *typeAnnotation, Type type = Declared)
+ : id(id), typeAnnotation(typeAnnotation, type)
{}
BoundName() = default;
QString typeName() const { return typeAnnotation ? typeAnnotation->type->toString() : QString(); }
+ bool isInjected() const { return typeAnnotation.tag() == Injected; }
};
struct BoundNames : public QVector<BoundName>
@@ -981,6 +987,7 @@ public:
// when used in a VariableDeclarationList
VariableScope scope = VariableScope::NoScope;
bool isForDeclaration = false;
+ bool isInjectedSignalParameter = false;
};
class QML_PARSER_EXPORT PatternElementList : public Node
diff --git a/src/qml/qml/qqmltypecompiler.cpp b/src/qml/qml/qqmltypecompiler.cpp
index e395439c10..064333a94c 100644
--- a/src/qml/qml/qqmltypecompiler.cpp
+++ b/src/qml/qml/qqmltypecompiler.cpp
@@ -473,6 +473,7 @@ bool SignalHandlerConverter::convertSignalHandlerExpressionsToFunctionDeclaratio
QStringView paramNameRef = compiler->newStringRef(param);
QQmlJS::AST::PatternElement *b = new (pool) QQmlJS::AST::PatternElement(paramNameRef, nullptr);
+ b->isInjectedSignalParameter = true;
paramList = new (pool) QQmlJS::AST::FormalParameterList(paramList, b);
}
diff --git a/tests/auto/qml/qqmllanguage/tst_qqmllanguage.cpp b/tests/auto/qml/qqmllanguage/tst_qqmllanguage.cpp
index f174b78e9b..54d6b05e1b 100644
--- a/tests/auto/qml/qqmllanguage/tst_qqmllanguage.cpp
+++ b/tests/auto/qml/qqmllanguage/tst_qqmllanguage.cpp
@@ -350,6 +350,7 @@ private slots:
void multiExtension();
void invalidInlineComponent();
+ void warnOnInjectedParameters();
private:
QQmlEngine engine;
@@ -6169,6 +6170,24 @@ void tst_qqmllanguage::invalidInlineComponent()
QVERIFY(c.errorString().contains("\"Window.visibility\" is not available in QtQuick 2.0."));
}
+void tst_qqmllanguage::warnOnInjectedParameters()
+{
+ QQmlEngine e;
+ QQmlComponent c(&engine);
+ QTest::ignoreMessage(QtWarningMsg, "qrc:/foo.qml:4:18 Parameter \"bar\" is not declared."
+ " Injection of parameters into signal handlers is deprecated."
+ " Use JavaScript functions with formal parameters instead.");
+ c.setData("import QtQml\n"
+ "QtObject {\n"
+ " signal foo(bar: string)\n"
+ " onFoo: print(bar)\n"
+ " Component.onCompleted: foo('baz')\n"
+ "}", QUrl("qrc:/foo.qml"));
+ QVERIFY2(c.isReady(), qPrintable(c.errorString()));
+ QTest::ignoreMessage(QtDebugMsg, "baz");
+ QScopedPointer<QObject> o(c.create());
+}
+
void tst_qqmllanguage::qtbug_86482()
{
QQmlEngine engine;