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..cfc41646 100644 --- a/rules/always-return.js +++ b/rules/always-return.js @@ -8,30 +8,112 @@ function isFunctionWithBlockStatement (node) { return false } -function isReturnOrThrowStatement (node) { - return node.type === 'ReturnStatement' || node.type === 'ThrowStatement' +function isThenCallExpression (node) { + return ( + node.type === 'CallExpression' && + node.callee.type === 'MemberExpression' && + node.callee.property.name === 'then' + ) +} + +function isFirstArgument (node) { + return ( + node.parent && + node.parent.arguments && + node.parent.arguments[0] === node + ) +} + +function isInlineThenFunctionExpression (node) { + return ( + isFunctionWithBlockStatement(node) && + isThenCallExpression(node.parent) && + isFirstArgument(node) + ) +} + +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 = [] + + function markCurrentBranchAsGood () { + var funcInfo = peek(funcInfoStack) + var currentBranchID = peek(funcInfo.branchIDStack) + funcInfo.branchInfoMap[currentBranchID].good = true + } + return { - MemberExpression: function (node) { - var firstArg, body, lastStatement + ReturnStatement: markCurrentBranchAsGood, + ThrowStatement: markCurrentBranchAsGood, - if (node.property.name !== 'then' || node.parent.type !== 'CallExpression') { - return - } + onCodePathSegmentStart: function (segment, node) { + var funcInfo = peek(funcInfoStack) + funcInfo.branchIDStack.push(segment.id) + funcInfo.branchInfoMap[segment.id] = {good: false, loc: node.loc} + }, + + onCodePathSegmentEnd: function (segment, node) { + var funcInfo = peek(funcInfoStack) + funcInfo.branchIDStack.pop() + }, - firstArg = node.parent.arguments[0] - if (!firstArg || !isFunctionWithBlockStatement(firstArg)) { + onCodePathStart: function (path, node) { + funcInfoStack.push({ + branchIDStack: [], + branchInfoMap: {} + }) + }, + + onCodePathEnd: function (path, node) { + var funcInfo = funcInfoStack.pop() + + if (!isInlineThenFunctionExpression(node)) { 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 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 + }) + } + }) } } } diff --git a/test/always-return.test.js b/test/always-return.test.js index c535274d..e89b281b 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,11 @@ 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 }, + { code: 'hey.then(x => { return; }, err=>{ log(err); })', parserOptions: parserOptions } ], invalid: [ @@ -40,9 +45,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 } ] } ] })