Update: make no-var fixable (fixes #6639)

[mysticatea]

Fixes #6639.

[mention-bot]

@mysticatea, thanks for your PR! By analyzing the annotation information on this pull request, we identified @vitorbal, @nzakas and @IanVS to be potential reviewers

[eslintbot]

LGTM

[ilyavolodin]

Need to update index.md as well.

[eslintbot]

LGTM

[mysticatea]

@ilyavolodin Thank you!

[mysticatea]

BTW, when I applied this rule to our codebase, this fix generated many no-case-declarations's warnings. Though I think that the behavior of this fix is no problem.

[ilyavolodin]

Hmm.. That's interesting. So by fixing one rule, you might introduce a bunch of errors in another rule. While it's a valid fix, I'm not 100% sure we want to create code that fails some other rule.

[mysticatea]

Probably there are people which are turning no-case-declarations off. This fixing has worth for those people.
But eslint:recommended includes no-case-declarations, so it may be worth not modifying var declarations when the declaration is a child of a SwitchCase node.

I don't have strong opinions here.

[platinumazure]

Couldn't we just special-case var declarations in switches and let them be manually fixed? That way users can choose whether to hoist the declaration or ignore/disable the no-case-declarations error. We probably don't want to fix into questionable/lint-polluted code, in the same way that we choose to avoid fixing into incorrect code with hoisted var declarations.

[nzakas]

My vote is to not autofix in a case.

[ilyavolodin]

As far as I remember, no-var doesn't currently check for case. That's part of another rule. If we were to add the code to prevent fixes inside case we would more or less will have to duplicate the logic from no-case-declarations rule.

[nzakas]

@ilyavolodin What I'm suggesting is that before applying a fix, we just check to see if the parent is a switch case (it's literally node.parent.type !== "SwitchCase"). That doesn't change what the rule does or duplicate any complicated logic.

[ilyavolodin]

It sort of feels like we are coupling rules here. But I guess I'm not opposed to a simple check like that.

[platinumazure]

@nzakas Just to make sure, you wouldn't see any problems with BlockStatements being interposed due to those creating new lexical scope (unlike cases), correct?

switch (foo) {
    case 1:
        var a = 1;      // Should be reported but not fixed?
        if (true) {
            var b = 1;  // Should be reported and fixed?
        }
}

[nzakas]

@ilyavolodin it's not really coupling rules -- we are just not "fixing" something to avoid a possible error. This doesn't add any additional warning, it just doesn't fix something that could be problematic.

@platinumazure yup, that's the intended behavior.

[eslintbot]

LGTM

[mysticatea]

Thank you, everyone.

I modified no-var rule to not fix var declarations on case chunks.

[ilyavolodin]

Can you add a comment to this line explaining why we are skipping SwitchCase. Otherwise next person who fixes the rule might think it was mistake.

[mysticatea]

Thank you for the suggestion.
I added some notes to the JSDoc comment of this function.

[eslintbot]

LGTM

[nzakas]

Lgtm. Awesome work!