Make the falsy part of any/unknown any/unknown #39529
Conversation
|
@typescript-bot pack this |
|
Heya @DanielRosenwasser, I've started to run the parallelized community code test suite on this PR at 485ef4e. You can monitor the build here. |
|
Heya @DanielRosenwasser, I've started to run the parallelized Definitely Typed test suite on this PR at 485ef4e. You can monitor the build here. |
|
Heya @DanielRosenwasser, I've started to run the extended test suite on this PR at 485ef4e. You can monitor the build here. |
|
Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at 485ef4e. You can monitor the build here. |
|
@typescript-bot perf test this |
|
Heya @DanielRosenwasser, I've started to run the perf test suite on this PR at 485ef4e. You can monitor the build here. Update: The results are in! |
|
Hey @DanielRosenwasser, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running |
|
Hey @DanielRosenwasser, something went wrong when looking for the build artifact. (You can check the log here). |
|
The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master. |
|
We're debating 4.0 vs 4.1 so please hold on merging for now |
|
@DanielRosenwasser Here they are:Comparison Report - master..39529
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Perf numbers seem reasonable, but someone should take another look. Office UI Fabric seems to have some existing issues I can't tease out. Here's a few user test changes Uglify had type differences https://github.com/typescript-bot/TypeScript/pull/58/files#diff-515e686b9c133473b3091193f6519572L138 - node_modules/uglify-js/lib/compress.js(6307,25): error TS2403: Subsequent variable declarations must have the same type. Variable 'code' must be of type 'string', but here has type '{ get: () => string; toString: () => string; indent: (half: any) => void; indentation: () => number; current_width: () => number; should_break: () => boolean; has_parens: () => boolean; newline: () => void; print: (str: any) => void; ... 23 more ...; parent: (n: any) => any; }'.
+ node_modules/uglify-js/lib/compress.js(6307,25): error TS2403: Subsequent variable declarations must have the same type. Variable 'code' must be of type 'string', but here has type '{ get: () => string; toString: () => string; indent: (half: any) => void; indentation: () => number; current_width: () => number; should_break: () => any; has_parens: () => boolean; newline: () => void; print: (str: any) => void; ... 23 more ...; parent: (n: any) => any; }'.That's sort of expected I suppose. https://github.com/typescript-bot/TypeScript/pull/58/files#diff-515e686b9c133473b3091193f6519572L150 - node_modules/uglify-js/lib/compress.js(7635,29): error TS2367: This condition will always return 'false' since the types 'boolean' and 'string' have no overlap.This one seems questionable, but I can't figure out what the file looked like at that point. VS Code had some breaks though: + [XX:XX:XX] Error: /vscode/src/vs/platform/workspace/common/workspace.ts(85,3): Type 'unknown' is not assignable to type 'boolean'.
+ [XX:XX:XX] Error: /vscode/src/vs/platform/workspace/common/workspace.ts(130,3): Type 'unknown' is not assignable to type 'boolean'.
+ [XX:XX:XX] Error: /vscode/src/vs/workbench/common/editor/editorGroup.ts(53,2): Type 'unknown' is not assignable to type 'boolean'.
+ [XX:XX:XX] Error: /vscode/src/vs/platform/workspace/common/workspace.ts(85,3): Type 'unknown' is not assignable to type 'boolean'.
+ [XX:XX:XX] Error: /vscode/src/vs/platform/workspace/common/workspace.ts(130,3): Type 'unknown' is not assignable to type 'boolean'.
+ [XX:XX:XX] Error: /vscode/src/vs/workbench/common/editor/editorGroup.ts(53,2): Type 'unknown' is not assignable to type 'boolean'.Note there are really 3 breaks, I'm not sure why the diff repeats itself. Some of these are technically correct. Kind of an obvious pattern around defining type predicates. |
|
I think the VS Code ones are good breaks. It’s not unique to type predicates, though they may be a common manifestation since the input is usually function isThing(x: any): boolean {
return x && typeof x === 'object' && x.blah === 'foo';
}This passes checking today but shouldn’t, since the actual domain of the return expression includes function isThing(x: string | number | Record<string, any>): boolean {
return x && typeof x === 'object' && x.blah === 'foo';
// Type 'boolean | "" | 0' is not assignable to type 'boolean'.
// Type '""' is not assignable to type 'boolean'.(2322)
} |
|
Yeah, if you're using |
|
I also don’t know how to see the uglify source either, but I can give you a simplified example of how this change would correctly remove that error you said looks questionable: playground |
|
(also, I don't know how to look at the other diffs, DevOps is too confusing) |
|
DefinitelyTyped tests |
|
I think RWC came back unaffected? |
|
The DefinitelyTyped failures are all unrelated; they exist in the nightly dtslint run against master. |
|
@typescript-bot pack this - playground build failed because it switched to the new repo and needed access - microsoft/TypeScript-Website#130 (comment) |
|
Hey @orta, 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. |
|
From design meeting:
|
This turns out to be a little bit difficult for two reasons:
|
|
Talked to @RyanCavanaugh and decided to merge this now—there may be no worthwhile way to track the falsy unknown back to the expression where it originated, and it’s better to get this in 4.1 sooner rather than later. |
typescript-bot
added
the
For Milestone Bug
|
Can you add this to https://github.com/microsoft/TypeScript/wiki/Breaking-Changes @andrewbranch? |
|
Done 👍 |
|
Thank you! |
Fixes #39113