mirror of
https://github.com/github/codeql.git
synced 2026-04-29 02:35:15 +02:00
JS: add query: js/useless-defensive-code
This commit is contained in:
Esben Sparre Andreasen
@@ -0,0 +1,91 @@
|
||||
<!DOCTYPE qhelp PUBLIC
|
||||
"-//Semmle//qhelp//EN"
|
||||
"qhelp.dtd">
|
||||
<qhelp>
|
||||
<overview>
|
||||
<p>
|
||||
|
||||
Defensive code can prevent unforeseen circumstances from
|
||||
causing fatal program behaviors.
|
||||
|
||||
A common defensive code pattern is to guard
|
||||
against dereferencing the values <code>null</code> or
|
||||
<code>undefined</code>.
|
||||
|
||||
However, if the situation that some defensive code guards
|
||||
against never can occur, then the defensive code serves no purpose and
|
||||
it can safely be removed.
|
||||
|
||||
</p>
|
||||
|
||||
</overview>
|
||||
<recommendation>
|
||||
|
||||
<p>
|
||||
|
||||
Examine the surrounding code to determine if the defensive
|
||||
code is worth keeping despite providing no practical use. If it is no
|
||||
longer needed, remove it.
|
||||
|
||||
</p>
|
||||
|
||||
</recommendation>
|
||||
<example>
|
||||
|
||||
<p>
|
||||
|
||||
The following example shows a <code>cleanupLater</code>
|
||||
function that asynchronously will perform a cleanup task after some
|
||||
delay. When the cleanup task completes, the function invokes the
|
||||
provided callback parameter <code>cb</code>.
|
||||
|
||||
To prevent a crash by invoking <code>cb</code> when it
|
||||
has the value <code>undefined</code>, defensive code guards
|
||||
the invocation by checking if <code>cb</code> is truthy.
|
||||
|
||||
</p>
|
||||
|
||||
<sample src="examples/UselessDefensiveProgramming1_bad.js" />
|
||||
|
||||
<p>
|
||||
|
||||
However, the <code>cleanupLater</code> function is always
|
||||
invoked with a callback argument, so the defensive code condition
|
||||
always holds, and it is therefore not
|
||||
required. The function can therefore be simplified to:
|
||||
|
||||
</p>
|
||||
|
||||
<sample src="examples/UselessDefensiveProgramming1_good.js" />
|
||||
|
||||
<p>
|
||||
|
||||
Guarding against the same situation multiple times is
|
||||
another example of defensive code that provides no practical use. The
|
||||
example below shows a function that assigns a value to a property of
|
||||
an object, where defensive code ensures that the assigned value is not
|
||||
<code>undefined</code> or <code>null</code>.
|
||||
|
||||
</p>
|
||||
|
||||
<sample src="examples/UselessDefensiveProgramming2_bad.js" />
|
||||
|
||||
<p>
|
||||
|
||||
However, due to coercion rules, <code>v ==
|
||||
undefined</code> holds for both the situation where <code>v</code>
|
||||
is<code>undefined</code> and the situation where <code>v</code>
|
||||
is<code>null</code>, so the <code>v == null</code>
|
||||
guard serves no purpose, and it can be removed:
|
||||
|
||||
</p>
|
||||
|
||||
<sample src="examples/UselessDefensiveProgramming2_good.js" />
|
||||
|
||||
</example>
|
||||
<references>
|
||||
|
||||
<li>Wikipedia: <a href="https://en.wikipedia.org/wiki/Defensive_programming">Defensive programming</a>.</li>
|
||||
|
||||
</references>
|
||||
</qhelp>
|
||||
19
javascript/ql/src/Expressions/UselessDefensiveProgramming.ql
Normal file
19
javascript/ql/src/Expressions/UselessDefensiveProgramming.ql
Normal file
@@ -0,0 +1,19 @@
|
||||
/**
|
||||
* @name Useless defensive code
|
||||
* @description If the situation some defensive code guards against never
|
||||
* happens, then the defensive code is not needed.
|
||||
* @kind problem
|
||||
* @problem.severity recommendation
|
||||
* @id js/useless-defensive-code
|
||||
* @tags correctness
|
||||
* external/cwe/cwe-570
|
||||
* external/cwe/cwe-571
|
||||
* @precision very-high
|
||||
*/
|
||||
|
||||
import javascript
|
||||
import semmle.javascript.DefensiveProgramming
|
||||
|
||||
from DefensiveExpression e, boolean cv
|
||||
where e.getTheTestResult() = cv
|
||||
select e, "This guard always evaluates to " + cv + "."
|
||||
@@ -0,0 +1,10 @@
|
||||
function cleanupLater(delay, cb) {
|
||||
setTimeout(function() {
|
||||
cleanup();
|
||||
if (cb) { // BAD: useless check, `cb` is always truthy
|
||||
cb();
|
||||
}
|
||||
}, delay)
|
||||
}
|
||||
|
||||
cleanupLater(1000, function(){console.log("Cleanup done")});
|
||||
@@ -0,0 +1,9 @@
|
||||
function cleanupLater(delay, cb) {
|
||||
setTimeout(function() {
|
||||
cleanupNow();
|
||||
// GOOD: no need to guard the invocation
|
||||
cb();
|
||||
}, delay)
|
||||
}
|
||||
|
||||
cleanupLater(function(){console.log("Cleanup done")});
|
||||
@@ -0,0 +1,5 @@
|
||||
function setSafeStringProp(o, prop, v) {
|
||||
// BAD: `v == null` is useless
|
||||
var safe = v == undefined || v == null? '': v;
|
||||
o[prop] = safe;
|
||||
}
|
||||
@@ -0,0 +1,5 @@
|
||||
function setSafeStringProp(o, prop, v) {
|
||||
// GOOD: `v == undefined` handles both `undefined` and `null`
|
||||
var safe = v == undefined? '': v;
|
||||
o[prop] = safe;
|
||||
}
|
||||
@@ -0,0 +1,78 @@
|
||||
| global-module-definition.js:10:8:10:11 | Mod2 | This guard always evaluates to false. |
|
||||
| module-environment-detection.js:3:13:3:41 | typeof ... efined' | This guard always evaluates to true. |
|
||||
| module-environment-detection.js:15:9:15:37 | typeof ... efined' | This guard always evaluates to true. |
|
||||
| module-environment-detection.js:23:8:23:36 | typeof ... efined' | This guard always evaluates to true. |
|
||||
| tst2.js:4:12:4:35 | typeof ... efined" | This guard always evaluates to true. |
|
||||
| tst.js:13:10:13:11 | u_ | This guard always evaluates to false. |
|
||||
| tst.js:14:10:14:11 | n_ | This guard always evaluates to false. |
|
||||
| tst.js:15:10:15:11 | o_ | This guard always evaluates to true. |
|
||||
| tst.js:18:5:18:5 | u | This guard always evaluates to false. |
|
||||
| tst.js:19:5:19:5 | n | This guard always evaluates to false. |
|
||||
| tst.js:20:5:20:5 | o | This guard always evaluates to true. |
|
||||
| tst.js:23:5:23:5 | u | This guard always evaluates to false. |
|
||||
| tst.js:24:5:24:5 | n | This guard always evaluates to false. |
|
||||
| tst.js:25:5:25:5 | o | This guard always evaluates to true. |
|
||||
| tst.js:28:5:28:6 | !u | This guard always evaluates to true. |
|
||||
| tst.js:29:5:29:6 | !n | This guard always evaluates to true. |
|
||||
| tst.js:30:5:30:6 | !o | This guard always evaluates to false. |
|
||||
| tst.js:33:5:33:7 | !!u | This guard always evaluates to false. |
|
||||
| tst.js:34:5:34:7 | !!n | This guard always evaluates to false. |
|
||||
| tst.js:35:5:35:7 | !!o | This guard always evaluates to true. |
|
||||
| tst.js:38:5:38:18 | u != undefined | This guard always evaluates to false. |
|
||||
| tst.js:39:5:39:18 | n != undefined | This guard always evaluates to false. |
|
||||
| tst.js:40:5:40:18 | o != undefined | This guard always evaluates to true. |
|
||||
| tst.js:43:5:43:18 | u == undefined | This guard always evaluates to true. |
|
||||
| tst.js:44:5:44:18 | n == undefined | This guard always evaluates to true. |
|
||||
| tst.js:45:5:45:18 | o == undefined | This guard always evaluates to false. |
|
||||
| tst.js:48:5:48:19 | u === undefined | This guard always evaluates to true. |
|
||||
| tst.js:49:5:49:19 | n === undefined | This guard always evaluates to false. |
|
||||
| tst.js:50:5:50:19 | o === undefined | This guard always evaluates to false. |
|
||||
| tst.js:53:9:53:9 | u | This guard always evaluates to false. |
|
||||
| tst.js:56:9:56:9 | n | This guard always evaluates to false. |
|
||||
| tst.js:59:9:59:9 | o | This guard always evaluates to true. |
|
||||
| tst.js:66:5:66:5 | u | This guard always evaluates to false. |
|
||||
| tst.js:67:5:67:5 | n | This guard always evaluates to false. |
|
||||
| tst.js:68:5:68:5 | o | This guard always evaluates to true. |
|
||||
| tst.js:71:9:71:23 | u !== undefined | This guard always evaluates to false. |
|
||||
| tst.js:74:9:74:23 | n !== undefined | This guard always evaluates to true. |
|
||||
| tst.js:77:9:77:23 | o !== undefined | This guard always evaluates to true. |
|
||||
| tst.js:84:9:84:22 | u == undefined | This guard always evaluates to true. |
|
||||
| tst.js:85:9:85:22 | n == undefined | This guard always evaluates to true. |
|
||||
| tst.js:86:9:86:22 | o == undefined | This guard always evaluates to false. |
|
||||
| tst.js:89:9:89:22 | u != undefined | This guard always evaluates to false. |
|
||||
| tst.js:90:9:90:22 | n != undefined | This guard always evaluates to false. |
|
||||
| tst.js:91:9:91:22 | o != undefined | This guard always evaluates to true. |
|
||||
| tst.js:94:9:94:32 | typeof ... efined" | This guard always evaluates to true. |
|
||||
| tst.js:95:9:95:32 | typeof ... efined" | This guard always evaluates to false. |
|
||||
| tst.js:96:9:96:32 | typeof ... efined" | This guard always evaluates to false. |
|
||||
| tst.js:100:5:100:27 | typeof ... nction" | This guard always evaluates to true. |
|
||||
| tst.js:101:5:101:27 | typeof ... nction" | This guard always evaluates to false. |
|
||||
| tst.js:114:5:114:15 | empty_array | This guard always evaluates to true. |
|
||||
| tst.js:115:5:115:22 | pseudo_empty_array | This guard always evaluates to true. |
|
||||
| tst.js:116:5:116:19 | non_empty_array | This guard always evaluates to true. |
|
||||
| tst.js:124:6:124:20 | u !== undefined | This guard always evaluates to false. |
|
||||
| tst.js:124:25:124:34 | u !== null | This guard always evaluates to true. |
|
||||
| tst.js:125:5:125:19 | u !== undefined | This guard always evaluates to false. |
|
||||
| tst.js:125:24:125:33 | u !== null | This guard always evaluates to true. |
|
||||
| tst.js:127:5:127:18 | u != undefined | This guard always evaluates to false. |
|
||||
| tst.js:127:23:127:31 | u != null | This guard always evaluates to false. |
|
||||
| tst.js:127:23:127:31 | u != null | This guard always evaluates to true. |
|
||||
| tst.js:128:5:128:18 | u == undefined | This guard always evaluates to true. |
|
||||
| tst.js:128:23:128:31 | u == null | This guard always evaluates to false. |
|
||||
| tst.js:128:23:128:31 | u == null | This guard always evaluates to true. |
|
||||
| tst.js:129:5:129:19 | u !== undefined | This guard always evaluates to false. |
|
||||
| tst.js:129:24:129:33 | u !== null | This guard always evaluates to true. |
|
||||
| tst.js:130:5:130:22 | !(u === undefined) | This guard always evaluates to false. |
|
||||
| tst.js:130:27:130:39 | !(u === null) | This guard always evaluates to true. |
|
||||
| tst.js:131:5:131:19 | u === undefined | This guard always evaluates to true. |
|
||||
| tst.js:131:24:131:33 | u === null | This guard always evaluates to false. |
|
||||
| tst.js:132:7:132:21 | u === undefined | This guard always evaluates to true. |
|
||||
| tst.js:132:26:132:35 | u === null | This guard always evaluates to false. |
|
||||
| tst.js:133:5:133:22 | !(u === undefined) | This guard always evaluates to false. |
|
||||
| tst.js:133:27:133:36 | u !== null | This guard always evaluates to true. |
|
||||
| tst.js:135:5:135:18 | u == undefined | This guard always evaluates to true. |
|
||||
| tst.js:135:23:135:31 | u == null | This guard always evaluates to true. |
|
||||
| tst.js:138:24:138:33 | x === null | This guard always evaluates to false. |
|
||||
| tst.js:140:13:140:22 | x === null | This guard always evaluates to false. |
|
||||
| tst.js:156:23:156:31 | x != null | This guard always evaluates to true. |
|
||||
| tst.js:158:13:158:21 | x != null | This guard always evaluates to true. |
|
||||
@@ -0,0 +1 @@
|
||||
Expressions/UselessDefensiveProgramming.ql
|
||||
@@ -0,0 +1,11 @@
|
||||
var Mod1;
|
||||
(function (Mod1) {
|
||||
Mod1.p = 42;
|
||||
})(Mod1 || (Mod1 = {}));
|
||||
|
||||
(function(){
|
||||
var Mod2;
|
||||
(function (Mod2) {
|
||||
Mod2.p = 42;
|
||||
})(Mod2 || (Mod2 = {})); // NOT OK
|
||||
});
|
||||
@@ -0,0 +1,24 @@
|
||||
var _ = (function() {
|
||||
if (typeof exports !== 'undefined') {
|
||||
if (typeof module !== 'undefined' && module.exports) {
|
||||
exports = module.exports = _;
|
||||
}
|
||||
exports._ = _;
|
||||
}
|
||||
return {
|
||||
define: function(name, factory) {
|
||||
}
|
||||
};
|
||||
})(this);
|
||||
|
||||
if (typeof exports !== 'undefined') {
|
||||
if (typeof module !== 'undefined' && module.exports) {
|
||||
exports = module.exports = emmet;
|
||||
}
|
||||
exports.emmet = emmet;
|
||||
}
|
||||
|
||||
(function(){
|
||||
var module;
|
||||
if(typeof module === 'undefined'); // NOT OK
|
||||
});
|
||||
@@ -0,0 +1,174 @@
|
||||
(function(){
|
||||
var v;
|
||||
var u = undefined;
|
||||
var n = null;
|
||||
var o = {};
|
||||
var x = functionWithUnknownReturnValue();
|
||||
|
||||
var u_ = u;
|
||||
var n_ = n;
|
||||
var o_ = o;
|
||||
var x_ = x;
|
||||
|
||||
u_ = u_ || e;
|
||||
n_ = n_ || e;
|
||||
o_ = o_ || e;
|
||||
x_ = x_ || e;
|
||||
|
||||
u && u.p;
|
||||
n && n.p;
|
||||
o && o.p;
|
||||
x && x.p;
|
||||
|
||||
u && u();
|
||||
n && n();
|
||||
o && o();
|
||||
x && x();
|
||||
|
||||
!u || u.p;
|
||||
!n || n.p;
|
||||
!o || o.p;
|
||||
!x || x.p;
|
||||
|
||||
!!u && u.p;
|
||||
!!n && n.p;
|
||||
!!o && o.p;
|
||||
!!x && x.p;
|
||||
|
||||
u != undefined && u.p;
|
||||
n != undefined && n.p;
|
||||
o != undefined && o.p;
|
||||
x != undefined && x.p;
|
||||
|
||||
u == undefined || u.p;
|
||||
n == undefined || n.p;
|
||||
o == undefined || o.p;
|
||||
x == undefined || x.p;
|
||||
|
||||
u === undefined || u.p;
|
||||
n === undefined || n.p;
|
||||
o === undefined || o.p; // T, D
|
||||
x === undefined || x.p;
|
||||
|
||||
if (u) {
|
||||
u.p;
|
||||
}
|
||||
if (n) {
|
||||
n.p;
|
||||
}
|
||||
if (o) {
|
||||
o.p;
|
||||
}
|
||||
if (x) {
|
||||
x.p;
|
||||
}
|
||||
|
||||
u? u():_;
|
||||
n? n(): _;
|
||||
o? o(): _;
|
||||
x? x(): _;
|
||||
|
||||
if (u !== undefined) {
|
||||
u.p;
|
||||
}
|
||||
if (n !== undefined) {
|
||||
n.p;
|
||||
}
|
||||
if (o !== undefined) {
|
||||
o.p;
|
||||
}
|
||||
if (x !== undefined) {
|
||||
x.p;
|
||||
}
|
||||
|
||||
if (u == undefined){}
|
||||
if (n == undefined){}
|
||||
if (o == undefined){}
|
||||
if (x == undefined){}
|
||||
|
||||
if (u != undefined){}
|
||||
if (n != undefined){}
|
||||
if (o != undefined){}
|
||||
if (x != undefined){}
|
||||
|
||||
if (typeof u === "undefined"){}
|
||||
if (typeof n === "undefined"){}
|
||||
if (typeof o === "undefined"){}
|
||||
if (typeof x === "undefined"){}
|
||||
|
||||
function f() { }
|
||||
typeof f === "function" && f();
|
||||
typeof u === "function" && u();
|
||||
typeof x === "function" && x();
|
||||
|
||||
var empty_array = [];
|
||||
var pseudo_empty_array = [''];
|
||||
var non_empty_array = ['foo'];
|
||||
var empty_string = "";
|
||||
var non_empty_string = "foo";
|
||||
var zero = 0;
|
||||
var neg = -1;
|
||||
var _true = true;
|
||||
var _false = false;
|
||||
|
||||
empty_array && empty_array.pop();
|
||||
pseudo_empty_array && pseudo_empty_array.pop();
|
||||
non_empty_array && non_empty_array.pop();
|
||||
empty_string && empty_string.charAt(0);
|
||||
non_empty_string && non_empty_string.charAt(0);
|
||||
zero && zero();
|
||||
neg && neg();
|
||||
_true && _true();
|
||||
_false && _false();D
|
||||
|
||||
(u !== undefined && u !== null) && u.p;
|
||||
u !== undefined && u !== null && u.p;
|
||||
|
||||
u != undefined && u != null;
|
||||
u == undefined || u == null;
|
||||
u !== undefined && u !== null;
|
||||
!(u === undefined) && !(u === null);
|
||||
u === undefined || u === null;
|
||||
!(u === undefined || u === null);
|
||||
!(u === undefined) && u !== null;
|
||||
u !== undefined && n !== null;
|
||||
u == undefined && u == null;
|
||||
x == undefined && x == null;
|
||||
|
||||
x === undefined && x === null;
|
||||
if (x === undefined) {
|
||||
if (x === null) {
|
||||
}
|
||||
}
|
||||
|
||||
x !== undefined && x !== null;
|
||||
if (x !== undefined) {
|
||||
if (x !== null) {
|
||||
}
|
||||
}
|
||||
|
||||
x == undefined && x == null;
|
||||
if (x == undefined) {
|
||||
if (x == null) {
|
||||
}
|
||||
}
|
||||
|
||||
x != undefined && x != null;
|
||||
if (x != undefined) {
|
||||
if (x != null) {
|
||||
}
|
||||
}
|
||||
|
||||
if (typeof x !== undefined);
|
||||
if (typeof window !== undefined);
|
||||
if (typeof x !== x);
|
||||
if (typeof x !== u);
|
||||
|
||||
if (typeof window !== "undefined");
|
||||
if (typeof module !== "undefined");
|
||||
if (typeof global !== "undefined");
|
||||
|
||||
if (typeof window !== "undefined" && window.document);
|
||||
if (typeof module !== "undefined" && module.exports);
|
||||
if (typeof global !== "undefined" && global.process);
|
||||
});
|
||||
@@ -0,0 +1,10 @@
|
||||
(function(){
|
||||
var v;
|
||||
(function(){
|
||||
if(typeof v === "undefined"){ // NOT OK
|
||||
v = 42;
|
||||
}
|
||||
for(var v in x){
|
||||
}
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user