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

Erroneous error when assigning to string literal union type #11714

Closed
pelotom opened this issue Oct 18, 2016 · 9 comments
Closed

Erroneous error when assigning to string literal union type #11714

pelotom opened this issue Oct 18, 2016 · 9 comments
Labels
Fixed A PR has been merged for this issue

Comments

@pelotom
Copy link

pelotom commented Oct 18, 2016

TypeScript Version: 2.0.3 / nightly (2.1.0-dev.201xxxxx)

Code

type T = {
  xOrY: 'x' | 'y'
}
const flag = true
const xOrY = flag ? 'x' : 'y' // inferred type is 'x' | 'y'
const t: T = { xOrY }

Expected behavior:

The code should type check.

Actual behavior:

The last line is flagged with an error:

Type '{ xOrY: string; }' is not assignable to type '{ xOrY: "x" | "y"; }'.
  Types of property 'xOrY' are incompatible.
    Type 'string' is not assignable to type '"x" | "y"'.

It can be fixed by giving xOrY a type annotation to say that it has type "x" | "y", but the compiler seems to have already inferred that it has this type (based on what atom-typescript's show-types-on-hover feature is telling me), so that doesn't seem like it should be necessary.

When the union is of non-string literal types, there is no such error.

@ahejlsberg
Copy link
Member

@pelotom This should be fixed by #10676 and I can't reproduce the error with the current nightly build. Can you confirm that you're seeing this problem with the nightly?

@ahejlsberg ahejlsberg added the Needs More Info The issue still hasn't been fully clarified label Oct 18, 2016
@pelotom
Copy link
Author

pelotom commented Oct 18, 2016

Yes, I just tried with typescript@next and it appears to be fixed. Thanks!

@pelotom pelotom closed this as completed Oct 18, 2016
@RyanCavanaugh RyanCavanaugh added Fixed A PR has been merged for this issue and removed Needs More Info The issue still hasn't been fully clarified labels Oct 19, 2016
@pelotom
Copy link
Author

pelotom commented Oct 19, 2016

I'm reopening this because I just found another very similar case which is still broken:

interface I {
  xOrY: 'x' | 'y'
}

class C implements I {
  xOrY = 'x'
}

This produces the error:

Class 'C' incorrectly implements interface 'I'.
  Types of property 'xOrY' are incompatible.
    Type 'string' is not assignable to type '"x" | "y"'.

Adding this seemingly superfluous type ascription fixes it:

class C implements I {
  xOrY = 'x' as 'x' | 'y'
}

@pelotom pelotom reopened this Oct 19, 2016
@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Oct 19, 2016

@pelotom this error is correct. Because the field isn't readonly, you could write code like this, which wouldn't be sound:

interface I {
  xOrY: 'x' | 'y'
}

class C implements I {
  xOrY = 'x'; // note: xOrY of type 'string'
}
let c = new C();
c.xOrY = 'z';
fn(c);
function fn(i: I) {
  if (i !== 'x' && i !== 'y') throw new Error('Inconceivable!');
}

Side note, correct inference for readonly fields is currently only in the master branch

@pelotom
Copy link
Author

pelotom commented Oct 19, 2016

But... xOrY is not of type string. The interface dictates that it must be of type 'x' | 'y'.

What I'm saying is that the inferred type of the field C.xOrY should be 'x' | 'y', because it could not type check any other way if it is to adhere to the interface I.

@pelotom
Copy link
Author

pelotom commented Oct 19, 2016

readonly doesn't enter into it, because even in the case where it's not readonly, if xOrY is understood to have type 'x' | 'y', you can't reassign something that doesn't conform:

interface I {
  xOrY: 'x' | 'y'
}
const c: I = { xOrY: 'x' }
c.xOrY = 'z' // ERROR: Type '"z"' is not assignable to type '"x" | "y"'.

@RyanCavanaugh
Copy link
Member

See #10570, #6118, #3667, #1373, etc

@mhegazy
Copy link
Contributor

mhegazy commented Oct 19, 2016

@pelotom implements clause does not contextually type class property declaration. this is a common issue we get. #10570 should be tracking this.

this is a different issue from the OP though. closing in favor of #10570.

@mhegazy mhegazy closed this as completed Oct 19, 2016
@pelotom
Copy link
Author

pelotom commented Oct 19, 2016

Ok, thanks for the pointer!

@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
Fixed A PR has been merged for this issue
Projects
None yet
Development

No branches or pull requests

4 participants