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

Declaration emit widens literal types in readonly properties #15881

Closed
evmar opened this issue May 16, 2017 · 11 comments
Closed

Declaration emit widens literal types in readonly properties #15881

evmar opened this issue May 16, 2017 · 11 comments
Assignees
Labels
Bug A bug in TypeScript Domain: Declaration Emit The issue relates to the emission of d.ts files
Milestone

Comments

@evmar
Copy link
Contributor

evmar commented May 16, 2017

TypeScript Version: 2.3.1
Code

$ cat > foo.ts
export const FOO = 'FOO';
export class Bar { readonly type = FOO; }
$ tsc --declaration foo.ts
$ cat foo.d.ts
export declare const FOO = "FOO";
export declare class Bar {
    readonly type: string;
}

EDIT: added readonly to clarify bug report

Expected behavior:
Bar.type is of type "FOO"

Actual behavior:
The type falls back to string

@mhegazy
Copy link
Contributor

mhegazy commented May 16, 2017

This is not specific to declaration emit.

The general idea is that a property declaration (or any other mutable declaration e.g. let or var) has no use of a literal type. you would not declare a variable with only one possible value usually. in these cases you would use a const. similarly for property declarations, you would use readonly.

export const FOO = "FOO";
export class Bar {
     readonly type = FOO
}

for non-const, non-readonly declaration the type is widened to its base type, in this case string.

You can always put a type annotation to tell the compiler what type you want the property to be.

export class Bar {
    type: typeof FOO = FOO
}

@mhegazy mhegazy added the Working as Intended The behavior described is the intended behavior; this is not a bug label May 16, 2017
@alex-okrushko
Copy link

alex-okrushko commented May 16, 2017

@mhegazy

Readonly is also widened:

cat > foo.ts
export const FOO = 'FOO'; 
export class Bar {readonly type = FOO;}

$ tsc --declaration foo.ts
$ cat foo.d.ts 

export declare const FOO = "FOO";
export declare class Bar {
    readonly type: string;
}

@evmar
Copy link
Contributor Author

evmar commented May 16, 2017

@mhegazy sorry I changed the report to add readonly

@evmar evmar closed this as completed May 16, 2017
@evmar evmar reopened this May 16, 2017
@mhegazy mhegazy added Bug A bug in TypeScript and removed Working as Intended The behavior described is the intended behavior; this is not a bug labels May 16, 2017
@mhegazy mhegazy added this to the TypeScript 2.4 milestone May 16, 2017
@mhegazy
Copy link
Contributor

mhegazy commented May 16, 2017

thanks for pointing that. it is indeed a bug in the declaration emitter.

@sandersn sandersn added this to Not started in Rolling Work Tracking May 16, 2017
@evmar
Copy link
Contributor Author

evmar commented May 18, 2017

Workaround: whenever you export a class, write properties with an extra typeof.

export class Bar {
  readonly type: typeof FOO = FOO;
}

OliverJAsh added a commit to OliverJAsh/twitter-api-ts that referenced this issue May 30, 2017
@sandersn sandersn moved this from Not started to In Progress in Rolling Work Tracking May 30, 2017
@sandersn sandersn changed the title tsc --declaration loses literal types Declaration emit widens literal types in readonly properties May 30, 2017
@sandersn
Copy link
Member

sandersn commented Jun 5, 2017

This is actually the correct behaviour for readonly as per the rules in #10676 (more succint explanation in #11126) Specifically, this sequence:

const foo = "FOO" // widening literal type
class C { readonly cfoo = foo } // still widening
let usage = new C().cfoo // widens to 'string'

Should behave similarly even if it's split across two files:

// @Filename: foo.d.ts
const foo = "FOO" // widening literal type (special-cased: see below)
class C { readonly cfoo: string } // don't use a non-widening literal type

// @Filename: use.ts
import { C } from './foo'
let usage = new C().cfoo // so usage will still be of type 'string'

Note that there is a special case for const but not for readonly: the declaration emit for const foo = "FOO" is const foo = "FOO" not const foo: "FOO". const foo = "FOO" is more flexible because it's a widening literal type. It's not wrong in the way that const foo: string or const foo: "FOO" is. But this special case only applies to simple constant assignments; for more complex assigments the emit is still const foo: string:

const foo = "FOO"
const bar = "BAR"
const both = foo || bar

emits:

const foo = "FOO"
const bar = "BAR"
const both: string

which is essentially the same as the emit for class C { readonly x = foo }, where the simple-assignment special case doesn't exist.

I believe that simple constant assignment is special-cased for historical reasons, and because people tend to simple constants. I suspect they do this a lot more than they export classes full of readonly literal types.

We could change readonly to have the same special case as const, but it would need some justification; this is a breaking change. Old versions don't allow class C { readonly x = "foo" } in a .d.ts. We could soften the break by allowing it and then changing declaration emit a version or two later. But that's a lot of work for something we don't know whether people actually use.

@gcanti @OliverJAsh @evmar can you give examples of use? I propose leaving this issue open until we have good evidence that (1) a lot of people would use this and (2) this pattern is better than what works today.

@sandersn sandersn removed this from In Progress in Rolling Work Tracking Jun 5, 2017
@sandersn sandersn modified the milestones: Future, TypeScript 2.4 Jun 5, 2017
@sandersn sandersn added the Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature label Jun 5, 2017
@gcanti
Copy link

gcanti commented Jun 8, 2017

My use case is simulating nominal types:

class A { readonly _tag = 'A' }
class B { readonly _tag = 'B' }

emits

export declare class A {
  readonly _tag: string;
}
export declare class B {
  readonly _tag: string;
}

which leads to

import { A, B } from 'foo'
declare function f(a: A): void
f(new B()) // <= no error

Adding an explicit type annotation

class A { readonly _tag: 'A' = 'A' }
class B { readonly _tag: 'A' = 'B' }

emits

export declare class A {
  readonly _tag: 'A';
}
export declare class B {
  readonly _tag: 'B';
}

and now I get the expected error

import { A, B } from 'foo'
declare function f(a: A): void
f(new B()) // <= ERROR: Type '"B"' is not assignable to type '"A"'

@OliverJAsh
Copy link
Contributor

I had the very same use case as @gcanti, which you can see I had to work around with explicit type annotations in OliverJAsh/twitter-api-ts@5282684.

OliverJAsh added a commit to OliverJAsh/twitter-api-ts that referenced this issue Jun 8, 2017
@sebinsua
Copy link

sebinsua commented Jun 15, 2017

Is this why const obj = Object.freeze({ a: 0, b: 50, c: 90 }) gives obj a type of ReadOnly<{a: number, b: number, c: number}> rather than a ReadOnly<{a: 0, b: 50, c: 90}>?

I've been playing around with this, since I was hoping that I could do some computation to help generate a type of 0 | 50 | 90. Since the types are being widened that doesn't seem to be possible...

@pimterry
Copy link
Contributor

pimterry commented Dec 1, 2017

I'm also hitting this, in similar circumstances. I want to define classes with a readonly field with a string literal type for a discriminated union, and use the value of those types as keys in a separate dictionary (minimal example below). This bug means that while this pattern works fine inside a single typescript module, it breaks completely if I try to export type definitions and use those types elsewhere.

This is happening because in my case this bug causes tsc to generates invalid type definitions (i.e. definitions will never type check).

One of my exported functions' type signatures only compiles when given string literals, so is fine in the normal TS that does use them, but throws Type 'T' cannot be used to index type 'ObjectWithTheLiteralsAsKeys' if you try to compile anything using the type definition output, because this bug means T is string, not 'a' | 'b' | 'c'. Playground example.

@thw0rted
Copy link

thw0rted commented Aug 17, 2018

I'm running into a similar problem but I can't tell if it's exactly what's described here, or if last week's patch will fix it. I'm trying to Object.freeze something with a union-literal type, but I can't assign the result to Readonly<MyType> because freeze is widening the union literal to string.

Playground example is probably easier to follow -- you should see the error Type 'string' is not assignable to type '"a" | "b" | "c"'..

Will the patch address this problem? If not, is it already tracked or should I put in a new ticket?

ETA: since I'm calling freeze on an object literal, I guess maybe my actual problem could be #20195 ? The literal's (member) type is automatically widened before it gets to freeze, so that call is returning the widened type. I don't know whether that needs to be addressed in this issue or that one...

@RyanCavanaugh RyanCavanaugh added Suggestion An idea for TypeScript and removed Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript labels Aug 20, 2018
@RyanCavanaugh RyanCavanaugh modified the milestones: TypeScript 3.1, Future Aug 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Domain: Declaration Emit The issue relates to the emission of d.ts files
Projects
None yet
Development

No branches or pull requests