Skip to content

Do not widen literal types when emitting declarations #55445

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Do not widen literal types when emitting declarations #55445

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47230,7 +47230,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
// Get type of the symbol if this is the valid symbol otherwise get type at location
const symbol = getSymbolOfDeclaration(declaration);
let type = symbol && !(symbol.flags & (SymbolFlags.TypeLiteral | SymbolFlags.Signature))
? getWidenedLiteralType(getTypeOfSymbol(symbol))
? getTypeOfSymbol(symbol)
: errorType;
if (
type.flags & TypeFlags.UniqueESSymbol &&
Expand Down
4 changes: 2 additions & 2 deletions tests/baselines/reference/ambientConstLiterals.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,5 +74,5 @@ declare const c9: {
declare const c10: number[];
declare const c11: string;
declare const c12: number;
declare const c13: string;
declare const c14: number;
declare const c13: "abc" | "def";
declare const c14: 123 | 456;
Comment on lines +77 to +78
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interestingly I think this case came up during our talks about isolatedDeclarations. The code for these two is:

const c13 = Math.random() > 0.5 ? "abc" : "def";
const c14 = Math.random() > 0.5 ? 123 : 456;

@RyanCavanaugh @DanielRosenwasser

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I just realized who reported the bug, it is directly related!

16 changes: 16 additions & 0 deletions tests/baselines/reference/declarationEmitBindingPatterns2.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
//// [tests/cases/compiler/declarationEmitBindingPatterns2.ts] ////

//// [declarationEmitBindingPatterns2.ts]
// https://github.com/microsoft/TypeScript/issues/55439

function foo(): { y: 1 } {
return { y: 1 };
}

export const { y = 0 } = foo();




//// [declarationEmitBindingPatterns2.d.ts]
export declare const y: 1 | 0;
17 changes: 17 additions & 0 deletions tests/baselines/reference/declarationEmitBindingPatterns2.symbols
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
//// [tests/cases/compiler/declarationEmitBindingPatterns2.ts] ////

=== declarationEmitBindingPatterns2.ts ===
// https://github.com/microsoft/TypeScript/issues/55439

function foo(): { y: 1 } {
>foo : Symbol(foo, Decl(declarationEmitBindingPatterns2.ts, 0, 0))
>y : Symbol(y, Decl(declarationEmitBindingPatterns2.ts, 2, 17))

return { y: 1 };
>y : Symbol(y, Decl(declarationEmitBindingPatterns2.ts, 3, 10))
}

export const { y = 0 } = foo();
>y : Symbol(y, Decl(declarationEmitBindingPatterns2.ts, 6, 14))
>foo : Symbol(foo, Decl(declarationEmitBindingPatterns2.ts, 0, 0))

21 changes: 21 additions & 0 deletions tests/baselines/reference/declarationEmitBindingPatterns2.types
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
//// [tests/cases/compiler/declarationEmitBindingPatterns2.ts] ////

=== declarationEmitBindingPatterns2.ts ===
// https://github.com/microsoft/TypeScript/issues/55439

function foo(): { y: 1 } {
>foo : () => { y: 1;}
>y : 1

return { y: 1 };
>{ y: 1 } : { y: 1; }
>y : 1
>1 : 1
}

export const { y = 0 } = foo();
>y : 1 | 0
>0 : 0
>foo() : { y: 1; }
>foo : () => { y: 1; }

2 changes: 1 addition & 1 deletion tests/baselines/reference/strictFunctionTypes1.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ declare function fs(x: string): void;
declare function fx(f: (x: "def") => void): void;
declare const x1: (x: string) => void;
declare const x2 = "abc";
declare const x3: string;
declare const x3: "def" | "abc";
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that there is this hidden widening thing encoded in the type and that it actually might widen when assigned to a mutable declaration. I think though that most people are not aware of this and it's more surprising that a type widened in the emitted declaration - people never audit them until something goes wrong.

The current behavior is also surprising in some cases. Let's expand on this very case here:

declare function f3<T>(obj: T, f1: (x: T) => void, f2: (f: (x: T) => void) => void): T;
declare function fo(x: Object): void;
declare function fx(f: (x: "def") => void): void;

const x3 = f3("abc", fo, fx);  // "abc" | "def"

let other: typeof x3 = x3
other = 'other' // error

As we can see we can't assign 'other' to other but yet in the emitted declaration file we can see this:

declare const x3: string;
declare let other: typeof x3;

It means that in practice we can end up with an error locally that wouldn't get reported if the same code would get used through the declaration file. I find that problematic.

The same could be said about the widening thing because "locally" an assignment would widen but if we change the declaration emit then the equivalent assignment would not widen since the widening behavior can't be encoded in the declaration file. I think though that's a smaller problem and a better tradeoff.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and it's more surprising that a type widened in the emitted declaration - people never audit them until something goes wrong.

Correct! And in most cases in declaration emit, the way we handle this is by preserving the literal initializer, rather than serializing a type annotation - that way the declaration has the same widening behavior as the original source. Eg, if you write

export const six = 6;

that is also, verbatim, the declaration emit. Unfortunately, right now this only covers simple cases of widening types - a heritage from our older, much simpler string-based declaration emitter.

IMO, rather than making an equally incorrect declaration in the other direction (preserving the literal types, rather than the widened type), we need to, once again, preserve the initializer to preserve the literal-widening behavior. In this case, that should mean making initializers with the same behavior.

Take the original issue:

function foo(): {y: 1} {
    return { y: 1 }
}

export const { y = 0 } = foo();

If we serialize it to

declare const _expr: {y: 1};
export declare const { y = 0 } = _expr;

y now behaves identically in the declaration file as it does in the original source file. Do note, this means that we'll need to allow identifiers in initializers in declaration files - something currently an error - but I think that's fine, especially since, despite the error, old versions of TS will still typecheck it correctly.

declare const x4: Func<string>;
declare const never: never;
declare const x10: string;
Expand Down
8 changes: 4 additions & 4 deletions tests/baselines/reference/stringLiteralTypesOverloads02.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,9 +104,9 @@ declare namespace Consts1 {
declare const string = "string";
declare const number = "number";
declare const boolean = "boolean";
declare const stringOrNumber: string;
declare const stringOrBoolean: string;
declare const booleanOrNumber: string;
declare const stringOrBooleanOrNumber: string;
declare const stringOrNumber: "string" | "number";
declare const stringOrBoolean: "string" | "boolean";
declare const booleanOrNumber: "number" | "boolean";
declare const stringOrBooleanOrNumber: "string" | "number" | "boolean";
declare namespace Consts2 {
}
11 changes: 11 additions & 0 deletions tests/cases/compiler/declarationEmitBindingPatterns2.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// @strict: true
// @declaration: true
// @emitDeclarationOnly: true

// https://github.com/microsoft/TypeScript/issues/55439

function foo(): { y: 1 } {
return { y: 1 };
}

export const { y = 0 } = foo();