From 71adc665b9649b173adc76f80723b8de20664ae1 Mon Sep 17 00:00:00 2001 From: Teddy Katz Date: Mon, 18 Mar 2019 11:44:00 -0400 Subject: [PATCH] Fix: avoid moving comments in implicit-arrow-linebreak (fixes #11521) (#11522) Currently, the implicit-arrow-linebreak rule contains a lot of logic to determine how comments should be adjusted in code when an autofix is needed. The goal is to be able to autofix cases where there is a comment between an arrow token and the start of an arrow function body. Most other core rules simply decide not to fix cases when there is a comment interfering with the fix. This logic accounts for a large fraction of the code in the rule, and seems to require a lot of different code for many individual cases. Unfortunately, bugs keep being reported identifying problems in the rule (e.g. #11268, #11521) and it's not clear that the fixes are moving us closer to making the rule correct in general, given that there are always more cases than we can explicitly account for. To address those problems, this commit updates the implicit-arrow-linebreak rule to just skip autofixing when comments interfere, rather than trying to do an autofix anyway and find an alternate location for the comments. I'm reluctant to make this change given that a lot of time has been invested in the autofixing logic, but I think this is ultimately a better solution than trying to guess where the user wants their comments to go, and crashing/producing incorrect code if we get it wrong. --- lib/rules/implicit-arrow-linebreak.js | 177 ++--------------- tests/lib/rules/implicit-arrow-linebreak.js | 201 ++++---------------- 2 files changed, 52 insertions(+), 326 deletions(-) diff --git a/lib/rules/implicit-arrow-linebreak.js b/lib/rules/implicit-arrow-linebreak.js index 50f64561847..6fc1268e753 100644 --- a/lib/rules/implicit-arrow-linebreak.js +++ b/lib/rules/implicit-arrow-linebreak.js @@ -4,10 +4,7 @@ */ "use strict"; -const { - isArrowToken, - isParenthesised -} = require("../util/ast-utils"); +const { isCommentToken, isNotOpeningParenToken } = require("../util/ast-utils"); //------------------------------------------------------------------------------ // Rule Definition @@ -38,142 +35,7 @@ module.exports = { create(context) { const sourceCode = context.getSourceCode(); - - //---------------------------------------------------------------------- - // Helpers - //---------------------------------------------------------------------- - /** - * Gets the applicable preference for a particular keyword - * @returns {string} The applicable option for the keyword, e.g. 'beside' - */ - function getOption() { - return context.options[0] || "beside"; - } - - /** - * Formats the comments depending on whether it's a line or block comment. - * @param {Comment[]} comments The array of comments between the arrow and body - * @param {Integer} column The column number of the first token - * @returns {string} A string of comment text joined by line breaks - */ - function formatComments(comments) { - - return `${comments.map(comment => { - - if (comment.type === "Line") { - return `//${comment.value}`; - } - - return `/*${comment.value}*/`; - }).join("\n")}\n`; - } - - /** - * Finds the first token to prepend comments to depending on the parent type - * @param {ASTNode} node The validated node - * @returns {Token|Node} The node to prepend comments to - */ - function findFirstToken(node) { - switch (node.parent.type) { - case "VariableDeclarator": - - // If the parent is first or only declarator, return the declaration, else, declarator - return sourceCode.getFirstToken( - node.parent.parent.declarations.length === 1 || - node.parent.parent.declarations[0].id.name === node.parent.id.name - ? node.parent.parent : node.parent - ); - case "CallExpression": - case "Property": - - // find the object key - return sourceCode.getFirstToken(node.parent); - default: - return node; - } - } - - /** - * Helper function for adding parentheses fixes for nodes containing nested arrow functions - * @param {Fixer} fixer Fixer - * @param {Token} arrow - The arrow token - * @param {ASTNode} arrowBody - The arrow function body - * @returns {Function[]} autofixer -- wraps function bodies with parentheses - */ - function addParentheses(fixer, arrow, arrowBody) { - const parenthesesFixes = []; - let closingParentheses = ""; - - let followingBody = arrowBody; - let currentArrow = arrow; - - while (currentArrow && followingBody.type !== "BlockStatement") { - if (!isParenthesised(sourceCode, followingBody)) { - parenthesesFixes.push( - fixer.insertTextAfter(currentArrow, " (") - ); - - closingParentheses = `\n)${closingParentheses}`; - } - - currentArrow = sourceCode.getTokenAfter(currentArrow, isArrowToken); - - if (currentArrow) { - followingBody = followingBody.body; - } - } - - return [...parenthesesFixes, - fixer.insertTextAfter(arrowBody, closingParentheses) - ]; - } - - /** - * Autofixes the function body to collapse onto the same line as the arrow. - * If comments exist, checks if the function body contains arrow functions, and appends the body with parentheses. - * Otherwise, prepends the comments before the arrow function. - * @param {Token} arrowToken The arrow token. - * @param {ASTNode|Token} arrowBody the function body - * @param {ASTNode} node The evaluated node - * @returns {Function} autofixer -- validates the node to adhere to besides - */ - function autoFixBesides(arrowToken, arrowBody, node) { - return fixer => { - const placeBesides = fixer.replaceTextRange([arrowToken.range[1], arrowBody.range[0]], " "); - - const comments = sourceCode.getCommentsInside(node).filter(comment => - comment.loc.start.line < arrowBody.loc.start.line); - - if (comments.length) { - - // If the grandparent is not a variable declarator - if ( - arrowBody.parent && - arrowBody.parent.parent && - arrowBody.parent.parent.type !== "VariableDeclarator" - ) { - - // If any arrow functions follow, return the necessary parens fixes. - if (node.body.type === "ArrowFunctionExpression" && - arrowBody.parent.parent.type !== "VariableDeclarator" - ) { - return addParentheses(fixer, arrowToken, arrowBody); - } - } - - const firstToken = findFirstToken(node); - - const commentBeforeExpression = fixer.insertTextBeforeRange( - firstToken.range, - formatComments(comments) - ); - - return [placeBesides, commentBeforeExpression]; - } - - return placeBesides; - }; - } + const option = context.options[0] || "beside"; /** * Validates the location of an arrow function body @@ -181,35 +43,30 @@ module.exports = { * @returns {void} */ function validateExpression(node) { - const option = getOption(); - - let tokenBefore = sourceCode.getTokenBefore(node.body); - const hasParens = tokenBefore.value === "("; - - if (node.type === "BlockStatement") { + if (node.body.type === "BlockStatement") { return; } - let fixerTarget = node.body; + const arrowToken = sourceCode.getTokenBefore(node.body, isNotOpeningParenToken); + const firstTokenOfBody = sourceCode.getTokenAfter(arrowToken); - if (hasParens) { - - // Gets the first token before the function body that is not an open paren - tokenBefore = sourceCode.getTokenBefore(node.body, token => token.value !== "("); - fixerTarget = sourceCode.getTokenAfter(tokenBefore); - } - - if (tokenBefore.loc.end.line === fixerTarget.loc.start.line && option === "below") { + if (arrowToken.loc.end.line === firstTokenOfBody.loc.start.line && option === "below") { context.report({ - node: fixerTarget, + node: firstTokenOfBody, messageId: "expected", - fix: fixer => fixer.insertTextBefore(fixerTarget, "\n") + fix: fixer => fixer.insertTextBefore(firstTokenOfBody, "\n") }); - } else if (tokenBefore.loc.end.line !== fixerTarget.loc.start.line && option === "beside") { + } else if (arrowToken.loc.end.line !== firstTokenOfBody.loc.start.line && option === "beside") { context.report({ - node: fixerTarget, + node: firstTokenOfBody, messageId: "unexpected", - fix: autoFixBesides(tokenBefore, fixerTarget, node) + fix(fixer) { + if (sourceCode.getFirstTokenBetween(arrowToken, firstTokenOfBody, { includeComments: true, filter: isCommentToken })) { + return null; + } + + return fixer.replaceTextRange([arrowToken.range[1], firstTokenOfBody.range[0]], " "); + } }); } } diff --git a/tests/lib/rules/implicit-arrow-linebreak.js b/tests/lib/rules/implicit-arrow-linebreak.js index 9244557066f..4e2c0e12c94 100644 --- a/tests/lib/rules/implicit-arrow-linebreak.js +++ b/tests/lib/rules/implicit-arrow-linebreak.js @@ -62,7 +62,7 @@ ruleTester.run("implicit-arrow-linebreak", rule, { bar => ( // another comment baz - ) + ) ) `, ` @@ -73,7 +73,7 @@ ruleTester.run("implicit-arrow-linebreak", rule, { `, ` /* text */ - () => bar; + () => bar; `, ` /* foo */ @@ -101,7 +101,7 @@ ruleTester.run("implicit-arrow-linebreak", rule, { const foo = { id: 'bar', // comment - prop: (foo1) => 'returning this string', + prop: (foo1) => 'returning this string', } `, ` @@ -118,7 +118,7 @@ ruleTester.run("implicit-arrow-linebreak", rule, { { code: ` // comment - async foo => 'string' + async foo => 'string' `, parserOptions: { ecmaVersion: 8 } }, @@ -219,10 +219,7 @@ ruleTester.run("implicit-arrow-linebreak", rule, { // test comment bar `, - output: unIndent` - // test comment - (foo) => bar - `, + output: null, errors: [UNEXPECTED_LINEBREAK] }, { code: unIndent` @@ -230,10 +227,7 @@ ruleTester.run("implicit-arrow-linebreak", rule, { // comment [] `, - output: unIndent` - // comment - const foo = () => [] - `, + output: null, errors: [UNEXPECTED_LINEBREAK] }, { code: ` @@ -257,14 +251,14 @@ ruleTester.run("implicit-arrow-linebreak", rule, { bar //comment ) - + `, output: ` (foo) => ( bar //comment ) - + `, errors: [UNEXPECTED_LINEBREAK] @@ -274,10 +268,7 @@ ruleTester.run("implicit-arrow-linebreak", rule, { // comment // another comment bar`, - output: unIndent` - // comment - // another comment - (foo) => bar`, + output: null, errors: [UNEXPECTED_LINEBREAK] }, { code: unIndent` @@ -287,21 +278,16 @@ ruleTester.run("implicit-arrow-linebreak", rule, { // another comment bar )`, - output: unIndent` - // comment - (foo) => ( - // another comment - bar - )`, + output: null, errors: [UNEXPECTED_LINEBREAK] }, { code: "() => // comment \n bar", - output: "// comment \n() => bar", + output: null, errors: [UNEXPECTED_LINEBREAK] }, { code: "(foo) => //comment \n bar", - output: "//comment \n(foo) => bar", + output: null, errors: [UNEXPECTED_LINEBREAK] }, { code: unIndent` @@ -309,10 +295,7 @@ ruleTester.run("implicit-arrow-linebreak", rule, { /* test comment */ bar `, - output: unIndent` - /* test comment */ - (foo) => bar - `, + output: null, errors: [UNEXPECTED_LINEBREAK] }, { code: unIndent` @@ -321,14 +304,7 @@ ruleTester.run("implicit-arrow-linebreak", rule, { bar => // there baz;`, - output: unIndent` - (foo) => ( - // hi - bar => ( - // there - baz - ) - );`, + output: null, errors: [UNEXPECTED_LINEBREAK, UNEXPECTED_LINEBREAK] }, { code: unIndent` @@ -339,15 +315,7 @@ ruleTester.run("implicit-arrow-linebreak", rule, { baz ) `, - output: unIndent` - (foo) => ( - // hi - bar => ( - // there - baz - ) - ) - `, + output: null, errors: [UNEXPECTED_LINEBREAK] }, { code: unIndent` @@ -355,29 +323,19 @@ ruleTester.run("implicit-arrow-linebreak", rule, { id: 'bar', prop: (foo1) => // comment - 'returning this string', - } - `, - output: unIndent` - const foo = { - id: 'bar', - // comment - prop: (foo1) => 'returning this string', + 'returning this string', } `, + output: null, errors: [UNEXPECTED_LINEBREAK] }, { code: unIndent` - [ foo => + [ foo => // comment 'bar' ] `, - output: unIndent` - [ // comment - foo => 'bar' - ] - `, + output: null, errors: [UNEXPECTED_LINEBREAK] }, { code: unIndent` @@ -386,11 +344,7 @@ ruleTester.run("implicit-arrow-linebreak", rule, { char ) `, - output: unIndent` - // comment - "foo".split('').map((char) => char - ) - `, + output: null, errors: [UNEXPECTED_LINEBREAK] }, { code: unIndent` @@ -399,11 +353,7 @@ ruleTester.run("implicit-arrow-linebreak", rule, { resolve() ) `, - output: unIndent` - new Promise(// comment - (resolve, reject) => resolve() - ) - `, + output: null, errors: [UNEXPECTED_LINEBREAK] }, { code: unIndent` @@ -415,14 +365,7 @@ ruleTester.run("implicit-arrow-linebreak", rule, { */ bar `, - output: unIndent` - /* - succinct - explanation - of code - */ - () => bar - `, + output: null, errors: [UNEXPECTED_LINEBREAK] }, { code: unIndent` @@ -435,18 +378,7 @@ ruleTester.run("implicit-arrow-linebreak", rule, { stepTwo => // then this happens stepThree`, - output: unIndent` - stepOne => ( - /* - here is - what is - happening - */ - stepTwo => ( - // then this happens - stepThree - ) - )`, + output: null, errors: [UNEXPECTED_LINEBREAK, UNEXPECTED_LINEBREAK] }, { code: unIndent` @@ -462,21 +394,7 @@ ruleTester.run("implicit-arrow-linebreak", rule, { */ baz `, - output: unIndent` - () => ( - /* - multi - line - */ - bar => ( - /* - many - lines - */ - baz - ) - ) - `, + output: null, errors: [UNEXPECTED_LINEBREAK, UNEXPECTED_LINEBREAK] }, { code: unIndent` @@ -485,11 +403,7 @@ ruleTester.run("implicit-arrow-linebreak", rule, { bar ) `, - output: unIndent` - // comment - foo('', boo => bar - ) - `, + output: null, errors: [UNEXPECTED_LINEBREAK] }, { code: unIndent` @@ -497,10 +411,7 @@ ruleTester.run("implicit-arrow-linebreak", rule, { // comment 'string' `, - output: unIndent` - // comment - async foo => 'string' - `, + output: null, parserOptions: { ecmaVersion: 8 }, errors: [UNEXPECTED_LINEBREAK] }, { @@ -510,11 +421,7 @@ ruleTester.run("implicit-arrow-linebreak", rule, { // another bar; `, - output: unIndent` - // comment - // another - async foo => bar; - `, + output: null, parserOptions: { ecmaVersion: 8 }, errors: [UNEXPECTED_LINEBREAK] }, { @@ -523,10 +430,7 @@ ruleTester.run("implicit-arrow-linebreak", rule, { // comment 'string' `, - output: unIndent` - // comment - async (foo) => 'string' - `, + output: null, parserOptions: { ecmaVersion: 8 }, errors: [UNEXPECTED_LINEBREAK] }, { @@ -536,12 +440,7 @@ ruleTester.run("implicit-arrow-linebreak", rule, { baz = () => // comment qux `, - output: unIndent` - const foo = 1, - bar = 2, - // comment - baz = () => qux - `, + output: null, errors: [UNEXPECTED_LINEBREAK] }, { code: unIndent` @@ -551,30 +450,19 @@ ruleTester.run("implicit-arrow-linebreak", rule, { bar = 2, baz = 3 `, - output: unIndent` - //comment - const foo = () => qux, - bar = 2, - baz = 3 - `, + output: null, errors: [UNEXPECTED_LINEBREAK] }, { code: unIndent` const foo = () => //two 1, - boo = () => + boo = () => //comment 2, bop = "what" `, - output: unIndent` - //two - const foo = () => 1, - //comment - boo = () => 2, - bop = "what" - `, + output: null, errors: [UNEXPECTED_LINEBREAK, UNEXPECTED_LINEBREAK] }, { code: unIndent` @@ -587,24 +475,14 @@ ruleTester.run("implicit-arrow-linebreak", rule, { /* catch seems to be needed here */ console.log('Error: ', err) })`, - output: unIndent` - /* If I put a comment here, eslint --fix breaks badly */ - start() - .then(() => process && typeof process.send === 'function' && process.send('ready') - ) - .catch(err => { - /* catch seems to be needed here */ - console.log('Error: ', err) - })`, + output: null, errors: [UNEXPECTED_LINEBREAK] }, { code: unIndent` hello(response => // comment response, param => param)`, - output: unIndent` - // comment - hello(response => response, param => param)`, + output: null, errors: [UNEXPECTED_LINEBREAK] }, { code: unIndent` @@ -616,16 +494,7 @@ ruleTester.run("implicit-arrow-linebreak", rule, { yyyy } )`, - output: unIndent` - start( - arr => ( - // cometh - bod => { - // soon - yyyy - } - ) - )`, + output: null, errors: [UNEXPECTED_LINEBREAK] },