Skip to content

Commit

Permalink
Fix: no-extra-parens autofix with in in a for-loop init (fixes #11706…
Browse files Browse the repository at this point in the history 
…) (#11848)
  • Loading branch information
@mdjermanovic @mysticatea
mdjermanovic authored and mysticatea committed Jul 17, 2019
1 parent 2dafe2d commit e4c450f
Show file tree
Hide file tree
Showing 2 changed files with 880 additions and 19 deletions.
248 changes: 231 additions & 17 deletions lib/rules/no-extra-parens.js
View file
Expand Up @@ -81,6 +81,8 @@ module.exports = {
const PRECEDENCE_OF_ASSIGNMENT_EXPR = precedence({ type: "AssignmentExpression" });
const PRECEDENCE_OF_UPDATE_EXPR = precedence({ type: "UpdateExpression" });

let reportsBuffer;

/**
* Determines if this rule should be enforced for a node given the current configuration.
* @param {ASTNode} node - The node to be checked.
Expand Down Expand Up @@ -316,19 +318,33 @@ module.exports = {
}
}

context.report({
node,
loc: leftParenToken.loc.start,
messageId: "unexpected",
fix(fixer) {
const parenthesizedSource = sourceCode.text.slice(leftParenToken.range[1], rightParenToken.range[0]);
/**
* Finishes reporting
* @returns {void}
* @private
*/
function finishReport() {
context.report({
node,
loc: leftParenToken.loc.start,
messageId: "unexpected",
fix(fixer) {
const parenthesizedSource = sourceCode.text.slice(leftParenToken.range[1], rightParenToken.range[0]);

return fixer.replaceTextRange([
leftParenToken.range[0],
rightParenToken.range[1]
], (requiresLeadingSpace(node) ? " " : "") + parenthesizedSource + (requiresTrailingSpace(node) ? " " : ""));
}
});
}

return fixer.replaceTextRange([
leftParenToken.range[0],
rightParenToken.range[1]
], (requiresLeadingSpace(node) ? " " : "") + parenthesizedSource + (requiresTrailingSpace(node) ? " " : ""));
}
});
if (reportsBuffer) {
reportsBuffer.reports.push({ node, finishReport });
return;
}

finishReport();
}

/**
Expand Down Expand Up @@ -498,6 +514,126 @@ module.exports = {
}
}

/**
* Finds the path from the given node to the specified ancestor.
* @param {ASTNode} node First node in the path.
* @param {ASTNode} ancestor Last node in the path.
* @returns {ASTNode[]} Path, including both nodes.
* @throws {Error} If the given node does not have the specified ancestor.
*/
function pathToAncestor(node, ancestor) {
const path = [node];
let currentNode = node;

while (currentNode !== ancestor) {

currentNode = currentNode.parent;

/* istanbul ignore if */
if (currentNode === null) {
throw new Error("Nodes are not in the ancestor-descendant relationship.");
}

path.push(currentNode);
}

return path;
}

/**
* Finds the path from the given node to the specified descendant.
* @param {ASTNode} node First node in the path.
* @param {ASTNode} descendant Last node in the path.
* @returns {ASTNode[]} Path, including both nodes.
* @throws {Error} If the given node does not have the specified descendant.
*/
function pathToDescendant(node, descendant) {
return pathToAncestor(descendant, node).reverse();
}

/**
* Checks whether the syntax of the given ancestor of an 'in' expression inside a for-loop initializer
* is preventing the 'in' keyword from being interpreted as a part of an ill-formed for-in loop.
*
* @param {ASTNode} node Ancestor of an 'in' expression.
* @param {ASTNode} child Child of the node, ancestor of the same 'in' expression or the 'in' expression itself.
* @returns {boolean} True if the keyword 'in' would be interpreted as the 'in' operator, without any parenthesis.
*/
function isSafelyEnclosingInExpression(node, child) {
switch (node.type) {
case "ArrayExpression":
case "ArrayPattern":
case "BlockStatement":
case "ObjectExpression":
case "ObjectPattern":
case "TemplateLiteral":
return true;
case "ArrowFunctionExpression":
case "FunctionExpression":
return node.params.includes(child);
case "CallExpression":
case "NewExpression":
return node.arguments.includes(child);
case "MemberExpression":
return node.computed && node.property === child;
case "ConditionalExpression":
return node.consequent === child;
default:
return false;
}
}

/**
* Starts a new reports buffering. Warnings will be stored in a buffer instead of being reported immediately.
* An additional logic that requires multiple nodes (e.g. a whole subtree) may dismiss some of the stored warnings.
*
* @returns {void}
*/
function startNewReportsBuffering() {
reportsBuffer = {
upper: reportsBuffer,
inExpressionNodes: [],
reports: []
};
}

/**
* Ends the current reports buffering.
* @returns {void}
*/
function endCurrentReportsBuffering() {
const { upper, inExpressionNodes, reports } = reportsBuffer;

if (upper) {
upper.inExpressionNodes.push(...inExpressionNodes);
upper.reports.push(...reports);
} else {

// flush remaining reports
reports.forEach(({ finishReport }) => finishReport());
}

reportsBuffer = upper;
}

/**
* Checks whether the given node is in the current reports buffer.
* @param {ASTNode} node Node to check.
* @returns {boolean} True if the node is in the current buffer, false otherwise.
*/
function isInCurrentReportsBuffer(node) {
return reportsBuffer.reports.some(r => r.node === node);
}

/**
* Removes the given node from the current reports buffer.
* @param {ASTNode} node Node to remove.
* @returns {void}
*/
function removeFromCurrentReportsBuffer(node) {
reportsBuffer.reports = reportsBuffer.reports.filter(r => r.node !== node);
}

return {
ArrayExpression(node) {
node.elements
Expand Down Expand Up @@ -540,7 +676,14 @@ module.exports = {
}
},

BinaryExpression: checkBinaryLogical,
BinaryExpression(node) {
if (reportsBuffer && node.operator === "in") {
reportsBuffer.inExpressionNodes.push(node);
}

checkBinaryLogical(node);
},

CallExpression: checkCallNew,

ConditionalExpression(node) {
Expand Down Expand Up @@ -602,17 +745,88 @@ module.exports = {
},

ForStatement(node) {
if (node.init && hasExcessParens(node.init)) {
report(node.init);
}

if (node.test && hasExcessParens(node.test) && !isCondAssignException(node)) {
report(node.test);
}

if (node.update && hasExcessParens(node.update)) {
report(node.update);
}

if (node.init) {
startNewReportsBuffering();

if (hasExcessParens(node.init)) {
report(node.init);
}
}
},

"ForStatement > *.init:exit"(node) {

/*
* Removing parentheses around `in` expressions might change semantics and cause errors.
*
* For example, this valid for loop:
* for (let a = (b in c); ;);
* after removing parentheses would be treated as an invalid for-in loop:
* for (let a = b in c; ;);
*/

if (reportsBuffer.reports.length) {
reportsBuffer.inExpressionNodes.forEach(inExpressionNode => {
const path = pathToDescendant(node, inExpressionNode);
let nodeToExclude;

for (let i = 0; i < path.length; i++) {
const pathNode = path[i];

if (i < path.length - 1) {
const nextPathNode = path[i + 1];

if (isSafelyEnclosingInExpression(pathNode, nextPathNode)) {

// The 'in' expression in safely enclosed by the syntax of its ancestor nodes (e.g. by '{}' or '[]').
return;
}
}

if (isParenthesised(pathNode)) {
if (isInCurrentReportsBuffer(pathNode)) {

// This node was supposed to be reported, but parentheses might be necessary.

if (isParenthesisedTwice(pathNode)) {

/*
* This node is parenthesised twice, it certainly has at least one pair of `extra` parentheses.
* If the --fix option is on, the current fixing iteration will remove only one pair of parentheses.
* The remaining pair is safely enclosing the 'in' expression.
*/
return;
}

// Exclude the outermost node only.
if (!nodeToExclude) {
nodeToExclude = pathNode;
}

// Don't break the loop here, there might be some safe nodes or parentheses that will stay inside.

} else {

// This node will stay parenthesised, the 'in' expression in safely enclosed by '()'.
return;
}
}
}

// Exclude the node from the list (i.e. treat parentheses as necessary)
removeFromCurrentReportsBuffer(nodeToExclude);
});
}

endCurrentReportsBuffering();
},

IfStatement(node) {
Expand Down

0 comments on commit e4c450f

Please sign in to comment.