Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: no-extra-parens autofix with `in` in a for-loop init (fixes #11706) #11848

Merged
merged 2 commits into from Jul 17, 2019
Merged
Changes from 1 commit
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -81,6 +81,8 @@ module.exports = {
const PRECEDENCE_OF_ASSIGNMENT_EXPR = precedence({ type: "AssignmentExpression" });
const PRECEDENCE_OF_UPDATE_EXPR = precedence({ type: "UpdateExpression" });

let forStatementInitInfo;

/**
* Determines if this rule should be enforced for a node given the current configuration.
* @param {ASTNode} node - The node to be checked.
@@ -316,19 +318,35 @@ 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) ? " " : ""));
}
});
}

if (forStatementInitInfo) {

return fixer.replaceTextRange([
leftParenToken.range[0],
rightParenToken.range[1]
], (requiresLeadingSpace(node) ? " " : "") + parenthesizedSource + (requiresTrailingSpace(node) ? " " : ""));
}
});
// This node is within ForStatement.init, reports will be flushed on exit
forStatementInitInfo.reports.push({ node, finishReport });
return;
}

finishReport();
}

/**
@@ -498,6 +516,32 @@ 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("node doesn't have the specified ancestor");
}

path.push(currentNode);
}

return path;
}

return {
ArrayExpression(node) {
node.elements
@@ -540,7 +584,14 @@ module.exports = {
}
},

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

checkBinaryLogical(node);
},

CallExpression: checkCallNew,

ConditionalExpression(node) {
@@ -602,17 +653,79 @@ 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) {
forStatementInitInfo = {
upper: forStatementInitInfo,
inExpressionNodes: [],
reports: []
};

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; ;);
*/

let { reports } = forStatementInitInfo;

if (reports.length) {

const { inExpressionNodes } = forStatementInitInfo;

inExpressionNodes.forEach(inExpressionNode => {
for (const pathNode of pathToAncestor(inExpressionNode, node).reverse()) {
if (isParenthesised(pathNode)) {
This conversation was marked as resolved by mysticatea

This comment has been minimized.

Copy link
@mysticatea

mysticatea Jun 18, 2019

Member

I guess any of the following nodes should break this loop because wrapping by any of (), [], {}, and ?: allows in operators.

  • Wrapping by ()
    • ArrowFunctionExpression if the child is in params.
    • CallExpression if the child is in arguments.
    • FunctionExpression if the child is in params.
    • NewExpression if the child is in arguments.
  • Wrapping by []
    • ArrayExpression
    • ArrayPattern
    • MemberExpression if the child is property and computed is true.
  • Wrapping by {}
    • BlockStatement
    • ObjectExpression
    • ObjectPattern
    • TemplateLiteral
  • Wrapping by ?:
    • ConditionalExpression if the child is consequent.

This comment has been minimized.

Copy link
@mdjermanovic

mdjermanovic Jun 18, 2019

Author Member

I was aware of this, but eventually decided it might be better not to implement this check, and left a comment at the end of the loop:

 /*
  * Other enclosing punctuators such as {} are omitted from this check,
  * in order to avoid dependencies with other rules.
  *
  * For example, these parentheses might be safe for removal:
  *      for (let a = b => { return (b in c) }; ;);
  * However, arrow-body-style could remove braces as well, and produce an invalid for-in loop:
  *      for (let a = b => b in c; ;);
  */

Though now I'm not sure about this particular example because the ranges overlap, perhaps the fixing algorithm would have already removed the braces in the previous iteration (while blocking the parens fix in that iteration).

Also, this additional check might be too much code for an edge case with not-so-terrible consequences (one pair of unnecessary parens will stay) of an already edge case ('in' in a for-loop initializer).

Of course, if this additional check can't produce subsequent errors, and you think it's worth of implementing, I'll do it.

This comment has been minimized.

Copy link
@mysticatea

mysticatea Jun 19, 2019

Member

ESLint doesn't apply two fixes which have overlapped range each other at the same time. So I think this will not conflict with arrow-body-style rule.

This comment has been minimized.

Copy link
@mdjermanovic

mdjermanovic Jun 19, 2019

Author Member

Ok, I'll do it by the list you provided


/*
* If this node was supposed to be reported, exclude it from the list
* (i.e. treat parentheses as necessary)
*/
reports = reports.filter(w => w.node !== pathNode);

/*
* In any case, this node is parenthesised and will stay parenthesised.
* All extra parentheses inside can be safely removed.
*/
return;
}

/*
* Other enclosing punctuators such as {} are omitted from this check,
* in order to avoid dependencies with other rules.
*
* For example, these parentheses might be safe for removal:
* for (let a = b => { return (b in c) }; ;);
* However, arrow-body-style could remove braces as well, and produce an invalid for-in loop:
* for (let a = b => b in c; ;);
*/
}
});

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

// ForStatement can be inside a function/arrow expression in another ForStatement.init
forStatementInitInfo = forStatementInitInfo.upper;
},

IfStatement(node) {
@@ -466,7 +466,31 @@ ruleTester.run("no-extra-parens", rule, {
"for ((let) in foo);",
"for ((let[foo]) in bar);",
"for ((let)[foo] in bar);",
"for ((let[foo].bar) in baz);"
"for ((let[foo].bar) in baz);",

// https://github.com/eslint/eslint/issues/11706 (also in invalid[])
"for (let a = (b in c); ;);",
"for (let a = (b && c in d); ;);",
"for (let a = (b in c && d); ;);",
"for (let a = (b => b in c); ;);",
"for (let a = b => (b in c); ;);",
"for (let a = b => { return (b in c) }; ;);",
"for (let a = (b in c in d); ;);",
"for (let a = (b in c), d = (e in f); ;);",
"for (let a = (b => c => b in c); ;);",
"for (let a = (b && c && d in e); ;);",
"for (let a = b && (c in d); ;);",
"for (let a = (b in c) && (d in e); ;);",
"for (let a = (b in c), d = () => { for ((e in f);;); }; ;);",
"for ((a in b); ;);",
"for (a = (b in c); ;);",
"for ((a in b && c in d && e in f); ;);",

// https://github.com/eslint/eslint/issues/11706 regression tests (also in invalid[])
"for (let a = b; a; a); a; a;",
"for (a; a; a); a; a;",
"for (; a; a); a; a;",
"for (let a = (b && c) === d; ;);"
],

invalid: [
@@ -1105,6 +1129,163 @@ ruleTester.run("no-extra-parens", rule, {
"(let)",
"Identifier",
1
)
),

// https://github.com/eslint/eslint/issues/11706 (also in valid[])
{
code: "for ((a = (b in c)); ;);",
output: "for ((a = b in c); ;);",
errors: [
{
messageId: "unexpected"
}
]
},
{
code: "for (let a = ((b in c) && (d in e)); ;);",
output: "for (let a = (b in c && d in e); ;);",
errors: Array(2).fill(
{
messageId: "unexpected"
}
)
},
{
code: "for (let a = ((b in c) in d); ;);",
output: "for (let a = (b in c in d); ;);",
errors: [
{
messageId: "unexpected"
}
]
},
{
code: "for (let a = (b && (c in d)), e = (f in g); ;);",
output: "for (let a = (b && c in d), e = (f in g); ;);",
errors: [
{
messageId: "unexpected"
}
]
},
{
code: "for (let a = (b + c), d = (e in f); ;);",
output: "for (let a = b + c, d = (e in f); ;);",
errors: [
{
messageId: "unexpected"
}
]
},

// https://github.com/eslint/eslint/issues/11706 regression tests (also in valid[])
{
code: "for (let a = (b); a > (b); a = (b)) a = (b); a = (b);",
output: "for (let a = b; a > b; a = b) a = b; a = b;",
errors: Array(5).fill(
{
messageId: "unexpected"
}
)
},
{
code: "for ((a = b); (a > b); (a = b)) (a = b); (a = b);",
output: "for (a = b; a > b; a = b) a = b; a = b;",
errors: Array(5).fill(
{
messageId: "unexpected"
}
)
},
{
code: "for (let a = b; a > (b); a = (b)) a = (b); a = (b);",
output: "for (let a = b; a > b; a = b) a = b; a = b;",
errors: Array(4).fill(
{
messageId: "unexpected"
}
)
},
{
code: "for (let a = b; (a > b); (a = b)) (a = b); (a = b);",
output: "for (let a = b; a > b; a = b) a = b; a = b;",
errors: Array(4).fill(
{
messageId: "unexpected"
}
)
},
{
code: "for (; a > (b); a = (b)) a = (b); a = (b);",
output: "for (; a > b; a = b) a = b; a = b;",
errors: Array(4).fill(
{
messageId: "unexpected"
}
)
},
{
code: "for (; (a > b); (a = b)) (a = b); (a = b);",
output: "for (; a > b; a = b) a = b; a = b;",
errors: Array(4).fill(
{
messageId: "unexpected"
}
)
},
{
code: "for (let a = (b); a = (b in c); a = (b in c)) a = (b in c); a = (b in c);",
output: "for (let a = b; a = b in c; a = b in c) a = b in c; a = b in c;",
errors: Array(5).fill(
{
messageId: "unexpected"
}
)
},
{
code: "for (let a = (b); (a in b); (a in b)) (a in b); (a in b);",
output: "for (let a = b; a in b; a in b) a in b; a in b;",
errors: Array(5).fill(
{
messageId: "unexpected"
}
)
},
{
code: "for (let a = b; a = (b in c); a = (b in c)) a = (b in c); a = (b in c);",
output: "for (let a = b; a = b in c; a = b in c) a = b in c; a = b in c;",
errors: Array(4).fill(
{
messageId: "unexpected"
}
)
},
{
code: "for (let a = b; (a in b); (a in b)) (a in b); (a in b);",
output: "for (let a = b; a in b; a in b) a in b; a in b;",
errors: Array(4).fill(
{
messageId: "unexpected"
}
)
},
{
code: "for (; a = (b in c); a = (b in c)) a = (b in c); a = (b in c);",
output: "for (; a = b in c; a = b in c) a = b in c; a = b in c;",
errors: Array(4).fill(
{
messageId: "unexpected"
}
)
},
{
code: "for (; (a in b); (a in b)) (a in b); (a in b);",
output: "for (; a in b; a in b) a in b; a in b;",
errors: Array(4).fill(
{
messageId: "unexpected"
}
)
}
]
});
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.