From ab71cdafca87513a4e214d3af056d8990bc1eddb Mon Sep 17 00:00:00 2001 From: Ulf Hermann Date: Thu, 11 Feb 2021 13:17:29 +0100 Subject: 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 --- src/qml/compiler/qv4codegen.cpp | 19 ++++++++++++- src/qml/compiler/qv4compilercontext.cpp | 11 ++++---- src/qml/compiler/qv4compilercontext_p.h | 6 ++-- src/qml/compiler/qv4compilerscanfunctions.cpp | 12 +++++--- .../auto/qml/qmllint/data/useBeforeDeclaration.qml | 8 ++++++ tests/auto/qml/qmllint/tst_qmllint.cpp | 5 ++++ tools/qmllint/checkidentifiers.cpp | 32 ++++++++++++++++------ 7 files changed, 71 insertions(+), 22 deletions(-) create mode 100644 tests/auto/qml/qmllint/data/useBeforeDeclaration.qml 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 #include #include +#include #include #include #include @@ -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 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; } -- cgit v1.2.3