-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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: curly errors with lexical and function declarations (fixes #11908) #11912
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks! I'll leave this open a bit longer in case other team members want to review.
options: ["multi", "consistent"] | ||
}, | ||
{ | ||
code: "if (true) function a() {} else { foo; bar; }", |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I fully understand some of the test cases, and it looks like mysticatea had some questions too. I'll have to review this in more depth later but I wanted to just raise the questions that had come to me in this session.
}, | ||
{ | ||
code: "if (true) function a() {} else { function b() {} }", | ||
options: ["multi", "consistent"] |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
).
There was a problem hiding this comment.
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).
@mysticatea @platinumazure Friendly ping - would love to be able to move forward with this PR! |
Closing this in favor of #12513, as described in this comment |
What is the purpose of this pull request? (put an "X" next to item)
[X] Bug fix #11908
What changes did you make? (Give an overview)
const
,let
,class
orfunction
declaration should not be reported.function
declarations (e.g.if (true) function foo () {}
) should not be reported.expected
, as it wasn't used as described in the comment (e.g.consistent
code was overriding explicitly setfalse
), and I guess it was a bit confusing.Is there anything you'd like reviewers to focus on?
The refactoring.
Test cases with the
consistent
modifier.