Missing type-check for generic types (?) #12023

Open
mindplay-dk opened this Issue Nov 3, 2016 · 18 comments

Projects

None yet

6 participants

@mindplay-dk
mindplay-dk commented Nov 3, 2016 edited

I've run into a mysterious case with a return-type that is clearly wrong - in methods like this one:

interface Foo {
    a: string
    b: number
}

class Hello {
    getFoo(): Promise<Foo> {
        return Promise.resolve("a string")
    }
}

The declared return-type in this example is Promise<Foo>, but the function returns Promise<string>, yet there is no notice/warning/error for this - it compiles without complaint.

In fact, it doesn't matter what I put in that return type-hint - I can return whatever I want, not just Promises, but any unrelated type, anything goes.

Is this really working as intended? What gives?

@HerringtonDarkholme
Contributor

Please follow contributing guide and provide a minimal reproducible example. I cannot know what happened without knowing http.post type.

@mindplay-dk

Sorry - I updated with a minimal example demonstrating the problem.

@mhegazy
Contributor
mhegazy commented Nov 3, 2016
c:\>type c:\test\a.ts
interface Foo {
    a: string
    b: number
}

class Hello {
    getFoo(): Promise<Foo> {
        return Promise.resolve("a string")
    }
}
c:\>tsc --v
Version 2.1.0-dev.20161103

c:\>tsc c:\test\a.ts  --lib es6
test/a.ts(8,16): error TS2322: Type 'Promise<string>' is not assignable to type 'Promise<Foo>'.
  Type 'string' is not assignable to type 'Foo'.
@mindplay-dk

It appears there's a bug in the current stable release.

$ tsc -v
Version 2.0.7

$ tsc hello.ts --lib es6

$ npm remove -g typescript
- typescript@2.0.7 node_modules\typescript

$ npm install -g typescript@next
...
$ tsc -v
Version 2.1.0-dev.20161104

$ tsc hello.ts --lib es6
hello.ts(8,16): error TS2322: Type 'Promise<string>' is not assignable to type 'Promise<Foo>'.
  Type 'string' is not assignable to type 'Foo'.
@mindplay-dk

For the record, upgrading a small codebase from the stable release to the current unstable version revealed dozens of type-related issues not reported by the stable version - at least 6 or 7 type declarations in a codebase of ~ 200 LOC had errors that weren't being reported, some related to Promise generics, some simply to generic Arrays.

This is a pretty serious problem. Unless release 2.1 is right around the corner, probably whatever was fixed ought to be merged to the stable version?

@HerringtonDarkholme
Contributor

Confirmed. This is a duplication of #10524 and #10785. Fixed in 9d4219a

@aluanhaddad
Contributor

@mindplay-dk 2.1 introduces a slew of very advanced new features. It is fantastic. So it is not that 2.0.x is not a viable option but rather that 2.1 exponentially improves the type system. Then we have async/await in es3/es5. What I do these days is build production apps using a project-local build, just updated to 2.0.7, and run VS Code powered by 2.1. That requires that I keep track of which features I can use, but the improvements in static analysis -> compiler catching real bugs makes it worth it.

@mindplay-dk

So it is not that 2.0.x is not a viable option but rather that 2.1 exponentially improves the type system. [...] the improvements in static analysis -> compiler catching real bugs makes it worth it.

This is a substantial change to compiler behavior - it's not merely an "improvement", it's either a breaking change (in which case the minor version increase is misleading and will cause a lot of problems for a lot of developers), or a bugfix.

I'm fairly certain it's a bugfix though, because I'm pretty sure this used to work correctly in the past? If I'm wrong about that (if this is in fact a "new feature") then labeling this as 2.1 is IMHO a very slippery slope, as existing code using a version constraint like ^2.0 will start to emit tons of errors after this update.

If this is a bugfix, IMO the responsible thing would be a bugfix release.

I'm grateful for the coming 2.1 release, but I'm concerned about the potential for problems like this in the future, as we're about to start exposing our entire team to TypeScript very soon - someone new to TS would not understand that this was a bug (if it is a bug) and would have likely written lots of bad code, or learning bad habits, before we'd have a chance to stop them...

@mindplay-dk
mindplay-dk commented Nov 4, 2016 edited

Anyone know when this bug was introduced? Maybe I should go back to a previous version for the time being, e.g. using a version constraint like 2.0.x until 2.1 is stable? (if no patched 2.0.x release is planned?)

@HerringtonDarkholme
Contributor

You can target at ES5 and uses a fixed promise definition.

@mindplay-dk

@HerringtonDarkholme my target language for this project is ES6.

@mindplay-dk

@HerringtonDarkholme my point is, are these case-specific work-arounds to a bug?

@HerringtonDarkholme
Contributor

Yes, manually excluding promise lib is a work around. If you cannot wait until 2.1 release.
The problem is promise definition is not well written, as the fix only modified promise definition, so you just need to avoid that lib. TS itself is working quite well here.

@mhegazy
Contributor
mhegazy commented Nov 4, 2016

The new error raised in not caused by a compiler change but by a library change. as @HerringtonDarkholme pointed out, you can have your custom version of the library that the compiler checks against. either by including specific pieces using --lib or by replacing the whole thing using --noLib.

The change is indeed breaking, and should be added to the list of breaking changes for the TypeScript 2.1 release.

@mhegazy mhegazy removed the Needs More Info label Nov 4, 2016
@mhegazy
Contributor
mhegazy commented Nov 4, 2016

I should add that library fixes do happen. we keep library changes in major releases, so you should not expect changes to library in a servicing release for 2.0.* for instance.

@mhegazy mhegazy added the Duplicate label Nov 4, 2016
@whymarrh

If I understand correctly this might be the same issue in v2.1.5 with the following example:

class A {
    constructor(readonly message: string) {}
}

class B extends A {
    lastCharacter() {
        return this.message.substring(this.message.length - 1);
    }
}

const pick: (s: string) => Promise<B> = (s: string) =>
    Promise.resolve(new A(s));
@whymarrh

Er, sorry, what issue is this a duplicate of?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment