Improve narrowing by boolean discriminant without strictNullChecks
#55291
Conversation
src/compiler/checker.ts
Outdated
| @@ -1205,7 +1205,7 @@ export const enum TypeFacts { | |||
| FalseStrictFacts = BaseBooleanStrictFacts | Falsy, | |||
| FalseFacts = BaseBooleanFacts, | |||
| TrueStrictFacts = BaseBooleanStrictFacts | Truthy, | |||
| TrueFacts = BaseBooleanFacts | Truthy, | |||
| TrueFacts = (BaseBooleanFacts & ~Falsy) | Truthy, | |||
There was a problem hiding this comment.
if this is too broad of a change then perhaps the fix (if it's even desired today) could be moved closer to this narrowType callback:
https://github.dev/microsoft/TypeScript/blob/e936eb13d2900f21d79553c32a704307c7ad03dd/src/compiler/checker.ts#L27121
| @@ -286,7 +286,7 @@ var resultIsString3 = null === undefined ? exprString1 : exprString2; | |||
| var resultIsObject3 = true || false ? exprIsObject1 : exprIsObject2; | |||
| >resultIsObject3 : Object | |||
| >true || false ? exprIsObject1 : exprIsObject2 : Object | |||
| >true || false : boolean | |||
| >true || false : true | |||
There was a problem hiding this comment.
This is the wrong result under strictNullChecks: false
There was a problem hiding this comment.
Could u ELI5 how to reason about strictNullChecks: false in situations like this?
There was a problem hiding this comment.
Any value can be falsy due to being null / undefined
There was a problem hiding this comment.
I know that from the assignability PoV but that doesnt help me explain your expected result here. Shouldnt intrinsic value like true be non-falsy?
Where is the line if the referenced issue got labeled as a bug?
There was a problem hiding this comment.
A value of type true might be inhabited by null. Obviously the expression true itself isn't, but that's not really under analysis
There was a problem hiding this comment.
If the compiler has to always account for the fact that the value might be inhabited by null then wouldn't it mean that one of the examples from the referenced issue shouldn't typecheck (and it does typecheck today)?
type Result = { success: true }
| { success: false, error: string }
function handleError2(res: Result) {
if (res.success !== false) {
return;
}
res.error; // OK, but shouldn't be?
}I always thought that strictNullChecks: false means that things can be null/undefined at runtime but that the type-checker is meant to ignore this fact completely.
There was a problem hiding this comment.
Either way - I pushed out an improved (but not cleaned up, code-wise) version of the fix for the linked issue. Please take a look if those semantics look any better.
There was a problem hiding this comment.
I always thought that strictNullChecks: false means that things can be null/undefined at runtime but that the type-checker is meant to ignore this fact completely.
This is a point of discussion that comes up sometimes on Discord - strictNullChecks: false is basically a completely different type checker. The flag does a lot more than its name would suggest, changing inferences, narrowing, etc. in sometimes very unexpected ways, and all of it is more or less by design.
Andarist
changed the title
Falsy from TypeFacts.TruthystrictNullChecks
…ation-no-strict-null-checks
…ation-no-strict-null-checks
|
@typescript-bot pack this |
|
Heya @DanielRosenwasser, I've started to run the parallelized Definitely Typed test suite on this PR at 6aaa73b. You can monitor the build here. Update: The results are in! |
|
Heya @DanielRosenwasser, I've started to run the diff-based top-repos suite on this PR at 6aaa73b. You can monitor the build here. Update: The results are in! |
|
Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at 6aaa73b. You can monitor the build here. |
|
Heya @DanielRosenwasser, I've started to run the regular perf test suite on this PR at 6aaa73b. You can monitor the build here. Update: The results are in! |
|
Heya @DanielRosenwasser, I've started to run the diff-based user code test suite on this PR at 6aaa73b. 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 There is also a playground for this build and an npm module you can use via |
|
@DanielRosenwasser Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Something interesting changed - please have a look. Details
|
|
@DanielRosenwasser 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 @DanielRosenwasser, the results of running the DT tests are ready. |
|
@DanielRosenwasser Here are the results of running the top-repos suite comparing Everything looks good! |
fixes #10564
strictNullChecks: false, I'm not even sure if this is the right fix or if the issue is supposed to get fixed (it's an old one). The non-strict rules are confusing to me :p