Skip to content

Issue#393 #1091

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Issue#393 #1091

wants to merge 1 commit into from

Conversation

dekajp
Copy link

@dekajp dekajp commented Nov 7, 2014

Adds /* falls through */ in case statement, if break missing.

Tests not included ( working on )
#393

I am trying to build in `jake local' and 'jake tests' getting an error error TS6053: File 'Data.ts' not found. but 'jake LKG' seems to be working

Adds /* falls through */ in case statement, if `break` missing. Tests not included ( working on )
var isBreakExists: boolean = false ;
isBreakExists = forEach(node.statements, node => {
if (node.kind === SyntaxKind.BreakStatement) {
return true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it could be something like this
case SyntaxKind.... : return writer.write(....); and this shouldn't be fall through

@DanielRosenwasser
Copy link
Member

First off, thanks for sending us a PR, we love contributions and interesting ideas for the compiler like this.

On one hand, this is nice because it's explicit and gives nice output. On the other, if the user didn't ask for it, I feel like this might be more trouble than it's worth, especially since I believe we'd preserve comments like /* fall through */ anyhow. Let me elaborate a bit.

@sheetalkamat gave a simple example for which this is currently misleading. But what about something marginally more complex like

switch (foo.kind) {
    case SyntaxKind.BarToken:
        if (foo.blah === "xyz") {
            break;
        }
        else {
            break;
        }
    case SyntaxKind.BazToken:
        // ...
        break;
}

So even if you also handled returns, in this case, you'd end up with something like

switch (foo.kind) {
    case 0 /* BarToken */:
        if (foo.blah === "xyz") {
            break;
        }
        else {
            break;
        }
        /* falls through */
    case 42 /* BazToken */:
        // ...
        break;
}

And so the user would be annoyed to see that /* fall through */ because it's wrong, and any reader other than the original author would be confused.

The answer might be to traverse the whole subtree with forEachChild to exhaustively check each branch, and @vladima might have recently done some work relating to this, but it might not be worth it.

@RyanCavanaugh
Copy link
Member

I thought the idea behind #393 was that the compiler code itself should have documented fall-throughs, not that we would auto-generate a comment indicating a fall-through. I don't see any value in the latter.

@DickvdBrink
Copy link
Contributor

#393 was a issue I created. What I intended was that the code should be documented that this was intended because if it wasn't intended it might cause weird side effects. So I did not intend automatic comment insertion.

@dekajp dekajp mentioned this pull request Nov 7, 2014
@dekajp
Copy link
Author

dekajp commented Nov 7, 2014

I am closing this PR , we can discuss this Issue on #393 .

@dekajp dekajp closed this Nov 7, 2014
@microsoft microsoft locked and limited conversation to collaborators Jun 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants