Add warning message empty THEN clause

[MartyIX]

Proposed patch for issue #5109.

[MartyIX]

I'm at loss how my pull request managed to break commentsBlocks.ts test.

[sandersn]

Can you add a test that shows that

if(someComplexConditionICantBeBotheredToInvert) {
}
else {
    // some code ...
}

does not warn? It's not good style, but it's intended, so I want to document that it will not warn.

[MartyIX]

Sure, if you guys decide that you want to merge this feature - #5375 (comment)

[mhegazy]

the fourslash test likely failed because of violating incremental parsing assumptions. i would move the check to cheker.ts.

[DanielRosenwasser]

After thinking about this more, I'm starting to wonder whether or not this is a good idea. We say many other errors can be taken care of by a linter. It sets a slightly odd precedent if we pick this case specially.

[MartyIX]

@DanielRosenwasser This pull request would add a very first warning to diagnosticMessages.json. It's up to you guys. :-)

[mhegazy]

After thinking about this more, I'm starting to wonder whether or not this is a good idea. We say many other errors can be taken care of by a linter. It sets a slightly odd precedent if we pick this case specially.

I think the line is fairly subjective, e.g. the rechability checks. but my reasoning is that this is an issue that is wrong 99.9% of the time, and not just a matter of best practices.

@MartyIX please let us know if you are still interested in perusing this PR, and if so, please move the check to the checker as i noted earlier, and update the tests.

[MartyIX]

@mhegazy Yes, I'm still interested in this PR, so I'll modify it. :)

[MartyIX]

@mhegazy I have updated the PR.

[RyanCavanaugh]

Just make this an error the same as any other (probably in the 1xxx category); we don't need to introduce a new concept of a "warning" right now

[MartyIX]

Do you mean a new concept for users? I can see Warning here: https://github.com/Microsoft/TypeScript/blob/master/src/compiler/types.ts#L2043 ...

[billti]

Interesting... I'll sometimes write code with just a // TODO: Handle blah... in the else (or even if) clause while work is in progress. So this is going to be an error in my compilations now until I finish the TODO?

I agree with @DanielRosenwasser . Seems firmly in the realm of linter-land. There is absolutely nothing wrong syntactically, semantically, or in the use of types by having an empty block.

[MartyIX]

@billti I'm not sure how your code looks like exactly, but the idea was that for the following script

// Show warning
if (1);

// Dont show warning
if (1) {
    // TODO: Handle blah...
}     

the compiler should return:

C:\Work\TypeScript>node built\local\tsc.js is5109.ts
is5109.ts(2,7): warning TS20000: The body of an 'if' statement cannot be the empty statement.

To clarify further, EmptyStatement is not the same as empty block.

[billti]

Ah, right. Fair enough then. Yes, almost certainly a bug to warn on :-)

[MartyIX]

@RyanCavanaugh I have made the modification as you requested.

[mhegazy]

thanks!

[RyanCavanaugh]

👍

[mhegazy]

thanks @MartyIX!