Update: support multiple fixes in a report (fixes #7348)

docs/developer-guide/working-with-rules.md

@@ -198,6 +198,15 @@ The `fixer` object has the following methods:
 * `replaceText(nodeOrToken, text)` - replaces the text in the given node or token
 * `replaceTextRange(range, text)` - replaces the text in the given range
 
+The above methods return a `fixing` object.
+The `fix()` function can return the following values:
+
+* A `fixing` object.
+* An array which includes `fixing` objects.
+* An iterable object which enumerates `fixing` objects. Especially, the `fix()` function can be a generator.
+
+If you make a `fix()` function which returns multiple `fixing` objects, those `fixing` objects must not be overlapped.
+
 Best practices for fixes:
 
 1. Avoid any fixes that could change the runtime behavior of code and cause it to stop working.
@@ -286,6 +295,7 @@ Once you have an instance of `SourceCode`, you can use the methods on it to work
 * `getNodeByRangeIndex(index)` - returns the deepest node in the AST containing the given source index.
 * `getLocFromIndex(index)` - returns an object with `line` and `column` properties, corresponding to the location of the given source index. `line` is 1-based and `column` is 0-based.
 * `getIndexFromLoc(loc)` - returns the index of a given location in the source code, where `loc` is an object with a 1-based `line` key and a 0-based `column` key.
+* `commentsExistBetween(nodeOrToken1, nodeOrToken2)` - returns `true` if comments exist between two nodes.
 
 > `skipOptions` is an object which has 3 properties; `skip`, `includeComments`, and `filter`. Default is `{skip: 0, includeComments: false, filter: null}`.
 > - `skip` is a positive integer, the number of skipping tokens. If `filter` option is given at the same time, it doesn't count filtered tokens as skipped.

lib/rule-context.js

@@ -8,6 +8,7 @@
 // Requirements
 //------------------------------------------------------------------------------
 
+const assert = require("assert");
 const ruleFixer = require("./util/rule-fixer");
 
 //------------------------------------------------------------------------------
@@ -60,6 +61,75 @@ const PASSTHROUGHS = [
 // Rule Definition
 //------------------------------------------------------------------------------
 
+/**
+ * Compares items in a fixes array by range.
+ * @param {Fix} a The first message.
+ * @param {Fix} b The second message.
+ * @returns {int} -1 if a comes before b, 1 if a comes after b, 0 if equal.
+ * @private
+ */
+function compareFixesByRange(a, b) {
+    return a.range[0] - b.range[0] || a.range[1] - b.range[1];
+}
+
+/**
+ * Merges the given fixes array into one.
+ * @param {Fix[]} fixes The fixes to merge.
+ * @param {SourceCode} sourceCode The source code object to get the text between fixes.
+ * @returns {void}
+ */
+function mergeFixes(fixes, sourceCode) {
+    if (fixes.length === 0) {
+        return null;
+    }
+    if (fixes.length === 1) {
+        return fixes[0];
+    }
+
+    fixes.sort(compareFixesByRange);
+
+    const originalText = sourceCode.text;
+    const start = fixes[0].range[0];
+    const end = fixes[fixes.length - 1].range[1];
+    let text = "";
+    let lastPos = Number.MIN_SAFE_INTEGER;
+
+    for (const fix of fixes) {
+        assert(fix.range[0] >= lastPos, "Fix objects must not be overlapped in a report.");
+
+        if (fix.range[0] >= 0) {
+            text += originalText.slice(Math.max(0, start, lastPos), fix.range[0]);
+        }
+        text += fix.text;
+        lastPos = fix.range[1];
+    }
+    text += originalText.slice(Math.max(0, start, lastPos), end);
+
+    return { range: [start, end], text };
+}
+
+/**
+ * Gets one fix object from the given descriptor.
+ * If the descriptor retrieves multiple fixes, this merges those to one.
+ * @param {Object} descriptor The report descriptor.
+ * @param {SourceCode} sourceCode The source code object to get text between fixes.
+ * @returns {Fix} The got fix object.
+ */
+function getFix(descriptor, sourceCode) {
+    if (typeof descriptor.fix !== "function") {
+        return null;
+    }
+
+    // @type {null | Fix | Fix[] | IterableIterator<Fix>}
+    const fix = descriptor.fix(ruleFixer);
+
+    // Merge to one.
+    if (fix && Symbol.iterator in fix) {
+        return mergeFixes(Array.from(fix), sourceCode);
+    }
+    return fix;
+}
+
 /**
  * Rule context class
  * Acts as an abstraction layer between rules and the main eslint object.
@@ -120,12 +190,7 @@ class RuleContext {
         // check to see if it's a new style call
         if (arguments.length === 1) {
             const descriptor = nodeOrDescriptor;
-            let fix = null;
-
-            // if there's a fix specified, get it
-            if (typeof descriptor.fix === "function") {
-                fix = descriptor.fix(ruleFixer);
-            }
+            const fix = getFix(descriptor, this.getSourceCode());
 
             this.eslint.report(
                 this.id,

lib/rules/arrow-body-style.js

@@ -65,6 +65,29 @@ module.exports = {
         const requireReturnForObjectLiteral = options[1] && options[1].requireReturnForObjectLiteral;
         const sourceCode = context.getSourceCode();
 
+        /**
+         * Checks whether the given node has ASI problem or not.
+         * @param {Token} token The token to check.
+         * @returns {boolean} `true` if it changes semantics if `;` or `}` followed by the token are removed.
+         */
+        function hasASIProblem(token) {
+            return token && token.type === "Punctuator" && /^[([/`+-]/.test(token.value);
+        }
+
+        /**
+         * Gets the closing parenthesis which is the pair of the given opening parenthesis.
+         * @param {Token} token The opening parenthesis token to get.
+         * @returns {Token} The found closing parenthesis token.
+         */
+        function findClosingParen(token) {
+            let node = sourceCode.getNodeByRangeIndex(token.range[1]);
+
+            while (!astUtils.isParenthesised(sourceCode, node)) {
+                node = node.parent;
+            }
+            return sourceCode.getTokenAfter(node);
+        }
+
         /**
          * Determines whether a arrow function body needs braces
          * @param {ASTNode} node The arrow function node.
@@ -91,47 +114,55 @@ module.exports = {
                         loc: arrowBody.loc.start,
                         message: "Unexpected block statement surrounding arrow body.",
                         fix(fixer) {
-                            if (blockBody.length !== 1 || blockBody[0].type !== "ReturnStatement" || !blockBody[0].argument) {
-                                return null;
+                            const fixes = [];
+
+                            if (blockBody.length !== 1 ||
+                                blockBody[0].type !== "ReturnStatement" ||
+                                !blockBody[0].argument ||
+                                hasASIProblem(sourceCode.getTokenAfter(arrowBody))
+                            ) {
+                                return fixes;
                             }
 
-                            const sourceText = sourceCode.getText();
-                            const returnKeyword = sourceCode.getFirstToken(blockBody[0]);
-                            const firstValueToken = sourceCode.getTokenAfter(returnKeyword);
-                            let lastValueToken = sourceCode.getLastToken(blockBody[0]);
-
-                            if (astUtils.isSemicolonToken(lastValueToken)) {
-
-                                /* The last token of the returned value is the last token of the ReturnExpression (if
-                                 * the ReturnExpression has no semicolon), or the second-to-last token (if the ReturnExpression
-                                 * has a semicolon).
-                                 */
-                                lastValueToken = sourceCode.getTokenBefore(lastValueToken);
+                            const openingBrace = sourceCode.getFirstToken(arrowBody);
+                            const closingBrace = sourceCode.getLastToken(arrowBody);
+                            const firstValueToken = sourceCode.getFirstToken(blockBody[0], 1);
+                            const lastValueToken = sourceCode.getLastToken(blockBody[0]);
+                            const commentsExist =
+                                sourceCode.commentsExistBetween(openingBrace, firstValueToken) ||
+                                sourceCode.commentsExistBetween(lastValueToken, closingBrace);
+
+                            // Remove tokens around the return value.
+                            // If comments don't exist, remove extra spaces as well.
+                            if (commentsExist) {
+                                fixes.push(
+                                    fixer.remove(openingBrace),
+                                    fixer.remove(closingBrace),
+                                    fixer.remove(sourceCode.getTokenAfter(openingBrace)) // return keyword
+                                );
+                            } else {
+                                fixes.push(
+                                    fixer.removeRange([openingBrace.range[0], firstValueToken.range[0]]),
+                                    fixer.removeRange([lastValueToken.range[1], closingBrace.range[1]])
+                                );
                             }
 
-                            const tokenAfterArrowBody = sourceCode.getTokenAfter(arrowBody);
-
-                            if (tokenAfterArrowBody && tokenAfterArrowBody.type === "Punctuator" && /^[([/`+-]/.test(tokenAfterArrowBody.value)) {
+                            // If the first token of the reutrn value is `{`,
+                            // enclose the return value by parentheses to avoid syntax error.
+                            if (astUtils.isOpeningBraceToken(firstValueToken)) {
+                                fixes.push(
+                                    fixer.insertTextBefore(firstValueToken, "("),
+                                    fixer.insertTextAfter(lastValueToken, ")")
+                                );
+                            }
 
-                                // Don't do a fix if the next token would cause ASI issues when preceded by the returned value.
-                                return null;
+                            // If the last token of the return statement is semicolon, remove it.
+                            // Non-block arrow body is an expression, not a statement.
+                            if (astUtils.isSemicolonToken(lastValueToken)) {
+                                fixes.push(fixer.remove(lastValueToken));
                             }
 
-                            const textBeforeReturn = sourceText.slice(arrowBody.range[0] + 1, returnKeyword.range[0]);
-                            const textBetweenReturnAndValue = sourceText.slice(returnKeyword.range[1], firstValueToken.range[0]);
-                            const rawReturnValueText = sourceText.slice(firstValueToken.range[0], lastValueToken.range[1]);
-                            const returnValueText = astUtils.isOpeningBraceToken(firstValueToken) ? `(${rawReturnValueText})` : rawReturnValueText;
-                            const textAfterValue = sourceText.slice(lastValueToken.range[1], blockBody[0].range[1] - 1);
-                            const textAfterReturnStatement = sourceText.slice(blockBody[0].range[1], arrowBody.range[1] - 1);
-
-                            /*
-                             * For fixes that only contain spaces around the return value, remove the extra spaces.
-                             * This avoids ugly fixes that end up with extra spaces after the arrow, e.g. `() =>   0 ;`
-                             */
-                            return fixer.replaceText(
-                                arrowBody,
-                                (textBeforeReturn + textBetweenReturnAndValue).replace(/^\s*$/, "") + returnValueText + (textAfterValue + textAfterReturnStatement).replace(/^\s*$/, "")
-                            );
+                            return fixes;
                         }
                     });
                 }
@@ -142,21 +173,37 @@ module.exports = {
                         loc: arrowBody.loc.start,
                         message: "Expected block statement surrounding arrow body.",
                         fix(fixer) {
-                            const lastTokenBeforeBody = sourceCode.getLastTokenBetween(sourceCode.getFirstToken(node), arrowBody, astUtils.isNotOpeningParenToken);
-                            const firstBodyToken = sourceCode.getTokenAfter(lastTokenBeforeBody);
-
-                            return fixer.replaceTextRange(
-                                [firstBodyToken.range[0], node.range[1]],
-                                `{return ${sourceCode.getText().slice(firstBodyToken.range[0], node.range[1])}}`
+                            const fixes = [];
+                            const arrowToken = sourceCode.getTokenBefore(arrowBody, astUtils.isArrowToken);
+                            const firstBodyToken = sourceCode.getTokenAfter(arrowToken);
+                            const lastBodyToken = sourceCode.getLastToken(node);
+                            const isParenthesisedObjectLiteral =
+                                astUtils.isOpeningParenToken(firstBodyToken) &&
+                                astUtils.isOpeningBraceToken(sourceCode.getTokenAfter(firstBodyToken));
+
+                            // Wrap the value by a block and a return statement.
+                            fixes.push(
+                                fixer.insertTextBefore(firstBodyToken, "{return "),
+                                fixer.insertTextAfter(lastBodyToken, "}")
                             );
+
+                            // If the value is object literal, remove parentheses which were forced by syntax.
+                            if (isParenthesisedObjectLiteral) {
+                                fixes.push(
+                                    fixer.remove(firstBodyToken),
+                                    fixer.remove(findClosingParen(firstBodyToken))
+                                );
+                            }
+
+                            return fixes;
                         }
                     });
                 }
             }
         }
 
         return {
-            ArrowFunctionExpression: validate
+            "ArrowFunctionExpression:exit": validate
         };
     }
 };

lib/testers/rule-tester.js

@@ -468,9 +468,11 @@ class RuleTester {
                     )
                 );
 
+                const hasMessageOfThisRule = messages.some(m => m.ruleId === ruleName);
+
                 for (let i = 0, l = item.errors.length; i < l; i++) {
-                    assert.ok(!("fatal" in messages[i]), `A fatal parsing error occurred: ${messages[i].message}`);
-                    assert.equal(messages[i].ruleId, ruleName, "Error rule name should be the same as the name of the rule being tested");
+                    assert(!messages[i].fatal, `A fatal parsing error occurred: ${messages[i].message}`);
+                    assert(hasMessageOfThisRule, "Error rule name should be the same as the name of the rule being tested");
 
                     if (typeof item.errors[i] === "string" || item.errors[i] instanceof RegExp) {
 

lib/token-store/index.js

@@ -12,6 +12,7 @@ const assert = require("assert");
 const cursors = require("./cursors");
 const ForwardTokenCursor = require("./forward-token-cursor");
 const PaddedTokenCursor = require("./padded-token-cursor");
+const utils = require("./utils");
 const astUtils = require("../ast-utils");
 
 //------------------------------------------------------------------------------
@@ -560,6 +561,26 @@ module.exports = class TokenStore {
         ).getAllTokens();
     }
 
+    //--------------------------------------------------------------------------
+    // Others.
+    //--------------------------------------------------------------------------
+
+    /**
+     * Checks whether any comments exist or not between the given 2 nodes.
+     *
+     * @param {ASTNode} left - The node to check.
+     * @param {ASTNode} right - The node to check.
+     * @returns {boolean} `true` if one or more comments exist.
+     */
+    commentsExistBetween(left, right) {
+        const index = utils.search(this[COMMENTS], left.range[1]);
+
+        return (
+            index < this[COMMENTS].length &&
+            this[COMMENTS][index].range[1] <= right.range[0]
+        );
+    }
+
     /**
      * Gets all comment tokens directly before the given node or token.
      * @param {ASTNode|token} nodeOrToken The AST node or token to check for adjacent comment tokens.