From 7015ef04b77f4e4318a46535951e4fa14450c6d3 Mon Sep 17 00:00:00 2001 From: Fawzi Mohamed Date: Fri, 29 Jan 2021 23:20:31 +0100 Subject: [PATCH] qmljs: add check for comparisons not depending on values currently we mainly try to warn about primitive == null/undefined or primitive === non primitive. There are other that we could warn about null==null null==undefined, but I feel that they might be triggered too much by clean code. Change-Id: Id43d838d60a4e13f361be34e4bb38211777a081e Reviewed-by: Fabian Kosmale --- src/libs/qmljs/qmljscheck.cpp | 36 ++++++++ src/libs/qmljs/qmljsstaticanalysismessage.cpp | 2 + src/libs/qmljs/qmljsstaticanalysismessage.h | 1 + .../qml/codemodel/check/equality-checks.qml | 89 ++++++++++++++++--- 4 files changed, 116 insertions(+), 12 deletions(-) diff --git a/src/libs/qmljs/qmljscheck.cpp b/src/libs/qmljs/qmljscheck.cpp index b0bfcdfd17b..efa69af4926 100644 --- a/src/libs/qmljs/qmljscheck.cpp +++ b/src/libs/qmljs/qmljscheck.cpp @@ -1275,6 +1275,30 @@ static bool shouldAvoidNonStrictEqualityCheck(const Value *lhs, const Value *rhs return false; } +static bool equalIsAlwaysFalse(const Value *lhs, const Value *rhs) +{ + if ((lhs->asNullValue() || lhs->asUndefinedValue()) + && (rhs->asNumberValue() || rhs->asBooleanValue() || rhs->asStringValue())) + return true; + return false; +} + +static bool strictCompareConstant(const Value *lhs, const Value *rhs) +{ + if (lhs->asUnknownValue() || rhs->asUnknownValue()) + return false; + if (lhs->asBooleanValue() && !rhs->asBooleanValue()) + return true; + if (lhs->asNumberValue() && !rhs->asNumberValue()) + return true; + if (lhs->asStringValue() && !rhs->asStringValue()) + return true; + if (lhs->asObjectValue() && (!rhs->asObjectValue() || !rhs->asNullValue() || !rhs->asUndefinedValue())) + return true; + return false; +} + + bool Check::visit(BinaryExpression *ast) { const QString source = _doc->source(); @@ -1300,6 +1324,18 @@ bool Check::visit(BinaryExpression *ast) || shouldAvoidNonStrictEqualityCheck(rhsValue, lhsValue)) { addMessage(MaybeWarnEqualityTypeCoercion, ast->operatorToken); } + if (equalIsAlwaysFalse(lhsValue, rhsValue) + || equalIsAlwaysFalse(rhsValue, lhsValue)) + addMessage(WarnLogicalValueDoesNotDependOnValues, ast->operatorToken); + } + if (ast->op == QSOperator::StrictEqual || ast->op == QSOperator::StrictNotEqual) { + Evaluate eval(&_scopeChain); + const Value *lhsValue = eval(ast->left); + const Value *rhsValue = eval(ast->right); + if (strictCompareConstant(lhsValue, rhsValue) + || strictCompareConstant(rhsValue, lhsValue)) { + addMessage(WarnLogicalValueDoesNotDependOnValues, ast->operatorToken); + } } // check odd + ++ combinations diff --git a/src/libs/qmljs/qmljsstaticanalysismessage.cpp b/src/libs/qmljs/qmljsstaticanalysismessage.cpp index f9b233150ef..86ac7e8b562 100644 --- a/src/libs/qmljs/qmljsstaticanalysismessage.cpp +++ b/src/libs/qmljs/qmljsstaticanalysismessage.cpp @@ -247,6 +247,8 @@ StaticAnalysisMessages::StaticAnalysisMessages() tr("Hit maximum recursion limit when visiting AST.")); newMsg(ErrTypeIsInstantiatedRecursively, Error, tr("Type cannot be instantiated recursively (%1)."), 1); + newMsg(WarnLogicalValueDoesNotDependOnValues, Warning, + tr("Logical value does not depend on actual values")); } } // anonymous namespace diff --git a/src/libs/qmljs/qmljsstaticanalysismessage.h b/src/libs/qmljs/qmljsstaticanalysismessage.h index 27e37a3c605..d7e2d32eb13 100644 --- a/src/libs/qmljs/qmljsstaticanalysismessage.h +++ b/src/libs/qmljs/qmljsstaticanalysismessage.h @@ -131,6 +131,7 @@ enum Type ErrShorterStringValueExpected = 322, ErrInvalidArrayValueLength = 323, ErrHitMaximumRecursion = 324, + WarnLogicalValueDoesNotDependOnValues = 325, WarnDuplicateImport = 400 }; diff --git a/tests/auto/qml/codemodel/check/equality-checks.qml b/tests/auto/qml/codemodel/check/equality-checks.qml index 72a6898e42b..b562625c23c 100644 --- a/tests/auto/qml/codemodel/check/equality-checks.qml +++ b/tests/auto/qml/codemodel/check/equality-checks.qml @@ -12,42 +12,42 @@ Rectangle { if (s == s) {} if (s == n) {} // 126 15 16 - if (s == N) {} - if (s == u) {} + if (s == N) {} // 325 15 16 + if (s == u) {} // 325 15 16 if (s == b) {} // 126 15 16 if (s == o) {} // 126 15 16 if (s == k) {} // 126 15 16 if (n == s) {} // 126 15 16 if (n == n) {} - if (n == N) {} - if (n == u) {} + if (n == N) {} // 325 15 16 + if (n == u) {} // 325 15 16 if (n == b) {} // 126 15 16 if (n == o) {} // 126 15 16 if (n == k) {} // 126 15 16 - if (N == s) {} - if (N == n) {} + if (N == s) {} // 325 15 16 + if (N == n) {} // 325 15 16 if (N == N) {} if (N == u) {} - if (N == b) {} + if (N == b) {} // 325 15 16 if (N == o) {} if (N == k) {} // 126 15 16 - if (u == s) {} - if (u == n) {} + if (u == s) {} // 325 15 16 + if (u == n) {} // 325 15 16 if (u == N) {} if (u == u) {} - if (u == b) {} + if (u == b) {} // 325 15 16 if (u == o) {} if (u == k) {} // 126 15 16 if (b == s) {} // 126 15 16 if (b == n) {} // 126 15 16 - if (b == N) {} - if (b == u) {} + if (b == N) {} // 325 15 16 + if (b == u) {} // 325 15 16 if (b == b) {} if (b == o) {} // 126 15 16 if (b == k) {} // 126 15 16 @@ -67,5 +67,70 @@ Rectangle { if (k == b) {} // 126 15 16 if (k == o) {} // 126 15 16 if (k == k) {} // 126 15 16 + + if (s === s) {} + if (s === n) {} // 325 15 17 + if (s === N) {} // 325 15 17 + if (s === u) {} // 325 15 17 + if (s === b) {} // 325 15 17 + if (s === o) {} // 325 15 17 + if (s === k) {} + if (s !== s) {} + if (s !== n) {} // 325 15 17 + if (s !== N) {} // 325 15 17 + if (s !== u) {} // 325 15 17 + if (s !== b) {} // 325 15 17 + if (s !== o) {} // 325 15 17 + if (s !== k) {} + + if (n === s) {} // 325 15 17 + if (n === n) {} + if (n === N) {} // 325 15 17 + if (n === u) {} // 325 15 17 + if (n === b) {} // 325 15 17 + if (n === o) {} // 325 15 17 + if (n === k) {} + + if (N === s) {} // 325 15 17 + if (N === n) {} // 325 15 17 + if (N === N) {} + if (N === u) {} + + if (N === b) {} // 325 15 17 + if (N === o) {} // 325 15 17 + if (N === k) {} + + if (u === s) {} // 325 15 17 + if (u === n) {} // 325 15 17 + if (u === N) {} + if (u === u) {} + if (u === b) {} // 325 15 17 + if (u === o) {} // 325 15 17 + if (u === k) {} + + if (b === s) {} // 325 15 17 + if (b === n) {} // 325 15 17 + + if (b === N) {} // 325 15 17 + if (b === u) {} // 325 15 17 + if (b === b) {} + if (b === o) {} // 325 15 17 + if (b === k) {} + + if (o === s) {} // 325 15 17 + if (o === n) {} // 325 15 17 + if (o === N) {} // 325 15 17 + if (o === u) {} // 325 15 17 + if (o === b) {} // 325 15 17 + if (o === o) {} // 325 15 17 + if (o === k) {} + + if (k === s) {} + if (k === n) {} + if (k === N) {} + if (k === u) {} + if (k === b) {} + if (k === o) {} + if (k === k) {} } }