Fix: Improve detection of fix functions that never return a fix in fixer-return rule (#143)

Behavior changes:
1. Catch fix functions that have a return statement but never actually return a fix (i.e. fix functions that only ever return undefined or similar values that are obviously not fixes).
2. Allow fix functions to have some code paths that do not return a fix (it's allowed for a fix function to bail out / return early and avoid fixing in some situations).

I would consider these both bug fixes to reduce false positives and false negatives.

Author: bmish

README.md

@@ -47,7 +47,7 @@ Then configure the rules you want to use under the rules section.
 Name | ✔️ | 🛠 | Description
 ----- | ----- | ----- | -----
 [consistent-output](https://github.com/not-an-aardvark/eslint-plugin-eslint-plugin/blob/master/docs/rules/consistent-output.md) |  |  | enforce consistent use of output assertions in rule tests
-[fixer-return](https://github.com/not-an-aardvark/eslint-plugin-eslint-plugin/blob/master/docs/rules/fixer-return.md) | ✔️ |  | require fixer function to always return a value.
+[fixer-return](https://github.com/not-an-aardvark/eslint-plugin-eslint-plugin/blob/master/docs/rules/fixer-return.md) | ✔️ |  | require fixer function to return a fix
 [meta-property-ordering](https://github.com/not-an-aardvark/eslint-plugin-eslint-plugin/blob/master/docs/rules/meta-property-ordering.md) |  | 🛠 | enforce the order of meta properties
 [no-deprecated-context-methods](https://github.com/not-an-aardvark/eslint-plugin-eslint-plugin/blob/master/docs/rules/no-deprecated-context-methods.md) |  | 🛠 | disallow usage of deprecated methods on rule context objects
 [no-deprecated-report-api](https://github.com/not-an-aardvark/eslint-plugin-eslint-plugin/blob/master/docs/rules/no-deprecated-report-api.md) | ✔️ | 🛠 | disallow use of the deprecated context.report() API

docs/rules/fixer-return.md

@@ -1,12 +1,12 @@
-# Enforces always return from a fixer function (fixer-return)
+# Require fixer function to return a fix (fixer-return)
 
 ✔️ The `"extends": "plugin:eslint-plugin/recommended"` property in a configuration file enables this rule.
 
-In a fixable rule, missing return from a fixer function will not apply fixes.
+In a [fixable](https://eslint.org/docs/developer-guide/working-with-rules#applying-fixes) rule, a fixer function is useless if it never returns anything.
 
 ## Rule Details
 
-This rule enforces that fixer functions always return a value.
+This rule enforces that a fixer function returns a fix in at least one situation.
 
 Examples of **incorrect** code for this rule:
 
@@ -17,7 +17,7 @@ module.exports = {
   create (context) {
     context.report({
       fix (fixer) {
-        fixer.foo();
+        fixer.insertTextAfter(node, 'foo');
       },
     });
   },
@@ -33,13 +33,26 @@ module.exports = {
   create (context) {
     context.report({
       fix (fixer) {
-        return fixer.foo();
+        return fixer.insertTextAfter(node, 'foo');
       },
     });
   },
 };
 ```
 
-## When Not To Use It
+```js
+/* eslint eslint-plugin/fixer-return: error */
 
-If you don't want to enforce always return from a fixer function, do not enable this rule.
+module.exports = {
+  create (context) {
+    context.report({
+      fix (fixer) {
+        if (foo) {
+          return; // no autofix in this situation
+        }
+        return fixer.insertTextAfter(node, 'foo');
+      },
+    });
+  },
+};
+```

lib/rules/fixer-return.js

@@ -1,5 +1,5 @@
 /**
- * @fileoverview Enforces always return from a fixer function
+ * @fileoverview Require fixer function to return a fix
  * @author 薛定谔的猫<[email protected]>
  */
 
@@ -10,6 +10,7 @@
 // ------------------------------------------------------------------------------
 
 const utils = require('../utils');
+const { getStaticValue } = require('eslint-utils');
 
 // ------------------------------------------------------------------------------
 // Rule Definition
@@ -19,50 +20,79 @@ module.exports = {
   meta: {
     type: 'problem',
     docs: {
-      description: 'require fixer function to always return a value.',
+      description: 'require fixer function to return a fix',
       category: 'Possible Errors',
       recommended: true,
     },
     fixable: null,
     schema: [],
+    messages: {
+      missingFix: 'Fixer function never returned a fix.',
+    },
   },
 
   create (context) {
-    const message = 'Expected fixer function to always return a value.';
     let funcInfo = {
       upper: null,
       codePath: null,
-      hasReturn: false,
-      hasYield: false,
+      hasReturnWithFixer: false,
+      hasYieldWithFixer: false,
       shouldCheck: false,
       node: null,
     };
     let contextIdentifiers;
 
     /**
-     * Checks whether or not the last code path segment is reachable.
-     * Then reports this function if the segment is reachable.
-     *
-     * If the last code path segment is reachable, there are paths which are not
-     * returned or thrown.
+     * As we exit the fix() function, ensure we have returned or yielded a real fix by this point.
+     * If not, report the function as a violation.
      *
      * @param {ASTNode} node - A node to check.
      * @returns {void}
      */
     function checkLastSegment (node) {
       if (
         funcInfo.shouldCheck &&
-        funcInfo.codePath.currentSegments.some(segment => segment.reachable) &&
-        (!node.generator || !funcInfo.hasYield)
+        (
+          (node.generator && !funcInfo.hasYieldWithFixer) || // Generator function never yielded a fix
+          (!node.generator && !funcInfo.hasReturnWithFixer) // Non-generator function never returned a fix
+        )
       ) {
         context.report({
           node,
           loc: (node.id || node).loc.start,
-          message,
+          messageId: 'missingFix',
         });
       }
     }
 
+    /**
+     * Check if a returned/yielded node is likely to be a fix or not.
+     * A fix is an object created by fixer.replaceText() for example and returned by the fix function.
+     * @param {ASTNode} node - node to check
+     * @param {Context} context
+     * @returns {boolean}
+     */
+    function isFix (node) {
+      if (node.type === 'ArrayExpression' && node.elements.length === 0) {
+        // An empty array is not a fix.
+        return false;
+      }
+
+      const staticValue = getStaticValue(node, context.getScope());
+      if (!staticValue) {
+        // If we can't find a static value, assume it's a real fix value.
+        return true;
+      }
+
+      if (Array.isArray(staticValue.value)) {
+        // If the static value is an array, then consider it a fix since fixes could have been added to it after creation.
+        return true;
+      }
+
+      // Any other static values (booleans, numbers, etc) are not fixes.
+      return false;
+    }
+
     return {
       Program (node) {
         contextIdentifiers = utils.getContextIdentifiers(context, node);
@@ -71,6 +101,8 @@ module.exports = {
       // Stacks this function's information.
       onCodePathStart (codePath, node) {
         const parent = node.parent;
+
+        // Whether we are inside the fixer function we care about.
         const shouldCheck = node.type === 'FunctionExpression' &&
           parent.parent.type === 'ObjectExpression' &&
           parent.parent.parent.type === 'CallExpression' &&
@@ -81,8 +113,8 @@ module.exports = {
         funcInfo = {
           upper: funcInfo,
           codePath,
-          hasYield: false,
-          hasReturn: false,
+          hasYieldWithFixer: false,
+          hasReturnWithFixer: false,
           shouldCheck,
           node,
         };
@@ -94,27 +126,20 @@ module.exports = {
       },
 
       // Yield in generators
-      YieldExpression () {
-        if (funcInfo.shouldCheck) {
-          funcInfo.hasYield = true;
+      YieldExpression (node) {
+        if (funcInfo.shouldCheck && node.argument && isFix(node.argument)) {
+          funcInfo.hasYieldWithFixer = true;
         }
       },
 
       // Checks the return statement is valid.
       ReturnStatement (node) {
-        if (funcInfo.shouldCheck) {
-          funcInfo.hasReturn = true;
-
-          if (!node.argument) {
-            context.report({
-              node,
-              message,
-            });
-          }
+        if (funcInfo.shouldCheck && node.argument && isFix(node.argument)) {
+          funcInfo.hasReturnWithFixer = true;
         }
       },
 
-      // Reports a given function if the last path is reachable.
+      // Ensure the current fixer function returned or yielded a fix.
       'FunctionExpression:exit': checkLastSegment,
     };
   },