Narrow by comparisons to boolean literals #53714
Conversation
e9a78f2 to
2540cc4
Compare
|
@typescript-bot test this |
|
Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at 586003b. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the extended test suite on this PR at 586003b. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the abridged perf test suite on this PR at 586003b. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at 586003b. 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 586003b. You can monitor the build here. Update: The results are in! |
|
@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! |
|
Heya @jakebailey, I've run the RWC suite on this PR - assuming you're on the TS core team, you can view the resulting diff here. |
|
@typescript-bot pack this Need to minimize an RWC change. |
|
Heya @jakebailey, I've started to run the tarball bundle task on this PR at 586003b. You can monitor the build here. |
|
@jakebailey Here they are:Comparison Report - main..53714
System
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
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 top-repos suite comparing Everything looks good! |
|
Hey @jakebailey, the results of running the DT tests are ready. |
|
@jakebailey I'd consider this an improvement because we get an error today here: export function isComputed(value: any, property?: string): boolean {
if (value === null || value === undefined) return false
if (property !== undefined) {
- if (isObservableObject(value) === false) return false
+ if (!isObservableObject(value)) return false
if (!value.$mobx.values[property]) return false
return true
}
return true
} |
src/compiler/checker.ts
Outdated
| @@ -26992,6 +26994,11 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| return type; | |||
| } | |||
|
|
|||
| function narrowTypeByBooleanComparison(type: Type, expr: Expression, bool: BooleanLiteral, operator: BinaryOperator, assumeTrue: boolean): Type { | |||
| assumeTrue = (assumeTrue !== (bool.kind === SyntaxKind.TrueKeyword)) !== (operator !== SyntaxKind.ExclamationEqualsEqualsToken && operator !== SyntaxKind.ExclamationEqualsToken); | |||
| return narrowType(type, expr, assumeTrue); | |||
There was a problem hiding this comment.
I probably should take another look at this and conditionally use narrowTypeByEquality or something like that. Gonna investigate this 🔜
There was a problem hiding this comment.
I don't have to do that since this is already handled by the branch that comes earlier in the code.
|
The TypeScript team hasn't accepted the linked issue #44366. If you can get it accepted, this PR will have a better chance of being reviewed. |
|
I came back to this after a while and I think that this PR is in good shape and can be reviewed. There are potentially some issues here related to coercion but this isn't a new problem: function test(a: string | number | boolean) {
if (a == false) {
a; // actual: false, expected: "" | 0 | false
}
}So I don't think that those should block this PR. |
| @@ -27610,6 +27612,11 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| return type; | |||
| } | |||
|
|
|||
| function narrowTypeByBooleanComparison(type: Type, expr: Expression, bool: BooleanLiteral, operator: BinaryOperator, assumeTrue: boolean): Type { | |||
| assumeTrue = (assumeTrue !== (bool.kind === SyntaxKind.TrueKeyword)) !== (operator !== SyntaxKind.ExclamationEqualsEqualsToken && operator !== SyntaxKind.ExclamationEqualsToken); | |||
There was a problem hiding this comment.
I think my only comment on this bit at this point is just how unreadable this expression is; I honestly have no idea what I'm looking at here. Is there a more obvious way to represent this?
There was a problem hiding this comment.
Iirc this is a double xor with booleans
There was a problem hiding this comment.
That makes me feel slightly better, but this is still like 3 nested equalities...
There was a problem hiding this comment.
I don't disagree 😅 at first I started with some nested if statements - and believe me... it didn't look good either. Then I realized that what I writing there was really just a xor so I managed to fold it all into this one line. It didn't look that great to me either. I think it's a very nice way of doing this but the readability suffers here if one doesnt know what this is. I tried to give it some names but I don't have any good ones so I gave up. The best alternative that I can offer would be smth like:
const a = assumeTrue;
const b = bool.kind === SyntaxKind.TrueKeyword;
const c = operator !== SyntaxKind.ExclamationEqualsEqualsToken && operator !== SyntaxKind.ExclamationEqualsToken
// boolean xors
assumeTrue = (a !== b) !== c|
@typescript-bot test top200 |
|
Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at 9568e9e. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the tsc-only perf test suite on this PR at 9568e9e. 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 9568e9e. 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 9568e9e. You can monitor the build here. Update: The results are in! |
|
@jakebailey Here they are:
CompilerComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@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! |
|
Hey @jakebailey, it looks like the DT test run failed. Please check the log for more details. |
|
@jakebailey Here are the results of running the top-repos suite comparing Everything looks good! |
|
@typescript-bot pack this |
|
Heya @jakebailey, I've started to run the tarball bundle task on this PR at 9568e9e. 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 |
There was a problem hiding this comment.
Inspecting Playground Link (as it's a little easier to read than the baselines), it seems like things are correct.
@RyanCavanaugh @DanielRosenwasser is this something we want for sure?
|
I think that Ryan confirmed in March that this is smth that the team would accept PR for (see here) |
|
Ah, yeah, you're right. |
fixes #31105
closes #44366