-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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.
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
Fix #2214. Support narrowing with typeof in switch condition. #21957
Fix #2214. Support narrowing with typeof in switch condition. #21957
Conversation
0960540
to
49acbe8
Compare
ping @mhegazy @RyanCavanaugh Sorry for the ping, just wondering if this got lost under a pile of other stuff. I'm not expecting anyone to drop what they're doing and look at this; I just want to make sure that it's clear I'm still actively wanting to try and merge this. |
Initial draft that works for union types First draft of PR ready code with tests Revert changed line for testing Add exhaustiveness checking and move narrowByTypeOfWitnesses Try caching mechanism Comment out exhaustiveness checking to find perf regression Re-enable exhaustiveness checking for typeof switches Check if changes to narrowByTypeOfWitnesses fix perf alone. Improve switch narrowing: + Take into account repeated clauses in the switch. + Handle unions of constrained type parameters. Add more tests Comments Revert back to if-like behaviour Remove redundant checks and simplify exhaustiveness checks Change comment for narrowBySwitchOnTypeOf Reduce implied type with getAssignmentReducedType Remove any annotations
49acbe8
to
0d79831
Compare
@weswigham can you please review this change |
src/compiler/checker.ts
Outdated
// } | ||
// | ||
// The implied type of the first clause number | string. | ||
// The implied type of the second clause is string (but this doesn't get used). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this comment correct? It seems a little off. Also could use a test which validates fallthrough behaviors, ie,
let x: string | number | boolean | object;
switch (typeof x) {
case 'number':
assertNumber(x)
case 'string':
assertStringOrNumber(x)
break;
default:
assertObject(x);
case 'number':
case 'boolean':
assertBooleanOrObject(x);
break;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the comment is wrong. The line for the second clause should read something like:
"The implied type of the second clause is never, but this does not get just because it includes a default case"
2bbb3d9
to
39d7d1e
Compare
After making some changes the CI failed due to strictNullChecks that weren't enabled when I initially developed the fix. The source of the problem was using undefined to mark the switch(typeof x) {
case "number": ...
case "string"...
default: ...
} is represented as the array I'm not convinced it's the most elegant solution, and would be more than happy to readdress the issue based on any feedback. |
39d7d1e
to
6391742
Compare
@weswigham can you take another look. |
I'm currently implementing a RemoteData pattern in TS and I think this would greatly help to simplify my |
I'm looking to add some more tests to this. If anyone has some examples that would be great! |
Pinging @RyanCavanaugh for visibility because I think mhegazy was the only one watching this and I believe they've left the TS team. |
@weswigham can you review the current iteration? Thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually looked at this yesterday but forgot to submit my upthumb. 👍
@jack-williams @RyanCavanaugh even though there's no conflicts, a merge with master in the branch prior to us pulling it in would be good - a few months passing may mean someone's added a test this may affect. |
@weswigham @RyanCavanaugh Thanks! Master merged. CI failed on node 6 because of missing jake, I'm not sure if this was caused by my changes. Just let me know if I need fix anything. |
Enable narrowing support for
typeof
switch conditions. Fixes #2214Implement narrow-by-assertion in switch with
typeof
, intending to replicate the equivalent behaviour withif-then-else
.@mhegazy Is there any possibility of this making the 3.0 milestone?
Basic Example:
Exhaustiveness Checking:
Switch exhaustiveness checking is extended to support
typeof
.Order sensitive narrowing:
Repeated cases do not get the same narrowing.
Comparison with
if-then-else
.1. Constrained parameters are not narrowed to
never
indefault
/else
2. Unions of constrained generic parameters.
Code Review
The code comments are primarily there for review purposes, they should probably be taken out afterwards. A few things to signpost for the review:
getSwitchClauseTypeOfWitnesses
performs no caching. The choice for this was because the cache type isType[]
, the types of the clause values. In the context oftypeof
the types of the clause values are less important, rather what they represent. There didn't seem to be much use for caching the types. I'd like to get feedback on this, I'm not sure my decision is correct.isExhaustiveSwitchStatement
is updated to handle switch statements withtypeof
conditions.narrowBySwitchOnTypeOf
be moved outside of the scope of reference flow typing?