From 367e27cde7d16563d0cab7af03e1b0f43e004bb8 Mon Sep 17 00:00:00 2001
From: Christian Kamm <christian.d.kamm@nokia.com>
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 <Thomas.Hartmann@nokia.com>
---
src/libs/qmljs/qmljscheck.cpp | 34 +++++-----
src/libs/qmljs/qmljsevaluate.cpp | 2 +
.../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 08560bd764a..1e63a8ff78a 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<NumericLiteral *>(exp)) {
- if (literal->value == 0 && !other->asNumberValue())
- return true;
- } else if ((cast<TrueLiteral *>(exp) || cast<FalseLiteral *>(exp)) && !other->asBooleanValue()) {
- return true;
- } else if (cast<NullExpression *>(exp)) {
+ // we currently use undefined as a "we don't know" value
+ if (lhs->asUndefinedValue() || rhs->asUndefinedValue())
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()) && !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 80680d7b2ed..29ee818493a 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 47dc26b789c..7213b6b3af4 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 b3a72ad5747..9d6067acc18 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);
--
GitLab