Skip to content

Commit

Permalink
Fix: avoid moving comments in implicit-arrow-linebreak (fixes #11521) (…
Browse files Browse the repository at this point in the history 
…#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.
  • Loading branch information
@not-an-aardvark
not-an-aardvark committed Mar 18, 2019
1 parent 1f715a2 commit 71adc66
Show file tree
Hide file tree
Showing 2 changed files with 52 additions and 326 deletions.
177 changes: 17 additions & 160 deletions lib/rules/implicit-arrow-linebreak.js
View file
Expand Up @@ -4,10 +4,7 @@
*/
"use strict";

const {
isArrowToken,
isParenthesised
} = require("../util/ast-utils");
const { isCommentToken, isNotOpeningParenToken } = require("../util/ast-utils");

//------------------------------------------------------------------------------
// Rule Definition
Expand Down Expand Up @@ -38,178 +35,38 @@ 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
* @param {ASTNode} node The arrow function body
* @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]], " ");
}
});
}
}
Expand Down

0 comments on commit 71adc66

Please sign in to comment.