From 673a58bc8420075ba698cee6762e17322a5263c3 Mon Sep 17 00:00:00 2001 From: Toru Nagashima Date: Mon, 19 Jun 2017 07:14:20 +0900 Subject: [PATCH] Update: support multiple fixes in a report (fixes #7348) (#8101) --- docs/developer-guide/working-with-rules.md | 10 ++ lib/rule-context.js | 77 +++++++++- lib/rules/arrow-body-style.js | 129 +++++++++++----- lib/testers/rule-tester.js | 6 +- lib/token-store/index.js | 21 +++ tests/lib/rule-context.js | 168 +++++++++++++++++++++ tests/lib/rules/arrow-body-style.js | 18 ++- tests/lib/rules/semi.js | 21 ++- tests/lib/token-store.js | 12 ++ 9 files changed, 409 insertions(+), 53 deletions(-) diff --git a/docs/developer-guide/working-with-rules.md b/docs/developer-guide/working-with-rules.md index 26c978f7c6e..67b5e79520c 100644 --- a/docs/developer-guide/working-with-rules.md +++ b/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. diff --git a/lib/rule-context.js b/lib/rule-context.js index 99221666af8..66987b86792 100644 --- a/lib/rule-context.js +++ b/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} + 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, diff --git a/lib/rules/arrow-body-style.js b/lib/rules/arrow-body-style.js index f2f14245be9..1630b893720 100644 --- a/lib/rules/arrow-body-style.js +++ b/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,13 +173,29 @@ 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; } }); } @@ -156,7 +203,7 @@ module.exports = { } return { - ArrowFunctionExpression: validate + "ArrowFunctionExpression:exit": validate }; } }; diff --git a/lib/testers/rule-tester.js b/lib/testers/rule-tester.js index d66cd175a43..9d747e96a88 100644 --- a/lib/testers/rule-tester.js +++ b/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) { diff --git a/lib/token-store/index.js b/lib/token-store/index.js index 86510bcb7c2..1446b9ff025 100644 --- a/lib/token-store/index.js +++ b/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. diff --git a/tests/lib/rule-context.js b/tests/lib/rule-context.js index c51e5560c23..274c3f3d872 100644 --- a/tests/lib/rule-context.js +++ b/tests/lib/rule-context.js @@ -83,6 +83,9 @@ describe("RuleContext", () => { mockESLint.expects("report") .once() .withArgs("fake-rule", 2, node, location, message, messageOpts, fixerObj); + mockESLint.expects("getSourceCode") + .once() + .returns(null); ruleContext.report({ node, @@ -95,6 +98,171 @@ describe("RuleContext", () => { fix.verify(); mockESLint.verify(); }); + + it("should merge fixes to one if 'fix' function returns an array of fixes.", () => { + const mockESLint = sandbox.mock(eslint); + + mockESLint.expects("getSourceCode") + .returns({ text: "var foo = 100;" }); + mockESLint.expects("report") + .once() + .withArgs( + sinon.match.any, + sinon.match.any, + sinon.match.any, + sinon.match.any, + sinon.match.any, + sinon.match.any, + sinon.match({ + range: [4, 13], + text: "bar = 234" + }) + ); + + ruleContext.report({ + node: {}, + loc: {}, + message: "Message", + fix(fixer) { + return [ + fixer.replaceTextRange([10, 13], "234"), + fixer.replaceTextRange([4, 7], "bar") + ]; + } + }); + + mockESLint.verify(); + }); + + it("should merge fixes to one if 'fix' function returns an iterator of fixes.", () => { + const mockESLint = sandbox.mock(eslint); + + mockESLint.expects("getSourceCode").returns({ text: "var foo = 100;" }); + mockESLint.expects("report").once().withArgs( + sinon.match.any, + sinon.match.any, + sinon.match.any, + sinon.match.any, + sinon.match.any, + sinon.match.any, + sinon.match({ + range: [4, 13], + text: "bar = 234" + }) + ); + + ruleContext.report({ + node: {}, + loc: {}, + message: "Message", + *fix(fixer) { + yield fixer.replaceTextRange([10, 13], "234"); + yield fixer.replaceTextRange([4, 7], "bar"); + } + }); + + mockESLint.verify(); + }); + + + it("should handle inserting BOM correctly.", () => { + const mockESLint = sandbox.mock(eslint); + + mockESLint.expects("getSourceCode") + .returns({ text: "var foo = 100;" }); + mockESLint.expects("report") + .once() + .withArgs( + sinon.match.any, + sinon.match.any, + sinon.match.any, + sinon.match.any, + sinon.match.any, + sinon.match.any, + sinon.match({ + range: [0, 13], + text: "\uFEFFvar bar = 234" + }) + ); + + ruleContext.report({ + node: {}, + loc: {}, + message: "Message", + fix(fixer) { + return [ + fixer.insertTextBeforeRange([0, 1], "\uFEFF"), + fixer.replaceTextRange([10, 13], "234"), + fixer.replaceTextRange([4, 7], "bar") + ]; + } + }); + + mockESLint.verify(); + }); + + + it("should handle removing BOM correctly.", () => { + const mockESLint = sandbox.mock(eslint); + + mockESLint.expects("getSourceCode") + .returns({ text: "var foo = 100;" }); + mockESLint.expects("report") + .once() + .withArgs( + sinon.match.any, + sinon.match.any, + sinon.match.any, + sinon.match.any, + sinon.match.any, + sinon.match.any, + sinon.match({ + range: [-1, 13], + text: "var bar = 234" + }) + ); + + ruleContext.report({ + node: {}, + loc: {}, + message: "Message", + fix(fixer) { + return [ + fixer.removeRange([-1, 0]), + fixer.replaceTextRange([10, 13], "234"), + fixer.replaceTextRange([4, 7], "bar") + ]; + } + }); + + mockESLint.verify(); + }); + + it("should throw an assertion error if ranges are overlapped.", () => { + const mockESLint = sandbox.mock(eslint); + + mockESLint.expects("getSourceCode") + .returns({ text: "var foo = 100;" }); + mockESLint.expects("report") + .never(); + + assert.throws(() => { + ruleContext.report({ + node: {}, + loc: {}, + message: "Message", + fix(fixer) { + return [ + fixer.removeRange([-1, 0]), + fixer.removeRange([-1, 0]) + ]; + } + }); + }, "Fix objects must not be overlapped in a report."); + + mockESLint.verify(); + }); + }); }); diff --git a/tests/lib/rules/arrow-body-style.js b/tests/lib/rules/arrow-body-style.js index 5b2204a477c..46baf9a5310 100644 --- a/tests/lib/rules/arrow-body-style.js +++ b/tests/lib/rules/arrow-body-style.js @@ -56,7 +56,7 @@ ruleTester.run("arrow-body-style", rule, { }, { code: "var foo = () => ({});", - output: "var foo = () => {return ({})};", + output: "var foo = () => {return {}};", options: ["always"], errors: [ { line: 1, column: 18, type: "ArrowFunctionExpression", message: "Expected block statement surrounding arrow body." } @@ -160,7 +160,7 @@ ruleTester.run("arrow-body-style", rule, { }, { code: "var foo = () => ({});", - output: "var foo = () => {return ({})};", + output: "var foo = () => {return {}};", options: ["as-needed", { requireReturnForObjectLiteral: true }], errors: [ { line: 1, column: 18, type: "ArrowFunctionExpression", message: "Expected block statement surrounding arrow body." } @@ -168,7 +168,7 @@ ruleTester.run("arrow-body-style", rule, { }, { code: "var foo = () => ({ bar: 0 });", - output: "var foo = () => {return ({ bar: 0 })};", + output: "var foo = () => {return { bar: 0 }};", options: ["as-needed", { requireReturnForObjectLiteral: true }], errors: [ { line: 1, column: 18, type: "ArrowFunctionExpression", message: "Expected block statement surrounding arrow body." } @@ -290,6 +290,18 @@ ruleTester.run("arrow-body-style", rule, { errors: [ { line: 2, column: 31, type: "ArrowFunctionExpression", message: "Unexpected block statement surrounding arrow body." } ] + }, + { + code: "var foo = () => ({foo: 1}).foo();", + output: "var foo = () => {return {foo: 1}.foo()};", + options: ["always"], + errors: ["Expected block statement surrounding arrow body."] + }, + { + code: "var foo = () => ({foo: 1}.foo());", + output: "var foo = () => {return {foo: 1}.foo()};", + options: ["always"], + errors: ["Expected block statement surrounding arrow body."] } ] }); diff --git a/tests/lib/rules/semi.js b/tests/lib/rules/semi.js index b4653014e6a..2c272b6ebb6 100644 --- a/tests/lib/rules/semi.js +++ b/tests/lib/rules/semi.js @@ -169,6 +169,25 @@ ruleTester.run("semi", rule, { { code: "export default (foo) => foo.bar();", output: "export default (foo) => foo.bar()", options: ["never"], parserOptions: { sourceType: "module" }, errors: [{ message: "Extra semicolon.", type: "ExportDefaultDeclaration" }] }, { code: "export default foo = 42;", output: "export default foo = 42", options: ["never"], parserOptions: { sourceType: "module" }, errors: [{ message: "Extra semicolon.", type: "ExportDefaultDeclaration" }] }, { code: "export default foo += 42;", output: "export default foo += 42", options: ["never"], parserOptions: { sourceType: "module" }, errors: [{ message: "Extra semicolon.", type: "ExportDefaultDeclaration" }] }, - { code: "a;\n++b", output: "a\n++b", options: ["never"], errors: [{ message: "Extra semicolon." }] } + { code: "a;\n++b", output: "a\n++b", options: ["never"], errors: [{ message: "Extra semicolon." }] }, + + // https://github.com/eslint/eslint/issues/7928 + { + options: ["never"], + code: [ + "/*eslint no-extra-semi: error */", + "foo();", + ";[0,1,2].forEach(bar)" + ].join("\n"), + errors: [ + "Extra semicolon.", + "Unnecessary semicolon." + ], + output: [ + "/*eslint no-extra-semi: error */", + "foo()", + ";[0,1,2].forEach(bar)" + ].join("\n") + } ] }); diff --git a/tests/lib/token-store.js b/tests/lib/token-store.js index 21c0c10c534..ddde3c5f812 100644 --- a/tests/lib/token-store.js +++ b/tests/lib/token-store.js @@ -1273,6 +1273,18 @@ describe("TokenStore", () => { }); }); + describe("when calling commentsExistBetween", () => { + + it("should retrieve false if comments don't exist", () => { + assert.isFalse(store.commentsExistBetween(AST.tokens[0], AST.tokens[1])); + }); + + it("should retrieve true if comments exist", () => { + assert.isTrue(store.commentsExistBetween(AST.tokens[1], AST.tokens[2])); + }); + + }); + describe("getCommentsBefore", () => { it("should retrieve comments before a node", () => { assert.equal(