From 28caa178a2ad1ea5fbdd1392108a5353062736d0 Mon Sep 17 00:00:00 2001 From: Andrew Huff Date: Wed, 5 Oct 2016 20:10:15 -0600 Subject: [PATCH 1/3] add code path analysis to always-return This will also change the reported location of errors to the inner function that failed to return, rather than referring to it's enclosing Promise I bumped up the the version number (2.0.1 -> 3.0.0) because the changed location reporting could un-suppress linter errors that have been suppressed with `// eslint-disable-line promise/always-return` fixes xjamundx/eslint-plugin-promise#8 fixes xjamundx/eslint-plugin-promise#18 fixes xjamundx/eslint-plugin-promise#24 --- README.md | 2 + package.json | 2 +- rules/always-return.js | 97 ++++++++++++++++++++++++++++++-------- test/always-return.test.js | 26 +++++++++- 4 files changed, 105 insertions(+), 22 deletions(-) diff --git a/README.md b/README.md index 1a77e33c..3629d4aa 100644 --- a/README.md +++ b/README.md @@ -61,6 +61,7 @@ We also allow someone to `throw` inside a `then()` which is essentially the same 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 @@ -68,6 +69,7 @@ myPromise.then(doSomething); // could be either ```js myPromise.then(function(val) {}); myPromise.then(() => { doSomething(); }); +myPromise.then((b) => { if (b) { return "yes" } else { forgotToReturn(); } }); ``` ### `param-names` diff --git a/package.json b/package.json index bc804177..601ee0d0 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "eslint-plugin-promise", - "version": "2.0.1", + "version": "3.0.0", "description": "Enforce best practices for JavaScript promises", "keywords": [ "eslint", diff --git a/rules/always-return.js b/rules/always-return.js index 8c7e6c7d..1fae69ac 100644 --- a/rules/always-return.js +++ b/rules/always-return.js @@ -8,31 +8,88 @@ function isFunctionWithBlockStatement (node) { return false } -function isReturnOrThrowStatement (node) { - return node.type === 'ReturnStatement' || node.type === 'ThrowStatement' +function isThenExpression (node) { + return ( + node.type === 'CallExpression' && + node.callee.type === 'MemberExpression' && + node.callee.property.name === 'then' + ) +} + +function isInlineThenFunctionExpression (node) { + return ( + isFunctionWithBlockStatement(node) && + isThenExpression(node.parent) + ) +} + +function includes (arr, val) { + return arr.indexOf(val) !== -1 +} + +function last (arr) { + return arr[arr.length - 1] } module.exports = { create: function (context) { - return { - MemberExpression: function (node) { - var firstArg, body, lastStatement - - if (node.property.name !== 'then' || node.parent.type !== 'CallExpression') { - return - } - - firstArg = node.parent.arguments[0] - if (!firstArg || !isFunctionWithBlockStatement(firstArg)) { - return - } - - body = firstArg.body.body - lastStatement = body[body.length - 1] - if (!lastStatement || !isReturnOrThrowStatement(lastStatement)) { - context.report(node, 'Each then() should return a value or throw') - } + var funcInfoStack = [] + var CPSIDStack = [] + + function isEveryBranchReturning (funcInfo) { + // We need to check noCurrentCPSIsOnTheCPSStack because of what + // seems like a bug in eslint where 'FunctionExpression:exit' events occur + // before all of their constituent codePathSegments have fired their + // 'onCodePathSegmentEnd' events + var currentIDs = funcInfo.codePath.currentSegments.map(x => x.id) + var noCurrentCPSIsOnTheCPSStack = !currentIDs.some((id) => includes(CPSIDStack, id)) + + var finalIDs = funcInfo.codePath.finalSegments.map(x => x.id) + var everyFinalCPSIsReturning = finalIDs.every((id) => includes(funcInfo.explicitlyReturningCPSIDs, id)) + + return noCurrentCPSIsOnTheCPSStack && everyFinalCPSIsReturning + } + + function onFunctionExpressionExit (node) { + if (!isInlineThenFunctionExpression(node)) { + return + } + + var funcInfo = last(funcInfoStack) + if (!isEveryBranchReturning(funcInfo)) { + context.report(node, 'Each then() should return a value or throw') } } + + function markCurrentCodePathSegmentAsReturning () { + var funcInfo = last(funcInfoStack) + var currentCPSID = last(CPSIDStack) + funcInfo.explicitlyReturningCPSIDs.push(currentCPSID) + } + + return { + onCodePathStart: function (codePath, node) { + funcInfoStack.push({ + codePath: codePath, + explicitlyReturningCPSIDs: [] + }) + }, + + onCodePathEnd: function (codePath, node) { + funcInfoStack.pop() + }, + + onCodePathSegmentEnd: function (segment, node) { + CPSIDStack.pop() + }, + onCodePathSegmentStart: function (segment, node) { + CPSIDStack.push(segment.id) + }, + + ReturnStatement: markCurrentCodePathSegmentAsReturning, + ThrowStatement: markCurrentCodePathSegmentAsReturning, + 'FunctionExpression:exit': onFunctionExpressionExit, + 'ArrowFunctionExpression:exit': onFunctionExpressionExit + } } } diff --git a/test/always-return.test.js b/test/always-return.test.js index c535274d..dcebd818 100644 --- a/test/always-return.test.js +++ b/test/always-return.test.js @@ -9,6 +9,7 @@ ruleTester.run('always-return', rule, { valid: [ { code: 'hey.then(x => x)', parserOptions: parserOptions }, { code: 'hey.then(x => ({}))', parserOptions: parserOptions }, + { code: 'hey.then(x => { return; })', parserOptions: parserOptions }, { code: 'hey.then(x => { return x * 10 })', parserOptions: parserOptions }, { code: 'hey.then(function() { return 42; })', parserOptions: parserOptions }, { code: 'hey.then(function() { return new Promise(); })', parserOptions: parserOptions }, @@ -19,7 +20,10 @@ ruleTester.run('always-return', rule, { { code: 'hey.then(function(x) { if (x) { return x; } throw new Error("no x"); })', parserOptions: parserOptions }, { code: 'hey.then(x => { throw new Error("msg"); })', parserOptions: parserOptions }, { code: 'hey.then(x => { if (!x) { throw new Error("no x"); } return x; })', parserOptions: parserOptions }, - { code: 'hey.then(x => { if (x) { return x; } throw new Error("no x"); })', parserOptions: parserOptions } + { code: 'hey.then(x => { if (x) { return x; } throw new Error("no x"); })', parserOptions: parserOptions }, + { code: 'hey.then(x => { var f = function() { }; return f; })', parserOptions: parserOptions }, + { code: 'hey.then(x => { if (x) { return x; } else { return x; } })', parserOptions: parserOptions }, + { code: 'hey.then(x => { return x; var y = "unreachable"; })', parserOptions: parserOptions } ], invalid: [ @@ -40,9 +44,29 @@ ruleTester.run('always-return', rule, { code: 'hey.then(function() { }).then(function() { })', errors: [ { message: message }, { message: message } ] }, + { + code: 'hey.then(function() { return; }).then(function() { })', + errors: [ { message: message } ] + }, { code: 'hey.then(function() { doSomethingWicked(); })', errors: [ { message: message } ] + }, + { + code: 'hey.then(function() { if (x) { return x; } })', + errors: [ { message: message } ] + }, + { + code: 'hey.then(function() { if (x) { return x; } else { }})', + errors: [ { message: message } ] + }, + { + code: 'hey.then(function() { if (x) { } else { return x; }})', + errors: [ { message: message } ] + }, + { + code: 'hey.then(function() { if (x) { return you.then(function() { return x; }); } })', + errors: [ { message: message } ] } ] }) From 5523a810bb8c62c6228fd157d779d61ed614448a Mon Sep 17 00:00:00 2001 From: Andrew Huff Date: Thu, 6 Oct 2016 23:11:34 -0600 Subject: [PATCH 2/3] refactor always-return Much cleaner now :D --- rules/always-return.js | 114 +++++++++++++++++++++++------------------ 1 file changed, 65 insertions(+), 49 deletions(-) diff --git a/rules/always-return.js b/rules/always-return.js index 1fae69ac..549634f1 100644 --- a/rules/always-return.js +++ b/rules/always-return.js @@ -23,73 +23,89 @@ function isInlineThenFunctionExpression (node) { ) } -function includes (arr, val) { - return arr.indexOf(val) !== -1 -} - -function last (arr) { +function peek (arr) { return arr[arr.length - 1] } module.exports = { create: function (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 explictly `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: } } } ] var funcInfoStack = [] - var CPSIDStack = [] - - function isEveryBranchReturning (funcInfo) { - // We need to check noCurrentCPSIsOnTheCPSStack because of what - // seems like a bug in eslint where 'FunctionExpression:exit' events occur - // before all of their constituent codePathSegments have fired their - // 'onCodePathSegmentEnd' events - var currentIDs = funcInfo.codePath.currentSegments.map(x => x.id) - var noCurrentCPSIsOnTheCPSStack = !currentIDs.some((id) => includes(CPSIDStack, id)) - var finalIDs = funcInfo.codePath.finalSegments.map(x => x.id) - var everyFinalCPSIsReturning = finalIDs.every((id) => includes(funcInfo.explicitlyReturningCPSIDs, id)) - - return noCurrentCPSIsOnTheCPSStack && everyFinalCPSIsReturning + function markCurrentBranchAsGood () { + var funcInfo = peek(funcInfoStack) + var currentBranchID = peek(funcInfo.branchIDStack) + funcInfo.branchInfoMap[currentBranchID].good = true } - function onFunctionExpressionExit (node) { - if (!isInlineThenFunctionExpression(node)) { - return - } + return { + ReturnStatement: markCurrentBranchAsGood, + ThrowStatement: markCurrentBranchAsGood, - var funcInfo = last(funcInfoStack) - if (!isEveryBranchReturning(funcInfo)) { - context.report(node, 'Each then() should return a value or throw') - } - } + onCodePathSegmentStart: function (segment, node) { + var funcInfo = peek(funcInfoStack) + funcInfo.branchIDStack.push(segment.id) + funcInfo.branchInfoMap[segment.id] = {good: false, loc: node.loc} + }, - function markCurrentCodePathSegmentAsReturning () { - var funcInfo = last(funcInfoStack) - var currentCPSID = last(CPSIDStack) - funcInfo.explicitlyReturningCPSIDs.push(currentCPSID) - } + onCodePathSegmentEnd: function (segment, node) { + var funcInfo = peek(funcInfoStack) + funcInfo.branchIDStack.pop() + }, - return { - onCodePathStart: function (codePath, node) { + onCodePathStart: function (path, node) { funcInfoStack.push({ - codePath: codePath, - explicitlyReturningCPSIDs: [] + branchIDStack: [], + branchInfoMap: {} }) }, - onCodePathEnd: function (codePath, node) { - funcInfoStack.pop() - }, + onCodePathEnd: function (path, node) { + var funcInfo = funcInfoStack.pop() - onCodePathSegmentEnd: function (segment, node) { - CPSIDStack.pop() - }, - onCodePathSegmentStart: function (segment, node) { - CPSIDStack.push(segment.id) - }, + if (!isInlineThenFunctionExpression(node)) { + return + } - ReturnStatement: markCurrentCodePathSegmentAsReturning, - ThrowStatement: markCurrentCodePathSegmentAsReturning, - 'FunctionExpression:exit': onFunctionExpressionExit, - 'ArrowFunctionExpression:exit': onFunctionExpressionExit + var finalBranchIDs = path.finalSegments.map(x => x.id) + finalBranchIDs.forEach((id) => { + var branch = funcInfo.branchInfoMap[id] + if (!branch.good) { + context.report({ + message: 'Each then() should return a value or throw', + loc: branch.loc + }) + } + }) + } } } } From 271e2399c4423e6586ac20efeabf7ec3cf72fb0d Mon Sep 17 00:00:00 2001 From: Andrew Huff Date: Fri, 7 Oct 2016 13:21:25 -0600 Subject: [PATCH 3/3] fix `hey.then(x => { return; }, err=>{ log(err); })` --- rules/always-return.js | 13 +++++++++++-- test/always-return.test.js | 3 ++- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/rules/always-return.js b/rules/always-return.js index 549634f1..cfc41646 100644 --- a/rules/always-return.js +++ b/rules/always-return.js @@ -8,7 +8,7 @@ function isFunctionWithBlockStatement (node) { return false } -function isThenExpression (node) { +function isThenCallExpression (node) { return ( node.type === 'CallExpression' && node.callee.type === 'MemberExpression' && @@ -16,10 +16,19 @@ function isThenExpression (node) { ) } +function isFirstArgument (node) { + return ( + node.parent && + node.parent.arguments && + node.parent.arguments[0] === node + ) +} + function isInlineThenFunctionExpression (node) { return ( isFunctionWithBlockStatement(node) && - isThenExpression(node.parent) + isThenCallExpression(node.parent) && + isFirstArgument(node) ) } diff --git a/test/always-return.test.js b/test/always-return.test.js index dcebd818..e89b281b 100644 --- a/test/always-return.test.js +++ b/test/always-return.test.js @@ -23,7 +23,8 @@ ruleTester.run('always-return', rule, { { code: 'hey.then(x => { if (x) { return x; } throw new Error("no x"); })', parserOptions: parserOptions }, { code: 'hey.then(x => { var f = function() { }; return f; })', parserOptions: parserOptions }, { code: 'hey.then(x => { if (x) { return x; } else { return x; } })', parserOptions: parserOptions }, - { code: 'hey.then(x => { return x; var y = "unreachable"; })', parserOptions: parserOptions } + { code: 'hey.then(x => { return x; var y = "unreachable"; })', parserOptions: parserOptions }, + { code: 'hey.then(x => { return; }, err=>{ log(err); })', parserOptions: parserOptions } ], invalid: [