Treat switch (typeof x) as exhaustive when all cases are handled for unconstrained type parameter
#56877
Conversation
…r unconstrained type parameter
| @@ -37014,7 +37014,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| if (!witnesses) { | |||
| return false; | |||
| } | |||
| const operandConstraint = getBaseConstraintOrType(checkExpressionCached((node.expression as TypeOfExpression).expression)); | |||
| const operandConstraint = getBaseConstraintOfType(checkExpressionCached((node.expression as TypeOfExpression).expression)) || unknownType; | |||
There was a problem hiding this comment.
Note that the constraint was already obtained here before so this doesn't really change that much. It just unifies how T and T extends unknown are handled here (switch (typeof x) was already treated as exhaustive for the latter).
On top of that, this is literally what getTypeFactsWorker (that is used later here) does as the very first thing. So this change doesn't even affect the other branch of the code in a meaningful way.
There was a problem hiding this comment.
I had to stare at this diff for a good 10 seconds before I even realized it was calling a different function. 😵💫 They differ by only a single character (and r and f look quite similar at small font sizes) and it feels like it would be really easy to mix these up when reading the code.
|
@typescript-bot test top200 @typescript-bot perf test this |
|
Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at 5adef66. 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 5adef66. 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 5adef66. 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 5adef66. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at 5adef66. You can monitor the build here. Update: The results are in! |
|
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 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: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@jakebailey 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
|
|
Hey @jakebailey, the results of running the DT tests are ready. |
|
@jakebailey Here are the results of running the top-repos suite comparing Everything looks good! |
| @@ -37014,7 +37014,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| if (!witnesses) { | |||
| return false; | |||
| } | |||
| const operandConstraint = getBaseConstraintOrType(checkExpressionCached((node.expression as TypeOfExpression).expression)); | |||
| const operandConstraint = getBaseConstraintOfType(checkExpressionCached((node.expression as TypeOfExpression).expression)) || unknownType; | |||
There was a problem hiding this comment.
This change as is breaks the following case:
// error on return type because switch is not seen as exhaustive
function woo(x: string): number {
switch (typeof x) {
case "string":
return 1;
}
}The problem is that getBaseConstraintOfType returns undefined for primitive types like string, in addition to returning undefined for an unconstrained type parameter, which is why we use getBaseConstraintOrType currently.
I think you might need to explicitly check the unconstrained type parameter case and use unknown as the operand constraint then.
There was a problem hiding this comment.
Good catch 👍 I pushed out a fix for this - could you take a look?
| @@ -37014,7 +37014,10 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| if (!witnesses) { | |||
| return false; | |||
| } | |||
| const operandConstraint = getBaseConstraintOrType(checkExpressionCached((node.expression as TypeOfExpression).expression)); | |||
| const type = checkExpressionCached((node.expression as TypeOfExpression).expression); | |||
| const operandConstraint = type.flags & (TypeFlags.Intersection | TypeFlags.Instantiable) ? | |||
There was a problem hiding this comment.
Why do we want to do this when the type is intersection or instantiable?
There was a problem hiding this comment.
Doing this for instantiables shouldn't fall into the trap that you have mentioned (with primitive types). I'm not sure about intersections though, I just followed the same logic that I found at the top of getTypeFactsWorker.
There was a problem hiding this comment.
I see.
I was going to say I don't know exactly if this is correct, but I realized there might be a simpler way to get what we want:
we can always say that the typeof switch is exhaustive if it has all possible typeof strings, regardless of the operand constraint.
So I think we can leave this all unchanged and just check if (TypeFacts.AllTypeofNE & notEqualFacts) === TypeFacts.AllTypeofNE is true, and if it is, we can return true (and otherwise we can do what we were already doing).
fixes #56786