Skip to content

Commit 9e1414e

Browse files
mdjermanovicmdjermanovic
authoredJun 19, 2020
New: Add no-promise-executor-return rule (fixes #12640) (#12648)
* New: Add no-promise-executor-return rule (fixes #12640) * Fix eslint comments in docs * Change error message
·
v9.23.0v7.3.0
1 parent 09cc0a2 commit 9e1414e

File tree

5 files changed

+522
-0
lines changed

5 files changed

+522
-0
lines changed
 

‎docs/rules/no-promise-executor-return.md

+96
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,96 @@
1+
# Disallow returning values from Promise executor functions (no-promise-executor-return)
2+
3+
The `new Promise` constructor accepts a single argument, called an *executor*.
4+
5+
```js
6+
const myPromise = new Promise(function executor(resolve, reject) {
7+
readFile('foo.txt', function(err, result) {
8+
if (err) {
9+
reject(err);
10+
} else {
11+
resolve(result);
12+
}
13+
});
14+
});
15+
```
16+
17+
The executor function usually initiates some asynchronous operation. Once it is finished, the executor should call `resolve` with the result, or `reject` if an error occurred.
18+
19+
The return value of the executor is ignored. Returning a value from an executor function is a possible error because the returned value cannot be used and it doesn't affect the promise in any way.
20+
21+
## Rule Details
22+
23+
This rule disallows returning values from Promise executor functions.
24+
25+
Only `return` without a value is allowed, as it's a control flow statement.
26+
27+
Examples of **incorrect** code for this rule:
28+
29+
```js
30+
/*eslint no-promise-executor-return: "error"*/
31+
32+
new Promise((resolve, reject) => {
33+
if (someCondition) {
34+
return defaultResult;
35+
}
36+
getSomething((err, result) => {
37+
if (err) {
38+
reject(err);
39+
} else {
40+
resolve(result);
41+
}
42+
});
43+
});
44+
45+
new Promise((resolve, reject) => getSomething((err, data) => {
46+
if (err) {
47+
reject(err);
48+
} else {
49+
resolve(data);
50+
}
51+
}));
52+
53+
new Promise(() => {
54+
return 1;
55+
});
56+
```
57+
58+
Examples of **correct** code for this rule:
59+
60+
```js
61+
/*eslint no-promise-executor-return: "error"*/
62+
63+
new Promise((resolve, reject) => {
64+
if (someCondition) {
65+
resolve(defaultResult);
66+
return;
67+
}
68+
getSomething((err, result) => {
69+
if (err) {
70+
reject(err);
71+
} else {
72+
resolve(result);
73+
}
74+
});
75+
});
76+
77+
new Promise((resolve, reject) => {
78+
getSomething((err, data) => {
79+
if (err) {
80+
reject(err);
81+
} else {
82+
resolve(data);
83+
}
84+
});
85+
});
86+
87+
Promise.resolve(1);
88+
```
89+
90+
## Further Reading
91+
92+
* [MDN Promise](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise)
93+
94+
## Related Rules
95+
96+
* [no-async-promise-executor](no-async-promise-executor.md)

‎lib/rules/index.js

+1
Original file line numberDiff line numberDiff line change
@@ -176,6 +176,7 @@ module.exports = new LazyLoadingRuleMap(Object.entries({
176176
"no-plusplus": () => require("./no-plusplus"),
177177
"no-process-env": () => require("./no-process-env"),
178178
"no-process-exit": () => require("./no-process-exit"),
179+
"no-promise-executor-return": () => require("./no-promise-executor-return"),
179180
"no-proto": () => require("./no-proto"),
180181
"no-prototype-builtins": () => require("./no-prototype-builtins"),
181182
"no-redeclare": () => require("./no-redeclare"),

‎lib/rules/no-promise-executor-return.js

+121
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,121 @@
1+
/**
2+
* @fileoverview Rule to disallow returning values from Promise executor functions
3+
* @author Milos Djermanovic
4+
*/
5+
6+
"use strict";
7+
8+
//------------------------------------------------------------------------------
9+
// Requirements
10+
//------------------------------------------------------------------------------
11+
12+
const { findVariable } = require("eslint-utils");
13+
14+
//------------------------------------------------------------------------------
15+
// Helpers
16+
//------------------------------------------------------------------------------
17+
18+
const functionTypesToCheck = new Set(["ArrowFunctionExpression", "FunctionExpression"]);
19+
20+
/**
21+
* Determines whether the given identifier node is a reference to a global variable.
22+
* @param {ASTNode} node `Identifier` node to check.
23+
* @param {Scope} scope Scope to which the node belongs.
24+
* @returns {boolean} True if the identifier is a reference to a global variable.
25+
*/
26+
function isGlobalReference(node, scope) {
27+
const variable = findVariable(scope, node);
28+
29+
return variable !== null && variable.scope.type === "global" && variable.defs.length === 0;
30+
}
31+
32+
/**
33+
* Finds function's outer scope.
34+
* @param {Scope} scope Function's own scope.
35+
* @returns {Scope} Function's outer scope.
36+
*/
37+
function getOuterScope(scope) {
38+
const upper = scope.upper;
39+
40+
if (upper.type === "function-expression-name") {
41+
return upper.upper;
42+
}
43+
return upper;
44+
}
45+
46+
/**
47+
* Determines whether the given function node is used as a Promise executor.
48+
* @param {ASTNode} node The node to check.
49+
* @param {Scope} scope Function's own scope.
50+
* @returns {boolean} `true` if the node is a Promise executor.
51+
*/
52+
function isPromiseExecutor(node, scope) {
53+
const parent = node.parent;
54+
55+
return parent.type === "NewExpression" &&
56+
parent.arguments[0] === node &&
57+
parent.callee.type === "Identifier" &&
58+
parent.callee.name === "Promise" &&
59+
isGlobalReference(parent.callee, getOuterScope(scope));
60+
}
61+
62+
//------------------------------------------------------------------------------
63+
// Rule Definition
64+
//------------------------------------------------------------------------------
65+
66+
module.exports = {
67+
meta: {
68+
type: "problem",
69+
70+
docs: {
71+
description: "disallow returning values from Promise executor functions",
72+
category: "Possible Errors",
73+
recommended: false,
74+
url: "https://eslint.org/docs/rules/no-promise-executor-return"
75+
},
76+
77+
schema: [],
78+
79+
messages: {
80+
returnsValue: "Return values from promise executor functions cannot be read."
81+
}
82+
},
83+
84+
create(context) {
85+
86+
let funcInfo = null;
87+
88+
/**
89+
* Reports the given node.
90+
* @param {ASTNode} node Node to report.
91+
* @returns {void}
92+
*/
93+
function report(node) {
94+
context.report({ node, messageId: "returnsValue" });
95+
}
96+
97+
return {
98+
99+
onCodePathStart(_, node) {
100+
funcInfo = {
101+
upper: funcInfo,
102+
shouldCheck: functionTypesToCheck.has(node.type) && isPromiseExecutor(node, context.getScope())
103+
};
104+
105+
if (funcInfo.shouldCheck && node.type === "ArrowFunctionExpression" && node.expression) {
106+
report(node.body);
107+
}
108+
},
109+
110+
onCodePathEnd() {
111+
funcInfo = funcInfo.upper;
112+
},
113+
114+
ReturnStatement(node) {
115+
if (funcInfo.shouldCheck && node.argument) {
116+
report(node);
117+
}
118+
}
119+
};
120+
}
121+
};

‎tests/lib/rules/no-promise-executor-return.js

+303
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,303 @@
1+
/**
2+
* @fileoverview Tests for the no-promise-executor-return rule
3+
* @author Milos Djermanovic
4+
*/
5+
6+
"use strict";
7+
8+
//------------------------------------------------------------------------------
9+
// Requirements
10+
//------------------------------------------------------------------------------
11+
12+
const rule = require("../../../lib/rules/no-promise-executor-return");
13+
const { RuleTester } = require("../../../lib/rule-tester");
14+
15+
//------------------------------------------------------------------------------
16+
// Helpers
17+
//------------------------------------------------------------------------------
18+
19+
/**
20+
* Creates an error object.
21+
* @param {number} [column] Reported column.
22+
* @param {string} [type="ReturnStatement"] Reported node type.
23+
* @returns {Object} The error object.
24+
*/
25+
function error(column, type = "ReturnStatement") {
26+
const errorObject = {
27+
messageId: "returnsValue",
28+
type
29+
};
30+
31+
if (column) {
32+
errorObject.column = column;
33+
}
34+
35+
return errorObject;
36+
}
37+
38+
//------------------------------------------------------------------------------
39+
// Tests
40+
//------------------------------------------------------------------------------
41+
42+
const ruleTester = new RuleTester({ parserOptions: { ecmaVersion: 2015 }, env: { es6: true } });
43+
44+
ruleTester.run("no-promise-executor-return", rule, {
45+
valid: [
46+
47+
//------------------------------------------------------------------------------
48+
// General
49+
//------------------------------------------------------------------------------
50+
51+
// not a promise executor
52+
"function foo(resolve, reject) { return 1; }",
53+
"function Promise(resolve, reject) { return 1; }",
54+
"(function (resolve, reject) { return 1; })",
55+
"(function foo(resolve, reject) { return 1; })",
56+
"(function Promise(resolve, reject) { return 1; })",
57+
"var foo = function (resolve, reject) { return 1; }",
58+
"var foo = function Promise(resolve, reject) { return 1; }",
59+
"var Promise = function (resolve, reject) { return 1; }",
60+
"(resolve, reject) => { return 1; }",
61+
"(resolve, reject) => 1",
62+
"var foo = (resolve, reject) => { return 1; }",
63+
"var Promise = (resolve, reject) => { return 1; }",
64+
"var foo = (resolve, reject) => 1",
65+
"var Promise = (resolve, reject) => 1",
66+
"var foo = { bar(resolve, reject) { return 1; } }",
67+
"var foo = { Promise(resolve, reject) { return 1; } }",
68+
"new foo(function (resolve, reject) { return 1; });",
69+
"new foo(function bar(resolve, reject) { return 1; });",
70+
"new foo(function Promise(resolve, reject) { return 1; });",
71+
"new foo((resolve, reject) => { return 1; });",
72+
"new foo((resolve, reject) => 1);",
73+
"new promise(function foo(resolve, reject) { return 1; });",
74+
"new Promise.foo(function foo(resolve, reject) { return 1; });",
75+
"new foo.Promise(function foo(resolve, reject) { return 1; });",
76+
"new Promise.Promise(function foo(resolve, reject) { return 1; });",
77+
"new Promise()(function foo(resolve, reject) { return 1; });",
78+
79+
// not a promise executor - Promise() without new
80+
"Promise(function (resolve, reject) { return 1; });",
81+
"Promise((resolve, reject) => { return 1; });",
82+
"Promise((resolve, reject) => 1);",
83+
84+
// not a promise executor - not the first argument
85+
"new Promise(foo, function (resolve, reject) { return 1; });",
86+
"new Promise(foo, (resolve, reject) => { return 1; });",
87+
"new Promise(foo, (resolve, reject) => 1);",
88+
89+
// global Promise doesn't exist
90+
"/* globals Promise:off */ new Promise(function (resolve, reject) { return 1; });",
91+
{
92+
code: "new Promise((resolve, reject) => { return 1; });",
93+
globals: { Promise: "off" }
94+
},
95+
{
96+
code: "new Promise((resolve, reject) => 1);",
97+
env: { es6: false }
98+
},
99+
100+
// global Promise is shadowed
101+
"let Promise; new Promise(function (resolve, reject) { return 1; });",
102+
"function f() { new Promise((resolve, reject) => { return 1; }); var Promise; }",
103+
"function f(Promise) { new Promise((resolve, reject) => 1); }",
104+
"if (x) { const Promise = foo(); new Promise(function (resolve, reject) { return 1; }); }",
105+
"x = function Promise() { new Promise((resolve, reject) => { return 1; }); }",
106+
107+
// return without a value is allowed
108+
"new Promise(function (resolve, reject) { return; });",
109+
"new Promise(function (resolve, reject) { reject(new Error()); return; });",
110+
"new Promise(function (resolve, reject) { if (foo) { return; } });",
111+
"new Promise((resolve, reject) => { return; });",
112+
"new Promise((resolve, reject) => { if (foo) { resolve(1); return; } reject(new Error()); });",
113+
114+
// throw is allowed
115+
"new Promise(function (resolve, reject) { throw new Error(); });",
116+
"new Promise((resolve, reject) => { throw new Error(); });",
117+
118+
// not returning from the promise executor
119+
"new Promise(function (resolve, reject) { function foo() { return 1; } });",
120+
"new Promise((resolve, reject) => { (function foo() { return 1; })(); });",
121+
"new Promise(function (resolve, reject) { () => { return 1; } });",
122+
"new Promise((resolve, reject) => { () => 1 });",
123+
"function foo() { return new Promise(function (resolve, reject) { resolve(bar); }) };",
124+
"foo => new Promise((resolve, reject) => { bar(foo, (err, data) => { if (err) { reject(err); return; } resolve(data); })});",
125+
126+
// promise executors do not have effect on other functions (tests function info tracking)
127+
"new Promise(function (resolve, reject) {}); function foo() { return 1; }",
128+
"new Promise((resolve, reject) => {}); (function () { return 1; });",
129+
"new Promise(function (resolve, reject) {}); () => { return 1; };",
130+
"new Promise((resolve, reject) => {}); () => 1;",
131+
132+
// does not report global return
133+
{
134+
code: "return 1;",
135+
env: { node: true }
136+
},
137+
{
138+
code: "return 1;",
139+
parserOptions: { ecmaFeatures: { globalReturn: true } }
140+
},
141+
{
142+
code: "return 1; function foo(){ return 1; } return 1;",
143+
env: { node: true }
144+
},
145+
{
146+
code: "function foo(){} return 1; var bar = function*(){ return 1; }; return 1; var baz = () => {}; return 1;",
147+
env: { node: true }
148+
},
149+
{
150+
code: "new Promise(function (resolve, reject) {}); return 1;",
151+
env: { node: true }
152+
}
153+
],
154+
155+
invalid: [
156+
157+
// full error tests
158+
{
159+
code: "new Promise(function (resolve, reject) { return 1; })",
160+
errors: [{ message: "Return values from promise executor functions cannot be read.", type: "ReturnStatement", column: 42, endColumn: 51 }]
161+
},
162+
{
163+
code: "new Promise((resolve, reject) => resolve(1))",
164+
errors: [{ message: "Return values from promise executor functions cannot be read.", type: "CallExpression", column: 34, endColumn: 44 }]
165+
},
166+
167+
// other basic tests
168+
{
169+
code: "new Promise(function foo(resolve, reject) { return 1; })",
170+
errors: [error()]
171+
},
172+
{
173+
code: "new Promise((resolve, reject) => { return 1; })",
174+
errors: [error()]
175+
},
176+
177+
// any returned value
178+
{
179+
code: "new Promise(function (resolve, reject) { return undefined; })",
180+
errors: [error()]
181+
},
182+
{
183+
code: "new Promise((resolve, reject) => { return null; })",
184+
errors: [error()]
185+
},
186+
{
187+
code: "new Promise(function (resolve, reject) { return false; })",
188+
errors: [error()]
189+
},
190+
{
191+
code: "new Promise((resolve, reject) => resolve)",
192+
errors: [error(34, "Identifier")]
193+
},
194+
{
195+
code: "new Promise((resolve, reject) => null)",
196+
errors: [error(34, "Literal")]
197+
},
198+
{
199+
code: "new Promise(function (resolve, reject) { return resolve(foo); })",
200+
errors: [error()]
201+
},
202+
{
203+
code: "new Promise((resolve, reject) => { return reject(foo); })",
204+
errors: [error()]
205+
},
206+
{
207+
code: "new Promise((resolve, reject) => x + y)",
208+
errors: [error(34, "BinaryExpression")]
209+
},
210+
{
211+
code: "new Promise((resolve, reject) => { return Promise.resolve(42); })",
212+
errors: [error()]
213+
},
214+
215+
// any return statement location
216+
{
217+
code: "new Promise(function (resolve, reject) { if (foo) { return 1; } })",
218+
errors: [error()]
219+
},
220+
{
221+
code: "new Promise((resolve, reject) => { try { return 1; } catch(e) {} })",
222+
errors: [error()]
223+
},
224+
{
225+
code: "new Promise(function (resolve, reject) { while (foo){ if (bar) break; else return 1; } })",
226+
errors: [error()]
227+
},
228+
229+
// absence of arguments has no effect
230+
{
231+
code: "new Promise(function () { return 1; })",
232+
errors: [error()]
233+
},
234+
{
235+
code: "new Promise(() => { return 1; })",
236+
errors: [error()]
237+
},
238+
{
239+
code: "new Promise(() => 1)",
240+
errors: [error(19, "Literal")]
241+
},
242+
243+
// various scope tracking tests
244+
{
245+
code: "function foo() {} new Promise(function () { return 1; });",
246+
errors: [error(45)]
247+
},
248+
{
249+
code: "function foo() { return; } new Promise(() => { return 1; });",
250+
errors: [error(48)]
251+
},
252+
{
253+
code: "function foo() { return 1; } new Promise(() => { return 2; });",
254+
errors: [error(50)]
255+
},
256+
{
257+
code: "function foo () { return new Promise(function () { return 1; }); }",
258+
errors: [error(52)]
259+
},
260+
{
261+
code: "function foo() { return new Promise(() => { bar(() => { return 1; }); return false; }); }",
262+
errors: [error(71)]
263+
},
264+
{
265+
code: "() => new Promise(() => { if (foo) { return 0; } else bar(() => { return 1; }); })",
266+
errors: [error(38)]
267+
},
268+
{
269+
code: "function foo () { return 1; return new Promise(function () { return 2; }); return 3;}",
270+
errors: [error(62)]
271+
},
272+
{
273+
code: "() => 1; new Promise(() => { return 1; })",
274+
errors: [error(30)]
275+
},
276+
{
277+
code: "new Promise(function () { return 1; }); function foo() { return 1; } ",
278+
errors: [error(27)]
279+
},
280+
{
281+
code: "() => new Promise(() => { return 1; });",
282+
errors: [error(27)]
283+
},
284+
{
285+
code: "() => new Promise(() => 1);",
286+
errors: [error(25, "Literal")]
287+
},
288+
{
289+
code: "() => new Promise(() => () => 1);",
290+
errors: [error(25, "ArrowFunctionExpression")]
291+
},
292+
293+
// edge cases for global Promise reference
294+
{
295+
code: "new Promise((Promise) => { return 1; })",
296+
errors: [error()]
297+
},
298+
{
299+
code: "new Promise(function Promise(resolve, reject) { return 1; })",
300+
errors: [error()]
301+
}
302+
]
303+
});

‎tools/rule-types.json

+1
Original file line numberDiff line numberDiff line change
@@ -163,6 +163,7 @@
163163
"no-plusplus": "suggestion",
164164
"no-process-env": "suggestion",
165165
"no-process-exit": "suggestion",
166+
"no-promise-executor-return": "problem",
166167
"no-proto": "suggestion",
167168
"no-prototype-builtins": "problem",
168169
"no-redeclare": "suggestion",

2 commit comments

Comments
 (2)

berere commented on Jun 20, 2020

@berere
berere
on Jun 20, 2020

@lsegal

berere commented on Jun 20, 2020

@berere
berere
on Jun 20, 2020

@eventualbuddha

Please sign in to comment.