From 367e27cde7d16563d0cab7af03e1b0f43e004bb8 Mon Sep 17 00:00:00 2001 From: Christian Kamm Date: Mon, 12 Sep 2011 10:44:11 +0200 Subject: [PATCH] QmlJS checks: Correct the check for dangerous == and add tests. Change-Id: Ie0f4062069bf241020868af34ce6d36146b4b0c7 Reviewed-on: http://codereview.qt-project.org/4646 Reviewed-by: Thomas Hartmann --- src/libs/qmljs/qmljscheck.cpp | 34 +++++------ src/libs/qmljs/qmljsevaluate.cpp | 2 + tests/auto/qml/codemodel/check/equality-checks.qml | 65 +++++++++++++++++----- tests/auto/qml/codemodel/check/tst_check.cpp | 10 +++- 4 files changed, 78 insertions(+), 33 deletions(-) diff --git a/src/libs/qmljs/qmljscheck.cpp b/src/libs/qmljs/qmljscheck.cpp index 08560bd764..1e63a8ff78 100644 --- a/src/libs/qmljs/qmljscheck.cpp +++ b/src/libs/qmljs/qmljscheck.cpp @@ -895,22 +895,24 @@ bool Check::visit(FunctionExpression *ast) return false; } -static bool shouldAvoidNonStrictEqualityCheck(ExpressionNode *exp, const Value *other) +static bool shouldAvoidNonStrictEqualityCheck(const Value *lhs, const Value *rhs) { - if (NumericLiteral *literal = cast(exp)) { - if (literal->value == 0 && !other->asNumberValue()) - return true; - } else if ((cast(exp) || cast(exp)) && !other->asBooleanValue()) { - return true; - } else if (cast(exp)) { + // we currently use undefined as a "we don't know" value + if (lhs->asUndefinedValue() || rhs->asUndefinedValue()) return true; - } else if (IdentifierExpression *ident = cast(exp)) { - if (ident->name && ident->name->asString() == QLatin1String("undefined")) - return true; - } else if (StringLiteral *literal = cast(exp)) { - if ((!literal->value || literal->value->asString().isEmpty()) && !other->asStringValue()) - return true; - } + + if (lhs->asStringValue() && rhs->asNumberValue()) + return true; // coerces string to number + + if (lhs->asObjectValue() && rhs->asNumberValue()) + return true; // coerces object to primitive + + if (lhs->asObjectValue() && rhs->asStringValue()) + return true; // coerces object to primitive + + if (lhs->asBooleanValue() && !rhs->asBooleanValue()) + return true; // coerces bool to number + return false; } @@ -922,8 +924,8 @@ bool Check::visit(BinaryExpression *ast) Evaluate eval(&_scopeChain); const Value *lhs = eval(ast->left); const Value *rhs = eval(ast->right); - warn = shouldAvoidNonStrictEqualityCheck(ast->left, rhs) - || shouldAvoidNonStrictEqualityCheck(ast->right, lhs); + warn = shouldAvoidNonStrictEqualityCheck(lhs, rhs) + || shouldAvoidNonStrictEqualityCheck(rhs, lhs); } if (warn) { warning(ast->operatorToken, tr("== and != perform type coercion, use === or !== instead to avoid")); diff --git a/src/libs/qmljs/qmljsevaluate.cpp b/src/libs/qmljs/qmljsevaluate.cpp index 80680d7b2e..29ee818493 100644 --- a/src/libs/qmljs/qmljsevaluate.cpp +++ b/src/libs/qmljs/qmljsevaluate.cpp @@ -267,6 +267,8 @@ bool Evaluate::visit(AST::ArrayLiteral *) bool Evaluate::visit(AST::ObjectLiteral *) { + // ### properties + _result = _valueOwner->newObject(); return false; } diff --git a/tests/auto/qml/codemodel/check/equality-checks.qml b/tests/auto/qml/codemodel/check/equality-checks.qml index 47dc26b789..7213b6b3af 100644 --- a/tests/auto/qml/codemodel/check/equality-checks.qml +++ b/tests/auto/qml/codemodel/check/equality-checks.qml @@ -6,20 +6,55 @@ Rectangle { } function foo() { - var st = "" - var nu = 0 - var un = undefined - if (st == "") {} // no warning - if (nu == "") {} // W 16 17 - if (un == "") {} // W 16 17 - if (st == 0) {} // W 16 17 - if (nu == 0) {} // no warning - if (un == 0) {} // W 16 17 - if (st == null) {} // W 16 17 - if (nu == null) {} // W 16 17 - if (un == null) {} // W 16 17 - if (st == undefined) {} // W 16 17 - if (nu == undefined) {} // W 16 17 - if (un == undefined) {} // W 16 17 + var s = "" + var n = 0 + var N = null + var u = undefined + var b = true + var o = {} + + if (s == s) {} + if (s == n) {} // W 15 16 + if (s == N) {} // ### should warn: always false + if (s == u) {} // W 15 16 + if (s == b) {} // W 15 16 + if (s == o) {} // W 15 16 + + if (n == s) {} // W 15 16 + if (n == n) {} + if (n == N) {} // ### should warn: always false + if (n == u) {} // W 15 16 + if (n == b) {} // W 15 16 + if (n == o) {} // W 15 16 + + if (N == s) {} // ### should warn: always false + if (N == n) {} // ### should warn: always false + if (N == N) {} + if (N == u) {} // W 15 16 + // ### should warn: always false + if (N == b) {} // W 15 16 + if (N == o) {} // ### should warn: always false + + if (u == s) {} // W 15 16 + if (u == n) {} // W 15 16 + if (u == N) {} // W 15 16 + if (u == u) {} // W 15 16 + if (u == b) {} // W 15 16 + if (u == o) {} // W 15 16 + + if (b == s) {} // W 15 16 + if (b == n) {} // W 15 16 + // ### should warn: always false + if (b == N) {} // W 15 16 + if (b == u) {} // W 15 16 + if (b == b) {} + if (b == o) {} // W 15 16 + + if (o == s) {} // W 15 16 + if (o == n) {} // W 15 16 + if (o == N) {} // ### should warn: always false + if (o == u) {} // W 15 16 + if (o == b) {} // W 15 16 + if (o == o) {} } } diff --git a/tests/auto/qml/codemodel/check/tst_check.cpp b/tests/auto/qml/codemodel/check/tst_check.cpp index b3a72ad574..9d6067acc1 100644 --- a/tests/auto/qml/codemodel/check/tst_check.cpp +++ b/tests/auto/qml/codemodel/check/tst_check.cpp @@ -171,9 +171,10 @@ void tst_Check::test() message); } - QCOMPARE(expectedMessages.size(), messages.size()); for (int i = 0; i < messages.size(); ++i) { - DiagnosticMessage expected = expectedMessages.at(i); + DiagnosticMessage expected; + if (i < expectedMessages.size()) + expected = expectedMessages.at(i); DiagnosticMessage actual = messages.at(i); bool fail = false; fail |= !QCOMPARE_NOEXIT(actual.message, expected.message); @@ -189,6 +190,11 @@ void tst_Check::test() return; } } + if (expectedMessages.size() > messages.size()) { + DiagnosticMessage missingMessage = expectedMessages.at(messages.size()); + qDebug() << "expected more messages: " << missingMessage.loc.startLine << missingMessage.message; + QFAIL("more messages expected"); + } } QTEST_MAIN(tst_Check); -- 2.11.0