Switch case fallthrough

[DickvdBrink]

There are numerous places where the code just fallsthrough a different case like here:
https://github.com/Microsoft/TypeScript/blob/02d0b024c695f059fb8209e301b30f80fddc3d08/src/compiler/checker.ts#L646 or here https://github.com/Microsoft/TypeScript/blob/02d0b024c695f059fb8209e301b30f80fddc3d08/src/compiler/emitter.ts#L118
There are also multiple places in the binder or parser where this is the case.
In my opinion it might be nice to write a comment there when it is intentional... or isn't it intentional?

Example for comment how for example jshint knows it is intentional:

switch(1) { 
    case 1: console.log("foo");
    /* falls through */
    case 2: console.log("bar");
}

In the above code both are written to the console.

[basarat]

👍

[dekajp]

PR #1091 - Let's discuss here

@RyanCavanaugh I thought JSHint is run on auto-generated JS files. Can you explain this compiler code itself should have documented fall-throughs @DickvdBrink I see why JSHint or Linters would like to have this documented in code . I feel a compiler options to warn user of `--WarnSwitchCaseFallThrough' could be helpful. @DanielRosenwasser agreed .

[dekajp]

Ah ! Similar issue is logged at Google Closure Compiler - https://code.google.com/p/closure-compiler/issues/detail?id=1104

[DickvdBrink]

@dekajp, if the compiler code itself, so for exapmle the two links in my start post had the /* falls through */ comment, then the intent from a developer perspective is clear... When I look at the code now, as somebody who isn't that familiar with the TypeScript compiler code base, I might think that somebody forgot the break or something.

Also, I created a linter rule for TypeScript files which was giving me errors about this stuff (I tried it on the TypeScript code base) (lint rule)

I agree an warning might be nice but as shown above, one could also used a linter to check the TypeScript files. So it is up to the other guys here if they want the extra code for a warning by the compiler or they might say to just use a linter

[dekajp]

@DickvdBrink if linter is already available to check this , i feel it is not necessary to have this check in compiler. You guys can decide

[dekajp]

@RyanCavanaugh @DanielRosenwasser Ping

[RyanCavanaugh]

What do you need here?

[dekajp]

emit /* fall through */ for all branches which does not return/break or we document code with intended /* fall through */

[RyanCavanaugh]

I don't think the compiler should try to emit comments on your behalf. It's confusing and would actively work against tools that try to check for those things for you.

We should, in the compiler code, add comments (us as humans 😄) to clarify where we have intentional fall-throughs, to avoid confusion.

[dekajp]

in reference to PR #1091 @DanielRosenwasser The answer might be to traverse the whole subtree with forEachChild to exhaustively check each branch,

I am working on this , and i think we should only consider if-then-else / do-while /try-catch-finally body must contain a break/return or subtree must contain break/return ,other conditional statement for or while does not guarantee it will execute hence case clause will always need /* fall through*/

So whole subtree may not be needed . below is one example , i have more such in my test cases ts file

case 31:
            var i = 0;
            for (; i > 10;) {
                i++;
               break;
               // never executes
            }
            /* fall through */

@RyanCavanaugh here is my working code , i am not sure how i should handle the comment /* fall through */ comparison. will need some direction here :) .https://github.com/dekajp/TypeScript/compare/issue_393_v1

[RyanCavanaugh]

This is already enforced by TSLint -- I'd prefer we not add linter-style rules to the compiler. We generally don't add style or convention-based rules, especially when existing tools can handle them fine.

See https://github.com/palantir/tslint/blob/master/src/rules/noSwitchCaseFallThroughRule.ts

[dekajp]

Ahh !! I think i did not understand your earlier message very well. I thought in same way , but did not get an affirmation ( check my previous message) . Anyways i see your point you want to add /* fall through */ in all compiler source files. basically make compiler code TSLint compliant.

[dekajp]

#1287 👍

[DanielRosenwasser]

I think we can close this now.

[DickvdBrink]

Yeah, think so :)