Fix fallthrough and default in new switch-true narrowing #55991
Conversation
typescript-bot
added
Author: Team
For Milestone Bug
|
@typescript-bot test top200 |
|
Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at 002973b. 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 002973b. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the regular perf test suite on this PR at 002973b. 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 002973b. 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 are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Everything looks good! |
|
@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: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Can we have a test of a default preceding a case?
Additionally, do we have any tests for case clauses narrowing subsequent case clauses?
type Shape =
| { kind: "circle", radius: number }
| { kind: "square", sideLength: number }
function wat(shape: Shape) {
switch (true) {
case shape.kind === "circle":
return Math.PI * shape.radius ** 2;
case shape.kind === "circle": // should error
}
if (shape.kind === "circle") {
return Math.PI * shape.radius ** 2;
}
else if (shape.kind === "circle") {
// ~~~~
// Property 'kind' does not exist on type 'never'.
}
}
|
@jakebailey Here are the results of running the top-repos suite comparing Everything looks good! |
|
@Andarist feel free to tackle this if you know the right answer here |
The more I think about this, the more I think this might be good behavior - at a high level, Footnotes
|
|
I feel like I must be wrong and we do have CFA within switch cases, I just didn't look long enough last night to figure it out... |
|
I am surely wrong, we totally create branch labels for case blocks and everything. |
I don't think your code would error in the way you expect; e.g. see what we do for typeof now: Playground Link |
|
@typescript-bot pack this |
|
Heya @jakebailey, I've started to run the tarball bundle task on this PR at a98c233. You can monitor the build here. |
|
I'm still not confident but I'd appreciate people trying the playground from the most recent change and try and break it. Clearly, this is not my area. |
|
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 |
|
Notably it feels suspicious that I haven't explicitly handled fallthrough, so I bet there's another edge case I'm missing where there needs to be CFA from one case group into another? |
src/compiler/checker.ts
Outdated
| // In the default case, we need to assume that all of the other cases were false, | ||
| // but also that any of the other cases in this block are true. |
There was a problem hiding this comment.
| // In the default case, we need to assume that all of the other cases were false, | |
| // but also that any of the other cases in this block are true. | |
| // In the default case, it's possible that we came from: | |
| // 1. a set of fallthrough cases, where their conditions might be true | |
| // 2. no case succeeding, in which case all conditions might be false |
There was a problem hiding this comment.
Actually...rereading it, is it the case that we gain no information from considering the fallthrough cases? Since you have to assume they could be both true and false?
There was a problem hiding this comment.
Yeah, that's the edge case I'm guessing at in #55991 (comment), I need to write a test that can show that possibility.
Not to mention that you could have a broad case and then narrow within the case and return, leaving only part of the type to fallthough...
There was a problem hiding this comment.
In thinking harder, I realized that what this function should do is just "what do I know about this thing based on only the case itself?"; you don't need to worry about control flow, fallthrough, etc, here at all, because CFA will combine this info with the other cases later.
So in reality, the bug being fixed is "multiple cases were not handled" and "default case was not handled", along with the combo of the two. I think I need a test for the combo, still, but I think I handled it.
|
@typescript-bot pack this |
|
Heya @jakebailey, I've started to run the tarball bundle task on this PR at 006f169. You can monitor the build here. |
|
Okay, another better version, but probably still not there. Please break it... |
|
Since it seems like it's working, I've rebased such that the fix and its test changes are all together for easier viewing. |
|
@jakebailey Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Details
|
|
The above shows that this PR is still not fully ready: Playground Link |
|
The effect on interface IProps {
one: boolean;
}
class Foo {
mine: string = "";
myMethod(x: IProps) {
const { one } = x;
switch (true) {
case one:
break;
default:
let x = this.mine;
}
}
} |
|
I'm currently away and just peeking here. The Ryan's case above is broken because: clausesType // never
hasDefaultClause // true
before // this
after // never
other // thisSince |
|
I think I figured it out... Will update shortly. I'm sure it'll be wrong in some all new way, though 😄 |
|
@typescript-bot test top200 |
|
Heya @jakebailey, I've started to run the tarball bundle task on this PR at 23f25c7. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at 23f25c7. 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 23f25c7. 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 23f25c7. 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 are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Everything looks good! |
|
@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 top-repos suite comparing Everything looks good! |
| @@ -28040,26 +28040,31 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| } | |||
|
|
|||
| function narrowTypeBySwitchOnTrue(type: Type, switchStatement: SwitchStatement, clauseStart: number, clauseEnd: number): Type { | |||
| const clauses = switchStatement.caseBlock.clauses.slice(clauseStart, clauseEnd); | |||
| const clausesType = narrowTypeForTrueClauses(type, clauses); | |||
| const defaultIndex = findIndex(switchStatement.caseBlock.clauses, clause => clause.kind === SyntaxKind.DefaultClause); | |||
| const hasDefaultClause = clauseStart === clauseEnd || (defaultIndex >= clauseStart && defaultIndex < clauseEnd); | |||
There was a problem hiding this comment.
Reading this in the below code was confusing for me because it's not about the switch having a default, it's about the given range including a default.
There was a problem hiding this comment.
This is just the terminology used above by the typeof narrowing. I can change it, but it will be inconsistent.
| } | ||
|
|
||
| // Now, narrow based on the cases in this set. | ||
| const clauses = switchStatement.caseBlock.clauses.slice(clauseStart, clauseEnd); |
There was a problem hiding this comment.
I could theoretically try and make this directly iterate instead of slicing and mapping, if that's desirable. We already do a lot of slicing/mapping/etc in narrowing that isn't really needed, so this isn't new.
There was a problem hiding this comment.
(see also narrowTypeBySwitchOnDiscriminant which just slices and doesn't try and be optimal)
| @@ -28043,6 +28039,36 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| return getUnionType(map(clauseWitnesses, text => text ? narrowTypeByTypeName(type, text) : neverType)); | |||
| } | |||
|
|
|||
| function narrowTypeBySwitchOnTrue(type: Type, switchStatement: SwitchStatement, clauseStart: number, clauseEnd: number): Type { | |||
| const defaultIndex = findIndex(switchStatement.caseBlock.clauses, clause => clause.kind === SyntaxKind.DefaultClause); | |||
There was a problem hiding this comment.
The only time switchStatement is used is to get caseBlock which is only used for clauses. Maybe we just pass that in or destructure that out?
There was a problem hiding this comment.
I don't really know what clauseStart === clauseEnd means, but you could also avoid the findIndex by folding the walk into the first walk, and then you'd be able to avoid the range check, right?
You are basically walking across every switch/case per group of clauses (or per clause) which is N^2, right?
There was a problem hiding this comment.
Like the other stuff, this is just me doing what the other switch case narrowings do. I can do whatever but it'll look different when it's possible that all of them could be improved.
There was a problem hiding this comment.
Fair enough, we should come back to these at another PR.
There was a problem hiding this comment.
Yeah, having now touched this, I do think there's a bunch to improve here and elsewhere. It's extremely likely that this just hasn't been touched in a while (when do we ever add new narrowings???)
There was a problem hiding this comment.
Do you want me to leave a TODO behind?
|
Just to note it, in talking to Daniel, it seems like there's some follow up stuff which would be nice to look at, e.g. if we just CFA'd between cases in switches, then it's likely that all of this code (and probably the other code for switch narrowing) would get to be deleted as we wouldn't have to re-encode that behavior into the checker (which we already do even before (Note also that Playground Link matches.) |
Fixes #55986
This fixes narrowing in fallthrough/default cases, narrowing the type properly against
assumeTruefor the other cases. It now behaves like if statements do.(We could totally also do
switch (false)but that seems like a bad move.)