From f40b3c22f4b101c4d0ae70718db71b9623caedfa Mon Sep 17 00:00:00 2001
From: Christian Kamm <christian.d.kamm@nokia.com>
Date: Thu, 25 Nov 2010 13:38:15 +0100
Subject: [PATCH] QmlJS: Add JSLint-style warnings for common JS traps.
Task-number: QTCREATORBUG-3071
Reviewed-by: Erik Verbruggen
---
src/libs/qmljs/qmljscheck.cpp | 350 ++++++++++++++++++++++++++++++++++
src/libs/qmljs/qmljscheck.h | 38 ++++
2 files changed, 388 insertions(+)
diff --git a/src/libs/qmljs/qmljscheck.cpp b/src/libs/qmljs/qmljscheck.cpp
index 9ac19d8ed08..de741da5379 100644
--- a/src/libs/qmljs/qmljscheck.cpp
+++ b/src/libs/qmljs/qmljscheck.cpp
@@ -188,6 +188,151 @@ public:
ExpressionNode *_ast;
};
+class FunctionBodyCheck : protected Visitor
+{
+public:
+ QList<DiagnosticMessage> operator()(FunctionExpression *function, Check::Options options)
+ {
+ _options = options;
+ _messages.clear();
+ _declaredFunctions.clear();
+ _declaredVariables.clear();
+ _possiblyUndeclaredUses.clear();
+ _seenNonDeclarationStatement = false;
+
+ _formalParameterNames.clear();
+ for (FormalParameterList *plist = function->formals; plist; plist = plist->next) {
+ if (plist->name)
+ _formalParameterNames += plist->name->asString();
+ }
+
+ Node::accept(function->body, this);
+ return _messages;
+ }
+
+protected:
+ void postVisit(Node *ast)
+ {
+ if (!_seenNonDeclarationStatement && ast->statementCast()
+ && !cast<VariableStatement *>(ast)) {
+ _seenNonDeclarationStatement = true;
+ }
+ }
+
+ bool visit(IdentifierExpression *ast)
+ {
+ if (!ast->name)
+ return false;
+ const QString name = ast->name->asString();
+ if (!_declaredFunctions.contains(name) && !_declaredVariables.contains(name))
+ _possiblyUndeclaredUses[name].append(ast->identifierToken);
+ return false;
+ }
+
+ bool visit(VariableStatement *ast)
+ {
+ if (_options & Check::WarnDeclarationsNotStartOfFunction && _seenNonDeclarationStatement) {
+ warning(ast->declarationKindToken, Check::tr("declarations should be at the start of a function"));
+ }
+ return true;
+ }
+
+ bool visit(VariableDeclaration *ast)
+ {
+ if (!ast->name)
+ return true;
+ const QString name = ast->name->asString();
+
+ if (_formalParameterNames.contains(name)) {
+ if (_options & Check::WarnDuplicateDeclaration)
+ warning(ast->identifierToken, Check::tr("already a formal parameter"));
+ return true;
+ }
+ if (_declaredFunctions.contains(name)) {
+ if (_options & Check::WarnDuplicateDeclaration)
+ warning(ast->identifierToken, Check::tr("already declared as function"));
+ return true;
+ }
+ if (_declaredVariables.contains(name)) {
+ if (_options & Check::WarnDuplicateDeclaration)
+ warning(ast->identifierToken, Check::tr("duplicate declaration"));
+ return true;
+ }
+
+ if (_possiblyUndeclaredUses.contains(name)) {
+ if (_options & Check::WarnUseBeforeDeclaration) {
+ foreach (const SourceLocation &loc, _possiblyUndeclaredUses.value(name)) {
+ warning(loc, Check::tr("variable is used before being declared"));
+ }
+ }
+ _possiblyUndeclaredUses.remove(name);
+ }
+ _declaredVariables[name] = ast;
+
+ return true;
+ }
+
+ bool visit(FunctionDeclaration *ast)
+ {
+ if (_options & Check::WarnDeclarationsNotStartOfFunction &&_seenNonDeclarationStatement) {
+ warning(ast->functionToken, Check::tr("declarations should be at the start of a function"));
+ }
+
+ return visit(static_cast<FunctionExpression *>(ast));
+ }
+
+ bool visit(FunctionExpression *ast)
+ {
+ if (!ast->name)
+ return false;
+ const QString name = ast->name->asString();
+
+ if (_formalParameterNames.contains(name)) {
+ if (_options & Check::WarnDuplicateDeclaration)
+ warning(ast->identifierToken, Check::tr("already a formal parameter"));
+ return false;
+ }
+ if (_declaredVariables.contains(name)) {
+ if (_options & Check::WarnDuplicateDeclaration)
+ warning(ast->identifierToken, Check::tr("already declared as var"));
+ return false;
+ }
+ if (_declaredFunctions.contains(name)) {
+ if (_options & Check::WarnDuplicateDeclaration)
+ warning(ast->identifierToken, Check::tr("duplicate declaration"));
+ return false;
+ }
+
+ if (FunctionDeclaration *decl = cast<FunctionDeclaration *>(ast)) {
+ if (_possiblyUndeclaredUses.contains(name)) {
+ if (_options & Check::WarnUseBeforeDeclaration) {
+ foreach (const SourceLocation &loc, _possiblyUndeclaredUses.value(name)) {
+ warning(loc, Check::tr("function is used before being declared"));
+ }
+ }
+ _possiblyUndeclaredUses.remove(name);
+ }
+ _declaredFunctions[name] = decl;
+ }
+
+ return false;
+ }
+
+private:
+ void warning(const SourceLocation &loc, const QString &message)
+ {
+ _messages.append(DiagnosticMessage(DiagnosticMessage::Warning, loc, message));
+ }
+
+ Check::Options _options;
+ QList<DiagnosticMessage> _messages;
+ QStringList _formalParameterNames;
+ QHash<QString, VariableDeclaration *> _declaredVariables;
+ QHash<QString, FunctionDeclaration *> _declaredFunctions;
+ QHash<QString, QList<SourceLocation> > _possiblyUndeclaredUses;
+ bool _seenNonDeclarationStatement;
+};
+
} // end of anonymous namespace
@@ -197,6 +342,10 @@ Check::Check(Document::Ptr doc, const Snapshot &snapshot, const Context *linkedC
, _context(*linkedContextNoScope)
, _scopeBuilder(&_context, doc, snapshot)
, _ignoreTypeErrors(false)
+ , _options(WarnDangerousNonStrictEqualityChecks | WarnBlocks | WarnWith
+ | WarnVoid | WarnCommaExpression | WarnExpressionStatement
+ | WarnAssignInCondition | WarnUseBeforeDeclaration | WarnDuplicateDeclaration
+ | WarnCaseWithoutFlowControlEnd)
, _lastValue(0)
{
}
@@ -212,6 +361,17 @@ QList<DiagnosticMessage> Check::operator()()
return _messages;
}
+bool Check::preVisit(Node *ast)
+{
+ _chain.append(ast);
+ return true;
+}
+
+void Check::postVisit(Node *)
+{
+ _chain.removeLast();
+}
+
bool Check::visit(UiProgram *)
{
return true;
@@ -372,6 +532,9 @@ bool Check::visit(FunctionDeclaration *ast)
bool Check::visit(FunctionExpression *ast)
{
+ FunctionBodyCheck bodyCheck;
+ _messages.append(bodyCheck(ast, _options));
+
Node::accept(ast->formals, this);
_scopeBuilder.push(ast);
Node::accept(ast->body, this);
@@ -379,6 +542,157 @@ bool Check::visit(FunctionExpression *ast)
return false;
}
+static bool shouldAvoidNonStrictEqualityCheck(ExpressionNode *exp)
+{
+ if (NumericLiteral *literal = cast<NumericLiteral *>(exp)) {
+ if (literal->value == 0)
+ return true;
+ } else if (cast<TrueLiteral *>(exp) || cast<FalseLiteral *>(exp) || cast<NullExpression *>(exp)) {
+ return true;
+ } else if (IdentifierExpression *ident = cast<IdentifierExpression *>(exp)) {
+ if (ident->name && ident->name->asString() == QLatin1String("undefined"))
+ return true;
+ } else if (StringLiteral *literal = cast<StringLiteral *>(exp)) {
+ if (!literal->value || literal->value->asString().isEmpty())
+ return true;
+ }
+ return false;
+}
+
+bool Check::visit(BinaryExpression *ast)
+{
+ if (ast->op == QSOperator::Equal || ast->op == QSOperator::NotEqual) {
+ if (_options & WarnAllNonStrictEqualityChecks
+ || (_options & WarnDangerousNonStrictEqualityChecks
+ && (shouldAvoidNonStrictEqualityCheck(ast->left)
+ || shouldAvoidNonStrictEqualityCheck(ast->right)))) {
+ warning(ast->operatorToken, tr("== and != perform type coercion, use === or !== instead to avoid"));
+ }
+ }
+ return true;
+}
+
+bool Check::visit(Block *ast)
+{
+ if (Node *p = parent()) {
+ if (_options & WarnBlocks
+ && !cast<UiScriptBinding *>(p)
+ && !cast<TryStatement *>(p)
+ && !cast<Catch *>(p)
+ && !cast<Finally *>(p)
+ && !cast<ForStatement *>(p)
+ && !cast<ForEachStatement *>(p)
+ && !cast<DoWhileStatement *>(p)
+ && !cast<WhileStatement *>(p)
+ && !cast<IfStatement *>(p)
+ && !cast<SwitchStatement *>(p)
+ && !cast<WithStatement *>(p)) {
+ warning(ast->lbraceToken, tr("blocks do not introduce a new scope, avoid"));
+ }
+ }
+ return true;
+}
+
+bool Check::visit(WithStatement *ast)
+{
+ if (_options & WarnWith)
+ warning(ast->withToken, tr("use of the with statement is not recommended, use a var instead"));
+ return true;
+}
+
+bool Check::visit(VoidExpression *ast)
+{
+ if (_options & WarnVoid)
+ warning(ast->voidToken, tr("use of void is usually confusing and not recommended"));
+ return true;
+}
+
+bool Check::visit(Expression *ast)
+{
+ if (_options & WarnCommaExpression && ast->left && ast->right) {
+ if (!cast<ForStatement *>(parent()))
+ warning(ast->commaToken, tr("avoid comma expressions"));
+ }
+ return true;
+}
+
+bool Check::visit(ExpressionStatement *ast)
+{
+ if (_options & WarnExpressionStatement && ast->expression) {
+ bool ok = cast<CallExpression *>(ast->expression)
+ || cast<DeleteExpression *>(ast->expression)
+ || cast<PreDecrementExpression *>(ast->expression)
+ || cast<PreIncrementExpression *>(ast->expression)
+ || cast<PostIncrementExpression *>(ast->expression)
+ || cast<PostDecrementExpression *>(ast->expression)
+ || cast<UiScriptBinding *>(parent());
+ if (BinaryExpression *binary = cast<BinaryExpression *>(ast->expression)) {
+ switch (binary->op) {
+ case QSOperator::Assign:
+ case QSOperator::InplaceAdd:
+ case QSOperator::InplaceAnd:
+ case QSOperator::InplaceDiv:
+ case QSOperator::InplaceLeftShift:
+ case QSOperator::InplaceRightShift:
+ case QSOperator::InplaceMod:
+ case QSOperator::InplaceMul:
+ case QSOperator::InplaceOr:
+ case QSOperator::InplaceSub:
+ case QSOperator::InplaceURightShift:
+ case QSOperator::InplaceXor:
+ ok = true;
+ default: break;
+ }
+ }
+
+ if (!ok) {
+ warning(locationFromRange(ast->firstSourceLocation(), ast->lastSourceLocation()),
+ tr("expression statements should be assignments, calls or delete expressions only"));
+ }
+ }
+ return true;
+}
+
+bool Check::visit(IfStatement *ast)
+{
+ if (ast->expression)
+ checkAssignInCondition(ast->expression);
+ return true;
+}
+
+bool Check::visit(ForStatement *ast)
+{
+ if (ast->condition)
+ checkAssignInCondition(ast->condition);
+ return true;
+}
+
+bool Check::visit(WhileStatement *ast)
+{
+ if (ast->expression)
+ checkAssignInCondition(ast->expression);
+ return true;
+}
+
+bool Check::visit(DoWhileStatement *ast)
+{
+ if (ast->expression)
+ checkAssignInCondition(ast->expression);
+ return true;
+}
+
+bool Check::visit(CaseClause *ast)
+{
+ checkEndsWithControlFlow(ast->statements, ast->caseToken);
+ return true;
+}
+
+bool Check::visit(DefaultClause *ast)
+{
+ checkEndsWithControlFlow(ast->statements, ast->defaultToken);
+ return true;
+}
+
/// When something is changed here, also change ReadingContext::lookupProperty in
/// texttomodelmerger.cpp
/// ### Maybe put this into the context as a helper method.
@@ -457,6 +771,34 @@ const Value *Check::checkScopeObjectMember(const UiQualifiedId *id)
return value;
}
+void Check::checkAssignInCondition(AST::ExpressionNode *condition)
+{
+ if (_options & WarnAssignInCondition) {
+ if (BinaryExpression *binary = cast<BinaryExpression *>(condition)) {
+ if (binary->op == QSOperator::Assign)
+ warning(binary->operatorToken, tr("avoid assignments in conditions"));
+ }
+ }
+}
+
+void Check::checkEndsWithControlFlow(StatementList *statements, SourceLocation errorLoc)
+{
+ // full flow analysis would be neat
+ if (!statements || !(_options & WarnCaseWithoutFlowControlEnd))
+ return;
+
+ Statement *lastStatement = 0;
+ for (StatementList *slist = statements; slist; slist = slist->next)
+ lastStatement = slist->statement;
+
+ if (!cast<ReturnStatement *>(lastStatement)
+ && !cast<ThrowStatement *>(lastStatement)
+ && !cast<BreakStatement *>(lastStatement)
+ && !cast<ContinueStatement *>(lastStatement)) {
+ warning(errorLoc, tr("case does not end with return, break, continue or throw"));
+ }
+}
+
void Check::error(const AST::SourceLocation &loc, const QString &message)
{
_messages.append(DiagnosticMessage(DiagnosticMessage::Error, loc, message));
@@ -466,3 +808,11 @@ void Check::warning(const AST::SourceLocation &loc, const QString &message)
{
_messages.append(DiagnosticMessage(DiagnosticMessage::Warning, loc, message));
}
+
+Node *Check::parent(int distance)
+{
+ const int index = _chain.size() - 2 - distance;
+ if (index < 0)
+ return 0;
+ return _chain.at(index);
+}
diff --git a/src/libs/qmljs/qmljscheck.h b/src/libs/qmljs/qmljscheck.h
index 97297eac357..7230b1f886d 100644
--- a/src/libs/qmljs/qmljscheck.h
+++ b/src/libs/qmljs/qmljscheck.h
@@ -50,7 +50,26 @@ public:
QList<DiagnosticMessage> operator()();
+ enum Option {
+ WarnDangerousNonStrictEqualityChecks = 1 << 0,
+ WarnAllNonStrictEqualityChecks = 1 << 1,
+ WarnBlocks = 1 << 2,
+ WarnWith = 1 << 3,
+ WarnVoid = 1 << 4,
+ WarnCommaExpression = 1 << 5,
+ WarnExpressionStatement = 1 << 6,
+ WarnAssignInCondition = 1 << 7,
+ WarnUseBeforeDeclaration = 1 << 8,
+ WarnDuplicateDeclaration = 1 << 9,
+ WarnDeclarationsNotStartOfFunction = 1 << 10,
+ WarnCaseWithoutFlowControlEnd = 1 << 11,
+ };
+ Q_DECLARE_FLAGS(Options, Option);
+
protected:
+ virtual bool preVisit(AST::Node *ast);
+ virtual void postVisit(AST::Node *ast);
+
virtual bool visit(AST::UiProgram *ast);
virtual bool visit(AST::UiObjectDefinition *ast);
virtual bool visit(AST::UiObjectBinding *ast);
@@ -61,14 +80,31 @@ protected:
virtual bool visit(AST::FunctionDeclaration *ast);
virtual bool visit(AST::FunctionExpression *ast);
+ virtual bool visit(AST::BinaryExpression *ast);
+ virtual bool visit(AST::Block *ast);
+ virtual bool visit(AST::WithStatement *ast);
+ virtual bool visit(AST::VoidExpression *ast);
+ virtual bool visit(AST::Expression *ast);
+ virtual bool visit(AST::ExpressionStatement *ast);
+ virtual bool visit(AST::IfStatement *ast);
+ virtual bool visit(AST::ForStatement *ast);
+ virtual bool visit(AST::WhileStatement *ast);
+ virtual bool visit(AST::DoWhileStatement *ast);
+ virtual bool visit(AST::CaseClause *ast);
+ virtual bool visit(AST::DefaultClause *ast);
+
private:
void visitQmlObject(AST::Node *ast, AST::UiQualifiedId *typeId,
AST::UiObjectInitializer *initializer);
const Interpreter::Value *checkScopeObjectMember(const AST::UiQualifiedId *id);
+ void checkAssignInCondition(AST::ExpressionNode *condition);
+ void checkEndsWithControlFlow(AST::StatementList *statements, AST::SourceLocation errorLoc);
void warning(const AST::SourceLocation &loc, const QString &message);
void error(const AST::SourceLocation &loc, const QString &message);
+ AST::Node *parent(int distance = 0);
+
Document::Ptr _doc;
Snapshot _snapshot;
@@ -78,8 +114,10 @@ private:
QList<DiagnosticMessage> _messages;
bool _ignoreTypeErrors;
+ Options _options;
const Interpreter::Value *_lastValue;
+ QList<AST::Node *> _chain;
};
QMLJS_EXPORT QColor toQColor(const QString &qmlColorString);
--
GitLab