Skip to content

Commit 5c9939b

Browse files
semmle-qlcisemmle-qlci
authored
Merge pull request #390 from esben-semmle/js/improve-useless-conditional-message
Approved by xiemaisi
2 parents 2846d80 + 651f325 commit 5c9939b

File tree

6 files changed

+111
-25
lines changed

6 files changed

+111
-25
lines changed

‎javascript/ql/src/Statements/UselessConditional.ql‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -138,10 +138,7 @@ where isConditional(cond, op.asExpr()) and
138138
// otherwise we just report that `op` always evaluates to `cv`
139139
else (
140140
sel = op.asExpr().stripParens() and
141-
if sel instanceof VarAccess then
142-
msg = "Variable '" + sel.(VarAccess).getVariable().getName() + "' always evaluates to " + cv + " here."
143-
else
144-
msg = "This expression always evaluates to " + cv + "."
141+
msg = "This " + describeExpression(sel) + " always evaluates to " + cv + "."
145142
)
146143

147144
select sel, msg

‎javascript/ql/src/semmle/javascript/Util.qll‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
import javascript
2+
13
/**
24
* Provides general-purpose utility predicates.
35
*/
@@ -34,3 +36,36 @@ bindingset[str, maxLength, explanation]
3436
string truncate(string str, int maxLength, string explanation) {
3537
if str.length() > maxLength then result = str.prefix(maxLength) + explanation else result = str
3638
}
39+
40+
/**
41+
* Gets a string that describes `e`.
42+
*/
43+
string describeExpression(Expr e) {
44+
if e instanceof InvokeExpr then
45+
exists (string prefix, string suffix |
46+
result = prefix + suffix |
47+
( if e instanceof NewExpr then
48+
prefix = "constructor call"
49+
else if e instanceof MethodCallExpr then
50+
prefix = "method call"
51+
else
52+
prefix = "call"
53+
) and
54+
(
55+
if exists(e.(InvokeExpr).getCalleeName()) then
56+
suffix = " to " + e.(InvokeExpr).getCalleeName()
57+
else
58+
suffix = ""
59+
)
60+
)
61+
else if e instanceof Comparison then
62+
result = "comparison"
63+
else if e instanceof VarAccess then
64+
result = "use of variable '" + e.(VarAccess).getName() + "'"
65+
else if e instanceof PropAccess and exists (e.(PropAccess).getPropertyName()) then
66+
result = "use of property '" + e.(PropAccess).getPropertyName() + "'"
67+
else if e instanceof LogNotExpr then
68+
result = "negation"
69+
else
70+
result = "expression"
71+
}

‎javascript/ql/test/library-tests/Util/describeExpression.expected‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
| tst.js:1:1:15:2 | (functi ... !'';\\n}) | expression |
2+
| tst.js:1:2:15:1 | functio ... !'';\\n} | expression |
3+
| tst.js:2:5:2:5 | v | use of variable 'v' |
4+
| tst.js:3:5:3:5 | o | use of variable 'o' |
5+
| tst.js:3:5:3:7 | o.m | use of property 'm' |
6+
| tst.js:3:5:3:9 | o.m() | method call to m |
7+
| tst.js:3:7:3:7 | m | expression |
8+
| tst.js:4:5:4:5 | f | use of variable 'f' |
9+
| tst.js:4:5:4:7 | f() | call to f |
10+
| tst.js:5:5:5:5 | x | use of variable 'x' |
11+
| tst.js:5:5:5:10 | x == y | comparison |
12+
| tst.js:5:10:5:10 | y | use of variable 'y' |
13+
| tst.js:6:5:6:5 | o | use of variable 'o' |
14+
| tst.js:6:5:6:7 | o.p | use of property 'p' |
15+
| tst.js:6:7:6:7 | p | expression |
16+
| tst.js:7:5:7:11 | new K() | constructor call to K |
17+
| tst.js:7:9:7:9 | K | use of variable 'K' |
18+
| tst.js:8:5:8:5 | a | use of variable 'a' |
19+
| tst.js:8:5:8:9 | a + b | expression |
20+
| tst.js:8:9:8:9 | b | use of variable 'b' |
21+
| tst.js:9:5:9:10 | new "" | constructor call |
22+
| tst.js:9:9:9:10 | "" | expression |
23+
| tst.js:10:5:10:6 | "" | expression |
24+
| tst.js:10:5:10:8 | ""() | call |
25+
| tst.js:11:5:11:5 | o | use of variable 'o' |
26+
| tst.js:11:5:11:8 | o[x] | expression |
27+
| tst.js:11:7:11:7 | x | use of variable 'x' |
28+
| tst.js:12:5:12:5 | o | use of variable 'o' |
29+
| tst.js:12:5:12:10 | o['x'] | use of property 'x' |
30+
| tst.js:12:7:12:9 | 'x' | expression |
31+
| tst.js:13:5:13:5 | o | use of variable 'o' |
32+
| tst.js:13:5:13:10 | o['x'] | use of property 'x' |
33+
| tst.js:13:5:13:12 | o['x']() | method call to x |
34+
| tst.js:13:7:13:9 | 'x' | expression |
35+
| tst.js:14:5:14:7 | !'' | negation |
36+
| tst.js:14:6:14:7 | '' | expression |

‎javascript/ql/test/library-tests/Util/describeExpression.ql‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
import semmle.javascript.Util
2+
3+
from Expr e
4+
select e, describeExpression(e)

‎javascript/ql/test/library-tests/Util/tst.js‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1,15 @@
1-
// used by qltest to identify the language
1+
(function() {
2+
v;
3+
o.m();
4+
f();
5+
x == y;
6+
o.p;
7+
new K();
8+
a + b;
9+
new "";
10+
""();
11+
o[x];
12+
o['x'];
13+
o['x']();
14+
!'';
15+
});

‎javascript/ql/test/query-tests/Statements/UselessConditional/UselessConditional.expected‎

Lines changed: 20 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1,22 +1,22 @@
1-
| UselessConditional.js:5:7:5:12 | !lines | This expression always evaluates to false. |
1+
| UselessConditional.js:5:7:5:12 | !lines | This negation always evaluates to false. |
22
| UselessConditional.js:12:34:12:79 | (v = ne ... k] = v) | This logical 'and' expression can be replaced with a comma expression. |
3-
| UselessConditional.js:17:9:17:9 | a | Variable 'a' always evaluates to false here. |
4-
| UselessConditional.js:18:9:18:9 | b | Variable 'b' always evaluates to false here. |
5-
| UselessConditional.js:21:9:21:9 | a | Variable 'a' always evaluates to false here. |
6-
| UselessConditional.js:22:9:22:9 | b | Variable 'b' always evaluates to false here. |
7-
| UselessConditional.js:26:6:26:6 | x | Variable 'x' always evaluates to true here. |
8-
| UselessConditional.js:27:7:27:13 | new X() | This expression always evaluates to true. |
9-
| UselessConditional.js:28:7:28:7 | x | Variable 'x' always evaluates to true here. |
10-
| UselessConditional.js:29:8:29:8 | x | Variable 'x' always evaluates to true here. |
11-
| UselessConditional.js:30:8:30:14 | new X() | This expression always evaluates to true. |
12-
| UselessConditional.js:33:7:33:7 | x | Variable 'x' always evaluates to false here. |
13-
| UselessConditional.js:54:9:54:13 | known | Variable 'known' always evaluates to false here. |
14-
| UselessConditional.js:60:9:60:15 | unknown | Variable 'unknown' always evaluates to false here. |
15-
| UselessConditional.js:65:5:65:5 | x | Variable 'x' always evaluates to true here. |
16-
| UselessConditional.js:76:13:76:13 | x | Variable 'x' always evaluates to true here. |
17-
| UselessConditional.js:82:13:82:13 | x | Variable 'x' always evaluates to true here. |
3+
| UselessConditional.js:17:9:17:9 | a | This use of variable 'a' always evaluates to false. |
4+
| UselessConditional.js:18:9:18:9 | b | This use of variable 'b' always evaluates to false. |
5+
| UselessConditional.js:21:9:21:9 | a | This use of variable 'a' always evaluates to false. |
6+
| UselessConditional.js:22:9:22:9 | b | This use of variable 'b' always evaluates to false. |
7+
| UselessConditional.js:26:6:26:6 | x | This use of variable 'x' always evaluates to true. |
8+
| UselessConditional.js:27:7:27:13 | new X() | This constructor call to X always evaluates to true. |
9+
| UselessConditional.js:28:7:28:7 | x | This use of variable 'x' always evaluates to true. |
10+
| UselessConditional.js:29:8:29:8 | x | This use of variable 'x' always evaluates to true. |
11+
| UselessConditional.js:30:8:30:14 | new X() | This constructor call to X always evaluates to true. |
12+
| UselessConditional.js:33:7:33:7 | x | This use of variable 'x' always evaluates to false. |
13+
| UselessConditional.js:54:9:54:13 | known | This use of variable 'known' always evaluates to false. |
14+
| UselessConditional.js:60:9:60:15 | unknown | This use of variable 'unknown' always evaluates to false. |
15+
| UselessConditional.js:65:5:65:5 | x | This use of variable 'x' always evaluates to true. |
16+
| UselessConditional.js:76:13:76:13 | x | This use of variable 'x' always evaluates to true. |
17+
| UselessConditional.js:82:13:82:13 | x | This use of variable 'x' always evaluates to true. |
1818
| UselessConditional.js:89:10:89:16 | x, true | This expression always evaluates to true. |
19-
| UselessConditionalGood.js:58:12:58:13 | x2 | Variable 'x2' always evaluates to false here. |
20-
| UselessConditionalGood.js:69:12:69:13 | xy | Variable 'xy' always evaluates to false here. |
21-
| UselessConditionalGood.js:85:12:85:13 | xy | Variable 'xy' always evaluates to false here. |
22-
| UselessConditionalGood.js:97:12:97:13 | xy | Variable 'xy' always evaluates to false here. |
19+
| UselessConditionalGood.js:58:12:58:13 | x2 | This use of variable 'x2' always evaluates to false. |
20+
| UselessConditionalGood.js:69:12:69:13 | xy | This use of variable 'xy' always evaluates to false. |
21+
| UselessConditionalGood.js:85:12:85:13 | xy | This use of variable 'xy' always evaluates to false. |
22+
| UselessConditionalGood.js:97:12:97:13 | xy | This use of variable 'xy' always evaluates to false. |

0 commit comments

Comments
 (0)