From 119c3d843fb93df896f6bee577487ac903e451fd Mon Sep 17 00:00:00 2001 From: ota-meshi Date: Fri, 30 Sep 2022 16:03:12 +0900 Subject: [PATCH 1/2] feat(always-return): add `ignoreLastCallback` option --- __tests__/always-return.js | 94 +++++++++++++++++- docs/rules/always-return.md | 56 ++++++++++- rules/always-return.js | 187 +++++++++++++++++++++++++++++------- 3 files changed, 298 insertions(+), 39 deletions(-) diff --git a/__tests__/always-return.js b/__tests__/always-return.js index df703e62..df319dc3 100644 --- a/__tests__/always-return.js +++ b/__tests__/always-return.js @@ -4,7 +4,7 @@ const rule = require('../rules/always-return') const RuleTester = require('eslint').RuleTester const ruleTester = new RuleTester({ parserOptions: { - ecmaVersion: 6, + ecmaVersion: 11, }, }) @@ -48,6 +48,59 @@ ruleTester.run('always-return', rule, { } return x })`, + { + code: 'hey.then(x => { console.log(x) })', + options: [{ ignoreLastCallback: true }], + }, + { + code: 'if(foo) { hey.then(x => { console.log(x) }) }', + options: [{ ignoreLastCallback: true }], + }, + { + code: 'void hey.then(x => { console.log(x) })', + options: [{ ignoreLastCallback: true }], + }, + { + code: ` + async function foo() { + await hey.then(x => { console.log(x) }) + }`, + options: [{ ignoreLastCallback: true }], + }, + { + code: `hey?.then(x => { console.log(x) })`, + options: [{ ignoreLastCallback: true }], + }, + { + code: `foo = (hey.then(x => { console.log(x) }), 42)`, + options: [{ ignoreLastCallback: true }], + }, + { + code: `(42, hey.then(x => { console.log(x) }))`, + options: [{ ignoreLastCallback: true }], + }, + { + code: ` + hey + .then(x => { console.log(x) }) + .catch(e => console.error(e))`, + options: [{ ignoreLastCallback: true }], + }, + { + code: ` + hey + .then(x => { console.log(x) }) + .catch(e => console.error(e)) + .finally(() => console.error('end'))`, + options: [{ ignoreLastCallback: true }], + }, + { + code: ` + hey + .then(x => { console.log(x) }) + .finally(() => console.error('end'))`, + options: [{ ignoreLastCallback: true }], + }, ], invalid: [ @@ -130,5 +183,44 @@ ruleTester.run('always-return', rule, { })`, errors: [{ message }], }, + { + code: ` + hey + .then(function(x) { console.log(x) /* missing return here */ }) + .then(function(y) { console.log(y) /* no error here */ })`, + options: [{ ignoreLastCallback: true }], + errors: [{ message, line: 3 }], + }, + { + code: `const foo = hey.then(function(x) {});`, + options: [{ ignoreLastCallback: true }], + errors: [{ message }], + }, + { + code: ` + function foo() { + return hey.then(function(x) {}); + }`, + options: [{ ignoreLastCallback: true }], + errors: [{ message }], + }, + { + code: ` + async function foo() { + return await hey.then(x => { console.log(x) }) + }`, + options: [{ ignoreLastCallback: true }], + errors: [{ message }], + }, + { + code: `const foo = hey?.then(x => { console.log(x) })`, + options: [{ ignoreLastCallback: true }], + errors: [{ message }], + }, + { + code: `const foo = (42, hey.then(x => { console.log(x) }))`, + options: [{ ignoreLastCallback: true }], + errors: [{ message }], + }, ], }) diff --git a/docs/rules/always-return.md b/docs/rules/always-return.md index 8a8a8eec..03ced2fa 100644 --- a/docs/rules/always-return.md +++ b/docs/rules/always-return.md @@ -10,10 +10,18 @@ as `return Promise.reject()`. #### Valid ```js -myPromise.then((val) => val * 2)); -myPromise.then(function(val) { return val * 2; }); -myPromise.then(doSomething); // could be either -myPromise.then((b) => { if (b) { return "yes" } else { return "no" } }); +myPromise.then((val) => val * 2) +myPromise.then(function (val) { + return val * 2 +}) +myPromise.then(doSomething) // could be either +myPromise.then((b) => { + if (b) { + return 'yes' + } else { + return 'no' + } +}) ``` #### Invalid @@ -31,3 +39,43 @@ myPromise.then((b) => { } }) ``` + +#### Options + +##### `ignoreLastCallback` + +You can pass an `{ ignoreLastCallback: true }` as an option to this rule to the +last `then()` callback in a promise chain does not warn if it does not have a +`return`. Default is `false`. + +```js +promise.then((x) => { + console.log(x) +}) // OK +void promise.then((x) => { + console.log(x) +}) // OK +await promise.then((x) => { + console.log(x) +}) // OK + +promise + .then((x) => { + console.log(x) + }) // NG + .then((x) => { + console.log(x) + }) // OK + +var v = promise.then((x) => { + console.log(x) +}) // NG +var v = await promise.then((x) => { + console.log(x) +}) // NG +function foo() { + return promise.then((x) => { + console.log(x) + }) // NG +} +``` diff --git a/rules/always-return.js b/rules/always-return.js index 4a4fbc7a..e5b3e71a 100644 --- a/rules/always-return.js +++ b/rules/always-return.js @@ -2,6 +2,20 @@ const getDocsUrl = require('./lib/get-docs-url') +/** + * @typedef {import('estree').Node} Node + * @typedef {import('estree').SimpleCallExpression} CallExpression + * @typedef {import('estree').FunctionExpression} FunctionExpression + * @typedef {import('estree').ArrowFunctionExpression} ArrowFunctionExpression + * @typedef {import('eslint').Rule.CodePath} CodePath + * @typedef {import('eslint').Rule.CodePathSegment} CodePathSegment + */ + +/** + * @typedef { (FunctionExpression | ArrowFunctionExpression) & { parent: CallExpression }} InlineThenFunctionExpression + */ + +/** @param {Node} node */ function isFunctionWithBlockStatement(node) { if (node.type === 'FunctionExpression') { return true @@ -12,28 +26,100 @@ function isFunctionWithBlockStatement(node) { return false } -function isThenCallExpression(node) { +/** + * @param {string} memberName + * @param {Node} node + * @returns {node is CallExpression} + */ +function isMemberCall(memberName, node) { return ( node.type === 'CallExpression' && node.callee.type === 'MemberExpression' && - node.callee.property.name === 'then' + !node.callee.computed && + node.callee.property.type === 'Identifier' && + node.callee.property.name === memberName ) } +/** @param {Node} node */ function isFirstArgument(node) { - return ( + return Boolean( node.parent && node.parent.arguments && node.parent.arguments[0] === node ) } +/** + * @param {Node} node + * @returns {node is InlineThenFunctionExpression} + */ function isInlineThenFunctionExpression(node) { return ( isFunctionWithBlockStatement(node) && - isThenCallExpression(node.parent) && + isMemberCall('then', node.parent) && isFirstArgument(node) ) } +/** + * Checks whether the given node is the last `then()` callback in a promise chain. + * @param {InlineThenFunctionExpression} node + */ +function isLastCallback(node) { + /** @type {Node} */ + let target = node.parent + /** @type {Node | undefined} */ + let parent = target.parent + while (parent) { + if (parent.type === 'ExpressionStatement') { + // e.g. { promise.then(() => value) } + return true + } + if (parent.type === 'UnaryExpression') { + // e.g. void promise.then(() => value) + return parent.operator === 'void' + } + /** @type {Node | null} */ + let nextTarget = null + if (parent.type === 'SequenceExpression') { + if (peek(parent.expressions) !== target) { + // e.g. (promise?.then(() => value), expr) + return true + } + nextTarget = parent + } else if ( + // e.g. promise?.then(() => value) + parent.type === 'ChainExpression' || + // e.g. await promise.then(() => value) + parent.type === 'AwaitExpression' + ) { + nextTarget = parent + } else if (parent.type === 'MemberExpression') { + if ( + parent.parent && + (isMemberCall('catch', parent.parent) || + isMemberCall('finally', parent.parent)) + ) { + // e.g. promise.then(() => value).catch(e => {}) + nextTarget = parent.parent + } + } + if (nextTarget) { + target = nextTarget + parent = target.parent + continue + } + return false + } + + // istanbul ignore next + return false +} + +/** + * @template T + * @param {T[]} arr + * @returns {T} + */ function peek(arr) { return arr[arr.length - 1] } @@ -44,38 +130,59 @@ module.exports = { docs: { url: getDocsUrl('always-return'), }, - schema: [], + schema: [ + { + type: 'object', + properties: { + ignoreLastCallback: { + type: 'boolean', + }, + }, + additionalProperties: false, + }, + ], }, create(context) { - // funcInfoStack is a stack representing the stack of currently executing - // functions - // funcInfoStack[i].branchIDStack is a stack representing the currently - // executing branches ("codePathSegment"s) within the given function - // funcInfoStack[i].branchInfoMap is an object representing information - // about all branches within the given function - // funcInfoStack[i].branchInfoMap[j].good is a boolean representing whether - // the given branch explicitly `return`s or `throw`s. It starts as `false` - // for every branch and is updated to `true` if a `return` or `throw` - // statement is found - // funcInfoStack[i].branchInfoMap[j].loc is a eslint SourceLocation object - // for the given branch - // example: - // funcInfoStack = [ { branchIDStack: [ 's1_1' ], - // branchInfoMap: - // { s1_1: - // { good: false, - // loc: } } }, - // { branchIDStack: ['s2_1', 's2_4'], - // branchInfoMap: - // { s2_1: - // { good: false, - // loc: }, - // s2_2: - // { good: true, - // loc: }, - // s2_4: - // { good: false, - // loc: } } } ] + const options = context.options[0] || {} + const ignoreLastCallback = !!options.ignoreLastCallback + /** + * @typedef {object} FuncInfo + * @property {string[]} branchIDStack This is a stack representing the currently + * executing branches ("codePathSegment"s) within the given function + * @property {Record} branchInfoMap This is an object representing information + * about all branches within the given function + * + * @typedef {object} BranchInfo + * @property {boolean} good This is a boolean representing whether + * the given branch explicitly `return`s or `throw`s. It starts as `false` + * for every branch and is updated to `true` if a `return` or `throw` + * statement is found + * @property {Node} node This is a estree Node object + * for the given branch + */ + + /** + * funcInfoStack is a stack representing the stack of currently executing + * functions + * example: + * funcInfoStack = [ { branchIDStack: [ 's1_1' ], + * branchInfoMap: + * { s1_1: + * { good: false, + * loc: } } }, + * { branchIDStack: ['s2_1', 's2_4'], + * branchInfoMap: + * { s2_1: + * { good: false, + * loc: }, + * s2_2: + * { good: true, + * loc: }, + * s2_4: + * { good: false, + * loc: } } } ] + * @type {FuncInfo[]} + */ const funcInfoStack = [] function markCurrentBranchAsGood() { @@ -91,6 +198,10 @@ module.exports = { 'ReturnStatement:exit': markCurrentBranchAsGood, 'ThrowStatement:exit': markCurrentBranchAsGood, + /** + * @param {CodePathSegment} segment + * @param {Node} node + */ onCodePathSegmentStart(segment, node) { const funcInfo = peek(funcInfoStack) funcInfo.branchIDStack.push(segment.id) @@ -109,6 +220,10 @@ module.exports = { }) }, + /** + * @param {CodePath} path + * @param {Node} node + */ onCodePathEnd(path, node) { const funcInfo = funcInfoStack.pop() @@ -116,6 +231,10 @@ module.exports = { return } + if (ignoreLastCallback && isLastCallback(node)) { + return + } + path.finalSegments.forEach((segment) => { const id = segment.id const branch = funcInfo.branchInfoMap[id] From 219ac16b5a1090e862e61fb96b5efbb5d30ccdb3 Mon Sep 17 00:00:00 2001 From: ota-meshi Date: Fri, 30 Sep 2022 16:07:37 +0900 Subject: [PATCH 2/2] docs: format --- docs/rules/always-return.md | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/docs/rules/always-return.md b/docs/rules/always-return.md index 03ced2fa..fa5fc901 100644 --- a/docs/rules/always-return.md +++ b/docs/rules/always-return.md @@ -49,33 +49,41 @@ last `then()` callback in a promise chain does not warn if it does not have a `return`. Default is `false`. ```js +// OK promise.then((x) => { console.log(x) -}) // OK +}) +// OK void promise.then((x) => { console.log(x) -}) // OK +}) +// OK await promise.then((x) => { console.log(x) -}) // OK +}) promise + // NG .then((x) => { console.log(x) - }) // NG + }) + // OK .then((x) => { console.log(x) - }) // OK + }) +// NG var v = promise.then((x) => { console.log(x) -}) // NG +}) +// NG var v = await promise.then((x) => { console.log(x) -}) // NG +}) function foo() { + // NG return promise.then((x) => { console.log(x) - }) // NG + }) } ```