summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorUlf Hermann <ulf.hermann@qt.io>2021-02-11 13:17:29 +0100
committerUlf Hermann <ulf.hermann@qt.io>2021-02-11 17:37:53 +0100
commitab71cdafca87513a4e214d3af056d8990bc1eddb (patch)
tree22ec625d15eba58c018e748fee55c25a231da709
parent80bc943025398d0cdb29e4edbe096d8ac127263e (diff)
QML: Warn about variables being used before their declaration
This collides with injected signal parameters. qmlcachegen cannot tell those cases apart. [ChangeLog][QML][Important Behavior Changes] QML warns about JavaScript variables being used before their declaration now. This is almost always a mistake. It is particularly dangerous in the presence of injected signal parameters because qmlcachegen cannot identify a name collision between an injected signal parameter and a variable being used before its declaration. It therefore miscompiles such code. You can turn off the deprecation warning using the "qt.qml.compiler" logging category. Pick-to: 6.1 Task-number: QTBUG-89943 Change-Id: I8a9424ca8c6edd562402fe5c560ba7e8344b5585 Reviewed-by: Fabian Kosmale <fabian.kosmale@qt.io>
-rw-r--r--src/qml/compiler/qv4codegen.cpp19
-rw-r--r--src/qml/compiler/qv4compilercontext.cpp11
-rw-r--r--src/qml/compiler/qv4compilercontext_p.h6
-rw-r--r--src/qml/compiler/qv4compilerscanfunctions.cpp12
-rw-r--r--tests/auto/qml/qmllint/data/useBeforeDeclaration.qml8
-rw-r--r--tests/auto/qml/qmllint/tst_qmllint.cpp5
-rw-r--r--tools/qmllint/checkidentifiers.cpp32
7 files changed, 71 insertions, 22 deletions
diff --git a/src/qml/compiler/qv4codegen.cpp b/src/qml/compiler/qv4codegen.cpp
index f642797761..38ac04ccbe 100644
--- a/src/qml/compiler/qv4codegen.cpp
+++ b/src/qml/compiler/qv4codegen.cpp
@@ -44,6 +44,7 @@
#include <QtCore/QStringList>
#include <QtCore/QStack>
#include <QtCore/qurl.h>
+#include <QtCore/qloggingcategory.h>
#include <QScopeGuard>
#include <private/qqmljsast_p.h>
#include <private/qqmljslexer_p.h>
@@ -65,7 +66,10 @@ static const bool disable_lookups = false;
#undef CONST
#endif
-QT_USE_NAMESPACE
+QT_BEGIN_NAMESPACE
+
+Q_LOGGING_CATEGORY(lcQmlCompiler, "qt.qml.compiler");
+
using namespace QV4;
using namespace QV4::Compiler;
using namespace QQmlJS;
@@ -2378,6 +2382,17 @@ Codegen::Reference Codegen::referenceForName(const QString &name, bool isLhs, co
if (resolved.isArgOrEval && isLhs)
// ### add correct source location
throwSyntaxError(SourceLocation(), QStringLiteral("Variable name may not be eval or arguments in strict mode"));
+
+ if (resolved.declarationLocation.isValid() && accessLocation.isValid()
+ && resolved.declarationLocation.begin() > accessLocation.end()) {
+ qCWarning(lcQmlCompiler).nospace().noquote()
+ << url().toString() << ":" << accessLocation.startLine
+ << ":" << accessLocation.startColumn << " Variable \"" << name
+ << "\" is used before its declaration at "
+ << resolved.declarationLocation.startLine << ":"
+ << resolved.declarationLocation.startColumn << ".";
+ }
+
Reference r;
switch (resolved.type) {
case Context::ResolvedName::Local:
@@ -4458,3 +4473,5 @@ QT_WARNING_POP
}
Q_UNREACHABLE();
}
+
+QT_END_NAMESPACE
diff --git a/src/qml/compiler/qv4compilercontext.cpp b/src/qml/compiler/qv4compilercontext.cpp
index 08fb2ee993..94c7f0631e 100644
--- a/src/qml/compiler/qv4compilercontext.cpp
+++ b/src/qml/compiler/qv4compilercontext.cpp
@@ -80,14 +80,14 @@ bool Context::Member::requiresTDZCheck(const SourceLocation &accessLocation, boo
if (accessAcrossContextBoundaries)
return true;
- if (!accessLocation.isValid() || !endOfInitializerLocation.isValid())
+ if (!accessLocation.isValid() || !declarationLocation.isValid())
return true;
- return accessLocation.begin() < endOfInitializerLocation.end();
+ return accessLocation.begin() < declarationLocation.end();
}
bool Context::addLocalVar(const QString &name, Context::MemberType type, VariableScope scope, FunctionExpression *function,
- const QQmlJS::SourceLocation &endOfInitializer)
+ const QQmlJS::SourceLocation &declarationLocation)
{
// ### can this happen?
if (name.isEmpty())
@@ -112,13 +112,13 @@ bool Context::addLocalVar(const QString &name, Context::MemberType type, Variabl
// hoist var declarations to the function level
if (contextType == ContextType::Block && (scope == VariableScope::Var && type != MemberType::FunctionDefinition))
- return parent->addLocalVar(name, type, scope, function, endOfInitializer);
+ return parent->addLocalVar(name, type, scope, function, declarationLocation);
Member m;
m.type = type;
m.function = function;
m.scope = scope;
- m.endOfInitializerLocation = endOfInitializer;
+ m.declarationLocation = declarationLocation;
members.insert(name, m);
return true;
}
@@ -146,6 +146,7 @@ Context::ResolvedName Context::resolveName(const QString &name, const QQmlJS::So
result.requiresTDZCheck = m.requiresTDZCheck(accessLocation, c != this);
if (c->isStrict && (name == QLatin1String("arguments") || name == QLatin1String("eval")))
result.isArgOrEval = true;
+ result.declarationLocation = m.declarationLocation;
return result;
}
const int argIdx = c->findArgument(name);
diff --git a/src/qml/compiler/qv4compilercontext_p.h b/src/qml/compiler/qv4compilercontext_p.h
index 4a14009c35..da9d637c5f 100644
--- a/src/qml/compiler/qv4compilercontext_p.h
+++ b/src/qml/compiler/qv4compilercontext_p.h
@@ -183,7 +183,7 @@ struct Context {
QQmlJS::AST::VariableScope scope = QQmlJS::AST::VariableScope::Var;
mutable bool canEscape = false;
QQmlJS::AST::FunctionExpression *function = nullptr;
- QQmlJS::SourceLocation endOfInitializerLocation;
+ QQmlJS::SourceLocation declarationLocation;
bool isLexicallyScoped() const { return this->scope != QQmlJS::AST::VariableScope::Var; }
bool requiresTDZCheck(const QQmlJS::SourceLocation &accessLocation, bool accessAcrossContextBoundaries) const;
@@ -331,7 +331,7 @@ struct Context {
}
bool addLocalVar(const QString &name, MemberType contextType, QQmlJS::AST::VariableScope scope, QQmlJS::AST::FunctionExpression *function = nullptr,
- const QQmlJS::SourceLocation &endOfInitializer = QQmlJS::SourceLocation());
+ const QQmlJS::SourceLocation &declarationLocation = QQmlJS::SourceLocation());
struct ResolvedName {
enum Type {
@@ -348,7 +348,7 @@ struct Context {
bool requiresTDZCheck = false;
int scope = -1;
int index = -1;
- QQmlJS::SourceLocation endOfDeclarationLocation;
+ QQmlJS::SourceLocation declarationLocation;
bool isValid() const { return type != Unresolved; }
};
ResolvedName resolveName(const QString &name, const QQmlJS::SourceLocation &accessLocation);
diff --git a/src/qml/compiler/qv4compilerscanfunctions.cpp b/src/qml/compiler/qv4compilerscanfunctions.cpp
index 4c8f0d0658..c4f46ecf05 100644
--- a/src/qml/compiler/qv4compilerscanfunctions.cpp
+++ b/src/qml/compiler/qv4compilerscanfunctions.cpp
@@ -330,9 +330,13 @@ bool ScanFunctions::visit(PatternElement *ast)
BoundNames names;
ast->boundNames(&names);
- QQmlJS::SourceLocation lastInitializerLocation = ast->lastSourceLocation();
- if (_context->lastBlockInitializerLocation.isValid())
- lastInitializerLocation = _context->lastBlockInitializerLocation;
+ QQmlJS::SourceLocation declarationLocation = ast->firstSourceLocation();
+ if (_context->lastBlockInitializerLocation.isValid()) {
+ declarationLocation.length = _context->lastBlockInitializerLocation.end()
+ - declarationLocation.offset;
+ } else {
+ declarationLocation.length = ast->lastSourceLocation().end() - declarationLocation.offset;
+ }
for (const auto &name : qAsConst(names)) {
if (_context->isStrict && (name.id == QLatin1String("eval") || name.id == QLatin1String("arguments")))
@@ -345,7 +349,7 @@ bool ScanFunctions::visit(PatternElement *ast)
return false;
}
if (!_context->addLocalVar(name.id, ast->initializer ? Context::VariableDefinition : Context::VariableDeclaration, ast->scope,
- /*function*/nullptr, lastInitializerLocation)) {
+ /*function*/nullptr, declarationLocation)) {
_cg->throwSyntaxError(ast->identifierToken, QStringLiteral("Identifier %1 has already been declared").arg(name.id));
return false;
}
diff --git a/tests/auto/qml/qmllint/data/useBeforeDeclaration.qml b/tests/auto/qml/qmllint/data/useBeforeDeclaration.qml
new file mode 100644
index 0000000000..1c0f8092c3
--- /dev/null
+++ b/tests/auto/qml/qmllint/data/useBeforeDeclaration.qml
@@ -0,0 +1,8 @@
+import QtQml
+QtObject {
+ signal sig()
+ onSig: ()=> {
+ argq = 12;
+ var argq;
+ }
+}
diff --git a/tests/auto/qml/qmllint/tst_qmllint.cpp b/tests/auto/qml/qmllint/tst_qmllint.cpp
index 1486a9e1d3..6e2a41263e 100644
--- a/tests/auto/qml/qmllint/tst_qmllint.cpp
+++ b/tests/auto/qml/qmllint/tst_qmllint.cpp
@@ -283,6 +283,11 @@ void TestQmllint::dirtyQmlCode_data()
<< QStringLiteral("SegFault.bad.qml")
<< QString()
<< QString();
+ QTest::newRow("VariableUsedBeforeDeclaration")
+ << QStringLiteral("useBeforeDeclaration.qml")
+ << QStringLiteral("Variable \"argq\" is used before its declaration at 5:9. "
+ "The declaration is at 6:13.")
+ << QString();
}
void TestQmllint::dirtyQmlCode()
diff --git a/tools/qmllint/checkidentifiers.cpp b/tools/qmllint/checkidentifiers.cpp
index 60efbc90ca..b41b68b719 100644
--- a/tools/qmllint/checkidentifiers.cpp
+++ b/tools/qmllint/checkidentifiers.cpp
@@ -281,7 +281,7 @@ bool CheckIdentifiers::operator()(
const MemberAccessChains &memberAccessChains,
const QQmlJSScope::ConstPtr &root, const QString &rootId) const
{
- bool noUnqualifiedIdentifier = true;
+ bool identifiersClean = true;
// revisit all scopes
QQueue<QQmlJSScope::ConstPtr> workQueue;
@@ -296,14 +296,28 @@ bool CheckIdentifiers::operator()(
auto memberAccessBase = memberAccessChain.takeFirst();
const auto jsId = currentScope->findJSIdentifier(memberAccessBase.m_name);
- if (jsId.has_value() && jsId->kind != QQmlJSScope::JavaScriptIdentifier::Injected)
+ if (jsId.has_value() && jsId->kind != QQmlJSScope::JavaScriptIdentifier::Injected) {
+ if (memberAccessBase.m_location.end() < jsId->location.begin()) {
+ m_colorOut->writePrefixedMessage(
+ QStringLiteral(
+ "Variable \"%1\" is used before its declaration at %2:%3. "
+ "The declaration is at %4:%5.\n")
+ .arg(memberAccessBase.m_name)
+ .arg(memberAccessBase.m_location.startLine)
+ .arg(memberAccessBase.m_location.startColumn)
+ .arg(jsId->location.startLine)
+ .arg(jsId->location.startColumn), Warning);
+ printContext(m_code, m_colorOut, memberAccessBase.m_location);
+ identifiersClean = false;
+ }
continue;
+ }
auto it = qmlIDs.find(memberAccessBase.m_name);
if (it != qmlIDs.end()) {
if (!it->isNull()) {
if (!checkMemberAccess(memberAccessChain, *it))
- noUnqualifiedIdentifier = false;
+ identifiersClean = false;
continue;
} else if (!memberAccessChain.isEmpty()) {
// It could be a qualified type name
@@ -315,7 +329,7 @@ bool CheckIdentifiers::operator()(
if (typeIt != m_types.end()) {
memberAccessChain.takeFirst();
if (!checkMemberAccess(memberAccessChain, *typeIt))
- noUnqualifiedIdentifier = false;
+ identifiersClean = false;
continue;
}
}
@@ -341,9 +355,9 @@ bool CheckIdentifiers::operator()(
.arg(memberAccessBase.m_location.startLine)
.arg(memberAccessBase.m_location.startColumn), Warning);
printContext(m_code, m_colorOut, memberAccessBase.m_location);
- noUnqualifiedIdentifier = false;
+ identifiersClean = false;
} else if (!checkMemberAccess(memberAccessChain, property.type(), &property)) {
- noUnqualifiedIdentifier = false;
+ identifiersClean = false;
}
continue;
@@ -368,11 +382,11 @@ bool CheckIdentifiers::operator()(
if (typeIt != m_types.end() && !typeIt->isNull()) {
if (!checkMemberAccess(memberAccessChain, *typeIt))
- noUnqualifiedIdentifier = false;
+ identifiersClean = false;
continue;
}
- noUnqualifiedIdentifier = false;
+ identifiersClean = false;
const auto location = memberAccessBase.m_location;
if (baseIsPrefixed) {
@@ -444,5 +458,5 @@ bool CheckIdentifiers::operator()(
for (auto const &childScope : childScopes)
workQueue.enqueue(childScope);
}
- return noUnqualifiedIdentifier;
+ return identifiersClean;
}