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

Union Type inference not working in class (ts version 1.4) #1706

Closed
erichillah opened this issue Jan 17, 2015 · 9 comments
Closed

Union Type inference not working in class (ts version 1.4) #1706

erichillah opened this issue Jan 17, 2015 · 9 comments
Labels
Help Wanted You can do this Suggestion An idea for TypeScript
Milestone

Comments

@erichillah
Copy link

Hi,
Union type inference is not working inside classes functions. For example trying to compile the below code :

type NameOrNameArray = string | string[];

class NameCreator {

    constructor(public name:NameOrNameArray) {}

    createName():string {
        if (typeof this.name === "string") {
            return name;
        }
        else {
            this.name.forEach((elem)=>{
                console.log(elem);
            });
            return this.name.join(" ");
        }
    }
}

gives the error :

Property 'forEach' does not exist on type 'string | string[]'.

The same code works well when using a function. Same error in Visual studio and in the Playground.

@erichillah
Copy link
Author

I am able to remove the error only by using explicit type assertion

(<string[]>this.name).forEach((elem)=>{
    console.log(elem);
 });
 return (<string[]>this.name).join(" ");

@ahejlsberg
Copy link
Member

Type guards only work on simple variables, not properties of an object, so you need to copy the property into a local variable.

type NameOrNameArray = string | string[];

class NameCreator {

    constructor(public name: NameOrNameArray) {}

    createName(): string {
        var name = this.name;  // Copy into variable
        if (typeof name === "string") {
            return name;
        }
        else {
            name.forEach(elem => {
                console.log(elem);
            });
            return name.join(" ");
        }
    }
}

We have this restriction because we want to track (to the best of our abilities) that there are no assignments to the variable in the guarded section. This is pretty much impossible to do correctly for properties without extensive flow and alias analysis. For example:

function printString(s: string) { ... }
class Foo {
    name: string | string[];
    foo() {
        if (typeof this.name === "string") {
            this.bar();
            printString(this.name);  // Ok?
        }
    }
    bar() {
        this.name = ["hello"];
    }
}

This is just a simple example. You can construct increasingly impossible ones with little effort.

A possible alternative to our current design is to allow type guards to work on dotted names of the form x.y.z or this.x.y.z and to ignore the effects of assignments to such names (or parts of them). But it would mean that we wouldn't catch errors like the one in the example above.

@ahejlsberg ahejlsberg added Suggestion An idea for TypeScript In Discussion Not yet reached consensus labels Jan 17, 2015
@erichillah
Copy link
Author

Thanks, i get a much better understanding of type guard. The use of class properties or 'global variables' doesn't really make sense as it's pretty much impossible to make sure that the condition typeof this.name === "string" will still be true accross the type guarded scope. So i guess it'll still be confusing to allow dotted name in type guards.

@antoinerousseau
Copy link

Hi, is this issue going to be fixed?

@RyanCavanaugh
Copy link
Member

We still need to work out the exact semantics of how this would work. It's less clear exactly what forms should be allowed since non-local effects are much easier on class members. Consider, for example:

class Thing {
    x: string|number;

    ensureXisString() {
        this.x = this.x.toString();
    }   

    someMethod() {
        if(typeof this.x === 'number') {
            this.ensureXisString();
            // Narrowed to this.x: number here, which is 100% wrong
        }
    }
}

@danieljsinclair
Copy link

It would be nice if type guards recognised Array.isArray()

this doesn't seem to work either (v1.4)
instanceof Array

@ahejlsberg
Copy link
Member

@danieljsinclair The x instanceof Array issue was fixed in #1657.

@RyanCavanaugh RyanCavanaugh added Help Wanted You can do this and removed In Discussion Not yet reached consensus labels Apr 27, 2015
@RyanCavanaugh RyanCavanaugh added this to the Community milestone Apr 27, 2015
@RyanCavanaugh
Copy link
Member

Tentatively approved. Need to see how this impacts real-world code.

@jakebailey
Copy link
Member

Just looking at old issues; this one was fixed in TS 2.0 when control flow analysis was introduced.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Help Wanted You can do this Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

6 participants