feat(rule): add no-identity-handlers rule

Resolves #12

README.md

@@ -58,6 +58,7 @@ Then configure the rules you want to use under the rules section.
     "promise/catch-or-return": "error",
     "promise/no-native": "off",
     "promise/no-nesting": "warn",
+    "promise/no-identity-handlers": "warn",
     "promise/no-promise-in-callback": "warn",
     "promise/no-callback-in-promise": "warn",
     "promise/avoid-new": "warn",
@@ -84,6 +85,7 @@ or start with the recommended rule set
 | [`always-return`][always-return]                         | Return inside each `then()` to create readable and reusable Promise chains.      | :bangbang:  |          |
 | [`no-native`][no-native]                                 | In an ES5 environment, make sure to create a `Promise` constructor before using. |             |          |
 | [`no-nesting`][no-nesting]                               | Avoid nested `then()` or `catch()` statements                                    | :warning:   |          |
+| [`no-identity-handlers`][no-identity-handlers]           | Avoid unnecessary identity functions in `then()` or `catch()`                    | :warning:   |          |
 | [`no-promise-in-callback`][no-promise-in-callback]       | Avoid using promises inside of callbacks                                         | :warning:   |          |
 | [`no-callback-in-promise`][no-callback-in-promise]       | Avoid calling `cb()` inside of a `then()` (use [nodeify][] instead)              | :warning:   |          |
 | [`avoid-new` ][avoid-new]                                | Avoid creating `new` promises outside of utility libs (use [pify][] instead)     | :warning:   |          |
@@ -116,6 +118,7 @@ or start with the recommended rule set
 [always-return]: docs/rules/always-return.md
 [no-native]: docs/rules/no-native.md
 [no-nesting]: docs/rules/no-nesting.md
+[no-identity-handlers]: docs/rules/no-identity-handlers.md
 [no-promise-in-callback]: docs/rules/no-promise-in-callback.md
 [no-callback-in-promise]: docs/rules/no-callback-in-promise.md
 [avoid-new]: docs/rules/avoid-new.md

docs/rules/no-identity-handlers.md

@@ -0,0 +1 @@
+# Avoid unnecessary identity functions in `then()` or `catch()` (no-identity-handlers)

index.js

@@ -12,6 +12,7 @@ module.exports = {
     'no-callback-in-promise': require('./rules/no-callback-in-promise'),
     'no-promise-in-callback': require('./rules/no-promise-in-callback'),
     'no-nesting': require('./rules/no-nesting'),
+    'no-identity-handlers': require('./rules/no-identity-handlers'),
     'avoid-new': require('./rules/avoid-new'),
     'no-return-in-finally': require('./rules/no-return-in-finally')
   },
@@ -31,6 +32,7 @@ module.exports = {
         'promise/catch-or-return': 'error',
         'promise/no-native': 'off',
         'promise/no-nesting': 'warn',
+        'promise/no-identity-handlers': 'warn',
         'promise/no-promise-in-callback': 'warn',
         'promise/no-callback-in-promise': 'warn',
         'promise/avoid-new': 'warn',

rules/no-identity-handlers.js

@@ -0,0 +1,143 @@
+'use strict'
+
+const getDocsUrl = require('./lib/get-docs-url')
+const isPromise = require('./lib/is-promise')
+
+const checkObjectPatternIdentity = createIdentityCheck({
+  type: 'ObjectExpression',
+  areAllIdentifiers: expression => {
+    return expression.properties.every(
+      property => property.key.type === 'Identifier'
+    )
+  },
+  toSetOfNames: node => {
+    return new Set(node.properties.map(property => property.key.name))
+  }
+})
+
+const checkArrayPatternIdentity = createIdentityCheck({
+  type: 'ArrayExpression',
+  areAllIdentifiers: expression => {
+    return expression.elements.every(element => element.type === 'Identifier')
+  },
+  toSetOfNames: node => {
+    return new Set(node.elements.map(element => element.name))
+  }
+})
+
+function createIdentityCheck(config) {
+  function difference(setA, setB) {
+    return new Set(Array.from(setA).filter(x => !setB.has(x)))
+  }
+
+  function containsSameIdentifierNames(expressionA, expressionB) {
+    const setA = config.toSetOfNames(expressionA)
+    const setB = config.toSetOfNames(expressionB)
+    return setA.size === setB.size && difference(setA, setB).size === 0
+  }
+
+  function checkIdentity(firstParam, body) {
+    return (
+      config.areAllIdentifiers(body) &&
+      containsSameIdentifierNames(firstParam, body)
+    )
+  }
+
+  return (firstParam, body) => {
+    switch (body.type) {
+      case config.type:
+        return checkIdentity(firstParam, body)
+      case 'BlockStatement':
+        const firstBodyStatement = body.body[0]
+        return firstBodyStatement.type === 'ReturnStatement' &&
+          firstBodyStatement.argument.type === config.type
+          ? checkIdentity(firstParam, firstBodyStatement.argument)
+          : false
+      default:
+        return false
+    }
+  }
+}
+
+function isFunctionExpression(node) {
+  return (
+    node.type === 'FunctionExpression' ||
+    node.type === 'ArrowFunctionExpression'
+  )
+}
+
+function getFirstParamName(node) {
+  const firstParam = node.params[0]
+  return firstParam && firstParam.type === 'Identifier' ? firstParam.name : null
+}
+
+function getBodyValueName(node) {
+  const body = node.body || {}
+  if (body.type === 'Identifier') {
+    return body.name
+  }
+  if (body.type === 'BlockStatement') {
+    const firstBodyStatement = body.body[0] || { type: '', argument: {} }
+    return (firstBodyStatement.type === 'ReturnStatement' ||
+      firstBodyStatement.type === 'ThrowStatement') &&
+      firstBodyStatement.argument.type === 'Identifier'
+      ? firstBodyStatement.argument.name
+      : null
+  }
+  return null
+}
+
+function isIdentityFunction(node) {
+  if (node.params.length === 1) {
+    const firstParam = node.params[0]
+    switch (firstParam.type) {
+      case 'Identifier':
+        return getFirstParamName(node) === getBodyValueName(node)
+      case 'ObjectPattern':
+        return checkObjectPatternIdentity(firstParam, node.body)
+      case 'ArrayPattern':
+        return checkArrayPatternIdentity(firstParam, node.body)
+      default:
+        return false
+    }
+  }
+  return false
+}
+
+module.exports = {
+  meta: {
+    docs: {
+      url: getDocsUrl('no-identity-handlers')
+    }
+  },
+  create(context) {
+    function checkIdentity(node) {
+      if (node && isFunctionExpression(node) && isIdentityFunction(node)) {
+        context.report({
+          node,
+          message: 'No identity handlers'
+        })
+      }
+    }
+
+    return {
+      CallExpression(node) {
+        if (!isPromise(node)) {
+          return
+        }
+
+        switch (node.callee.property.name) {
+          case 'then':
+            checkIdentity(node.arguments[0])
+            checkIdentity(node.arguments[1])
+            break
+          case 'catch':
+            checkIdentity(node.arguments[0])
+            break
+          default:
+            break
+        }
+      }
+    }
+  }
+}

test/no-identity-handlers.js

@@ -0,0 +1,79 @@
+'use strict'
+
+const rule = require('../rules/no-identity-handlers')
+const RuleTester = require('eslint').RuleTester
+const ruleTester = new RuleTester({
+  parserOptions: {
+    ecmaVersion: 6
+  }
+})
+
+ruleTester.run('no-identity-handlers', rule, {
+  valid: [
+    'Promise.resolve(2).then(value => value * 2)',
+    'Promise.resolve(2).then(value => { return value + 1 })',
+    'Promise.resolve(2).then(() => null)',
+    'Promise.resolve(2).then(() => doSomethingTotallyDifferent())',
+    'somePromise().then(value => doSomethingWith(value))',
+    'somePromise().then(handler)',
+    'Promise.reject(Error()).catch(err => { console.error(err); throw err; })',
+    'Promise.reject(Error()).catch(err => { throw doSomethingTo(err) })',
+    'somePromise().catch(handler)',
+    'somePromise().catch(createHandler())',
+    'somePromise().then(value => value * 2, err => { console.error(err); throw err; })',
+    'somePromise().then(func, func)',
+    'getObject().then(({ a, b, c }) => ({ a, b, c: c, d: calculate(a, b, c) }))',
+    'getObject().then(({ a: rename, b, c }) => ({ rename, b, c }))',
+    'getArray().then(([a, b]) => [a, b, ...getSomeOtherArray()])',
+
+    // edge cases that aren't really valid but shouldn't throw or report
+    'Promise.resolve(2).then()',
+    'Promise.reject(Error()).catch()'
+  ],
+  invalid: [
+    {
+      code: 'Promise.resolve(2).then(_ => _)',
+      errors: [{ message: 'No identity handlers' }]
+    },
+    {
+      code: 'Promise.resolve(2).then(val => { return val })',
+      errors: [{ message: 'No identity handlers' }]
+    },
+    {
+      code: 'Promise.resolve(2).then(function (value) { return value })',
+      errors: [{ message: 'No identity handlers' }]
+    },
+    {
+      code: 'getObject().then(({ a, b, c }) => ({ a, b, c }))',
+      errors: [{ message: 'No identity handlers' }]
+    },
+    {
+      code: 'getObject().then(({ a, b, c }) => { return { a, b, c } })',
+      errors: [{ message: 'No identity handlers' }]
+    },
+    {
+      code: 'getArray().then(([a, b]) => [a, b])',
+      errors: [{ message: 'No identity handlers' }]
+    },
+    {
+      code: 'getArray().then(([a, b]) => { return [a, b] })',
+      errors: [{ message: 'No identity handlers' }]
+    },
+    {
+      code: 'Promise.reject(Error()).catch(err => { throw err })',
+      errors: [{ message: 'No identity handlers' }]
+    },
+    {
+      code: 'Promise.reject(Error()).catch(function (e) { throw e })',
+      errors: [{ message: 'No identity handlers' }]
+    },
+    {
+      code: 'Promise.reject(Error()).then(null, error => { throw error })',
+      errors: [{ message: 'No identity handlers' }]
+    },
+    {
+      code: 'Promise.reject(Error()).then(null, function (e) { throw e })',
+      errors: [{ message: 'No identity handlers' }]
+    }
+  ]
+})