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>

diff --git a/src/libs/qmljs/qmljscheck.cpp b/src/libs/qmljs/qmljscheck.cpp
index 08560bd..1e63a8f 100644 (file)
--- 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 80680d7..29ee818 100644 (file)
--- 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 47dc26b..7213b6b 100644 (file)
--- 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 b3a72ad..9d6067a 100644 (file)
--- 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);