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.

Sign up for GitHub

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

Jump to bottom

Fix: curly errors with lexical and function declarations (fixes #11908) #11912

Closed
wants to merge 1 commit into from
Closed

Fix: curly errors with lexical and function declarations (fixes #11908) #11912

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
65 changes: 40 additions & 25 deletions lib/rules/curly.js
View file
Open in desktop
Expand Up @@ -117,6 +117,21 @@ module.exports = {
return node.type === "FunctionDeclaration" || node.type === "ClassDeclaration";
}

/**
* Determines if the given node is a lexical declaration, or a block statement containing
* only a single lexical declaration.
* @param {ASTNode} node The node to check
* @returns {boolean} True if the node is a single lexical declaration
* @private
*/
function isSingleLexicalDeclaration(node) {
if (node.type === "BlockStatement") {
return node.body.length === 1 && isLexicalDeclaration(node.body[0]);
}

return isLexicalDeclaration(node);
}

/**
* Checks if the given token is an `else` token or not.
*
Expand Down Expand Up @@ -227,20 +242,26 @@ module.exports = {
* @param {ASTNode} body The body node to check for blocks.
* @param {string} name The name to report if there's a problem.
* @param {{ condition: boolean }} opts Options to pass to the report functions
* @returns {Object} a prepared check object, with "actual", "expected", "check" properties.
* @returns {Object} a prepared check object, with "actual", "expected", "unchangeable", "check" properties.
* "actual" will be `true` or `false` whether the body is already a block statement.
* "expected" will be `true` or `false` if the body should be a block statement or not, or
* `null` if it doesn't matter, depending on the rule options. It can be modified to change
* the final behavior of "check".
* "check" will be a function reporting appropriate problems depending on the other
* properties.
* "expected" will be `true` or `false` if the body should be a block statement or not.
* If both `true` and `false` can be valid by the rule options, "expected" will be set to "actual".
* "unchangeable" will be `true` if the body, in its actual state (block or not a block), must not be
* changed because it would produce a syntax error or change the semantics.
* "check" will be a function reporting appropriate problems depending on the other properties.
*/
function prepareCheck(node, body, name, opts) {
const hasBlock = (body.type === "BlockStatement");
let expected = null;

if (node.type === "IfStatement" && node.consequent === body && requiresBraceOfConsequent(node)) {
let expected = hasBlock;
let unchangeable = false;

if (hasBlock && body.body.length >= 2) {
unchangeable = true;
} else if (isSingleLexicalDeclaration(body)) {
unchangeable = true;
} else if (node.type === "IfStatement" && node.consequent === body && requiresBraceOfConsequent(node)) {
expected = true;
unchangeable = true;
} else if (multiOnly) {
if (hasBlock && body.body.length === 1) {
expected = false;
Expand All @@ -252,13 +273,8 @@ module.exports = {
} else if (multiOrNest) {
if (hasBlock && body.body.length === 1 && isOneLiner(body.body[0])) {
const leadingComments = sourceCode.getCommentsBefore(body.body[0]);
const isLexDef = isLexicalDeclaration(body.body[0]);

if (isLexDef) {
expected = true;
} else {
expected = leadingComments.length > 0;
}
expected = leadingComments.length > 0;
} else if (!isOneLiner(body)) {
expected = true;
}
Expand All @@ -269,8 +285,9 @@ module.exports = {
return {
actual: hasBlock,
expected,
unchangeable,
check() {
if (this.expected !== null && this.expected !== this.actual) {
if (this.expected !== this.actual) {
if (this.expected) {
context.report({
node,
Expand Down Expand Up @@ -349,16 +366,14 @@ module.exports = {
* all have braces.
* If all nodes shouldn't have braces, make sure they don't.
*/
const expected = preparedChecks.some(preparedCheck => {
if (preparedCheck.expected !== null) {
return preparedCheck.expected;
}
return preparedCheck.actual;
});
const expected = preparedChecks.some(preparedCheck => preparedCheck.expected);

preparedChecks.forEach(preparedCheck => {
preparedCheck.expected = expected;
});
preparedChecks
.forEach(preparedCheck => {
if (!preparedCheck.unchangeable) {
preparedCheck.expected = expected;
}
});
}

return preparedChecks;
Expand Down
188 changes: 188 additions & 0 deletions tests/lib/rules/curly.js
View file
Open in desktop
Expand Up @@ -231,6 +231,100 @@ ruleTester.run("curly", rule, {
// https://github.com/feross/standard/issues/664
code: "if (true) foo()\n;[1, 2, 3].bar()",
options: ["multi-line"]
},

// https://github.com/eslint/eslint/issues/11908 (also in invalid)
{
code: "if (true) { const a = 1; }",
options: ["multi"],
parserOptions: { ecmaVersion: 6 }
},
{
code: "if (true) { let a; }",
options: ["multi"],
parserOptions: { ecmaVersion: 6 }
},
{
code: "if (true) { class a{} }",
options: ["multi"],
parserOptions: { ecmaVersion: 6 }
},
{
code: "if (true) { function a() {} }",
options: ["multi"]
},
"if (true) function a() {}",
{
code: "if (true) \n function a() {}",
options: ["multi-line"]
},
{
code: "if (true) function a() \n {}",
options: ["multi-line"]
},
{
code: "if (true) function a() \n {}",
options: ["multi-or-nest"]
},
{
code: "if (true) { let a; } else { foo; }",
options: ["multi", "consistent"],
parserOptions: { ecmaVersion: 6 }
},
{
code: "if (true) { function a() {} } else { foo; }",
options: ["multi", "consistent"]
},
{
code: "if (true) function a() {} else b;",
options: ["multi", "consistent"]
},
{
code: "if (true) function a() {} else { foo; bar; }",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this is correct. I feel that all clauses should be blocks.

options: ["multi", "consistent"]
},
{
code: "if (true) function a() {} else { let a }",
options: ["multi", "consistent"],
parserOptions: { ecmaVersion: 6 }
},
{
code: "if (true) function a() {} else { function b() {} }",
options: ["multi", "consistent"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused. Why is it valid for the consequent section to have no block and the alternate section to have a block?

Is it that this is really "invalid", but the block can't be removed in the alternate section because it would change the scope/hoisting of the function?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is to treat both as 'valid', thus keep the scope of this rule to only whether the user should simply put or remove braces around certain statements. Cases like this would need additional steps if not a serious refactoring to fix the code (I meant user's code, not the code of this rule).

This is in line with how the multi-or-nest option is already implemented: a single-line function declaration in a single-statement block is not an error (I'm not able to verify this again at the moment, though). This PR implements the same for the multi option, but also prevents reporting and fixing function declarations that are not in a block regardless of the option, so now there are situations where a chain cannot be consistent.

Also, this 'wrong' usage of function declarations will be reported by no-inner-declarations (included in eslint:recommended).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is how the rule already works with multi-or-nest option, which was fixed in #11675:

/*eslint curly: ["error", "multi-or-nest"]*/

// warning
if (foo) {    
    baz();
}

// no warning
if (foo) {   
   function x() {}
}

The second block would be 'invalid' by this option because it's one statement in one line, but since it has a lexical declaration (the block cannot be removed) it is treated as 'valid' ( = no warning). I think it's okay to not report this block.

When the consistent modifier is on, this block will even force braces around other one-liners in the same chain:

/*eslint curly: ["error", "multi-or-nest", "consistent"]*/

// warning on `if`, not on `else`
if (foo)  
    baz();
else {
   function x() {}
}

auto-fixed to:

/*eslint curly: ["error", "multi-or-nest", "consistent"]*/

if (foo) 
    {baz();}
else {
   function x() {}
}

This PR does the same with the multi option (which wasn't fixed earlier), and the rule will never report a block that has a lexical declaration inside.

In addition, this PR also fixes the behavior of this rule in cases when a function declaration is not in a block. The presumption is that these two statements are not equivalent:

if (foo) 
   function x() {}

if (foo) 
  {function x() {}}

so, to be consistent with the fact that 'unchangeable' statements shouldn't be reported, after this PR the rule will not report this:

/*eslint curly: ["error"]*/
// default option, blocks are always required

// no warning
if (foo) 
   function x() {}

currently, which I believe is not correct, the rule reports and fixes this code to:

/*eslint curly: ["error"]*/
// default option, blocks are always required

if (foo) 
   {function x() {}}

The problem with the consistent modifier is the presumption that all statements can be safely surrounded by braces, which leads to situations like this:

/*eslint curly: ["error", "multi", "consistent"]*/

if (foo) 
   function x() {}
else {
    function x() {}
}

This isn't a consistent chain and it can't be, but I think it's okay to leave it as is ( = not report anything).

},
{
code: "if (true) function a() {} else { foo; }",
options: ["multi-line", "consistent"]
},
{
code: "if (true) { let a; } else { foo; }",
options: ["multi-or-nest", "consistent"],
parserOptions: { ecmaVersion: 6 }
},
{
code: "if (true) { function a() {} } else { foo; }",
options: ["multi-or-nest", "consistent"]
},
{
code: "if (true) function a() {} else { foo; bar; }",
options: ["multi-or-nest", "consistent"]
},
{
code: "if (true) function a() \n {} else { foo; bar; }",
options: ["multi-or-nest", "consistent"]
},
{
code: "if (true) function a() {} else { function b() \n {} }",
options: ["multi-or-nest", "consistent"]
},
{
code: "if (true) function a() {} else { let a }",
options: ["multi-or-nest", "consistent"],
parserOptions: { ecmaVersion: 6 }
},
{
code: "if (true) function a() {} else { function b() {} }",
options: ["multi-or-nest", "consistent"]
}
],
invalid: [
Expand Down Expand Up @@ -897,6 +991,100 @@ ruleTester.run("curly", rule, {
output: "if (true)\n{foo()\n;}[1, 2, 3].bar()",
options: ["multi-line"],
errors: [{ messageId: "missingCurlyAfterCondition", data: { name: "if" }, type: "IfStatement" }]
},

// https://github.com/eslint/eslint/issues/11908 (also in valid)
{
code: "if (foo) { let a; } else b;",
output: "if (foo) { let a; } else {b;}",
parserOptions: { ecmaVersion: 6 },
errors: [{ messageId: "missingCurlyAfter", data: { name: "else" }, type: "IfStatement" }]
},
{
code: "if (foo) a; else { const b = 1; }",
output: "if (foo) {a;} else { const b = 1; }",
parserOptions: { ecmaVersion: 6 },
errors: [{ messageId: "missingCurlyAfterCondition", data: { name: "if" }, type: "IfStatement" }]
},
{
code: "if (foo) function a() {} else b;",
output: "if (foo) function a() {} else {b;}",
errors: [{ messageId: "missingCurlyAfter", data: { name: "else" }, type: "IfStatement" }]
},
{
code: "if (foo) { let a; } else b;",
output: "if (foo) { let a; } else {b;}",
options: ["multi", "consistent"],
parserOptions: { ecmaVersion: 6 },
errors: [{ messageId: "missingCurlyAfter", data: { name: "else" }, type: "IfStatement" }]
},
{
code: "if (foo) a; else { const b = 1; }",
output: "if (foo) {a;} else { const b = 1; }",
options: ["multi", "consistent"],
parserOptions: { ecmaVersion: 6 },
errors: [{ messageId: "missingCurlyAfterCondition", data: { name: "if" }, type: "IfStatement" }]
},
{
code: "if (foo) a; else if (bar) { class b {} } else function c() {}",
output: "if (foo) {a;} else if (bar) { class b {} } else function c() {}",
options: ["multi", "consistent"],
parserOptions: { ecmaVersion: 6 },
errors: [{ messageId: "missingCurlyAfterCondition", data: { name: "if" }, type: "IfStatement" }]
},
{
code: "if (foo) { a; } else function b() {}",
output: "if (foo) a; else function b() {}",
options: ["multi", "consistent"],
errors: [{ messageId: "unexpectedCurlyAfterCondition", data: { name: "if" }, type: "IfStatement" }]
},
{
code: "if (foo) { let a; } else b;",
output: "if (foo) { let a; } else {b;}",
options: ["multi-line", "consistent"],
parserOptions: { ecmaVersion: 6 },
errors: [{ messageId: "missingCurlyAfter", data: { name: "else" }, type: "IfStatement" }]
},
{
code: "if (foo) a; else { const b = 1; }",
output: "if (foo) {a;} else { const b = 1; }",
options: ["multi-line", "consistent"],
parserOptions: { ecmaVersion: 6 },
errors: [{ messageId: "missingCurlyAfterCondition", data: { name: "if" }, type: "IfStatement" }]
},
{
code: "if (foo) a; else if (bar) { class b {} } else function c() {}",
output: "if (foo) {a;} else if (bar) { class b {} } else function c() {}",
options: ["multi-line", "consistent"],
parserOptions: { ecmaVersion: 6 },
errors: [{ messageId: "missingCurlyAfterCondition", data: { name: "if" }, type: "IfStatement" }]
},
{
code: "if (foo) { let a; } else b;",
output: "if (foo) { let a; } else {b;}",
options: ["multi-or-nest", "consistent"],
parserOptions: { ecmaVersion: 6 },
errors: [{ messageId: "missingCurlyAfter", data: { name: "else" }, type: "IfStatement" }]
},
{
code: "if (foo) a; else { const b = 1; }",
output: "if (foo) {a;} else { const b = 1; }",
options: ["multi-or-nest", "consistent"],
parserOptions: { ecmaVersion: 6 },
errors: [{ messageId: "missingCurlyAfterCondition", data: { name: "if" }, type: "IfStatement" }]
},
{
code: "if (foo) a; else if (bar) { class b {} } else function c() {}",
output: "if (foo) {a;} else if (bar) { class b {} } else function c() {}",
options: ["multi-or-nest", "consistent"],
parserOptions: { ecmaVersion: 6 },
errors: [{ messageId: "missingCurlyAfterCondition", data: { name: "if" }, type: "IfStatement" }]
},
{
code: "if (foo) { a; } else function b() {}",
output: "if (foo) a; else function b() {}",
options: ["multi-or-nest", "consistent"],
errors: [{ messageId: "unexpectedCurlyAfterCondition", data: { name: "if" }, type: "IfStatement" }]
}
]
});