Skip to content

Commit 0e79d3d

Browse files
author
Esben Sparre Andreasen
authored
Merge pull request github#2065 from erik-krogh/noReturn
JS: use of returnless function
2 parents ea63414 + 0933235 commit 0e79d3d

File tree

9 files changed

+251
-16
lines changed

9 files changed

+251
-16
lines changed

‎change-notes/1.23/analysis-javascript.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,8 @@
1717
| Unused index variable (`js/unused-index-variable`) | correctness | Highlights loops that iterate over an array, but do not use the index variable to access array elements, indicating a possible typo or logic error. Results are shown on LGTM by default. |
1818
| Loop bound injection (`js/loop-bound-injection`) | security, external/cwe/cwe-834 | Highlights loops where a user-controlled object with an arbitrary .length value can trick the server to loop indefinitely. Results are not shown on LGTM by default. |
1919
| Suspicious method name (`js/suspicious-method-name-declaration`) | correctness, typescript, methods | Highlights suspiciously named methods where the developer likely meant to write a constructor or function. Results are shown on LGTM by default. |
20+
| Use of returnless function (`js/use-of-returnless-function`) | maintainability, correctness | Highlights calls where the return value is used, but the callee never returns a value. Results are shown on LGTM by default. |
21+
2022

2123
## Changes to existing queries
2224

‎javascript/ql/src/Statements/UseOfReturnlessFunction.qhelp

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
<overview>
6+
<p>
7+
JavaScript functions that do not return an expression will implicitly return
8+
<code>undefined</code>. Using the implicit return value from such a function
9+
is not an error in itself, but it is a pattern indicating that some
10+
misunderstanding has occurred.
11+
</p>
12+
13+
</overview>
14+
<recommendation>
15+
16+
<p>
17+
Do not use the return value from a function that does not return an expression.
18+
</p>
19+
20+
</recommendation>
21+
<example>
22+
23+
<p>
24+
In the example below, the function <code>renderText</code> is used to render
25+
text through side effects, and the function does not return an expression.
26+
However, the programmer still uses the return value from
27+
<code>renderText</code> as if the function returned an expression, which is
28+
clearly an error.
29+
</p>
30+
31+
<sample src="examples/UseOfReturnlessFunction.js" />
32+
33+
<p>
34+
The program can be fixed either by removing the use of the value returned by
35+
<code>renderText</code>, or by changing the <code>renderText</code> function
36+
to return an expression.
37+
</p>
38+
39+
</example>
40+
<references>
41+
<li>Mozilla Developer Network: <a href="https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/return">Return</a>.</li>
42+
</references>
43+
</qhelp>

‎javascript/ql/src/Statements/UseOfReturnlessFunction.ql

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,98 @@
1+
/**
2+
* @name Use of returnless function
3+
* @description Using the return value of a function that does not return an expression is indicative of a mistake.
4+
* @kind problem
5+
* @problem.severity warning
6+
* @id js/use-of-returnless-function
7+
* @tags maintainability,
8+
* correctness
9+
* @precision high
10+
*/
11+
12+
import javascript
13+
import Declarations.UnusedVariable
14+
import Expressions.ExprHasNoEffect
15+
import Statements.UselessConditional
16+
17+
predicate returnsVoid(Function f) {
18+
not f.isGenerator() and
19+
not f.isAsync() and
20+
not exists(f.getAReturnedExpr())
21+
}
22+
23+
predicate isStub(Function f) {
24+
f.getBody().(BlockStmt).getNumChild() = 0
25+
or
26+
f instanceof ExternalDecl
27+
}
28+
29+
/**
30+
* Holds if `e` is in a syntactic context where it likely is fine that the value of `e` comes from a call to a returnless function.
31+
*/
32+
predicate benignContext(Expr e) {
33+
inVoidContext(e) or
34+
35+
// A return statement is often used to just end the function.
36+
e = any(Function f).getAReturnedExpr()
37+
or
38+
exists(ConditionalExpr cond | cond.getABranch() = e and benignContext(cond))
39+
or
40+
exists(LogicalBinaryExpr bin | bin.getAnOperand() = e and benignContext(bin))
41+
or
42+
exists(Expr parent | parent.getUnderlyingValue() = e and benignContext(parent))
43+
or
44+
any(VoidExpr voidExpr).getOperand() = e
45+
46+
or
47+
// weeds out calls inside HTML-attributes.
48+
e.getParent() instanceof CodeInAttribute or
49+
// and JSX-attributes.
50+
e = any(JSXAttribute attr).getValue() or
51+
52+
exists(AwaitExpr await | await.getOperand() = e and benignContext(await))
53+
or
54+
// Avoid double reporting with js/trivial-conditional
55+
isExplicitConditional(_, e)
56+
or
57+
// Avoid double reporting with js/comparison-between-incompatible-types
58+
any(Comparison binOp).getAnOperand() = e
59+
or
60+
// Avoid double reporting with js/property-access-on-non-object
61+
any(PropAccess ac).getBase() = e
62+
or
63+
// Avoid double-reporting with js/unused-local-variable
64+
exists(VariableDeclarator v | v.getInit() = e and v.getBindingPattern().getVariable() instanceof UnusedLocal)
65+
or
66+
// Avoid double reporting with js/call-to-non-callable
67+
any(InvokeExpr invoke).getCallee() = e
68+
or
69+
// arguments to Promise.resolve (and promise library variants) are benign.
70+
e = any(ResolvedPromiseDefinition promise).getValue().asExpr()
71+
}
72+
73+
predicate oneshotClosure(InvokeExpr call) {
74+
call.getCallee().getUnderlyingValue() instanceof ImmediatelyInvokedFunctionExpr
75+
}
76+
77+
predicate alwaysThrows(Function f) {
78+
exists(ReachableBasicBlock entry, DataFlow::Node throwNode |
79+
entry = f.getEntryBB() and
80+
throwNode.asExpr() = any(ThrowStmt t).getExpr() and
81+
entry.dominates(throwNode.getBasicBlock())
82+
)
83+
}
84+
85+
from DataFlow::CallNode call
86+
where
87+
not call.isIndefinite(_) and
88+
forex(Function f | f = call.getACallee() |
89+
returnsVoid(f) and not isStub(f) and not alwaysThrows(f)
90+
) and
91+
92+
not benignContext(call.asExpr()) and
93+
94+
// anonymous one-shot closure. Those are used in weird ways and we ignore them.
95+
not oneshotClosure(call.asExpr())
96+
select
97+
call, "the function $@ does not return anything, yet the return value is used.", call.getACallee(), call.getCalleeName()
98+

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

Lines changed: 1 addition & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import javascript
1616
import semmle.javascript.RestrictedLocations
1717
import semmle.javascript.dataflow.Refinements
1818
import semmle.javascript.DefensiveProgramming
19+
import UselessConditional
1920

2021
/**
2122
* Gets the unique definition of `v`.
@@ -123,22 +124,6 @@ predicate whitelist(Expr e) {
123124
isConstantBooleanReturnValue(e)
124125
}
125126

126-
/**
127-
* Holds if `e` is part of a conditional node `cond` that evaluates
128-
* `e` and checks its value for truthiness, and the return value of `e`
129-
* is not used for anything other than this truthiness check.
130-
*/
131-
predicate isExplicitConditional(ASTNode cond, Expr e) {
132-
e = cond.(IfStmt).getCondition()
133-
or
134-
e = cond.(LoopStmt).getTest()
135-
or
136-
e = cond.(ConditionalExpr).getCondition()
137-
or
138-
isExplicitConditional(_, cond) and
139-
e = cond.(Expr).getUnderlyingValue().(LogicalBinaryExpr).getAnOperand()
140-
}
141-
142127
/**
143128
* Holds if `e` is part of a conditional node `cond` that evaluates
144129
* `e` and checks its value for truthiness.

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

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
/**
2+
* Provides predicates for working with useless conditionals.
3+
*/
4+
5+
import javascript
6+
7+
/**
8+
* Holds if `e` is part of a conditional node `cond` that evaluates
9+
* `e` and checks its value for truthiness, and the return value of `e`
10+
* is not used for anything other than this truthiness check.
11+
*/
12+
predicate isExplicitConditional(ASTNode cond, Expr e) {
13+
e = cond.(IfStmt).getCondition()
14+
or
15+
e = cond.(LoopStmt).getTest()
16+
or
17+
e = cond.(ConditionalExpr).getCondition()
18+
or
19+
isExplicitConditional(_, cond) and
20+
e = cond.(Expr).getUnderlyingValue().(LogicalBinaryExpr).getAnOperand()
21+
}

‎javascript/ql/src/Statements/examples/UseOfReturnlessFunction.js

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
var stage = require("./stage")
2+
3+
function renderText(text, id) {
4+
document.getElementById(id).innerText = text;
5+
}
6+
7+
var text = renderText("Two households, both alike in dignity", "scene");
8+
9+
stage.show(text);

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
| tst.js:20:17:20:33 | onlySideEffects() | the function $@ does not return anything, yet the return value is used. | tst.js:11:5:13:5 | functio ... )\\n } | onlySideEffects |
2+
| tst.js:24:13:24:29 | onlySideEffects() | the function $@ does not return anything, yet the return value is used. | tst.js:11:5:13:5 | functio ... )\\n } | onlySideEffects |
3+
| tst.js:30:20:30:36 | onlySideEffects() | the function $@ does not return anything, yet the return value is used. | tst.js:11:5:13:5 | functio ... )\\n } | onlySideEffects |
4+
| tst.js:53:10:53:34 | bothOnl ... fects() | the function $@ does not return anything, yet the return value is used. | tst.js:11:5:13:5 | functio ... )\\n } | bothOnlyHaveSideEffects |
5+
| tst.js:53:10:53:34 | bothOnl ... fects() | the function $@ does not return anything, yet the return value is used. | tst.js:48:2:50:5 | functio ... )\\n } | bothOnlyHaveSideEffects |

‎javascript/ql/test/query-tests/Statements/UseOfReturnlessFunction/UseOfReturnlessFunction.qlref

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Statements/UseOfReturnlessFunction.ql

‎javascript/ql/test/query-tests/Statements/UseOfReturnlessFunction/tst.js

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
(function () {
2+
function stub() {
3+
throw new Error("Not implemented!");
4+
}
5+
6+
function returnsValue() {
7+
var x = 3;
8+
return x * 2;
9+
}
10+
11+
function onlySideEffects() {
12+
console.log("Boo!")
13+
}
14+
15+
var arrow = () => onlySideEffects();
16+
17+
console.log(returnsValue())
18+
console.log(stub())
19+
20+
console.log(onlySideEffects()); // Not OK!
21+
22+
var a = Math.random() > 0.5 ? returnsValue() : onlySideEffects(); // OK! A is never used.
23+
24+
var b = onlySideEffects();
25+
console.log(b);
26+
27+
var c = 42 + (onlySideEffects(), 42); // OK, value is thrown away.
28+
console.log(c);
29+
30+
var d = 42 + (42, onlySideEffects()); // NOT OK!
31+
console.log(d);
32+
33+
if (onlySideEffects()) {
34+
// nothing.
35+
}
36+
37+
for (i = 0; onlySideEffects(); i++) {
38+
// nothing.
39+
}
40+
41+
var myObj = {
42+
onlySideEffects: onlySideEffects
43+
}
44+
45+
var e = myObj.onlySideEffects.apply(this, arguments); // NOT OK!
46+
console.log(e);
47+
48+
function onlySideEffects2() {
49+
console.log("Boo!")
50+
}
51+
52+
var bothOnlyHaveSideEffects = Math.random() > 0.5 ? onlySideEffects : onlySideEffects2;
53+
var f = bothOnlyHaveSideEffects(); // NOT OK!
54+
console.log(f);
55+
56+
var oneOfEach = Math.random() > 0.5 ? onlySideEffects : returnsValue;
57+
var g = oneOfEach(); // OK
58+
console.log(g);
59+
60+
function alwaysThrows() {
61+
if (Math.random() > 0.5) {
62+
console.log("Whatever!")
63+
} else {
64+
console.log("Boo!")
65+
}
66+
throw new Error("Important error!")
67+
}
68+
69+
var h = returnsValue() || alwaysThrows(); // OK!
70+
console.log(h);
71+
})();

0 commit comments

Comments
 (0)