Do not widen literal types when emitting declarations #55445
Conversation
typescript-bot
added
the
For Uncommitted Bug
| @@ -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"; | |||
There was a problem hiding this comment.
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' // errorAs 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.
There was a problem hiding this comment.
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.
Andarist
force-pushed
the
fix/do-not-widen-literals-in-dts-emit
branch
from
a03e6aa
to
718120d
Compare
| declare const c13: "abc" | "def"; | ||
| declare const c14: 123 | 456; |
There was a problem hiding this comment.
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;There was a problem hiding this comment.
Oh, I just realized who reported the bug, it is directly related!
typescript-bot
added
For Backlog Bug
|
I don't know if this will actually net any test changes but: @typescript-bot test top200 |
|
Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at 718120d. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at 718120d. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at 718120d. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the regular perf test suite on this PR at 718120d. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the tarball bundle task on this PR at 718120d. You can monitor the build here. |
|
Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
|
@jakebailey Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Everything looks good! |
|
@jakebailey Here they are:
CompilerComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
StartupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Hey @jakebailey, it looks like the DT test run failed. Please check the log for more details. |
|
@typescript-bot run dt |
|
Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at 718120d. You can monitor the build here. Update: The results are in! |
|
@jakebailey Here are the results of running the top-repos suite comparing Everything looks good! |
|
Hey @jakebailey, the results of running the DT tests are ready. |
| @@ -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"; | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
While talking to @weswigham, I thought of this example:
// @filename: a.ts
export const value = Math.random() > 0.5 ? "foo" : "bar";
// @filename: b.ts
import * as a from "./a";
export const x1 = a.value;
export let x2 = a.value;When you run tsc from this PR, you get:
// @filename: a.d.ts
export declare const value: "foo" | "bar";
// @filename: b.d.ts
export declare const x1: "foo" | "bar";
export declare let x2: string;Now, remove a.ts to simulate using it externally (or probably even in build mode), then rerun tsc, and you get:
// @filename: b.d.ts
export declare const x1: "foo" | "bar";
export declare let x2: "foo" | "bar";That seems pretty bad. Not sure how to proceed at this point.
Though, weren't we banning ternaries in isolatedDeclarations anyway?
|
I could implement @weswigham's suggestion but - like @jakebailey mentioned - it's unclear what to do about ternaries. I'd prefer to work on the further improvements to this PR in one go so I'm waiting for further feedback/decision about this.
I think that the behavior in question is confusing even outside of this mode. Perhaps it should be considered first without taking this mode into account. I imagine that a solution for this issue would automatically make |
|
FWIW I think this PR can be closed, just because I don't think we're going to be able to fix this by just not widening; we need some other mechanism to do this. |
fixes #55439