Skip to content
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

Allow switch statement type guards (need help..) #2230

Closed
wants to merge 1 commit into from

Conversation

bsteephenson
Copy link

Hi. This PR resolves issue #2214 allowing the following code to work

function logNumber(v: number) { console.log("number:", v); }
function logString(v: string) { console.log("string:", v); }

function foo1(v: number|string) {
    switch (typeof v) {
        case 'number':
            logNumber(v);
            break;
        case 'string':
            logString(v);
            break;
        default:
            throw new Error("unsupported type");
    }
}

I need some help (I'm kinda new to this..)

  1. TypeOfExpression doesn't have a member "text" but the Javascript equivalent does. What can I cast TypeOfExpression to to get access to "text"?
  2. I'm not sure where I should be writing tests.. (Or what a .types file is)
  3. This might be too permissive. In the previous example, if I were to have a case 'Watermelon' it would still work. Should I check to see that the type is a subset of v's type?

Thank you!

@msftclas
Copy link

msftclas commented Mar 6, 2015

Hi @bsteephenson, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

This seems like a small (but important) contribution, so no Contribution License Agreement is required at this point. Real humans will now evaluate your PR.

TTYL, MSBOT;

@yuit
Copy link
Contributor

yuit commented Mar 6, 2015

Regarding test in our repository, all tests are located inside tests/cases. For Typeguard feature, there is a folder in tests/cases/conformance/expressions/typeGuards. You can certainly put your tests in there. I would try to be explicit with naming the test file. By default test file is emitted below ES6 if you would like the test to be emitted as es6 including "// @target: es6" at the first line of the file. and include "ES6" as part of the file name.

The test file is simply .ts file. When the test runner runs, it generates corresponding .js (plain javascript file), .type (this file only get generated if the ts file is compiled with no error. '.type' file specifies a type of each variable in each statement) and .errors.txt (only if there are errors).

case SyntaxKind.CaseClause:
// In a case clause of a switch statement using typeof
var parent = (<CaseClause>node).parent;
if((<SwitchStatement>parent).expression.kind === SyntaxKind.TypeOfExpression){
Copy link
Member

Choose a reason for hiding this comment

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

Space after if.

Copy link
Contributor

Choose a reason for hiding this comment

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

Make sure the parent is guaranteed to be a switch statement. It is possible that the parser lets through case labels that are not parented by a switch. Please add either an assert or a bailout condition.

@DanielRosenwasser
Copy link
Member

To add to what @yuit said, you'll need to ensure that you're writing uniquely named tests.

Decent test names for this might be

  • typeGuardsSwitchStatement[SpecificTestName].ts
  • typeGuardsSwitchStatement[SpecificTestName]ES6.ts

Feel free to take inspiration for existing tests; in fact, you should be testing at least the same features as are tested in if/else-style type guards, but with switches. For instance, if you notice the test tests/cases/conformance/expressions/typeGuard/typeGuardsDefeat.ts, you should do basically the same thing.

You should be testing with strings like

  • "string"
  • "number"
  • "boolean"
  • "symbol"
  • "object"
  • "watermelon" (or something more seasonal for March - perhaps "apple" or "rhubarb")

Fourslash tests would need to be added too. If we actually move further along, we can guide you through this process.

This might be too permissive. In the previous example, if I were to have a case 'Watermelon' it would still work. Should I check to see that the type is a subset of v's type?

Just as we don't do anything in an if type guard, nothing should be done there.


I would imagine that this change immediately prompts the need for getting the appropriate union type in the case of a fallthrough, or potentially eliminating accounted cases within the default clause.

function f(v: string | number | boolean) {
    switch (typeof v) {
        case 'number':
            // 'v' has type 'number'
        case 'string':
            // 'v' now has type 'number | string'
            break;
        default:
            // 'v' has type 'boolean'
    }

So we would have to do some sort of control flow analysis to do this. We could have this change temporarily, but I imagine the behavior might be somewhat surprising. I think we should discuss it further.

@JsonFreeman
Copy link
Contributor

To address the question of permissiveness, please follow the same model as narrowTypeByEquality

@ghost
Copy link

ghost commented Apr 24, 2015

@bsteephenson, this repo is using .editorextension. If you are developing using VS, you would probably want to install editorconfig extension, which automagically take editor formatting configuration from the .editorconfig file and apply to VS.

https://visualstudiogallery.msdn.microsoft.com/c8bccfe2-650c-4b42-bc5c-845e21f96328

Extensions are also available for vim, nodepad++, sublime etc.

@mhegazy
Copy link
Contributor

mhegazy commented Apr 24, 2015

This PR seems stale. @bsteephenson please refresh and resubmit if this is still something you still want to pursue.

@mhegazy mhegazy closed this Apr 24, 2015
@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.

6 participants