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

TS is incorrectly disallowing a variable assignment. #12005

Closed
DeegC opened this issue Nov 2, 2016 · 7 comments
Closed

TS is incorrectly disallowing a variable assignment. #12005

DeegC opened this issue Nov 2, 2016 · 7 comments
Labels
Question An issue which isn't directly actionable in code

Comments

@DeegC
Copy link

DeegC commented Nov 2, 2016

TypeScript Version: 2.0.3

Code

All of the code in this issue can be run in the playground.

The following gives this compile error: "Type 'EntityInstance' is not assignable to type 'EntityInstance'."

export class EntityInstance {
    public deleted = false;
    private children = new EntityArray<EntityInstance>();

    getChildren(): EntityArray<EntityInstance> {
        return this.children;
    }
}

export class ExtendedInstance extends EntityInstance {
    public anotherProperty = true;

    getChildren(): EntityArray<ExtendedInstance> {
        return super.getChildren() as EntityArray<ExtendedInstance>;
    }
}

let ei = new ExtendedInstance();
ei.getChildren()[0].anotherProperty = false;

export class EntityArray<EntityInstance> extends Array<EntityInstance> {

    delete(index?: number) {
        let ei = new EntityInstance();
        ei = this.splice( index, 1 )[0];
        ei.deleted = true;
    }
}

Expected behavior:

This should be allowed. It appears that TS thinks that EntityInstance as specified in "EntityArray" is a different type from EntityInstance. The former EntityInstance doesn't appear to have type information. For example, if I rewrite the delete() as follows there is an error because TS doesn't know about the 'deleted' property:

    delete(index?: number) {
        let ei = this.splice( index, 1 )[0];
        ei.deleted = true;
    }

Actual behavior:

TS raises compile error.

More notes:

I could define EntityArray without the <> (which then correctly determines the types in delete) but then I lose type information when I call ExtendedInstance.getChildren(). For example, the above code fails when rewitten as:

export class EntityInstance {
    public deleted = false;
    private children = new EntityArray();

    getChildren(): EntityArray {
        return this.children;
    }
}

export class ExtendedInstance extends EntityInstance {
    public anotherProperty = true;

    getChildren(): EntityArray {
        return super.getChildren();
    }
}

let ei = new ExtendedInstance();
ei.getChildren()[0].anotherProperty = false;

export class EntityArray extends Array<EntityInstance> {

    delete(index?: number) {
        let ei = new EntityInstance();
        ei = this.splice( index, 1 )[0];
        ei.deleted = true;
    }
}

I can get by the original error by casting to in the delete method but who wants to do that in Typescript?

delete(index?: number) { 
    let ei = this.splice( index, 1 )[0] as any; 
    ei.deleted = true; 
}
@RyanCavanaugh RyanCavanaugh added the Working as Intended The behavior described is the intended behavior; this is not a bug label Nov 2, 2016
@RyanCavanaugh
Copy link
Member

It appears that TS thinks that EntityInstance as specified in "EntityArray" is a different type from EntityInstance

It is. You declared a type parameter that shadowed the name:

class EntityArray<EntityInstance> extends Array<EntityInstance> {

You probably meant to write

class EntityArray extends Array<EntityInstance> {

@RyanCavanaugh RyanCavanaugh added Question An issue which isn't directly actionable in code and removed Working as Intended The behavior described is the intended behavior; this is not a bug labels Nov 2, 2016
@RyanCavanaugh
Copy link
Member

Or perhaps this:

class EntityArray<T extends EntityInstance> extends Array<T> {

@DeegC
Copy link
Author

DeegC commented Nov 3, 2016

Thanks Ryan, that solved my problem. It does seem like a confusing error message; would it be possible to flag the original EntityArray<EntityInstance> as an error for attempting to create a type with an already-existing name?

Either way, I appreciate the help.

@aluanhaddad
Copy link
Contributor

Shadowing is, for better or worse, a part of JavaScript that is fairly ubiquitous. Of course these are types, not Java Script values but it makes sense that the same naming rules would apply. Maybe just displaying that it's a type parameter in the error message would be helpful.

@RyanCavanaugh
Copy link
Member

It's kind of necessary to allow type name shadowing, otherwise we wouldn't be able to safely add things to the global namespace without the risk of a breaking change.

There's perhaps a suggestion lurking here which is that our error messages should do something (I don't know what) if we ever issue a message like "Cannot assign X to X" where both X's have identical spelling. I mean ideally you'd see something like "Cannot assign X (Type Parameter) to X (Interface)", but it's hard to guess if even that would cover all the cases.

@aluanhaddad
Copy link
Contributor

aluanhaddad commented Nov 4, 2016

There's perhaps a suggestion lurking here which is that our error messages should do something (I don't know what) if we ever issue a message like "Cannot assign X to X" where both X's have identical spelling. I mean ideally you'd see something like "Cannot assign X (Type Parameter) to X (Interface)", but it's hard to guess if even that would cover all the cases.

@RyanCavanaugh I was going to suggest this at first but there are a lot of lenses via which to look at a type so it could become ambiguous, or a just a best guess, in a lot of cases.

Maybe a simpler, higher value option would be to do something like

given Types A and B over assignment
where not A assignable to B
report IncompatableAssignementOfIdenticallyNamedTypesDiagnostic
when A name is B name
otherwise report IncompatibleTypesDiagnostic

So it would say something like "Type 'B' is not assignable to type 'A'. (note that they are not same Declaration)"

@DeegC
Copy link
Author

DeegC commented Nov 5, 2016

Would it be possible to make it illegal for the name of a generic type to shadow a previously defined type? I'm obviously no TS expert (yet!) but I can't think of a reason one would need to shadow a currently existing type. The error message could be something like "Generic type name 'EntityInstance' not allowed because it would shadow an existing type.'

That would potentially break some current client code; if that's forbidden maybe make it a warning?

@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Question An issue which isn't directly actionable in code
Projects
None yet
Development

No branches or pull requests

3 participants