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

[mdjermanovic]

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)

1. Block statements with a single const, let, class or function declaration should not be reported.
2. Single statement function declarations (e.g. if (true) function foo () {}) should not be reported.
3. A small refactoring to avoid three-state boolean logic for expected, as it wasn't used as described in the comment (e.g. consistent code was overriding explicitly set false), 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.

[not-an-aardvark]

LGTM, thanks! I'll leave this open a bit longer in case other team members want to review.

[mysticatea]

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

to doubt about the consistent option

[platinumazure]

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.

[platinumazure]

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?

[mdjermanovic]

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).

[mdjermanovic]

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).

[kaicataldo]

@mysticatea @platinumazure Friendly ping - would love to be able to move forward with this PR!

[mdjermanovic]

Closing this in favor of #12513, as described in this comment