Added unnecessary-else Rule

[debsmita1]

PR checklist

• Addresses an existing issue: fixes Rule suggestion: unnecessary-else #4429
• New feature
  • Includes tests
• Documentation update

Overview of change:

This Rule disallows else blocks following if blocks ending with a return , throw, break or continuestatement.

Is there anything you'd like reviewers to focus on?

This is my very first open-source contribution, so all kinds of feedback is appreciated.

CHANGELOG.md entry:

[new-rule] unnecessary-else

[palantirtech]

Thanks for your interest in palantir/tslint, @debsmita1! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

[JoshuaKGoldberg]

Great start! Some architectural changes should be made.

[debsmita1]

I have addressed all your comments.

Cant figure out why these 2 are failing. Could you please review it again @JoshuaKGoldberg

[JoshuaKGoldberg]

Looks close, thanks for the docs & code examples! Just waiting on simplifying the recursive calls and adding handling for non-block if bodies.

[JoshuaKGoldberg]

It looks like you have a few different ts.forEachChilds: individual ones for the node's elseStatement and thenStatement if it's an IfStatement, but just a regular recursion otherwise? You should be able to simplify this a bit by only having one call to ts.forEachChild that's always called.

Also, if jumpStatement is undefined but node.elseStatement isn't, this code will skip it. That seems like a potential bug?

[debsmita1]

Done be2f8b4

[JoshuaKGoldberg]

Sorry @debsmita1 I think I wasn't very clear here. I'm suggesting you don't need to have ts.forEachChild statements in the big if (utils.isIfStatement block:

if (utils.isIfStatement(node)) {
    // all your glorious rule logic here
    // no ts.forEachChild in this area
}

ts.forEachChild(node, cb);

Note the lack of else statement: a single ts.forEachChild should recurse on all the node's children no matter what. Could you please switch to that? It's a bit less code and better practice to minimize recursive calls (since they can get tricky in larger rules).

[debsmita1]

Done f36d420

[JoshuaKGoldberg]

This is looking great! A few simplifications to the internals and error message to go, I think.

Also, about the build failures: the build is running linting with this rule enabled and finding failures in TSLint's code base. So it looks like the rule is working! 😄 You'll need to fix those failures to get the build to pass.

[JoshuaKGoldberg]

Sorry @debsmita1 I think I wasn't very clear here. I'm suggesting you don't need to have ts.forEachChild statements in the big if (utils.isIfStatement block:

if (utils.isIfStatement(node)) {
    // all your glorious rule logic here
    // no ts.forEachChild in this area
}

ts.forEachChild(node, cb);

Note the lack of else statement: a single ts.forEachChild should recurse on all the node's children no matter what. Could you please switch to that? It's a bit less code and better practice to minimize recursive calls (since they can get tricky in larger rules).

[JoshuaKGoldberg]

I also can't figure out why those are failing at first glance. Very spooky. If they're still failing after the introduced lint complaints are fixed, we can take a deeper look...

[JoshuaKGoldberg]

Whoohoo, this looks great! Thanks for sticking through review @debsmita1; very exciting to have this rule in! 🙌

(additional thanks for fixing all the complaints it found across existing source files...!)

[adidahiya]

I find this rule pretty opinionated, sometimes impairing readability, and I don't want to enable this in the tslint codebase. Sorry I didn't get around to look at this before it got merged, but I think we should disabled this for tslint's own code in tslint.json at the root of this repo, and I would not add it to tslint:recommended or tslint:latest.

[adidahiya]

I think this is a purely stylistic rule; is there any functional difference?