Merge pull request #25 from ahuff44/always-return-squashed

add code path analysis to always-return

README.md

@@ -61,13 +61,15 @@ 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
 
 ```js
 myPromise.then(function(val) {});
 myPromise.then(() => { doSomething(); });
+myPromise.then((b) => { if (b) { return "yes" } else { forgotToReturn(); } });
 ```
 
 ### `param-names`

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",

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: <loc> } } },
+    //     { branchIDStack: ['s2_1', 's2_4'],
+    //       branchInfoMap:
+    //        { s2_1:
+    //           { good: false,
+    //             loc: <loc> },
+    //          s2_2:
+    //           { good: true,
+    //             loc: <loc> },
+    //          s2_4:
+    //           { good: false,
+    //             loc: <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
+            })
+          }
+        })
       }
     }
   }

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 } ]
     }
   ]
 })